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
> <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> >
> > From: Nuno Sa <nuno.sa@xxxxxxxxxx>
> >
> > 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 <nuno.sa@xxxxxxxxxx>
> > ---
> > 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á
>
