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

[Matti Vaittinen]

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

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