Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

[Nuno Sá]

On Fri, Aug 08, 2025 at 12:09:21PM +0300, Matti Vaittinen wrote:
> On 08/08/2025 12:00, Nuno Sá wrote:
> > On Fri, Aug 08, 2025 at 08:37:07AM +0300, Matti Vaittinen wrote:
> > > On 07/08/2025 16:10, Nuno Sá wrote:
> > > > On Thu, Aug 07, 2025 at 01:41:31PM +0100, Nuno Sá wrote:
> > > > > On Thu, Aug 07, 2025 at 12:34:52PM +0300, Matti Vaittinen wrote:
> > > > > > The ad7476 driver defines separate chan_spec structures for operation
> > > > > > with and without convstart GPIO. At quick glance this may seem as if the
> > > > > > driver did provide more than 1 data-channel to users - one for the
> > > > > > regular data, other for the data obtained with the convstart GPIO.
> > > > > > 
> > > > > > The only difference between the 'convstart' and 'non convstart'
> > > > > > -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in
> > > > > > channel's flags.
> > > > > > 
> > > > > > We can drop the convstart channel spec, and related convstart macro, by
> > > > > > allocating a mutable per driver instance channel spec an adding the flag
> > > > > > in probe if needed. This will simplify the driver with the cost of added
> > > > > > memory consumption.
> > > > > > 
> > > > > > Assuming there aren't systems with very many ADCs and very few
> > > > > > resources, this tradeoff seems worth making.
> > > > > > 
> > > > > > Simplify the driver by dropping the 'convstart' channel spec and
> > > > > > allocating the chan spec for each driver instance.
> > > > > 
> > > > > I do not agree with this one. Looking at the diff, code does not look
> > > > > simpler to me...
> > > > 
> > > > Ok, on a second thought I'm ok with this. It makes adding devices easier
> > > > and (IIUC) for the one you're adding later we only have "convst_channel"
> > > > channels.
> > > 
> > > Yes, that's right. The BD79105 requires the convstart.
> > > 
> > > > On comment though...
> > > > 
> > > > > 
> > > > > - Nuno Sá
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > > > > > 
> > > > > > ---
> > > > > > Revision history:
> > > > > >    v1 => v2:
> > > > > >    - New patch
> > > > > > 
> > > > > > I considered squashing this change with the one limiting the chip_info
> > > > > > scope. Having this as a separate change should help reverting if someone
> > > > > > complains about the increased memory consumption though.
> > > > > > ---
> > > > > >    drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
> > > > > >    1 file changed, 18 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > > > > > index e97742912b8e..a30eb016c11c 100644
> > > > > > --- a/drivers/iio/adc/ad7476.c
> > > > > > +++ b/drivers/iio/adc/ad7476.c
> > > > > > @@ -29,8 +29,6 @@ struct ad7476_state;
> > > > > >    struct ad7476_chip_info {
> > > > > >    	unsigned int			int_vref_mv;
> > > > > >    	struct iio_chan_spec		channel[2];
> > > > > > -	/* channels used when convst gpio is defined */
> > > > > > -	struct iio_chan_spec		convst_channel[2];
> > > > > >    	void (*reset)(struct ad7476_state *);
> > > > > >    	bool				has_vref;
> > > > > >    	bool				has_vdrive;
> > > > > > @@ -41,6 +39,7 @@ struct ad7476_state {
> > > > > >    	struct gpio_desc		*convst_gpio;
> > > > > >    	struct spi_transfer		xfer;
> > > > > >    	struct spi_message		msg;
> > > > > > +	struct iio_chan_spec		channel[2];
> > > > > >    	int				scale_mv;
> > > > > >    	/*
> > > > > >    	 * DMA (thus cache coherency maintenance) may require the
> > > > > > @@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
> > > > > >    #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
> > > > > >    		BIT(IIO_CHAN_INFO_RAW))
> > > > > >    #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
> > > > > > -#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
> > > > > > -		BIT(IIO_CHAN_INFO_RAW))
> > > > > >    #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
> > > > > >    		BIT(IIO_CHAN_INFO_RAW))
> > > > > >    static const struct ad7476_chip_info ad7091_chip_info = {
> > > > > >    	.channel[0] = AD7091R_CHAN(12),
> > > > > >    	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> > > > > > -	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
> > > > > > -	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> > > > > >    	.reset = ad7091_reset,
> > > > > >    };
> > > > > >    static const struct ad7476_chip_info ad7091r_chip_info = {
> > > > > >    	.channel[0] = AD7091R_CHAN(12),
> > > > > >    	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> > > > > > -	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
> > > > > > -	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> > > > > >    	.int_vref_mv = 2500,
> > > > > >    	.has_vref = true,
> > > > > >    	.reset = ad7091_reset,
> > > > > > @@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
> > > > > >    	const struct ad7476_chip_info *chip_info;
> > > > > >    	struct ad7476_state *st;
> > > > > >    	struct iio_dev *indio_dev;
> > > > > > -	int ret;
> > > > > > +	int ret, i;
> > > > > >    	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > > > >    	if (!indio_dev)
> > > > > > @@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
> > > > > >    	if (IS_ERR(st->convst_gpio))
> > > > > >    		return PTR_ERR(st->convst_gpio);
> > > > > > +	/*
> > > > > > +	 * This will never realize. Unless someone changes the channel specs
> > > > > > +	 * in this driver. And if someone does, without changing the loop
> > > > > > +	 * below, then we'd better immediately produce a big fat error, before
5eefac72163e88cc6697bec77c54e4393788e1bf> > > > > > +	 * the change proceeds from that developer's table.
> > > > > > +	 */
> > > > > > +	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));
> > > > 
> > > > I guess it make sense but still looks too fancy for this :)
> > > 
> > > Nothing else but a developer's carefulness keeps the number of channels "in
> > > sync" for these two structs. I was originally doing WARN_ON() - but then I
> > > thought that it's be even better to catch this at build time. Then I found
> > > the BUILD_BUG_ON(). I see Andy suggested static_assert() instead - I've no
> > > idea why one is preferred over other though. Let's see if I get educated by
> > > Andy :)
> > > 
> > > > 
> > > > > > +	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
> > > > > > +		st->channel[i] = chip_info->channel[i];
> > > > > > +		if (st->convst_gpio)
> > > > 
> > > > I would flip this an do:
> > > > 	if (!st->convst_gpio)
> > > > 		break;
> > > 
> > > To me this would just add an extra line of code, and more complex flow. I
> > > would definitely agree if there were more operations to be done for the
> > > 'convstart channels' - but since this is really just "if it's convstart,
> > > then set a bit" - the
> > > 
> > > if (foo)
> > > 	bar;
> > > 
> > > seems simpler than
> > > 
> > > if (!foo)
> > > 	break;
> > > bar;
> > 
> > Yes but in this particular case, you likely would not need to do any
> > line break afterward because of indentation. Logically it also makes
> > sense because st->convst_gpio is a device property (not a channel one).
> > So it makes no sense to check it for all channels (I know we only have two
> > channels). So if you prefer, you could even do:
> > 
> > if (st->convst_gpio) {
> > 	for (...)
> > 		__set_bit(...);
> > }
> > 
> > which also would make more sense to me.
> 
> I considered this option, but I need to populate all the channels in
> st->channel with the template data from chip_info->channel anyways. Hence I
> want to loop through the channels also when the st->convst_gpio is not there
> :)

Ahh right! I completely missed that line:

st->channel[i] = chip_info->channel[i];

- Nuno Sá

> 
> > 
> > Up to you now :)
> 
> Well, I already sent the v3. (Sorry, I should've waited a bit more but
> wanted to get it out before the weekend). I kept the same logic as in v2.
> You can still suggest improvements there!
> 
> Yours,
> 	-- Matti