The ad7476 supports two IC variants which may have a 'convstart' -GPIO
for starting the conversion. Currently the driver calls a function which
tries to access the GPIO for all of the IC variants, whether they
support 'convstart' or not. This is not an error because this function
returns early if GPIO information is not populated.

We can do a tad better by calling this function only for the ICs which
have the 'convstart' by providing a function pointer to the convstart
function from the chip_info structure, and calling this function only
for the ICs which have the function pointer set.

This does also allow to support ICs which require different convstart
handling than the currently supported ICs.

Call convstart function only on the ICs which can support it and allow
IC-specific convstart functions for the ICs which require different
handling.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
 v1 => v2:
 - Adapt to the change which removed the chip_info pointer from the
  driver's state structure.

The follow-up patch adding support for the ROHM BD79105 will bring
different 'convstart' functions in use. The IC specific pointer will
also prepare the way for this.
---
 drivers/iio/adc/ad7476.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
index a30eb016c11c..8914861802be 100644
--- a/drivers/iio/adc/ad7476.c
+++ b/drivers/iio/adc/ad7476.c
@@ -30,6 +30,7 @@ struct ad7476_chip_info {
 	unsigned int			int_vref_mv;
 	struct iio_chan_spec		channel[2];
 	void (*reset)(struct ad7476_state *);
+	void (*conversion_pre_op)(struct ad7476_state *st);
 	bool				has_vref;
 	bool				has_vdrive;
 };
@@ -37,6 +38,7 @@ struct ad7476_chip_info {
 struct ad7476_state {
 	struct spi_device		*spi;
 	struct gpio_desc		*convst_gpio;
+	void (*conversion_pre_op)(struct ad7476_state *st);
 	struct spi_transfer		xfer;
 	struct spi_message		msg;
 	struct iio_chan_spec		channel[2];
@@ -68,7 +70,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
 	struct ad7476_state *st = iio_priv(indio_dev);
 	int b_sent;
 
-	ad7091_convst(st);
+	if (st->conversion_pre_op)
+		st->conversion_pre_op(st);
 
 	b_sent = spi_sync(st->spi, &st->msg);
 	if (b_sent < 0)
@@ -158,12 +161,14 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
 static const struct ad7476_chip_info ad7091_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.conversion_pre_op = ad7091_convst,
 	.reset = ad7091_reset,
 };
 
 static const struct ad7476_chip_info ad7091r_chip_info = {
 	.channel[0] = AD7091R_CHAN(12),
 	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	.conversion_pre_op = ad7091_convst,
 	.int_vref_mv = 2500,
 	.has_vref = true,
 	.reset = ad7091_reset,
@@ -319,6 +324,7 @@ static int ad7476_probe(struct spi_device *spi)
 			return ret;
 	}
 
+	st->conversion_pre_op = chip_info->conversion_pre_op;
 	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
 						  "adi,conversion-start",
 						  GPIOD_OUT_LOW);
-- 
2.50.1

