Re: [PATCH v2 09/10] iio: adc: ad7476: Support ROHM BD79105

[Nuno Sá]

On Fri, Aug 08, 2025 at 09:11:03AM +0300, Matti Vaittinen wrote:
> On 07/08/2025 16:01, Nuno Sá wrote:
> > On Thu, Aug 07, 2025 at 12:35:25PM +0300, Matti Vaittinen wrote:
> > > The ROHM BD79105 is a simple 16-bit ADC accessible via SPI*.
> > > 
> > > The BD79105 has a CONVSTART pin, which must be set high to start the ADC
> > > conversion. Unlike with the ad7091 and ad7091r which also have a
> > > CONVSTART pin, the BD79105 requires that the pin must remain high also
> > > for the duration of the SPI access.
> > > 
> > > (*) Couple of words about the SPI. The BD79105 has pins named as
> > > CONVSTART, SCLK, DIN and DOUT. For the curious reader, DIN is not SPI
> > > ISO.
> > > 
> > > DIN is a signal which can be used as a chip-select. When DIN is pulled
> > > low, the ADC will output the completed measurement via DOUT as SCLK is
> > > clocked. According to the data-sheet, the DIN can also be used for
> > > daisy-chaining multiple ADCs. Furthermore, DOUT can be used also for a
> > > 'data-ready' -IRQ. These modes aren't supported by this driver.
> > > 
> > > Support reading ADC scale and data from the BD79105 using SPI, when DIN
> > > is used as a chip-select.
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > > ---
> > > Revision history:
> > >   v1 => v2:
> > >   - Fix the conversion delay for the BD79105
> > >   - Drop unnecessary GPIO check from the convstart disable
> > >   - Drop unintended whitespace change
> > >   - Fix spelling
> > > ---
> > 
> > IIUC, for this chip the CONV GPIO is actually mandatory no?
> 
> Yes. You're right.
> 
> > If so, we
> > should likely fail probe in case there's no GPIO. And we could also change> the dt bindings accordingly.
> 
> I did change the dt-binding (patch 8/10):
> +  # Devices with a convstart GPIO where it is not optional
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - rohm,bd79105
> +    then:
> +      required:
> +        - adi,conversion-start-gpios
> +
> 
> I didn't want to complicate the probe with extra checks for the GPIO based
> on the IC-type. But I am having second thoughts - maybe it is the right
> thing to do as you say :) Thanks!
> 
> > Some more comments inline...
> > >   drivers/iio/adc/ad7476.c | 35 +++++++++++++++++++++++++++++++++++
> > >   1 file changed, 35 insertions(+)
> > > 
> > > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > > index 8914861802be..aa8a522633eb 100644
> > > --- a/drivers/iio/adc/ad7476.c
> > > +++ b/drivers/iio/adc/ad7476.c
> > > @@ -31,6 +31,7 @@ struct ad7476_chip_info {
> > >   	struct iio_chan_spec		channel[2];
> > >   	void (*reset)(struct ad7476_state *);
> > >   	void (*conversion_pre_op)(struct ad7476_state *st);
> > > +	void (*conversion_post_op)(struct ad7476_state *st);
> > >   	bool				has_vref;
> > >   	bool				has_vdrive;
> > >   };
> > > @@ -39,6 +40,7 @@ struct ad7476_state {
> > >   	struct spi_device		*spi;
> > >   	struct gpio_desc		*convst_gpio;
> > >   	void (*conversion_pre_op)(struct ad7476_state *st);
> > > +	void (*conversion_post_op)(struct ad7476_state *st);
> > 
> > Pointer duplication again :)
> > 
> > >   	struct spi_transfer		xfer;
> > >   	struct spi_message		msg;
> > >   	struct iio_chan_spec		channel[2];
> > > @@ -63,6 +65,21 @@ static void ad7091_convst(struct ad7476_state *st)
> > >   	udelay(1); /* Conversion time: 650 ns max */
> > >   }
> > > +static void bd79105_convst_disable(struct ad7476_state *st)
> > > +{
> > > +	gpiod_set_value(st->convst_gpio, 0);
> > > +}
> > > +
> > > +static void bd79105_convst_enable(struct ad7476_state *st)
> > > +{
> > > +	if (!st->convst_gpio)
> > > +		return;
> > 
> > I think the pattern for optional GPIOs is to just call
> > gpiod_set_value_*() and the lib handles NULL pointers. Also the above is
> > not coeherent with bd79105_convst_disable().
> 
> I definitely don't want to do *delay() if there is no reason. Haven't
> checked the code lately, but I suppose the ndelay() is a busy-wait, blocking
> _everything_ on the core it is executed.
> 
> I dropped the check from the _disable() variant since it doesn't call the
> delay().

It's a valid point but...

> 
> But now that you (and Andy) have commented on these checks...
> 
> (even though I don't really think these checks are THAT bad. It's almost as
> if there were some reviewer's "unconditionally comment this"-list where NULL
> check for the GPIO API's was written ;) These check's are quick and very
> clear, and they avoid a blocking busy-wait)
> 
> ...I see two other options. One is adding the check in probe as you suggest.

I do think this is the right approach. We should make sure no one tries
to probe this device without any gpio because it will be pretty much
useless so better to fail probe in the first place. I'm also not sure
it's that complicated. Maybe just a chip_info flag like
'convgpio_mandatory' (likelly a bad name) and act accordingly when
checking the return value.

- Nuno Sá

> This check will however be substantially more complicated code than this
> NULL check here, because it needs to be performed only for the ICs which
> _require_ the convstart. Good thing is that it will error-out early and
> clearly, whereas current solution will just lead bogus values to be read if
> convstart is not correctly populated.
> 
> Other option would be making the conversion_*_op to return an error. I would
> still keep the NULL check but the bd79105_convst_enable() could error out.
> Delay would be avoided, user would get the error notification and
> bd79105_convst_disable() wouldn't get called.
> 
> TLDR; I will see how the "check in probe" you suggested plays out. Then I
> can omit these checks here :)
> 
> > 
> > > +
> > > +	gpiod_set_value(st->convst_gpio, 1);
> > 
> > gpiod_set_value_cansleep()? I do see the driver is calling the same API
> > in other places but I do not see a reason for it... So, precursor patch?
> 
> Ah. Great catch. *kicking himself*. I should've noticed...
> 
> Thanks!
> 
> Yours,
> 	-- Matti