On Wed, Jun 03, 2026 at 01:08:33PM +0100, Jonathan Cameron wrote: > On Wed, 3 Jun 2026 09:28:26 +0100 > Nuno Sá <[email protected]> wrote: > > > On Tue, Jun 02, 2026 at 05:33:53PM +0100, Rodrigo Alencar via B4 Relay > > wrote: > > > From: Rodrigo Alencar <[email protected]> > > > > > > Add RESET pin GPIO support through an optional reset control, which is > > > local to the probe function. Also, include delays for power-up time and > > > reset pulse width. > > > > > > Signed-off-by: Rodrigo Alencar <[email protected]> > > > --- > > > drivers/iio/dac/ad5686.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c > > > index 4a8c587ff116..345ca2436332 100644 > > > --- a/drivers/iio/dac/ad5686.c > > > +++ b/drivers/iio/dac/ad5686.c > > > @@ -8,12 +8,14 @@ > > > #include <linux/array_size.h> > > > #include <linux/bitfield.h> > > > #include <linux/bitops.h> > > > +#include <linux/delay.h> > > > #include <linux/dev_printk.h> > > > #include <linux/errno.h> > > > #include <linux/export.h> > > > #include <linux/kstrtox.h> > > > #include <linux/module.h> > > > #include <linux/regulator/consumer.h> > > > +#include <linux/reset.h> > > > #include <linux/sysfs.h> > > > #include <linux/wordpart.h> > > > > > > @@ -471,6 +473,7 @@ int ad5686_probe(struct device *dev, > > > const struct ad5686_chip_info *chip_info, > > > const char *name, const struct ad5686_bus_ops *ops) > > > { > > > + struct reset_control *rstc; > > > struct ad5686_state *st; > > > struct iio_dev *indio_dev; > > > int ret, i; > > > @@ -506,6 +509,16 @@ int ad5686_probe(struct device *dev, > > > return dev_err_probe(dev, -EINVAL, > > > "invalid or not provided vref voltage\n"); > > > > > > + rstc = devm_reset_control_get_optional_exclusive(dev, NULL); > > > + if (IS_ERR(rstc)) > > > + return dev_err_probe(dev, PTR_ERR(rstc), > > > + "Failed to get reset control\n"); > > > > On top of what Andy stated, I'm fairly sure > > devm_reset_control_get_optional_exclusive() returns with the GPIO > > asserted. > > We've been getting reports on that not being the case from Sashiko > and when I last looked into one of those it definitely isn't documented > as doing so and I got the impression it is a reset controller specific > thing. Do we are fine here because the gpio reset controller > reset_gpio_probe() > includes: > priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > if (IS_ERR(priv->reset)) > return dev_err_probe(dev, PTR_ERR(priv->reset), > "Could not get reset gpios\n"); > Which I guess puts it in to reset? > > So do we assume gpio reset or not for this sort of driver that specifies > in the binding reset-gpios. Now if the following is implying we need > a deasserted to asserted transition (maybe?) then we'd need to force > a deassert first.
Yeah, fair enough. I think in practice it works but we are relying in internals of other piece of code for it to work which is not great. - Nuno Sá > > Btw I used claude to explore this and it hallucinated the reverse polarity > providing otherwise correct code for what was in reset_gpio_probe() but > oddly editing that one line. I was being lazy and using the web UI rather > than a version with access to my git tree so maybe it scraped some > buggy code from a downstream tree. Anyhow watch out for subtle garbage! > It also took a few requests to get it to figure out the logical nature > of the GPIO signals rather than assuming they were controlling whether > the line was high or low directly. > > Jonathan > > > > > > > + > > > + udelay(5); /* power-up time */ > > > + reset_control_assert(rstc); > > > + udelay(1); /* reset pulse: comfortably bigger than the spec */ > > > + reset_control_deassert(rstc); > > > + > > > /* Initialize masks to all ones */ > > > st->pwr_down_mask = ~0; > > > st->pwr_down_mode = ~0; > > > > > > -- > > > 2.43.0 > > > > > > >
