Message-ID: <20170717223216.0000656d@huawei.com>
Date:   Mon, 17 Jul 2017 22:32:16 +0800
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Stefan Bruens <stefan.bruens@...h-aachen.de>
CC:     Jonathan Cameron <jic23@...nel.org>, <linux-iio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Hartmut Knaack <knaack.h@....de>,
        "Lars-Peter Clausen" <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        "Marc Titinger" <mtitinger@...libre.com>,
        <manivannanece23@...il.com>
Subject: Re: [PATCH 2/2] iio: adc: Allow setting Shunt Voltage PGA gain and
 Bus Voltage range

On Mon, 17 Jul 2017 20:04:57 +0800
Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:

> On Mon, 17 Jul 2017 00:08:32 +0200
> Stefan Bruens <stefan.bruens@...h-aachen.de> wrote:
> 
> > On Sonntag, 30. April 2017 18:19:39 CEST Jonathan Cameron wrote:  
> > > On 29/04/17 21:37, Stefan Bruens wrote:    
> > > > On Mittwoch, 26. April 2017 08:59:47 CEST Jonathan Cameron wrote:    
> > > >> On 26/04/17 07:19, Jonathan Cameron wrote:    
> > > >>> On 17/04/17 23:08, Stefan Bruens wrote:    
> > > >>>> On Freitag, 14. April 2017 17:12:03 CEST Jonathan Cameron wrote:    
> > > > [...]
> > > >     
> > > >>>> 4. Any user of the gain settings had to be made aware of the
> > > >>>> possibility
> > > >>>> to
> > > >>>> change it, no matter how it is exposed. Making it part of the scale,
> > > >>>> and
> > > >>>> thus changing the meaning of the raw values, would be breaking the
> > > >>>> existing ABI.>    
> > > >>> 
> > > >>> The raw values should indeed not change.  That was a missunderstanding
> > > >>> on
> > > >>> my part.  Usually when a device has a PGA it is not compensated for in
> > > >>> the output. So normally it's up to the driver to 'apply' the effective
> > > >>> gain to the incoming reading.  When that isn't the case, it can be
> > > >>> considered some sort of internal trim - hence the use of calibscale for
> > > >>> this case.    
> > > >> 
> > > >> Mulling this over, calibscale might not work either in this case. The
> > > >> datasheet helpfully sometimes uses ranges and sometimes uses scale
> > > >> factors.
> > > >> There is also obviously the calibration register kicking around which
> > > >> would
> > > >> also be handled with calibscale if exposed to userspace (currently it
> > > >> isn't)
> > > >> 
> > > >> I'm out of time tonight so will think it bit more about this and get back
> > > >> to you in the next few days...    
> > > > 
> > > > hardwaregain may be a viable option. For the shunt voltage, available
> > > > values would be [0.125, 0.25, 0.5, 1.0], for the bus range we would have
> > > > either [0.5, 1.0] or [1.0, 2.0] for bus ranges [32V, 16V].
> > > > 
> > > > Does hardwaregain have the right semantics for shunt voltage gain and/or
> > > > bus range?    
> > > 
> > > Description we currently have in
> > > Documentation/ABI/testing/sysfs-bus-iio is:
> > > 		Hardware applied gain factor. If shared across all channels,
> > > 		<type>_hardwaregain is used.
> > > 
> > > Just thinking about the use cases, it is mostly used for cases where the
> > > gain is not of the measurement being acquired, but rather of something
> > > related (like the gain on time of flight sensors or pulse counters).
> > > 
> > > It also gets used for output devices and amplifiers though so kind of
> > > similar as in those cases we felt calibrationscale was a bit of a stretch!
> > > 
> > > So yes, I can see that working.  Whether it is a better choice than
> > > simply allowing the range attributes (documented for this narrow
> > > case to say they should only be used when the range is independent of
> > > the scale) is an open question.  Given we have always preferred scales
> > > to ranges if you think you can make hardwaregain fit well then lets
> > > go with that, perhaps updating the docs to make this usecase explicit.
> > > 
> > > Looking back at the original emails we were actually thinking of
> > > transistioning calibscale to hardwaregain in general as it covered
> > > describing both uses, but it never happened...    
> > 
> > Hi John,
> > 
> > as all other patches for INA2xx went into or on their way into mainline, its 
> > time to revisit the INA219/220 bus range and shunt voltage gain again.
> > 
> > TLDR: Using HARDWAREGAIN fits existing uses/semantics.
> > 
> > I had a look at current users of IIO_CHAN_INFO_HARDWAREGAIN:
> > 
> > amplifiers/ad8366.c: Variable gain amplifier without ADC or DAC, so no SCALE 
> > attribute
> > 
> > light/vl6180.c: ToF sensor with ambient light sensor. The ALS sensor has two 
> > settings affecting the RAW sensor readout, HARDWAREGAIN and INTegration_TIME. 
> > Baseline settings are gain=1 and integration time=0.1(seconds), with a 
> > corresponding raw reading of 1 ^= 0.32 lux.
> > The SCALE value is correct for the baseline setting, but although modifying 
> > HARDWAREGAIN and/or INT_TIME affects the RAW readout, this is not reflected in 
> > the SCALE attribute, i.e. to get the correct physical value, one has to use:
> > Light[lux] = raw_value * SCALE * (0.1s/INT_TIME) / HARDWAREGAIN  
> This isn't right.  User space should be able to just apply the single scale
> value to get the correct real world value, not this complex interaction.
> 
> So I'd count this driver as buggy unfortunately. 
> 
> > 
> > light/adjd_s311.c: HARDWAREGAIN affects the RAW readout, but as there is no 
> > given fixed relationship between RAW values and irradiance, there is no SCALE 
> > attribute.  
> 
> > 
> > adc/stx104.c: The ADC has a software controllable HARDWAREGAIN and a hardware 
> > controlled (jumper) offset and single ended/differential setting with software 
> > readback. HARDWAREGAIN and offset/differential are reflected in the SCALE and 
> > OFFSET attributes, i.e. the physical value can be determined by:
> > U[V] = (raw_value * SCALE) + OFFSET
> > 
> > So we have two users of HARDWAREGAIN with contradicting behaviour regarding 
> > SCALE. IMHO, the stx104 behaviour is the correct one.  
> I go the other way, simply because we don't want to complicate the userspace
> interface if we don't have to.
Sorry, I was clearly talking rubbish here.

The stx104 is the right way.
> > 
> > For the INA2xx, neither INT_TIME nor AVERAGE affect the RAW <-> physical value 
> > relationship, i.e. the SCALE is fixed. The same is true for the INA219/220 bus 
> > range/shunt voltage gain. So using HARDWAREGAIN for both shunt voltage gain 
> > and bus voltage range does match existing semantics.  
> 
> I'm uncomfortable with applying a second scaling within all user space code.
> That should be handled in the kernel rather than pushing on the burden.
> It's not a fast path so doesn't matter if we have to some nasty maths to
> work out the right value.
Again complete rubbish.  I'll blame lack of coffee :(

If it's not effecting the output, then hardware gain is absolutely fine.

Jonathan
> 
> Jonathan
> > 
> > Kind regards,
> > 
> > Stefan
> >   
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists