Re: [RFC 1/4] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Marc Titinger <mtitinger@baylibre.com>]

On 11/11/2015 11:14, Daniel Baluta wrote:
> On Tue, Nov 10, 2015 at 6:07 PM, Marc Titinger <mtitinger@baylibre.com> wrote:
>> Basic support or direct IO raw read, with averaging attribute.
>> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>>

Hi Daniel,

thanks for the review,
comments bellow,

Marc.


>> IIO context has 1 devices:
> Is this from libiio right? Perhaps you should specify this:
>
> "libiio context has 1 devices"

It's the raw output from iio_info to give a quick overview of the features.

>
>>       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
>>
>>
>
> Link to datasheet?

Ok, will add it.

http://www.ti.com/lit/gpn/ina226


...


>>
>> +config INA2XX_IIO
>> +        tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> +       depends on I2C
>
> Since you are using regmap you should select it here.

Right.

>
>> +        help
>> +          Say yes here to build support for TI INA2xx familly Power Monitors.
>> +
>> +         Note that this is not the existing hwmon interface for this device.
>> +
>> +
>
> Config symbol should be in alphabetical order.

Ok
>
...


>>   obj-$(CONFIG_AD7266) += ad7266.o
>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>
> The same here.

Ok.

>
>>   obj-$(CONFIG_AD7291) += ad7291.o
>>   obj-$(CONFIG_AD7298) += ad7298.o
>>   obj-$(CONFIG_AD7923) += ad7923.o
>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
>> new file mode 100644
>> index 0000000..257d8d5
>> --- /dev/null
>> +++ b/drivers/iio/adc/ina2xx-iio.c
>> @@ -0,0 +1,404 @@
>> +/*
>> + * INA2XX Current and Power Monitors
>> + *

...

>> + *
>> + * Licensed under the GPL-2 or later.
>
> If you know the I2C address its recommended to mention it here.

ok.

...

>> +
>> +static struct regmap_config ina2xx_regmap_config = {
>> +       .reg_bits = 8,
>> +       .val_bits = 16,
> Specify here which registers are cacheable, read/write.

ok.

...

>> +
>> +       return iio_device_register(indio_dev);
>
> If there is no reverse operation for ina2xx_init (e.g ina2xx_poweroff) then here
> you can use devm_iio_device_register and completely remove
> ina2xx_remove function.

right, thanks!