[PATCH v3] iio: amplifiers: ad8366: Change devm_gpiod_get() to optional and add the missed check

[PATCH v3] iio: amplifiers: ad8366: Change devm_gpiod_get() to optional and add the missed check

[Chuhong Yuan]

Since if there is no GPIO, nothing happens, replace devm_gpiod_get()
with devm_gpiod_get_optional().
Also add IS_ERR() to fix the missing-check bug.

Fixes: cee211f4e5a0 ("iio: amplifiers: ad8366: Add support for the ADA4961 DGA")
Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
Changes in v3:
  - Change devm_gpiod_get() to optional.
  - Modify description.

 drivers/iio/amplifiers/ad8366.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/amplifiers/ad8366.c b/drivers/iio/amplifiers/ad8366.c
index 62167b87caea..8819e8997f76 100644
--- a/drivers/iio/amplifiers/ad8366.c
+++ b/drivers/iio/amplifiers/ad8366.c
@@ -262,8 +262,12 @@ static int ad8366_probe(struct spi_device *spi)
 	case ID_ADA4961:
 	case ID_ADL5240:
 	case ID_HMC1119:
-		st->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
+		st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
 			GPIOD_OUT_HIGH);
+		if (IS_ERR(st->reset_gpio)) {
+			ret = PTR_ERR(st->reset_gpio);
+			goto error_disable_reg;
+		}
 		indio_dev->channels = ada4961_channels;
 		indio_dev->num_channels = ARRAY_SIZE(ada4961_channels);
 		break;
-- 
2.26.2

Re: [PATCH v3] iio: amplifiers: ad8366: Change devm_gpiod_get() to optional and add the missed check

[Ardelean, Alexandru]

On Wed, 2020-06-03 at 17:26 +0800, Chuhong Yuan wrote:
> Since if there is no GPIO, nothing happens, replace devm_gpiod_get()
> with devm_gpiod_get_optional().
> Also add IS_ERR() to fix the missing-check bug.
> 

Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Fixes: cee211f4e5a0 ("iio: amplifiers: ad8366: Add support for the ADA4961
> DGA")
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
> Changes in v3:
>   - Change devm_gpiod_get() to optional.
>   - Modify description.
> 
>  drivers/iio/amplifiers/ad8366.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/amplifiers/ad8366.c b/drivers/iio/amplifiers/ad8366.c
> index 62167b87caea..8819e8997f76 100644
> --- a/drivers/iio/amplifiers/ad8366.c
> +++ b/drivers/iio/amplifiers/ad8366.c
> @@ -262,8 +262,12 @@ static int ad8366_probe(struct spi_device *spi)
>  	case ID_ADA4961:
>  	case ID_ADL5240:
>  	case ID_HMC1119:
> -		st->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> +		st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
>  			GPIOD_OUT_HIGH);
> +		if (IS_ERR(st->reset_gpio)) {
> +			ret = PTR_ERR(st->reset_gpio);
> +			goto error_disable_reg;
> +		}
>  		indio_dev->channels = ada4961_channels;
>  		indio_dev->num_channels = ARRAY_SIZE(ada4961_channels);
>  		break;

Re: [PATCH v3] iio: amplifiers: ad8366: Change devm_gpiod_get() to optional and add the missed check

[Jonathan Cameron]

On Wed, 3 Jun 2020 10:19:12 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Wed, 2020-06-03 at 17:26 +0800, Chuhong Yuan wrote:
> > Since if there is no GPIO, nothing happens, replace devm_gpiod_get()
> > with devm_gpiod_get_optional().
> > Also add IS_ERR() to fix the missing-check bug.
> >   
> 
> Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied with layout tweaked a bit as per Markus' comment which made
sense in this particular case.

I also tweaked the description to say "missing-check" warning given
as we've established the failure to check for an error wasn't a problem
as such.  On that basis I thought about dropping the fixes tag, but
it's 'sort' of a fix so I'll leave it there.  Not sending this the
fast path however as is not urgent (shout if you disagree!)

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> > Fixes: cee211f4e5a0 ("iio: amplifiers: ad8366: Add support for the ADA4961
> > DGA")
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > ---
> > Changes in v3:
> >   - Change devm_gpiod_get() to optional.
> >   - Modify description.
> > 
> >  drivers/iio/amplifiers/ad8366.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/amplifiers/ad8366.c b/drivers/iio/amplifiers/ad8366.c
> > index 62167b87caea..8819e8997f76 100644
> > --- a/drivers/iio/amplifiers/ad8366.c
> > +++ b/drivers/iio/amplifiers/ad8366.c
> > @@ -262,8 +262,12 @@ static int ad8366_probe(struct spi_device *spi)
> >  	case ID_ADA4961:
> >  	case ID_ADL5240:
> >  	case ID_HMC1119:
> > -		st->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> > +		st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> >  			GPIOD_OUT_HIGH);
> > +		if (IS_ERR(st->reset_gpio)) {
> > +			ret = PTR_ERR(st->reset_gpio);
> > +			goto error_disable_reg;
> > +		}
> >  		indio_dev->channels = ada4961_channels;
> >  		indio_dev->num_channels = ARRAY_SIZE(ada4961_channels);
> >  		break;  

Re: [PATCH v3] iio: amplifiers: ad8366: Change devm_gpiod_get() to optional and add the missed check

[Markus Elfring]

> Since if there is no GPIO, nothing happens, replace devm_gpiod_get()
> with devm_gpiod_get_optional().

…
> +++ b/drivers/iio/amplifiers/ad8366.c
> @@ -262,8 +262,12 @@  static int ad8366_probe(struct spi_device *spi)
…
>  	case ID_HMC1119:
> -		st->reset_gpio = devm_gpiod_get(&spi->dev, "reset",
> +		st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
>  			GPIOD_OUT_HIGH);
…

I suggest to choose an other source code formatting for this statement.

A)
+		st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
+							 GPIOD_OUT_HIGH);

B)
+		st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_HIGH);
-			GPIOD_OUT_HIGH);


Regards,
Markus