Re: [PATCH v2 07/10] iio: adc: ad7476: Conditionally call convstart

[Nuno Sá]

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