Re: [PATCH 1/8] iio: adc: ad9467: fix reset gpio handling

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2023-12-06 at 16:09 -0600, David Lechner wrote:
> On Tue, Dec 5, 2023 at 11:06 AM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> > 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > 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. Also note that we now use
> > gpiod_set_value_cansleep() instead of gpiod_direction_output() as we
> > already request the pin as output.
> > 
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  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..5ecf486bf5d1 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;
> 
> same comment as before about replacing two ifs with one:
> 
>         if (IS_ERR_OR_NULL(gpio))
>                 return PTR_ERR_OR_ZERO(gpio);
> 

Bah, forgot about that one... will do as suggested.

- Nuno Sá

> 
> > +
> > +       fsleep(1);
> > +       gpiod_set_value_cansleep(gpio, 0);
> > +       fsleep(10 * USEC_PER_MSEC);
> > +
> > +       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.43.0
> > 
> > 
> 
> With the suggested change above:
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>