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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.com>
>>>
>>> ---
>>> 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
>>> +	 * 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;

> 		
>>> +			st->channel[i].info_mask_separate |=
>>> +				BIT(IIO_CHAN_INFO_RAW);
> 		
> 		__set_bit()...

Ok. Thanks.


Thanks for the review(s) Nuno!

Yours,
	-- Matti