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@xxxxxxxxxxxxxxx >>> 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@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