On Thu, Aug 07, 2025 at 12:35:03PM +0300, Matti Vaittinen wrote:
> The ad7476 supports two IC variants which may have a 'convstart' -GPIO
> for starting the conversion. Currently the driver calls a function which
> tries to access the GPIO for all of the IC variants, whether they
> support 'convstart' or not. This is not an error because this function
> returns early if GPIO information is not populated.
> 
> We can do a tad better by calling this function only for the ICs which
> have the 'convstart' by providing a function pointer to the convstart
> function from the chip_info structure, and calling this function only
> for the ICs which have the function pointer set.
> 
> This does also allow to support ICs which require different convstart
> handling than the currently supported ICs.
> 
> Call convstart function only on the ICs which can support it and allow
> IC-specific convstart functions for the ICs which require different
> handling.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> Revision history:
>  v1 => v2:
>  - Adapt to the change which removed the chip_info pointer from the
>   driver's state structure.
> 
> The follow-up patch adding support for the ROHM BD79105 will bring
> different 'convstart' functions in use. The IC specific pointer will
> also prepare the way for this.
> ---
>  drivers/iio/adc/ad7476.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> index a30eb016c11c..8914861802be 100644
> --- a/drivers/iio/adc/ad7476.c
> +++ b/drivers/iio/adc/ad7476.c
> @@ -30,6 +30,7 @@ struct ad7476_chip_info {
>  	unsigned int			int_vref_mv;
>  	struct iio_chan_spec		channel[2];
>  	void (*reset)(struct ad7476_state *);
> +	void (*conversion_pre_op)(struct ad7476_state *st);
>  	bool				has_vref;
>  	bool				has_vdrive;
>  };
> @@ -37,6 +38,7 @@ struct ad7476_chip_info {
>  struct ad7476_state {
>  	struct spi_device		*spi;
>  	struct gpio_desc		*convst_gpio;
> +	void (*conversion_pre_op)(struct ad7476_state *st);

Ok, I was going to reply to patch patch 5 saying I was not sure about
the change. And now this makes it clear. My point would be that it's
fairly easiy to end up needing chip info after probe. The above function
pointer only has to exist because of patch 5. So I would better drop
patch 5 and...

>  	struct spi_transfer		xfer;
>  	struct spi_message		msg;
>  	struct iio_chan_spec		channel[2];
> @@ -68,7 +70,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
>  	struct ad7476_state *st = iio_priv(indio_dev);
>  	int b_sent;
>  
> -	ad7091_convst(st);
> +	if (st->conversion_pre_op)
> +		st->conversion_pre_op(st);
>  
>  	b_sent = spi_sync(st->spi, &st->msg);
>  	if (b_sent < 0)
> @@ -158,12 +161,14 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
>  static const struct ad7476_chip_info ad7091_chip_info = {
>  	.channel[0] = AD7091R_CHAN(12),
>  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> +	.conversion_pre_op = ad7091_convst,
>  	.reset = ad7091_reset,
>  };
>  
>  static const struct ad7476_chip_info ad7091r_chip_info = {
>  	.channel[0] = AD7091R_CHAN(12),
>  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> +	.conversion_pre_op = ad7091_convst,
>  	.int_vref_mv = 2500,
>  	.has_vref = true,
>  	.reset = ad7091_reset,
> @@ -319,6 +324,7 @@ static int ad7476_probe(struct spi_device *spi)
>  			return ret;
>  	}
>  
> +	st->conversion_pre_op = chip_info->conversion_pre_op;

... no need for the above. There's no real reason for the above to be
done at runtime.

- Nuno Sá

>  	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
>  						  "adi,conversion-start",
>  						  GPIOD_OUT_LOW);
> -- 
> 2.50.1
> 

On 07/08/2025 15:47, Nuno Sá wrote:
> On Thu, Aug 07, 2025 at 12:35:03PM +0300, Matti Vaittinen wrote:
>> The ad7476 supports two IC variants which may have a 'convstart' -GPIO
>> for starting the conversion. Currently the driver calls a function which
>> tries to access the GPIO for all of the IC variants, whether they
>> support 'convstart' or not. This is not an error because this function
>> returns early if GPIO information is not populated.
>>
>> We can do a tad better by calling this function only for the ICs which
>> have the 'convstart' by providing a function pointer to the convstart
>> function from the chip_info structure, and calling this function only
>> for the ICs which have the function pointer set.
>>
>> This does also allow to support ICs which require different convstart
>> handling than the currently supported ICs.
>>
>> Call convstart function only on the ICs which can support it and allow
>> IC-specific convstart functions for the ICs which require different
>> handling.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> Revision history:
>>   v1 => v2:
>>   - Adapt to the change which removed the chip_info pointer from the
>>    driver's state structure.
>>
>> The follow-up patch adding support for the ROHM BD79105 will bring
>> different 'convstart' functions in use. The IC specific pointer will
>> also prepare the way for this.
>> ---
>>   drivers/iio/adc/ad7476.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
>> index a30eb016c11c..8914861802be 100644
>> --- a/drivers/iio/adc/ad7476.c
>> +++ b/drivers/iio/adc/ad7476.c
>> @@ -30,6 +30,7 @@ struct ad7476_chip_info {
>>   	unsigned int			int_vref_mv;
>>   	struct iio_chan_spec		channel[2];
>>   	void (*reset)(struct ad7476_state *);
>> +	void (*conversion_pre_op)(struct ad7476_state *st);
>>   	bool				has_vref;
>>   	bool				has_vdrive;
>>   };
>> @@ -37,6 +38,7 @@ struct ad7476_chip_info {
>>   struct ad7476_state {
>>   	struct spi_device		*spi;
>>   	struct gpio_desc		*convst_gpio;
>> +	void (*conversion_pre_op)(struct ad7476_state *st);
> 
> Ok, I was going to reply to patch patch 5 saying I was not sure about
> the change. And now this makes it clear. My point would be that it's
> fairly easiy to end up needing chip info after probe. The above function
> pointer only has to exist because of patch 5. So I would better drop
> patch 5 and...

Andy had the same comment. I personally like to only carry around stuff 
that is used after probe in the driver's private data. In my eyes it 
makes things clearer (and cleaner) as you know what is used. But yes, 
(also) here it leads to some duplication.

Well, I'll drop the patch 5.

Thanks!

Yours,
	-- Matti

On Fri, Aug 08, 2025 at 08:43:18AM +0300, Matti Vaittinen wrote:
> On 07/08/2025 15:47, Nuno Sá wrote:
> > On Thu, Aug 07, 2025 at 12:35:03PM +0300, Matti Vaittinen wrote:
> > > The ad7476 supports two IC variants which may have a 'convstart' -GPIO
> > > for starting the conversion. Currently the driver calls a function which
> > > tries to access the GPIO for all of the IC variants, whether they
> > > support 'convstart' or not. This is not an error because this function
> > > returns early if GPIO information is not populated.
> > > 
> > > We can do a tad better by calling this function only for the ICs which
> > > have the 'convstart' by providing a function pointer to the convstart
> > > function from the chip_info structure, and calling this function only
> > > for the ICs which have the function pointer set.
> > > 
> > > This does also allow to support ICs which require different convstart
> > > handling than the currently supported ICs.
> > > 
> > > Call convstart function only on the ICs which can support it and allow
> > > IC-specific convstart functions for the ICs which require different
> > > handling.
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > ---
> > > Revision history:
> > >   v1 => v2:
> > >   - Adapt to the change which removed the chip_info pointer from the
> > >    driver's state structure.
> > > 
> > > The follow-up patch adding support for the ROHM BD79105 will bring
> > > different 'convstart' functions in use. The IC specific pointer will
> > > also prepare the way for this.
> > > ---
> > >   drivers/iio/adc/ad7476.c | 8 +++++++-
> > >   1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > > index a30eb016c11c..8914861802be 100644
> > > --- a/drivers/iio/adc/ad7476.c
> > > +++ b/drivers/iio/adc/ad7476.c
> > > @@ -30,6 +30,7 @@ struct ad7476_chip_info {
> > >   	unsigned int			int_vref_mv;
> > >   	struct iio_chan_spec		channel[2];
> > >   	void (*reset)(struct ad7476_state *);
> > > +	void (*conversion_pre_op)(struct ad7476_state *st);
> > >   	bool				has_vref;
> > >   	bool				has_vdrive;
> > >   };
> > > @@ -37,6 +38,7 @@ struct ad7476_chip_info {
> > >   struct ad7476_state {
> > >   	struct spi_device		*spi;
> > >   	struct gpio_desc		*convst_gpio;
> > > +	void (*conversion_pre_op)(struct ad7476_state *st);
> > 
> > Ok, I was going to reply to patch patch 5 saying I was not sure about
> > the change. And now this makes it clear. My point would be that it's
> > fairly easiy to end up needing chip info after probe. The above function
> > pointer only has to exist because of patch 5. So I would better drop
> > patch 5 and...
> 
> Andy had the same comment. I personally like to only carry around stuff that
> is used after probe in the driver's private data. In my eyes it makes things
> clearer (and cleaner) as you know what is used. But yes, (also) here it
> leads to some duplication.

And also remember that like this you're pretending that const stuff needs to
be set at runtime which is really not the case.

- Nuno Sá

> 
> Well, I'll drop the patch 5.
> 
> Thanks!
> 
> Yours,
> 	-- Matti