Re: [PATCH 2/2] iio: adc: Add driver for TI ADS1100 and ADS1000 chips

[Mike Looijmans <mike.looijmans@topic.nl>]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 25-02-2023 18:01, Jonathan Cameron wrote:
> On Sat, 25 Feb 2023 12:03:05 +0100
> Mike Looijmans <mike.looijmans@topic.nl> wrote:
>
>> Comments below - mailserver has a top-post fetish and will inject a
>> signature here somewhere...
>>
>> No further comment from me means "agree, will implement in v2"...
> One 'nice to have' when replying where you have chunks like that
> is to just crop them out so it's easier to spot the interesting bits.
>
> I've done that below.
>
>> On 18-02-2023 17:48, Jonathan Cameron wrote:
>>> On Fri, 17 Feb 2023 10:31:28 +0100
>>> Mike Looijmans <mike.looijmans@topic.nl> wrote:
>>>   
>>>> The ADS1100 is a 16-bit ADC (at 8 samples per second).
>>>> The ADS1000 is similar, but has a fixed data rate.
>>>>
>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>> Hi Mike,
>>>
>>> I probably overlapped a lot in here with what Andy said, so just ignore
>>> anything you've already fixed.
>>>
>>> Biggest thing is HARDWAREGAIN.
>>> That is very rarely used with ADCs.  It exists to support cases where
>>> the gain is not directly coupled to the thing being measured (light
>>> sensitivity of a time of flight sensor for example).  Userspace expects
>>> to use SCALE to both control amplifier gains and to multiply with
>>> the _raw value to get a value in the real world units.
>>>
>>> It's a bit fiddly to do the computation, but typically at start up time
>>> you work out what the combination of each PGA gain and the reference
>>> voltage means for the scales available and stash that somewhere to use later.
>> Complicating factor with this ADC is that the final gain value depends
>> on the sampling rate as well as the power supply voltage. Which would
>> lead to the "available" list being different after changing the sample
>> rate and confusing userspace. If a userspace app would read the
>> available sample rates and gains, and then proceeded to set them, the
>> results would be weird, as the order in which they were set would
>> matter. Setting the gain after setting the sample rate would likely
>> result in an EINVAL error because the selected gain is no longer applicable.
>>
>> To me it seemed more logical to provide the analog amplification and
>> sample rate as separate, unrelated values.
> It may be logical, but it isn't how the IIO ABI has ever worked and it doesn't
> extend to more complex cases.  It's in general true that a PGA will result
> in a change to the scale that userspace needs to apply. There are devices
> where it doesn't. There are lots of things that at first glance 'could'
> affect scale but often do it in complex non linear ways that userspace
> simply can't know about - hence if we are pushing calculations into userspace
> we need it to just be (_raw + _offset) * _scale.
> Note that there is some wiggle room with how raw "raw" is.
>
> There are two solutions that have been taken in drivers.
> 1) The above software flow is broken as any ABI write in IIO is allowed
>     to affect other ABI elements.  This is less than ideal though.
> 2) Let the scale free float.  So the attempt is to keep as close as possible
>     to the set value as other things change (i.e. the sampling frequency).
>     The scale_avail reflects current settings of everything else, and indeed
>     changes with other ABI wirtes (this is quite common) but the interface is
>     made more intuitive by matching as closely as possible. Thus if you change the
>     sampling rate and the scale changes then you attempt to modify the PGA
>     to keep it approximately the same.  Obviously it clamps at end points
>     but nothing we can do about that.
>
> However, having said that, I don't 'think' we need either here...
> A common thing to do for scale vs sampling rate (which is typically
> either oversampling based, or based on another SAR cycle) is to just shift
> the raw data to incorporate it - a common sensor design is to justify it
> so that the unused bits are the LSBs - so may be a case of just not shifting
> it to compensate for the datarate change.. That's not true here if I read
> the datasheet correctly, but a simple
> sysfs raw read value == raw_value << (16 - bits for data rate) should fix that.

I agree, a bit of shifting (pun intended) is by far the better solution.

> Interesting the datasheet argues they deliberately right aligned and sign extended
> to allow oversampling without shifts, though counter argument is they made everyone
> who wants a real scale apply a shift.  I guess it depends on the expected use case.

My guess is that the chip internally always runs at 128Hz and simply 
adds the sampled values together in a 16-bit register for the lower 
sampling rates. Someone came up with that datasheet text later on.

>>>> +
>>>> +static const struct iio_chan_spec ads1100_channel = {
>>>> +	.type = IIO_VOLTAGE,
>>>> +	.differential = 0,
>>>> +	.indexed = 0,
>>>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> +	.info_mask_shared_by_all =
>>>> +				BIT(IIO_CHAN_INFO_SCALE) |
>>>> +				BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>>>> +				BIT(IIO_CHAN_INFO_SAMP_FREQ),
>>>> +	.info_mask_shared_by_all_available =
>>>> +				BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
>>> Hardware gain is normally only used for cases where the gain is not
>>> directly related to scale.  Things like the gain on a signal that is
>>> being used for measuring time of flight.  Here you should probably
>>> just be using SCALE.
>> In this case, SCALE depends on SAMP_FREQ as well as GAIN. Which will be
>> very confusing to userspace.
>>
>> Hoping for some leeway here...
> Sorry no. Though I think applying the shift as mentioned above deals
> with your data rate dependent scaling and makes this all a lot easier.

True.

...
>>>> +	iio_device_unregister(indio_dev);
>>>> +
>>>> +	ads1100_set_conv_mode(data, ADS1100_SINGLESHOT);
>>>> +
>>>> +	pm_runtime_disable(&client->dev);
>>> I'd expect runtime pm to be disable before messing with the conv mode.
>>> With a little care you can use devm_runtime_pm_enable()
>> Setting the conv mode involves I2C traffic. After runtime_disable, the
>> power supply to the chip may have been disabled, leading to
>> communication errors on the I2C bus. Hence I thought it appropriate to
>> write the config register before turning off power.
> pm_runtime_disable() is disabling the runtime management of the power
> not the power itself.  That you need to do after turning off the
> management (thus avoiding any races)

Ah. But does that mean I'd need to disable the Vdd power supply here as 
well?

-- 
Mike Looijmans