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
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 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!
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
