[04/12] iio: adc: ad9467: fix reset gpio handling

Commit Message

Nuno Sa via B4 Relay Nov. 21, 2023, 10:20 a.m. UTC

From: Nuno Sa <[e-mail]>

The reset gpio was being requested with GPIOD_OUT_LOW which means, not
asserted. Then it was being asserted but never de-asserted which means
the devices was left in reset. Fix it by de-asserting the gpio.

While at it, moved the handling to it's own function and dropped
'reset_gpio' from the 'struct ad9467_state' as we only need it during
probe. On top of that, refactored things so that we now request the gpio
asserted (i.e in reset) and then de-assert it.

Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
Signed-off-by: Nuno Sa <[e-mail]>
---
 drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

Comments

David Lechner Nov. 30, 2023, 9:41 p.m. UTC | #1

On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
<[e-mail]> wrote:
>
> From: Nuno Sa <[e-mail]>
>
> The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> asserted. Then it was being asserted but never de-asserted which means
> the devices was left in reset. Fix it by de-asserting the gpio.

It could be helpful to update the devicetree bindings to state the
expected active-high or active-low setting for this gpio so it is
clear which state means asserted.

>
> While at it, moved the handling to it's own function and dropped
> 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> probe. On top of that, refactored things so that we now request the gpio
> asserted (i.e in reset) and then de-assert it.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <[e-mail]>
> ---
>  drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 39eccc28debe..368ea57be117 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -121,7 +121,6 @@ struct ad9467_state {
>         unsigned int                    output_mode;
>
>         struct gpio_desc                *pwrdown_gpio;
> -       struct gpio_desc                *reset_gpio;
>  };
>
>  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
>         return ad9467_outputmode_set(st->spi, st->output_mode);
>  }
>
> +static int ad9467_reset(struct device *dev)
> +{
> +       struct gpio_desc *gpio;
> +
> +       gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(gpio))
> +               return PTR_ERR(gpio);
> +       if (!gpio)
> +               return 0;

can be done in one test instead of 2:

if (IS_ERR_OR_NULL(gpio))
        return PTR_ERR_OR_ZERO(gpio);

> +
> +       fsleep(1);
> +       gpiod_direction_output(gpio, 0);
> +       fsleep(10);

Previous version was 10 milliseconds instead of 10 microseconds. Was
this change intentional? If yes, it should be mentioned it in the
commit message.

> +
> +       return 0;
> +}
> +
>  static int ad9467_probe(struct spi_device *spi)
>  {
>         const struct ad9467_chip_info *info;
> @@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
>         if (IS_ERR(st->pwrdown_gpio))
>                 return PTR_ERR(st->pwrdown_gpio);
>
> -       st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> -                                                GPIOD_OUT_LOW);
> -       if (IS_ERR(st->reset_gpio))
> -               return PTR_ERR(st->reset_gpio);
> -
> -       if (st->reset_gpio) {
> -               udelay(1);
> -               ret = gpiod_direction_output(st->reset_gpio, 1);
> -               if (ret)
> -                       return ret;
> -               mdelay(10);
> -       }
> +       ret = ad9467_reset(&spi->dev);
> +       if (ret)
> +               return ret;
>
>         conv->chip_info = &info->axi_adc_info;
>
>
> --
> 2.42.1
>
>

Nuno Sá Dec. 1, 2023, 8:47 a.m. UTC | #2

On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> <[e-mail]> wrote:
> > 
> > From: Nuno Sa <[e-mail]>
> > 
> > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > asserted. Then it was being asserted but never de-asserted which means
> > the devices was left in reset. Fix it by de-asserting the gpio.
> 
> It could be helpful to update the devicetree bindings to state the
> expected active-high or active-low setting for this gpio so it is
> clear which state means asserted.
> 

You could state that the chip is active low but I don't see that change that
important for now. Not sure if this is clear and maybe that's why your comment.
GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me the
pin in the asserted state".

