Re: [PATCH 0/4] iio: adc: Maxim max961x driver

[Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

On 07/03/17 19:12, jacopo mondi wrote:
> Hi Jonathan,
> 
>    + Neil Amstrong from Baylibre
> 
> On 05/03/2017 11:49, Jonathan Cameron wrote:
>> On 27/02/17 09:09, jacopo mondi wrote:
>>> Hi Jonathan,
>>> On 25/02/2017 17:09, Jonathan Cameron wrote:
>>>> On 24/02/17 15:05, Jacopo Mondi wrote:
>>>>> Hello!
>>>>>
>>>>> This series adds driver and documentation for Maxim max9611/max9612
>>>>> high-side current sense amplifier with 12-bit ADC and I2c interface.
>>>>> It also registers two devices installed on VDD_0.8V and DVFS_0.8V lines
>>>>> in Renesas r87796 Salvator-X board.
>>>>>
>>>>> The device provides several functionalities, and only some of them are
>>>>> currently supported by this driver.
>>>>> Particularly, the on-board op-amp and analog comparator are not currently
>>>>> supported.
>>>> More fun to come ;)
>>>>>
>>>>> A simplified integration schematic drawing is here reported:
>>>>>
>>>>>  ----o----/\/\/-----o-------|LOAD|---
>>>>>      |    shunt     |
>>>>>  ____|______________|___
>>>>>  |  RS+            RS-  |
>>>>>  |   |-----gain-----|   |
>>>>>  |          |           |
>>>>>  |          |           |
>>>>>  |max961x   |->| ADC |===== I2c
>>>>>  |______________________|
>>>>>
>>>>>
>>>>> The device provides though its 12-bits ADC the following informations:
>>>> information is it's own plural (silly English quirk of the day!)
>>>>
>>>>> - Common input voltage on RS+
>>>>> - Voltage drop between RS+ and RS- ends
>>>>> - Die temperature
>>>>>
>>>>> All of the above ones are exposed though IIO with _raw and _scale values
>>>>> (plus _input for milli degree Celsius die temperature).
>>>>>
>>>>> From the above values the driver calculates the current flowing between
>>>>> RS+ and RS- ends, using the shunt resistor value provided by device tree, and
>>>>> the power load. Again this values are exposed through _raw and _scale
>>>>> attributes, which I'm not completely sure it's acceptables as they are
>>>>> calculated values and not natively provided by the current sense amplifier.
>>>>> I would like to hear IIO people opinions on this, if they should be better
>>>>> exposed though some other attributes which are not _raw and _scale, or if
>>>>> their calculation should be completely left to userspace tools.
>>>> So one element of the implementation is a problem.
>>>> Because of the dynamic gain adjustment you are doing in the driver (which I
>>>> rather like BTW) there will be instabilities in the computed values that
>>>> userspace comes up with when we are near a transition for in the current
>>>> sense amplifier gain.  We can't do that as crazy outputs will result
>>>> (read offset and scale for possible different values of that gain then
>>>> read the actual ADC value for a possible 3rd choice resulting in complete
>>>> garbage).
>>>>
>>>> So there are two ways to avoid this:
>>>> 1. Move all that magic into the original reads and apply the gain and offset
>>>> before they get to userspace.
>>>> 2. Provide enough information for userspace to be able to compute everything
>>>> iff it is using buffered mode with a 'scan' covering all the channels in one
>>>> go.  Even then we'd have to explicitly allow reading of the PGA gain as a
>>>> channel, or make userspace responsible for doing your autogain stuff.
>>>>
>>>> 1 is probably easier but will make implementing 2 as a follow up will be tricky
>>>> and would be needed if you want to read this stuff faster than sysfs will
>>>> allow.
>>>>
>>>> It's a shame, but my gut feeling is you want to drop the autogain stuff
>>>> as then it all becomes straight forward.
>>>>
>>>> What do others think?
>>>>
>>>
>>> Just to add the following element to the discussion.
>>>
>>> The device supports cycling between channels by itself, sequentially cycling between them every 2 msec.
>>> At the same time, gain setting is not involved in this procedure, and last applied is used on current sense voltage channel.
>>>
>>> Still this does not help with un-synchronized reads of gain-dependent values, as offset and scale are.
>>>
>>> Also, removing auto-gain setting routine will simplify stuff, but I don't see how all becomes straight forward then. I mean, if we keep gain at a fixed value, yes everything's easier, but it will work under a much more limited pre-conditions set...
>>
>> You'd be moving the issue to userspace.  This would be beneficial if you
>> were looking to use the buffered interfaces (through a kfifo).  In those
>> cases processed values are often a non starter as they don't have well
>> defined 'sizes' in the same way that a reading directly from the chip does.
>>
>> So under those circumstances, your userspace code might elect to modify
>> the gain to get in the 'right' region only at startup, or after some initial
>> 'calibration data capture'.  Then it could merily stream data out without
>> caring about it.  To run remotely fast and consistenty you'd have to stop
>> doing the multiple passes needed for autogain.
>>
>> However, if that interface never makes sense for this device anyway then
>> it the approach of doing it all in kernel makes more sense.
>>
>> With a 500sps device like this it's on the boundary to my mind...
>>
>> Quick enough that using the chardev access makes sense, but the usecase
>> is perhaps such that we should leave this all in kernel space.
>>
>> It's slightly horrible but there is a path forward eventually if we
>> were to add buffered support having gone with the do it all in the kernel:
>> * Add a 'autorange_enable' attribute to allow us to turn it off or implicitly
>> assume that it is turned off if we are doing buffered capture (which is a bit
>> nasty from a 'doing what you'd expect' viewpoint).
>> * Add scale attributes at that stage.
>>
>> Now, if the auto range stuff was done on the actual chip we could figure
>> out how to export that as pseudo channels alongside the real ones but
>> that might get uggly and isn't the case here anyway.
>>
>> Jonathan
>>
> 
> Let me summarize this a bit, so to make sure we're on the same page.
> 
> - No buffered reads:
> -- current and power are processed only channels.
> -- processed channels do not live well with buffered reads.
> -- no scale and offset attributes are exposed to userland
> -- kernel takes care of autogain (introducing a little delay in each read)
> 
> - Buffered reads:
> -- userspace needs to "calibrate" the autogain
> -- scale and offset depends on gain settings and shall be created after gain selection
spot on.
> 
> Now, I'm under the impression what really makes a difference here is
> the use case this driver has to address, and sincerely, I cannot
> define a precise one right now, at least not in terms of typical
> sampling frequency of power monitoring tools.
> 
> That's why I have now copied Neil Armstrong, as Baylibre's ACME is a
> sort-of-standard out there, and they may have faced the same issues
> when implementing ina2x driver (which is used for power sensing in
> ACME cape if I'm not wrong). I hope he may be able to follow here,
> and provide some wisdom on required data access speed.
> 
> imho, if no strict speed requirements are present, and we can live
> with chrdev access only, exposing 2 processed channels, and let
That would be sysfs only, buffered reads are the chrdev ones (confusing
terminology :()
> kernel deal with gain configuration is a more desirable solutions in
> terms of interface clearness. But again, let's wait for more people
> to comment here!
Agreed, guess we give it a little longer!

Jonathan
> 
Thanks
>    j
> 
> 
> 
> 
>>
>>>
>>> Thanks
>>>    j
>>>
>>>
>>>> Jonathan
>>>>>
>>>>> Thanks
>>>>>    j
>>>>>
>>>>> Jacopo Mondi (4):
>>>>>   Documentation: dt-bindings: iio: Add max961x
>>>>>   iio: Documentation: Add max961x sysfs documentation
>>>>>   iio: adc: Add max9611/9612 ADC driver
>>>>>   arm64: dts: salvator-x: Add current sense amplifiers
>>>>>
>>>>>  .../ABI/testing/sysfs-bus-iio-adc-max961x          |   5 +
>>>>>  .../devicetree/bindings/iio/adc/max961x.txt        |  27 +
>>>>>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  17 +
>>>>>  drivers/iio/adc/Kconfig                            |  10 +
>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>  drivers/iio/adc/max961x.c                          | 648 +++++++++++++++++++++
>>>>>  6 files changed, 708 insertions(+)
>>>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max961x
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max961x.txt
>>>>>  create mode 100644 drivers/iio/adc/max961x.c
>>>>>
>>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html