lkml.org 
[lkml]   [2023]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] iio: adc: Add driver for TI ADS1100 and ADS1000 chips

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

\
 
 \ /
  Last update: 2023-03-27 00:35    [W:6.815 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site