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

[David Lechner]

On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Wed, 13 Dec 2023 05:21:20 -0600
> David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Stefan Popa <stefan.popa@xxxxxxxxxx>
> > Signed-off-by: David Lechner <dlechner@xxxxxxxxxxxx>
>
> 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?

>
> 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.

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