> > While at it, moved the handling to it's own function and dropped
> > 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> > probe. On top of that, refactored things so that we now request the gpio
> > asserted (i.e in reset) and then de-assert it.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <[e-mail]>
> > ---
> >  drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
> >  1 file changed, 20 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> > index 39eccc28debe..368ea57be117 100644
> > --- a/drivers/iio/adc/ad9467.c
> > +++ b/drivers/iio/adc/ad9467.c
> > @@ -121,7 +121,6 @@ struct ad9467_state {
> >         unsigned int                    output_mode;
> > 
> >         struct gpio_desc                *pwrdown_gpio;
> > -       struct gpio_desc                *reset_gpio;
> >  };
> > 
> >  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> > @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc_conv
> > *conv)
> >         return ad9467_outputmode_set(st->spi, st->output_mode);
> >  }
> > 
> > +static int ad9467_reset(struct device *dev)
> > +{
> > +       struct gpio_desc *gpio;
> > +
> > +       gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +       if (IS_ERR(gpio))
> > +               return PTR_ERR(gpio);
> > +       if (!gpio)
> > +               return 0;
> 
> can be done in one test instead of 2:
> 
> if (IS_ERR_OR_NULL(gpio))
>         return PTR_ERR_OR_ZERO(gpio);
> 

Yep, better that way...

> > +
> > +       fsleep(1);
> > +       gpiod_direction_output(gpio, 0);
> > +       fsleep(10);
> 
> Previous version was 10 milliseconds instead of 10 microseconds. Was
> this change intentional? If yes, it should be mentioned it in the
> commit message.

Oh, good catch! Copy past thing with even realizing the differences in the arguments
:face_palm:

- Nuno Sá


>

David Lechner Dec. 1, 2023, 5:01 p.m. UTC | #3

On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <[e-mail]> wrote:
>
> On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > <[e-mail]> wrote:
> > >
> > > From: Nuno Sa <[e-mail]>
> > >
> > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > asserted. Then it was being asserted but never de-asserted which means
> > > the devices was left in reset. Fix it by de-asserting the gpio.
> >
> > It could be helpful to update the devicetree bindings to state the
> > expected active-high or active-low setting for this gpio so it is
> > clear which state means asserted.
> >
>
> You could state that the chip is active low but I don't see that change that
> important for now. Not sure if this is clear and maybe that's why your comment.
> GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me the
> pin in the asserted state".
>

I would assume that this bug happened in the first place because
someone forgot GPIOD_OUT_LOW in the devicetree when they were
developing the driver. So this is why I suggested that updating the
devicetree binding docs so that future users are less likely to make
the same mistake. Currently, the bindings don't even have reset-gpios
in the examples.

Nuno Sá Dec. 2, 2023, 8:36 a.m. UTC | #4

On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <[e-mail]> wrote:
> > 
> > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <[e-mail]> wrote:
> > > > 
> > > > From: Nuno Sa <[e-mail]>
> > > > 
> > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > asserted. Then it was being asserted but never de-asserted which means
> > > > the devices was left in reset. Fix it by de-asserting the gpio.
> > > 
> > > It could be helpful to update the devicetree bindings to state the
> > > expected active-high or active-low setting for this gpio so it is
> > > clear which state means asserted.
> > > 
> > 
> > You could state that the chip is active low but I don't see that change that
> > important for now. Not sure if this is clear and maybe that's why your comment.
> > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me
> > the
> > pin in the asserted state".
> > 
> 
> I would assume that this bug happened in the first place because
> someone forgot GPIOD_OUT_LOW in the devicetree when they were
> developing the driver. So this is why I suggested that updating the
> devicetree binding docs so that future users are less likely to make
> the same mistake. Currently, the bindings don't even have reset-gpios
> in the examples.

Hmm, I think you're missing the point... The bug has nothing to do with devicetree.
This is what was happening:

1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means is
that you get an output gpio deasserted. Hence the device is out of reset. And here is
the important part... what you have in dts does not matter. If you have active low,
it means the pin level will be 1. If you have high, the pin level is 0. And this is
all handled by gpiolib for you.

2) Then, we called gpiod_direction_output(..., 1), which means set the direction out
(which is actually not needed since it was already done when getting the pin) and
assert the pin. Hence, reset the device. And we were never de-asserting the pin so
the device would be left in reset.

- Nuno Sá

Jonathan Cameron Dec. 4, 2023, 3:15 p.m. UTC | #5

On Sat, 02 Dec 2023 09:36:47 +0100
Nuno Sá <[e-mail]> wrote:

> On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> > On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <[e-mail]> wrote:  
> > > 
> > > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:  
> > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > <[e-mail]> wrote:  
> > > > > 
> > > > > From: Nuno Sa <[e-mail]>
> > > > > 
> > > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > > asserted. Then it was being asserted but never de-asserted which means
> > > > > the devices was left in reset. Fix it by de-asserting the gpio.  
> > > > 
> > > > It could be helpful to update the devicetree bindings to state the
> > > > expected active-high or active-low setting for this gpio so it is
> > > > clear which state means asserted.
> > > >   
> > > 
> > > You could state that the chip is active low but I don't see that change that
> > > important for now. Not sure if this is clear and maybe that's why your comment.
> > > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me
> > > the
> > > pin in the asserted state".
> > >   
> > 
> > I would assume that this bug happened in the first place because
> > someone forgot GPIOD_OUT_LOW in the devicetree when they were
> > developing the driver. So this is why I suggested that updating the
> > devicetree binding docs so that future users are less likely to make
> > the same mistake. Currently, the bindings don't even have reset-gpios
> > in the examples.  
> 
> Hmm, I think you're missing the point... The bug has nothing to do with devicetree.
> This is what was happening:
> 
> 1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means is
> that you get an output gpio deasserted. Hence the device is out of reset. And here is
> the important part... what you have in dts does not matter. If you have active low,
> it means the pin level will be 1. If you have high, the pin level is 0. And this is
> all handled by gpiolib for you. 
> 
> 2) Then, we called gpiod_direction_output(..., 1), which means set the direction out
> (which is actually not needed since it was already done when getting the pin) and
> assert the pin. Hence, reset the device. And we were never de-asserting the pin so
> the device would be left in reset.

