staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

[PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Taha Narimani 4 days, 18 hours ago

The driver currently utilizes devm_gpiod_get() for the 'busy' line,
which makes the GPIO mandatory. However, the busy pin is hardware-optional
depending on the specific board configuration.

Switch to devm_gpiod_get_optional() to allow boards that do not have
this pin wired up to still probe the driver successfully, and remove
the redundant conditional chip-ID check since the optional API handles
missing descriptors gracefully.

Signed-off-by: Taha Narimani <tahanarimani3443@gmail.com>
---
Changes in v2:
  - Fixed trailing whitespace and missing newline at the end of the file.
  - Converted the file format to Unix (LF) to remove carriage returns.
  - Removed the explicit chip-ID check around the busy pin logic.
  - Improved the commit message to provide clear architectural justification.

 drivers/staging/iio/adc/ad7816.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
index 0eac484..039b34d 100644
--- a/drivers/staging/iio/adc/ad7816.c
+++ b/drivers/staging/iio/adc/ad7816.c
@@ -84,7 +84,7 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
 		gpiod_set_value(chip->convert_pin, 1);
 	}
 
-	if (chip->id == ID_AD7817) {
+	if (chip->busy_pin) {
 		while (gpiod_get_value(chip->busy_pin))
 			cpu_relax();
 	}
@@ -380,15 +380,14 @@ static int ad7816_probe(struct spi_device *spi_dev)
 			ret);
 		return ret;
 	}
-	if (chip->id == ID_AD7817) {
-		chip->busy_pin = devm_gpiod_get(&spi_dev->dev, "busy",
-						GPIOD_IN);
-		if (IS_ERR(chip->busy_pin)) {
-			ret = PTR_ERR(chip->busy_pin);
-			dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
-				ret);
-			return ret;
-		}
+
+	chip->busy_pin = devm_gpiod_get_optional(&spi_dev->dev, "busy",
+						 GPIOD_IN);
+	if (IS_ERR(chip->busy_pin)) {
+		ret = PTR_ERR(chip->busy_pin);
+		dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
+			ret);
+		return ret;
 	}
 
 	indio_dev->name = spi_get_device_id(spi_dev)->name;
-- 
2.53.0

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Jonathan Cameron 4 days, 20 hours ago

On Wed,  3 Jun 2026 12:33:33 +0000
Taha Narimani <tahanarimani3443@gmail.com> wrote:

> The driver currently utilizes devm_gpiod_get() for the 'busy' line,
> which makes the GPIO mandatory. However, the busy pin is hardware-optional
> depending on the specific board configuration.
> 
> Switch to devm_gpiod_get_optional() to allow boards that do not have
> this pin wired up to still probe the driver successfully, and remove
> the redundant conditional chip-ID check since the optional API handles
> missing descriptors gracefully.
> 
> Signed-off-by: Taha Narimani <tahanarimani3443@gmail.com>
I tried to pick this up, but it doesn't apply. I suspect that's because
you've put it on top of your previous patch which modified the checks on chip->id

Please send it as a single patch. Also this is fixing a false assumption in the
driver so it should have an appropriate Fixes tag.

Trivial comment inline.

Thanks,

Jonathan

> ---
> Changes in v2:
>   - Fixed trailing whitespace and missing newline at the end of the file.
>   - Converted the file format to Unix (LF) to remove carriage returns.
>   - Removed the explicit chip-ID check around the busy pin logic.
>   - Improved the commit message to provide clear architectural justification.
> 
>  drivers/staging/iio/adc/ad7816.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 0eac484..039b34d 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -84,7 +84,7 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  		gpiod_set_value(chip->convert_pin, 1);
>  	}
>  
> -	if (chip->id == ID_AD7817) {
> +	if (chip->busy_pin) {
>  		while (gpiod_get_value(chip->busy_pin))
>  			cpu_relax();
>  	}
> @@ -380,15 +380,14 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  			ret);
>  		return ret;
>  	}
> -	if (chip->id == ID_AD7817) {
> -		chip->busy_pin = devm_gpiod_get(&spi_dev->dev, "busy",
> -						GPIOD_IN);
> -		if (IS_ERR(chip->busy_pin)) {
> -			ret = PTR_ERR(chip->busy_pin);
> -			dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
> -				ret);
> -			return ret;
> -		}
> +
> +	chip->busy_pin = devm_gpiod_get_optional(&spi_dev->dev, "busy",
> +						 GPIOD_IN);
Trivial:  We are more relaxed on line lengths these days so for cases like this
where it would only go a little past 80 chars to have it on one line I would
generally prefer it that way.

> +	if (IS_ERR(chip->busy_pin)) {
> +		ret = PTR_ERR(chip->busy_pin);
> +		dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
> +			ret);
> +		return ret;
>  	}
>  
>  	indio_dev->name = spi_get_device_id(spi_dev)->name;

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Andy Shevchenko 4 days, 21 hours ago

On Wed, Jun 03, 2026 at 12:33:33PM +0000, Taha Narimani wrote:
> The driver currently utilizes devm_gpiod_get() for the 'busy' line,
> which makes the GPIO mandatory. However, the busy pin is hardware-optional
> depending on the specific board configuration.
> 
> Switch to devm_gpiod_get_optional() to allow boards that do not have
> this pin wired up to still probe the driver successfully, and remove
> the redundant conditional chip-ID check since the optional API handles
> missing descriptors gracefully.

...

