lkml.org 
[lkml]   [2015]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v2 1/2] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors
From
Date
On 14/11/2015 19:59, Jonathan Cameron wrote:
> On 12/11/15 12:57, Marc Titinger wrote:
>> Basic support or direct IO raw read, with averaging attribute.
>> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>>
>> Output of iio_info:
>>
>> iio:device0: ina226
>> 4 channels found:
>> power3: (input)
>> 1 channel-specific attributes found:
>> attr 0: raw value: 1.150000
>> voltage0: (input)
>> 1 channel-specific attributes found:
>> attr 0: raw value: 0.000003
>> voltage1: (input)
>> 1 channel-specific attributes found:
>> attr 0: raw value: 4.277500
>> current2: (input)
>> 1 channel-specific attributes found:
>> attr 0: raw value: 0.268000
>> 2 device-specific attributes found:
>> attr 0: in_calibscale value: 10000
>> attr 1: in_mean_raw value: 4
>> attr 2: in_sampling_frequency value: 455
>>
>> Tested with ina226, on BeagleBoneBlack.
>>
>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> Hi Marc
>

Hi Jonathan,

> To express a personal preference, please don't have series of patches as
> replies to the original thread. It gets really hard to follow after
> a couple of revisions!
>
Ok, sorry for that.

> May seem a random question, but what do you want to gain from an IIO
> driver over what the hwmon provides?

Good question. In the frame of Baylibre's ACME power and temperature
monitoring demo based on Sigrock, we wish to add a remote mode for the
GUI and processing front-end as well as explore other possibilities like
using an on-board accelerator to capture the sensor data and stream it
back to the front-end. This work is a pathfinder as IIO seems
appropriate for what we want to do.

>
> Various bits inline..

Thanks for the review, further questions below!

Marc.

>
> Mostly looks pretty good though.
>
> Jonathan
>> ---
>>

...

>> +#define INA2XX_RSHUNT_DEFAULT 10000
>> +
>> +/* bit mask for reading the averaging setting in the configuration register */
>> +#define INA226_AVG_RD_MASK 0x0E00
> genmask is always good for these.
>

ok.

..

>> +
>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> + return (reg == INA2XX_CONFIG)
>> + || (reg == INA2XX_CALIBRATION);
> On one line I think this is still way less than 80 chars..
>> +}

ok

...


>> +struct ina2xx_chip_info {
>> + struct iio_dev *indio_dev;
> Having a pointer back to the indio_dev is usually a sign of
> something 'unusual' going on... Will be interesting to see what it is for ;)
>
> Ah, that was easy, you don't use it that I can see.
>

Ah, forgot to remove it when splitting into two patches, but yeah you're
right, I shall pass the indio_dev ptr as data in the first place.

...

>> +
>> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>> + unsigned int regval, int *val, int *uval)
>> +{
>> + *val = 0;
>> +
>> + switch (reg) {
>> + case INA2XX_SHUNT_VOLTAGE:
>> + /* signed register */
>> + *uval = DIV_ROUND_CLOSEST((s16) regval,
>> + chip->config->shunt_div);
>> + *val = *uval/1000000;
>> + *uval = *uval - *val*1000000;
>> + return IIO_VAL_INT_PLUS_MICRO;
> I'm somewhat unconvinced that this wrapper is adding anything over
> just doing this where you care about the result.
> Also, this is a straight forward linear conversion. Do it in userspace
> by providing the relevant 'scale' element.

got it! A practical question: can you specify a divider rather than a
multiplier as a scale so that userspace does the division?

>> +
>> + case INA2XX_BUS_VOLTAGE:
>> + *uval = (regval >> chip->config->bus_voltage_shift)
>> + * chip->config->bus_voltage_lsb;
>> + *val = *uval/1000000;
>> + *uval = *uval - *val*1000000;
>> + return IIO_VAL_INT_PLUS_MICRO;
...

>> + tmp = config;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_AVERAGE_RAW:
>> +
>> + ret = ina226_set_average(chip, val, &tmp);
> This isn't what the ABI uses the info_average_raw attribute for - it's
> for outputing the average of a channel rather than setting a mean
> filter width (which is what you have here). Probably need some new ABI
> for this as our current filter description ABI is rather limited!
>
Ah ok, should this go into a sysfs attribute then, until the ABI section
is defined ? How about specifying the ABI section while we are at it ?

...

.driver_module = THIS_MODULE,
>> +};
>> +
>> +/*
> Single line comment, use single line comment syntax.

ok




\
 
 \ /
  Last update: 2015-11-16 11:01    [W:0.042 / U:12.400 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site