Functionally I believe David is correct.   Flipping the DT would 'fix' this.
It's all down to a nreset vs reset pin description.

In this case I guess it's defined a a 'not reset' on the datasheet which is what
is causing the confusion.  It's not uncommon for people to refer to a reset when
they mean a "not reset" with assumptions on polarity to match.

Jonathan



> 
> - Nuno Sá

Nuno Sá Dec. 4, 2023, 4:41 p.m. UTC | #6

On Mon, 2023-12-04 at 15:15 +0000, Jonathan Cameron wrote:
> On Sat, 02 Dec 2023 09:36:47 +0100
> Nuno Sá <[e-mail]> wrote:
> 
> > On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> > > On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <[e-mail]> wrote:  
> > > > 
> > > > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:  
> > > > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > > > <[e-mail]> wrote:  
> > > > > > 
> > > > > > From: Nuno Sa <[e-mail]>
> > > > > > 
> > > > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > > > asserted. Then it was being asserted but never de-asserted which means
> > > > > > the devices was left in reset. Fix it by de-asserting the gpio.  
> > > > > 
> > > > > It could be helpful to update the devicetree bindings to state the
> > > > > expected active-high or active-low setting for this gpio so it is
> > > > > clear which state means asserted.
> > > > >   
> > > > 
> > > > You could state that the chip is active low but I don't see that change that
> > > > important for now. Not sure if this is clear and maybe that's why your
> > > > comment.
> > > > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get
> > > > me
> > > > the
> > > > pin in the asserted state".
> > > >   
> > > 
> > > I would assume that this bug happened in the first place because
> > > someone forgot GPIOD_OUT_LOW in the devicetree when they were
> > > developing the driver. So this is why I suggested that updating the
> > > devicetree binding docs so that future users are less likely to make
> > > the same mistake. Currently, the bindings don't even have reset-gpios
> > > in the examples.  
> > 
> > Hmm, I think you're missing the point... The bug has nothing to do with
> > devicetree.
> > This is what was happening:
> > 
> > 1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means
> > is
> > that you get an output gpio deasserted. Hence the device is out of reset. And
> > here is
> > the important part... what you have in dts does not matter. If you have active
> > low,
> > it means the pin level will be 1. If you have high, the pin level is 0. And this
> > is
> > all handled by gpiolib for you. 
> > 
> > 2) Then, we called gpiod_direction_output(..., 1), which means set the direction
> > out
> > (which is actually not needed since it was already done when getting the pin) and
> > assert the pin. Hence, reset the device. And we were never de-asserting the pin
> > so
> > the device would be left in reset.
> 
> Functionally I believe David is correct.   Flipping the DT would 'fix' this.
> It's all down to a nreset vs reset pin description.
> 

Ahh I see. Well would not really be a fix :)

- Nuno Sá

Patch

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 39eccc28debe..368ea57be117 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -121,7 +121,6 @@  struct ad9467_state {
 	unsigned int			output_mode;
 
 	struct gpio_desc		*pwrdown_gpio;
-	struct gpio_desc		*reset_gpio;
 };
 
 static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
@@ -378,6 +377,23 @@  static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
 	return ad9467_outputmode_set(st->spi, st->output_mode);
 }
 
+static int ad9467_reset(struct device *dev)
+{
+	struct gpio_desc *gpio;
+
+	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	if (!gpio)
+		return 0;
+
+	fsleep(1);
+	gpiod_direction_output(gpio, 0);
+	fsleep(10);
+
+	return 0;
+}
+
 static int ad9467_probe(struct spi_device *spi)
 {
 	const struct ad9467_chip_info *info;
@@ -408,18 +424,9 @@  static int ad9467_probe(struct spi_device *spi)
 	if (IS_ERR(st->pwrdown_gpio))
 		return PTR_ERR(st->pwrdown_gpio);
 
-	st->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
-						 GPIOD_OUT_LOW);
-	if (IS_ERR(st->reset_gpio))
-		return PTR_ERR(st->reset_gpio);
-
-	if (st->reset_gpio) {
-		udelay(1);
-		ret = gpiod_direction_output(st->reset_gpio, 1);
-		if (ret)
-			return ret;
-		mdelay(10);
-	}
+	ret = ad9467_reset(&spi->dev);
+	if (ret)
+		return ret;
 
 	conv->chip_info = &info->axi_adc_info;