Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Andrew F. Davis]

On 04/12/2016 03:29 AM, jic23@xxxxxxxxxxxxxxxxxxxxx wrote:
> On 11.04.2016 16:48, Andrew F. Davis wrote:
>> On 04/10/2016 06:57 AM, Jonathan Cameron wrote:
>>> On 08/04/16 19:19, Andrew F. Davis wrote:
>>>> The INA3221 is a three-channel, high-side current and bus voltage
>>>> monitor
>>>> with an I2C and SMBUS compatible interface.
>>>>
>>>> Signed-off-by: Andrew F. Davis <afd@xxxxxx>
>>> Hi Andrew,
>>>
>>> My immediate thought on this one is whether it would be better off in
>>> hwmon.
>>> I'm not convinced either way from a quick glance at the data sheet,
>>> but the
>>> question needs to be addressed.
>>>
>>
>> I was on the fence with this also, I was leaning towards hwmod until I
>> looked onto how the ina2xx driver has been ported to iio. This is almost
>> the same part but the ina3x has three monitors vs one. In addition it
>> looks like NVIDIA has written a hwmod driver for this part
>> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
>>
>> but then also ported it over to IIO (although doesn't appear to be
>> upstream ready or ever has been submitted for such)
>> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
>>
>> So I figured this was the way things are moving, but I have no problem
>> working this as a hwmod driver. The IIO work is already done here, I'll
>> write the hwmod version also but keep working this, I see no reason we
>> cant have both if needed. (unless using this and just using iio_hwmod.c
>> if needed is more acceptable?)
>>
>>> It's not exactly a clean fit for the IIO ABI either at the moment though
>>> I think some elements of that could be easily sorted out.
>>> The key think in my mind is to look at what is actually being measured
>>> rather than assume all the external components will be as the datasheet
>>> assumes them to be...
>>>
>>> + where do the alert lines actually go?
>>>
>>> Jonathan
>>>> ---
>>>>  .../ABI/testing/sysfs-bus-iio-adc-ina3221          |  23 ++
>>>>  drivers/iio/adc/Kconfig                            |   7 +
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/ina3221.c                          | 417
>>>> +++++++++++++++++++++
>>>>  4 files changed, 448 insertions(+)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>>>  create mode 100644 drivers/iio/adc/ina3221.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>>> b/Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>>> new file mode 100644
>>>> index 0000000..bbe4f8c
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>>> @@ -0,0 +1,23 @@
>>>> +What:       
>>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_shunt_(raw|scale)
>>>> +What:       
>>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_bus_(raw|scale)
>>>> +Date:        April 2016
>>>> +KernelVersion:    4.7
>>>> +Contact:    Andrew F. Davis <afd@xxxxxx>
>>>> +Description:
>>>> +        Shunt and Bus voltage for each channel.
>>> Units etc need specifying or at least a reference to the main
>>> in_voltageY_raw
>>> docs etc.  These are really completely separate measurements be it
>>> differential measurements where the + on one is the - on the other
>>> (second is really a
>>> unipolar measurement as it's relative to 0).  I wonder if we are
>>> better off supporting them as such so define this device as having
>>> the channels.
>>> in_voltage1-voltage0_raw (shunt voltage 1)
>>> in_voltage0_raw (bus voltage 1)
>>> in_voltage3-voltage2_raw (shunt voltage 2)
>>> in_voltage2_raw (bus voltage 2)
>>> in_voltage5-voltage4_raw (shunt voltage 3)
>>> in_voltage4_raw
>>>
>>> That I think reflects the actual measuring options, without applying
>>> assumptions on what is connected to them.  Yes the datasheet says you
>>> should
>>> put a shunt between the first two connections and the load between the
>>> second connection and the 0 volt line, but I can't see anything
>>> preventing
>>> this being used differently...
>>
>> I feel this may become confusing to some, but I have no real objection
>> to this.
> Guenter's point that the shunt measurements are really current measures
> may mean it makes
> more sense to handle these by allowing say device tree to provide the
> resistance values and
> then converting them to a direct current measure.

I think so as well, and yesterday I converted the driver to hwmod with
this in mind: http://www.spinics.net/lists/kernel/msg2231424.html
Here we do the calculation in software, but I feel there may be a reason
it was not performed in hardware to begin with, I'm not sure if there
are use-cases where this math will not be correct (low/high-side wiring
changes or something).

>>
>>>> +
>>>> +What:       
>>>> /sys/bus/iio/devices/iio:deviceX/shunt_integration_time[_available]
>>>> +What:       
>>>> /sys/bus/iio/devices/iio:deviceX/bus_integration_time[_available]
>>>> +Date:        April 2016
>>>> +KernelVersion:    4.7
>>>> +Contact:    Andrew F. Davis <afd@xxxxxx>
>>>> +Description:
>>>> +        Shunt and Bus integration time for each channel.
>>> I'd keep these tightly associated with the channels as then they become
>>> standard abi elements, just for channels with extended names.
>>
>> The other option is to have each channel have an integration_time, but
>> this may give the false impression that they are individually adjustable
>> when they are actually all linked together, change one, they all change
>> (of the same type (bus/shunt)).
> Hmm. It is a little fiddly as we don't support grouping by extended name
> like this.
> It's far from uncommon to have a set of channel parameters tied together
> and the ABI
> allows for this.  Any parameter can change any other.  I think I'd
> rather stay within
> the standard abi if at all possible.  Perhaps this will all be cleaned
> up anyway if we
> move to having the shunt voltages output as currents?
> 

That may work.

> 
>>
>>>> +
>>>> +What:       
>>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_shunt_critical_(raw|scale)
>>>> +What:       
>>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_shunt_warning_(raw|scale)
>>>> +Date:        April 2016
>>>> +KernelVersion:    4.7
>>>> +Contact:    Andrew F. Davis <afd@xxxxxx>
>>>> +Description:
>>>> +        Shunt voltage critical and warning trigger levels.
>>> This is why I think this may really belong in hwmon.
>>> The way you currently have this done is a bodge on the relevant IIO
>>> interfaces.
>>> If there is good reason to look at multiple equivalent event types on a
>>> given channel in IIO we can look at adding that support to the core.
>>> This is a lot more common in explicit hardware monitoring chips than in
>>> more general ADCs.
>>>
>>
>> Agree, we already have threshold events, perhaps a way to set the
>> threshold for devices like this that have adjustable ones could be
>> useful, but I see what you mean, why would regular ADCs need an
>> adjustable threshold?
>>
> I'm a little confused as all the thresholds are adjustable... Issue is we
> only currently allow one event of a given type per channel - here you
> effectively
> had two.
> 
>>> Also I see nothing in the driver that is actually detecting if these
>>> conditions have been passed?  If that is assumed to be a problem for
>>> external
>>> hardware then it should be clearly documented as such.
>>>
>>
>> I was thinking about leaving these to the user to do something with,
>> they may want them tie them to an alert event somehow. Or I could
>> probably tie them to channel events?
> Channel event I think, but to support this properly we need the ability to
> have two such thresholds on one channel.

This is why I have not attempted interrupt event handling yet, I believe
expanding channel events may be what is needed to start merging hwmon
and iio. Again not volunteering just yet :)

Andrew