Re: [PATCH 06/12] iio: dac: ad5686: consume optional reset signal

[Jonathan Cameron]

On Wed, 3 Jun 2026 09:28:26 +0100
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Tue, Jun 02, 2026 at 05:33:53PM +0100, Rodrigo Alencar via B4 Relay wrote:
> > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > 
> > 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 <rodrigo.alencar@xxxxxxxxxx>
> > ---
> >  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.

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
> > 
> >