linux-kernel - Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs

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: <20231214123613.00002b69@Huawei.com>
Date:   Thu, 14 Dec 2023 12:36:13 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     David Lechner <dlechner@...libre.com>
CC:     <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Michael Hennerich <michael.hennerich@...log.com>,
        Nuno Sá <nuno.sa@...log.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        <linux-kernel@...r.kernel.org>,
        Stefan Popa <stefan.popa@...log.com>
Subject: Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs

On Thu, 14 Dec 2023 11:33:51 +0100
David Lechner <dlechner@...libre.com> wrote:

> On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron
> <Jonathan.Cameron@...wei.com> wrote:
> >
> > On Wed, 13 Dec 2023 05:21:20 -0600
> > David Lechner <dlechner@...libre.com> wrote:
> >  
> > > This adds a new driver for the AD7380 family ADCs.
> > >
> > > The driver currently implements basic support for the AD7380, AD7381,
> > > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > > single-ended and 4-channel chips that use the same register map as well
> > > as additional features of the chip will be added in future patches.
> > >
> > > Co-developed-by: Stefan Popa <stefan.popa@...log.com>
> > > Signed-off-by: Stefan Popa <stefan.popa@...log.com>
> > > Signed-off-by: David Lechner <dlechner@...libre.com>  
> >
> > Just one additional comment.  I 'might' sort both this an Nuno's comment
> > if Mark is fine with the SPI and no on else has review comments.
> > Feel free to send a v3 though if you like ;)
> >
> >  
> > > +/* fully differential */
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
> > > +/* pseudo differential */
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
> > > +
> > > +/* Since this is simultaneous sampling, we don't allow individual channels. */
> > > +static const unsigned long ad7380_2_channel_scan_masks[] = {
> > > +     GENMASK(2, 0), /* both ADC channels and soft timestamp */
> > > +     GENMASK(1, 0), /* both ADC channels, no timestamp */  
> >
> > https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567
> > See the comment (added recently!)  
> 
> I did see this comment but this is already sorted in order of
> preference, so I'm not sure why you are calling it out. Just FYI, I
> guess?

No. Order of preference would be turn on the minimal if that is enough.
First item is the highest preference (if the requested channels are a subset of
that we don't look any further).  Here that means we always stop on the first
entry and never look at the second.

> 
> >
> > Also, if I remember how this works correctly there is no need to include
> > the timestamp in the mask.  We do special handling for it to avoid having to double
> > the number of provided masks.  The details being that it uses
> > iio_scan_el_ts_store rather than iio_scan_el_Store.  
> 
> Indeed. I've been working ahead on adding more features and noticed
> this. So we will need to find a way to say that we the timestamp
> should not be allowed under certain conditions. But that will be a
> discussion for a later series.

Interesting - you have cases where it's not valid at all?
It sometimes becomes inaccurate because we are interpolating across
data from a fifo, but I've not seen a case where we can't provide anything
useful.  Ah well - as you say I'll wait for that later series!

Jonathan

> 
> >
> > So as you have it I think you'll always end up with the first entry
> > and that will include a bonus bit that isn't a problem as it will match
> > anyway.
> >
> > So just have the second entry and 0.
> >
> > Jonathan
> >  
> > > +     0
> > > +};