Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

From: Laxman Dewangan
Date: Fri Jun 03 2016 - 07:39:39 EST

On Friday 03 June 2016 03:36 PM, Jonathan Cameron wrote:
On 01/06/16 13:34, Laxman Dewangan wrote:
Add support for INA3221 SW driver via IIO ADC interface. The device is
register as iio-device and provides interface for voltage/current and power
monitor. Also provide interface for setting oneshot/continuous mode and
critical/warning threshold for the shunt voltage drop.

Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
Hi Laxman,

As ever with any driver lying on the border of IIO and hwmon, please include
a short justification of why you need an IIO driver and also cc the
hwmon list + maintainers. (cc'd on this reply).

I simply won't take a driver where the hwmon maintainers aren't happy.
As it stands I'm not seeing obvious reasons in the code for why this
should be an IIO device.

I thought that all ADC or monitors are going to be part of IIO device framework. I saw the ina2xx which is same (single channel) which was my reference point.

Funily enough I know this datasheet a little as was evaluating
it for use on some boards at the day job a week or so ago.

Various comments inline. Major points are:
* Don't use 'fake' channels to control events. If the events infrastructure
doesn't handle your events, then fix that rather than working around it.
* There is a lot of ABI in here concerned with oneshot vs continuous.
This seems to me to be more than it should be. We wouldn't expect to
see stuff changing as a result of switching between these modes other
than wrt to when the data shows up. So I'd expect to not see this
directly exposed at all - but rather sit in oneshot unless either:
1) Buffered mode is running (not currently supported)
2) Alerts are on - which I think requires it to be in continuous mode.

Other question to my mind is whether we should be reporting vshunt or
(using device tree to pass resistance) current.

This is bus and shunt voltage device for power monitoring. In our platforms, we use this device for bus current and so power monitor.

We have two usecases, one is one shot, read when it needs it. And other continuous when we have multiple core running then continuous mode to get the power consumption by rail.

Yaah, alert is used only on continuous mode and mainly used for throttling when rail power goes beyond some limit.