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

[Maxwell Doose]

Hi Taha,

On Fri Jul 10, 2026 at 11:00 AM CDT
Taha Narimani <tahanarimani3443@xxxxxxxxx> 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. Clean up
> the redundant busy_pin conditional checks as gpiod_get_value() safely
> handles NULL descriptors. Additionally, use dev_err_probe() to prevent
> bootlog spamming during deferred probing.
>
> Fixes: 3e5971b2ddb6 ("base: original ad7816.c")

If this is a fix, then why are we making style changes?

> Signed-off-by: Taha Narimani <tahanarimani3443@xxxxxxxxx>
> ---
> Changes in v3:
>   - Removed redundant conditional check around gpiod_get_value() as suggested by Andy Shevchenko and Dan Carpenter.
>   - Switched to dev_err_probe() to avoid deferred probe spamming.
>   - Introduced local struct device *dev pointer in probe and kept the GPIO request on a single line for better readability.

Unless someone requested this during review this doesn't make sense.
Please see my above comment, and I'd also suggest to take a look at
Documentation/process/submitting-patches.rst.

>   - Added the missing Fixes tag requested by Jonathan Cameron.
>
>  drivers/staging/iio/adc/ad7816.c | 67 +++++++++++++-------------------
>  1 file changed, 28 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> index 988eee3..807cf57 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> Changes in v3:
>   - Removed redundant conditional check around gpiod_get_value() as suggested by Andy Shevchenko and Dan Carpenter.
>   - Switched to dev_err_probe() to avoid deferred probe spamming.
>   - Introduced local struct device *dev pointer in probe and kept the GPIO request on a single line for better readability.
>   - Added the missing Fixes tag requested by Jonathan Cameron.
>

Wrong spot for this + causes the patch to not apply. It should be below
the --- right after the list of tags.

> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -22,20 +22,20 @@
>  /*
>   * AD7816 config masks
>   */
> -#define AD7816_FULL			0x1
> -#define AD7816_PD			0x2
> -#define AD7816_CS_MASK			0x7
> -#define AD7816_CS_MAX			0x4
> +#define AD7816_FULL                     0x1
> +#define AD7816_PD                       0x2
> +#define AD7816_CS_MASK                  0x7
> +#define AD7816_CS_MAX                   0x4
>  

Nothing changed? Seems like a stray change.

>  /*
>   * AD7816 temperature masks
>   */
> -#define AD7816_VALUE_OFFSET		6
> -#define AD7816_BOUND_VALUE_BASE		0x8
> -#define AD7816_BOUND_VALUE_MIN		-95
> -#define AD7816_BOUND_VALUE_MAX		152
> -#define AD7816_TEMP_FLOAT_OFFSET	2
> -#define AD7816_TEMP_FLOAT_MASK		0x3
> +#define AD7816_VALUE_OFFSET             6
> +#define AD7816_BOUND_VALUE_BASE         0x8
> +#define AD7816_BOUND_VALUE_MIN          -95
> +#define AD7816_BOUND_VALUE_MAX          152
> +#define AD7816_TEMP_FLOAT_OFFSET        2
> +#define AD7816_TEMP_FLOAT_MASK          0x3
>  

Ditto.

>  /*
>   * struct ad7816_chip_info - chip specific information
> @@ -48,7 +48,7 @@ struct ad7816_chip_info {
>  	struct gpio_desc *convert_pin;
>  	struct gpio_desc *busy_pin;
>  	u8  oti_data[AD7816_CS_MAX + 1];
> -	u8  channel_id;	/* 0 always be temperature */
> +	u8  channel_id; /* 0 always be temperature */
>  	u8  mode;
>  };
>  

Here as well.

> @@ -84,10 +84,8 @@ static int ad7816_spi_read(struct ad7816_chip_info *chip, u16 *data)
>  		gpiod_set_value(chip->convert_pin, 1);
>  	}
>  
> -if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> -		while (gpiod_get_value(chip->busy_pin))
> -			cpu_relax();
> -	}
> +	while (gpiod_get_value(chip->busy_pin))
> +		cpu_relax();
>

Can you explain why we're removing this check? There also seems to be a
spot in _probe() where the check gets removed as well.

>  	gpiod_set_value(chip->rdwr_pin, 0);
>  	gpiod_set_value(chip->rdwr_pin, 1);
> @@ -254,8 +252,8 @@ static const struct attribute_group ad7816_attribute_group = {
>   * temperature bound events
>   */
>  
> -#define IIO_EVENT_CODE_AD7816_OTI IIO_UNMOD_EVENT_CODE(IIO_TEMP,	\
> -						       0,		\
> +#define IIO_EVENT_CODE_AD7816_OTI IIO_UNMOD_EVENT_CODE(IIO_TEMP,        \
> +						       0,               \
>  						       IIO_EV_TYPE_THRESH, \
>  						       IIO_EV_DIR_FALLING)
>  

Seems like another stray change.

> @@ -351,11 +349,12 @@ static const struct iio_info ad7816_info = {
>  
>  static int ad7816_probe(struct spi_device *spi_dev)
>  {
> +	struct device *dev = &spi_dev->dev;

Again, why are we doing style changes? This seems to be a fix instead of
a style patch.

>  	struct ad7816_chip_info *chip;
>  	struct iio_dev *indio_dev;
>  	int i, ret;
>  
> -	indio_dev = devm_iio_device_alloc(&spi_dev->dev, sizeof(*chip));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  	chip = iio_priv(indio_dev);

...

> @@ -431,7 +421,6 @@ static const struct spi_device_id ad7816_id[] = {
>  	{ "ad7818", ID_AD7818 },
>  	{ }
>  };
> -

Stray change.

>  MODULE_DEVICE_TABLE(spi, ad7816_id);
>  
>  static struct spi_driver ad7816_driver = {


-- 
best regards,
max

Attachment: signature.asc
Description: PGP signature