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: <20231214101415.0000060c@Huawei.com>
Date:   Thu, 14 Dec 2023 10:14:15 +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 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!)

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.

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
> +};