| Message ID | 20231205-iio-backend-prep-v1-1-7c9bc18d612b@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: ad9467 and axi-adc cleanups | expand |
On Tue, 05 Dec 2023 18:06:41 +0100 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. If I understand this correctly, it's really just inverted polarity compared to the expectation? If so just call it that rather than discussing what happens in detail. > > 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; > + > + 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; > >
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); > + > + 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>
On Wed, 2023-12-06 at 18:03 +0000, Jonathan Cameron wrote: > On Tue, 05 Dec 2023 18:06:41 +0100 > 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. > > If I understand this correctly, it's really just inverted polarity > compared to the expectation? If so just call it that rather than > discussing what happens in detail. > Hmm, honestly I do not know but likely you're right. The driver was just playing games with polarity and expecting people made things "right" in devicetree. Will change the message... - Nuno Sá >
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>
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; + + 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;