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

[Nuno Sá]

On Wed, Jun 03, 2026 at 01:08:33PM +0100, Jonathan Cameron wrote:
> 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.

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