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

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: David Lechner <dlechner@baylibre.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Michael Hennerich" <michael.hennerich@analog.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Mark Brown" <broonie@kernel.org>,
	linux-kernel@vger.kernel.org,
	"Stefan Popa" <stefan.popa@analog.com>
Subject: Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
Date: Thu, 14 Dec 2023 10:14:15 +0000	[thread overview]
Message-ID: <20231214101415.0000060c@Huawei.com> (raw)
In-Reply-To: <20231213-ad7380-mainline-v2-3-cd32150d84a3@baylibre.com>

On Wed, 13 Dec 2023 05:21:20 -0600
David Lechner <dlechner@baylibre.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@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: David Lechner <dlechner@baylibre.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
> +};