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.

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));
+	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
+		st->channel[i] = chip_info->channel[i];
+		if (st->convst_gpio)
+			st->channel[i].info_mask_separate |=
+				BIT(IIO_CHAN_INFO_RAW);
+	}
+
 	st->spi = spi;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = chip_info->channel;
-	indio_dev->num_channels = 2;
+	indio_dev->channels = st->channel;
+	indio_dev->num_channels = ARRAY_SIZE(st->channel);
 	indio_dev->info = &ad7476_info;
 
-	if (st->convst_gpio)
-		indio_dev->channels = chip_info->convst_channel;
 	/* Setup default message */
 
 	st->xfer.rx_buf = &st->data;
-- 
2.50.1

On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
<mazziesaccount@gmail.com> 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

and adding

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

channel

(you already used 'channel spec' above, be consistent)

...

> -       int ret;
> +       int ret, i;

Why? Is 'i' going to be used to hold a signed value?

...

> +       /*
> +        * This will never realize. Unless someone changes the channel specs

realize --> happen

> +        * 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));

We have static_assert(). Why can't it be used?

-- 
With Best Regards,
Andy Shevchenko

On 08/08/2025 00:16, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> <mazziesaccount@gmail.com> 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
> 
> and adding
> 
>> 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.
> 
> channel
> 
> (you already used 'channel spec' above, be consistent)
> 
> ...
> 
>> -       int ret;
>> +       int ret, i;
> 
> Why? Is 'i' going to be used to hold a signed value?
> 
> ...
> 
>> +       /*
>> +        * This will never realize. Unless someone changes the channel specs
> 
> realize --> happen
> 
>> +        * 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));
> 
> We have static_assert(). Why can't it be used?

Don't know. Can you please enlighten me why one is preferred over the other?

Thanks for the review Andy!

Yours,
	-- Matti

On Fri, Aug 8, 2025 at 7:38 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 08/08/2025 00:16, Andy Shevchenko wrote:
> > On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:

...

> >> +       BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));
> >
> > We have static_assert(). Why can't it be used?
>
> Don't know. Can you please enlighten me why one is preferred over the other?

Despite already made changes, I answer to this. The static_assert()
has at least two benefits over BUILD_BUG_ON():
- it can be declared in a global scope
- it produces more condensed (to the point) error message

That's why in general it's preferable over BUILD_BUG_ON().

-- 
With Best Regards,
Andy Shevchenko

On 08/08/2025 15:52, Andy Shevchenko wrote:
> On Fri, Aug 8, 2025 at 7:38 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>> On 08/08/2025 00:16, Andy Shevchenko wrote:
>>> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>>>> +       BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));
>>>
>>> We have static_assert(). Why can't it be used?
>>
>> Don't know. Can you please enlighten me why one is preferred over the other?
> 
> Despite already made changes, I answer to this. The static_assert()
> has at least two benefits over BUILD_BUG_ON():
> - it can be declared in a global scope
> - it produces more condensed (to the point) error message
> 
> That's why in general it's preferable over BUILD_BUG_ON().

Thanks. It's always good to learn something new. One of the great things 
when working upstream :) (Although neither of those points seem to make 
a big difference here. Oh, and AFAIR, there was a variant of 
BUILD_BUG_ON which allows you to add a message(?))

Yours,
	-- Matti

On Fri, Aug 8, 2025 at 3:30 PM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
> On 08/08/2025 15:52, Andy Shevchenko wrote:
> > On Fri, Aug 8, 2025 at 7:38 AM Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >> On 08/08/2025 00:16, Andy Shevchenko wrote:
> >>> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> >>> <mazziesaccount@gmail.com> wrote:

...

> >>>> +       BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));
> >>>
> >>> We have static_assert(). Why can't it be used?
> >>
> >> Don't know. Can you please enlighten me why one is preferred over the other?
> >
> > Despite already made changes, I answer to this. The static_assert()
> > has at least two benefits over BUILD_BUG_ON():
> > - it can be declared in a global scope
> > - it produces more condensed (to the point) error message
> >
> > That's why in general it's preferable over BUILD_BUG_ON().
>
> Thanks. It's always good to learn something new. One of the great things
> when working upstream :) (Although neither of those points seem to make
> a big difference here.

I think the second one is the main point in my comment. Yes, the first
one doesn't matter.

> Oh, and AFAIR, there was a variant of
> BUILD_BUG_ON which allows you to add a message(?))

static_assert() can be one or two args, the second variant is with the
custom message added.

-- 
With Best Regards,
Andy Shevchenko

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

- 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));
> +	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
> +		st->channel[i] = chip_info->channel[i];
> +		if (st->convst_gpio)
> +			st->channel[i].info_mask_separate |=
> +				BIT(IIO_CHAN_INFO_RAW);
> +	}
> +
>  	st->spi = spi;
>  
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = chip_info->channel;
> -	indio_dev->num_channels = 2;
> +	indio_dev->channels = st->channel;
> +	indio_dev->num_channels = ARRAY_SIZE(st->channel);
>  	indio_dev->info = &ad7476_info;
>  
> -	if (st->convst_gpio)
> -		indio_dev->channels = chip_info->convst_channel;
>  	/* Setup default message */
>  
>  	st->xfer.rx_buf = &st->data;
> -- 
> 2.50.1
> 

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.

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 :)

> > +	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;
		
> > +			st->channel[i].info_mask_separate |=
> > +				BIT(IIO_CHAN_INFO_RAW);
		
		__set_bit()...

- Nuno Sá

> > +	}
> > +
> >  	st->spi = spi;
> >  
> >  	indio_dev->name = spi_get_device_id(spi)->name;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > -	indio_dev->channels = chip_info->channel;
> > -	indio_dev->num_channels = 2;
> > +	indio_dev->channels = st->channel;
> > +	indio_dev->num_channels = ARRAY_SIZE(st->channel);
> >  	indio_dev->info = &ad7476_info;
> >  
> > -	if (st->convst_gpio)
> > -		indio_dev->channels = chip_info->convst_channel;
> >  	/* Setup default message */
> >  
> >  	st->xfer.rx_buf = &st->data;
> > -- 
> > 2.50.1
> > 
> 
> 

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

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@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;

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.

Up to you now :)

- Nuno Sá

> 
> > 		
> > > > +			st->channel[i].info_mask_separate |=
> > > > +				BIT(IIO_CHAN_INFO_RAW);
> > 		
> > 		__set_bit()...
> 
> Ok. Thanks.
> 
> 
> Thanks for the review(s) Nuno!
> 
> Yours,
> 	-- Matti

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@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;
> 
> 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 :)

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

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