linux-kernel - Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240506144616.0b90664b@jic23-huawei>
Date: Mon, 6 May 2024 14:46:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Julien Stephan <jstephan@...libre.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, Nuno
 Sá <nuno.sa@...log.com>, David Lechner
 <dlechner@...libre.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 kernel test robot <lkp@...el.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for
 resolution boost

On Mon, 06 May 2024 10:55:46 +0200
Nuno Sá <noname.nuno@...il.com> wrote:

> On Wed, 2024-05-01 at 16:55 +0200, Julien Stephan wrote:
> > ad738x chips are able to use an additional 2 bits of resolution when
> > using oversampling. The 14-bits chips can have up to 16 bits of
> > resolution and the 16-bits chips can have up to 18 bits of resolution.
> > 
> > In order to dynamically allow to enable/disable the resolution boost
> > feature, we have to set iio realbits/storagebits to the maximum per chips.
> > This means that for iio, data will always be on the higher resolution
> > available, and to cope with that we adjust the iio scale and iio offset
> > depending on the resolution boost status.
> > 
> > The available scales can be displayed using the regular _scale_available
> > attributes and can be set by writing the regular _scale attribute.
> > The available scales depend on the oversampling status.
> > 
> > Signed-off-by: Julien Stephan <jstephan@...libre.com>
> > 
> > ---
> > 
> > In order to support the resolution boost (additional 2 bits of resolution)
> > we need to set realbits/storagebits to the maximum value i.e :
> >   - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> >   - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
> > 
> > For 14-bits chips this does not have a major impact, but this
> > has the drawback of forcing 16-bits chip users to always use 32-bits
> > words in buffers even if they are not using oversampling and resolution
> > boost. Is there a better way of implementing this? For example
> > implementing dynamic scan_type?
> >   
> 
> Yeah, I don't think it's that bad in this case. But maybe dynamic scan types is
> something we may need at some point yes (or IOW that I would like to see supported
> :)). We do some ADCs (eg: ad4630) where we use questionably use FW properties to set
> a specific operating mode exactly because we have a different data layout (scan
> elements) depending on the mode.

Yeah. Fixed scan modes were somewhat of a bad design decision a long time back.
However, the big advantage is that it got people to think hard about whether it is
worth supporting low precision modes. For slow devices it very rarely is and
forcing people to make a decision and the advantage we never supported
low precision on those parts.

Having said that there are good reasons for dynamic resolution changing
(the main one being the storage case you have here) so I'd be happy to
see some patches adding it. It might be easier than I've always thought
to bolt on.

This and inkernel event consumers have been the two significant features
where I keep expecting it to happen, but every time people decide they really
don't care enough to make them work :(

>  
> > Another issue is the location of the timestamps. I understood the need
> > for ts to be consistent between chips, but right now I do not have a
> > better solution.. I was thinking of maybe adding the timestamps at the
> > beginning? That would imply to change the iio_chan_spec struct and maybe
> > add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> > that an option?

You have lost me on this one.  Why does the timestamp need to be in a consistent
location?  We have lots of drivers where it moves about between different
chips they support, or indeed what channels are currently turned on.

I haven't actually looked at the latest code yet, so maybe it will become
obvious!

Jonathan