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@gmail.com>
---
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
---
 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);
 	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;
+
+	gpiod_set_value(st->convst_gpio, 1);
+	/* Worst case, 2790 nS required for conversion */
+	ndelay(2790);
+}
+
 static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
 {
 	struct iio_poll_func *pf = p;
@@ -80,6 +97,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
 	iio_push_to_buffers_with_ts(indio_dev, st->data, sizeof(st->data),
 				    iio_get_time_ns(indio_dev));
 done:
+	if (st->conversion_post_op)
+		st->conversion_post_op(st);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -271,6 +290,20 @@ static const struct ad7476_chip_info ltc2314_14_chip_info = {
 	.has_vref = true,
 };
 
+static const struct ad7476_chip_info bd79105_chip_info = {
+	.channel[0] = AD7091R_CHAN(16),
+	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
+	/*
+	 * The BD79105 starts ADC data conversion when the CONVSTART line is
+	 * set HIGH. The CONVSTART must be kept HIGH until the data has been
+	 * read from the ADC.
+	 */
+	.conversion_pre_op = bd79105_convst_enable,
+	.conversion_post_op = bd79105_convst_disable,
+	.has_vref = true,
+	.has_vdrive = true,
+};
+
 static const struct iio_info ad7476_info = {
 	.read_raw = &ad7476_read_raw,
 };
@@ -325,6 +358,7 @@ static int ad7476_probe(struct spi_device *spi)
 	}
 
 	st->conversion_pre_op = chip_info->conversion_pre_op;
+	st->conversion_post_op = chip_info->conversion_post_op;
 	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
 						  "adi,conversion-start",
 						  GPIOD_OUT_LOW);
@@ -400,6 +434,7 @@ static const struct spi_device_id ad7476_id[] = {
 	{ "ads7866", (kernel_ulong_t)&ads7866_chip_info },
 	{ "ads7867", (kernel_ulong_t)&ads7867_chip_info },
 	{ "ads7868", (kernel_ulong_t)&ads7868_chip_info },
+	{ "bd79105", (kernel_ulong_t)&bd79105_chip_info },
 	/*
 	 * The ROHM BU79100G is identical to the TI's ADS7866 from the software
 	 * point of view. The binding document mandates the ADS7866 to be
-- 
2.50.1

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

...

> +static void bd79105_convst_enable(struct ad7476_state *st)
> +{
> +       if (!st->convst_gpio)
> +               return;

Still consider this unneeded churn. 3us delay is tolerable in almost
any setup with this driver.

> +       gpiod_set_value(st->convst_gpio, 1);
> +       /* Worst case, 2790 nS required for conversion */

nS --> ns (SI unit for seconds is 's')

> +       ndelay(2790);
> +}

...

> +       /*
> +        * The BD79105 starts ADC data conversion when the CONVSTART line is
> +        * set HIGH. The CONVSTART must be kept HIGH until the data has been
> +        * read from the ADC.

Is this terminology in absolute levels of the pin or logical ones
(that implied active-low)? If it's the latter, use active/inactive
instead as the GPIO subsystem does.

> +        */

-- 
With Best Regards,
Andy Shevchenko

On 08/08/2025 00:28, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 11:35 AM Matti Vaittinen
> <mazziesaccount@gmail.com> 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.
> 
> ...
> 
>> +static void bd79105_convst_enable(struct ad7476_state *st)
>> +{
>> +       if (!st->convst_gpio)
>> +               return;
> 
> Still consider this unneeded churn. 3us delay is tolerable in almost
> any setup with this driver.

Very simple and 100% clear if (), which avoids uninterruptible 
busy-wait. I _really_ 100% disagree with you.

Luckily Nuno suggested a solution which I think will mitigate the need 
to fight over this. :)


>> +       gpiod_set_value(st->convst_gpio, 1);
>> +       /* Worst case, 2790 nS required for conversion */
> 
> nS --> ns (SI unit for seconds is 's')
> 
>> +       ndelay(2790);
>> +}
> 
> ...
> 
>> +       /*
>> +        * The BD79105 starts ADC data conversion when the CONVSTART line is
>> +        * set HIGH. The CONVSTART must be kept HIGH until the data has been
>> +        * read from the ADC.
> 
> Is this terminology in absolute levels of the pin or logical ones
> (that implied active-low)? If it's the latter, use active/inactive
> instead as the GPIO subsystem does.

Ah. This is electrical high. I have:
adi,conversion-start-gpios = <&gpio2 2 GPIO_ACTIVE_HIGH>;
in the DT. Open to suggestions how to make this more obvious. Thanks!

Yours,
	-- Matti

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@gmail.com>
> ---
> 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? If so, we
should likely fail probe in case there's no GPIO. And we could also change
the dt bindings accordingly.

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

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

- Nuno Sá

> +	/* Worst case, 2790 nS required for conversion */
> +	ndelay(2790);
> +}
> +
>  static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -80,6 +97,8 @@ static irqreturn_t ad7476_trigger_handler(int irq, void  *p)
>  	iio_push_to_buffers_with_ts(indio_dev, st->data, sizeof(st->data),
>  				    iio_get_time_ns(indio_dev));
>  done:
> +	if (st->conversion_post_op)
> +		st->conversion_post_op(st);
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
> @@ -271,6 +290,20 @@ static const struct ad7476_chip_info ltc2314_14_chip_info = {
>  	.has_vref = true,
>  };
>  
> +static const struct ad7476_chip_info bd79105_chip_info = {
> +	.channel[0] = AD7091R_CHAN(16),
> +	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> +	/*
> +	 * The BD79105 starts ADC data conversion when the CONVSTART line is
> +	 * set HIGH. The CONVSTART must be kept HIGH until the data has been
> +	 * read from the ADC.
> +	 */
> +	.conversion_pre_op = bd79105_convst_enable,
> +	.conversion_post_op = bd79105_convst_disable,
> +	.has_vref = true,
> +	.has_vdrive = true,
> +};
> +
>  static const struct iio_info ad7476_info = {
>  	.read_raw = &ad7476_read_raw,
>  };
> @@ -325,6 +358,7 @@ static int ad7476_probe(struct spi_device *spi)
>  	}
>  
>  	st->conversion_pre_op = chip_info->conversion_pre_op;
> +	st->conversion_post_op = chip_info->conversion_post_op;
>  	st->convst_gpio = devm_gpiod_get_optional(&spi->dev,
>  						  "adi,conversion-start",
>  						  GPIOD_OUT_LOW);
> @@ -400,6 +434,7 @@ static const struct spi_device_id ad7476_id[] = {
>  	{ "ads7866", (kernel_ulong_t)&ads7866_chip_info },
>  	{ "ads7867", (kernel_ulong_t)&ads7867_chip_info },
>  	{ "ads7868", (kernel_ulong_t)&ads7868_chip_info },
> +	{ "bd79105", (kernel_ulong_t)&bd79105_chip_info },
>  	/*
>  	 * The ROHM BU79100G is identical to the TI's ADS7866 from the software
>  	 * point of view. The binding document mandates the ADS7866 to be
> -- 
> 2.50.1
> 

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@gmail.com>
>> ---
>> 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

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@gmail.com>
> > > ---
> > > 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

On 08/08/2025 11:54, Nuno Sá wrote:
> 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@gmail.com>
>>>> ---
>>>> 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
>>>> ---
>>>

...

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

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

Just sent v3 couple of minutes ago, and this was exactly what I did. 
(although with another name for the flag).

Thanks! :)

Yours,
	-- Matti