> -	if (chip->id == ID_AD7817) {
> +	if (chip->busy_pin) {

If we get GPIO optional, this check wouldn't be necessary anymore as the below
should return 0 IIRC in this case.

>  		while (gpiod_get_value(chip->busy_pin))
>  			cpu_relax();
>  	}

> +	chip->busy_pin = devm_gpiod_get_optional(&spi_dev->dev, "busy",
> +						 GPIOD_IN);

Make it a single line. Perhaps with a help of

	struct device *dev = &spi_dev->dev;

added to the top of the function.

> +	if (IS_ERR(chip->busy_pin)) {
> +		ret = PTR_ERR(chip->busy_pin);
> +		dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
> +			ret);
> +		return ret;

You can't do that, it will spam bootlog very quickly if this GPIO is deferred
and never appears. The proper way is to

		return dev_err_probe(dev, PTR_ERR(chip->busy_pin), "...");

>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Dan Carpenter 4 days, 21 hours ago

On Wed, Jun 03, 2026 at 12:26:11PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 03, 2026 at 12:33:33PM +0000, Taha Narimani wrote:
> > The driver currently utilizes devm_gpiod_get() for the 'busy' line,
> > which makes the GPIO mandatory. However, the busy pin is hardware-optional
> > depending on the specific board configuration.
> > 
> > Switch to devm_gpiod_get_optional() to allow boards that do not have
> > this pin wired up to still probe the driver successfully, and remove
> > the redundant conditional chip-ID check since the optional API handles
> > missing descriptors gracefully.
> 
> ...
> 
> > -	if (chip->id == ID_AD7817) {
> > +	if (chip->busy_pin) {
> 
> If we get GPIO optional, this check wouldn't be necessary anymore as the below
> should return 0 IIRC in this case.
> 

No, it's still necessary.  It can be NULL because of the CONFIG_
in which case, sure, gpiod_get_value() is a no-op.  But it can
also be NULL because of the device tree and in that case we need
the check to avoid a NULL pointer dereference.

> >  		while (gpiod_get_value(chip->busy_pin))
> >  			cpu_relax();
> >  	}

regards,
dan carpenter

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Andy Shevchenko 4 days, 20 hours ago

On Wed, Jun 03, 2026 at 01:20:08PM +0300, Dan Carpenter wrote:
> On Wed, Jun 03, 2026 at 12:26:11PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 03, 2026 at 12:33:33PM +0000, Taha Narimani wrote:

...

> > > -	if (chip->id == ID_AD7817) {
> > > +	if (chip->busy_pin) {
> > 
> > If we get GPIO optional, this check wouldn't be necessary anymore as the below
> > should return 0 IIRC in this case.
> 
> No, it's still necessary.  It can be NULL because of the CONFIG_
> in which case, sure, gpiod_get_value() is a no-op.  But it can
> also be NULL because of the device tree and in that case we need
> the check to avoid a NULL pointer dereference.

Can you elaborate on the latter more? I fail to see that.
What I see is that the function either implemented or not is NULL-aware.

> > >  		while (gpiod_get_value(chip->busy_pin))
> > >  			cpu_relax();
> > >  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Dan Carpenter 4 days, 20 hours ago

On Wed, Jun 03, 2026 at 01:37:32PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 03, 2026 at 01:20:08PM +0300, Dan Carpenter wrote:
> > On Wed, Jun 03, 2026 at 12:26:11PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 03, 2026 at 12:33:33PM +0000, Taha Narimani wrote:
> 
> ...
> 
> > > > -	if (chip->id == ID_AD7817) {
> > > > +	if (chip->busy_pin) {
> > > 
> > > If we get GPIO optional, this check wouldn't be necessary anymore as the below
> > > should return 0 IIRC in this case.
> > 
> > No, it's still necessary.  It can be NULL because of the CONFIG_
> > in which case, sure, gpiod_get_value() is a no-op.  But it can
> > also be NULL because of the device tree and in that case we need
> > the check to avoid a NULL pointer dereference.
> 
> Can you elaborate on the latter more? I fail to see that.
> What I see is that the function either implemented or not is NULL-aware.
> 

Ah, yes.  You're right.  The VALIDATE_DESC() macro has a return hiding
inside.  I hadn't seen that.

regards,
dan carpenter

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Joshua Crofts 4 days, 22 hours ago

On Wed, 3 Jun 2026 at 11:07, Taha Narimani <tahanarimani3443@gmail.com> wrote:
> ---
> +       chip->busy_pin = devm_gpiod_get_optional(&spi_dev->dev, "busy",
> +                                                GPIOD_IN);
> +       if (IS_ERR(chip->busy_pin)) {
> +               ret = PTR_ERR(chip->busy_pin);
> +               dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
> +                       ret);

Looking at this, you could eventually move all of the dev_err() calls in the
probe() function to use dev_err_probe(). (For another patch though).

-- 
Kind regards

CJD

Re: [PATCH v2] staging: iio: adc: ad7816: Use devm_gpiod_get_optional() for busy GPIO

Posted by Andy Shevchenko 4 days, 21 hours ago

On Wed, Jun 03, 2026 at 11:13:17AM +0200, Joshua Crofts wrote:
> On Wed, 3 Jun 2026 at 11:07, Taha Narimani <tahanarimani3443@gmail.com> wrote:

...

> > +       chip->busy_pin = devm_gpiod_get_optional(&spi_dev->dev, "busy",
> > +                                                GPIOD_IN);
> > +       if (IS_ERR(chip->busy_pin)) {
> > +               ret = PTR_ERR(chip->busy_pin);
> > +               dev_err(&spi_dev->dev, "Failed to request busy GPIO: %d\n",
> > +                       ret);
> 
> Looking at this, you could eventually move all of the dev_err() calls in the
> probe() function to use dev_err_probe(). (For another patch though).

No, it must be in this patch, otherwise it might regress quite badly (from
the user, who wants to see a bootlog nice and clean, perspective).

-- 
With Best Regards,
Andy Shevchenko