RE: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio

[Tanislav, Cosmin]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, November 12, 2022 7:07 PM
> To: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Cc: Tanislav, Cosmin <Cosmin.Tanislav@xxxxxxxxxx>; Lars-Peter Clausen
> <lars@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; Rob Herring <robh+dt@xxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 5/5] iio: addac: ad74413r: add support for reset-gpio
> 
> [External]
> 
> On Fri, 11 Nov 2022 15:39:21 +0100
> Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > We have a board where the reset pin of the ad74412 is connected to a
> > gpio, but also pulled low by default. Hence to get the chip out of
> > reset, the driver needs to know about that gpio and set it high before
> > attempting to communicate with it.
> 
> I'm a little confused on polarity here.  The pin is a !reset so
> we need to drive it low briefly to trigger a reset.
> I'm guessing for your board the pin is set to active low? (an example
> in the dt would have made that clearer) Hence the pulse
> in here to 1 is actually briefly driving it low before restoring to high?
> 
> For a pin documented as !reset that seems backwards though you have
> called it reset so that is fine, but this description doesn't make that
> celar.

My opinion is that the driver shouldn't exactly know the polarity of the reset,
and just assume that setting the reset GPIO to 1 means putting it in reset,
and setting it to 0 means bringing out of reset.

> 
> Perhaps just add some more description here to make it clear the GPIO
> is active low, and then refer to setting it to true and false to avoid
> the confusing high / low terminology which are inverted...
> 
> >
> > When a reset-gpio is given in device tree, use that instead of the
> > software reset. According to the data sheet, the two methods are
> > functionally equivalent.
> >
> > Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/iio/addac/ad74413r.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
> > index 9f77d2f514de..af09d43f921c 100644
> > --- a/drivers/iio/addac/ad74413r.c
> > +++ b/drivers/iio/addac/ad74413r.c
> > @@ -71,6 +71,7 @@ struct ad74413r_state {
> >  	struct regmap			*regmap;
> >  	struct device			*dev;
> >  	struct iio_trigger		*trig;
> > +	struct gpio_desc		*reset_gpio;
> >
> >  	size_t			adc_active_channels;
> >  	struct spi_message	adc_samples_msg;
> > @@ -393,6 +394,13 @@ static int ad74413r_reset(struct ad74413r_state
> *st)
> >  {
> >  	int ret;
> >
> > +	if (st->reset_gpio) {
> > +		gpiod_set_value_cansleep(st->reset_gpio, 1);
> > +		fsleep(50);
> > +		gpiod_set_value_cansleep(st->reset_gpio, 0);
> > +		return 0;
> > +	}
> > +
> >  	ret = regmap_write(st->regmap, AD74413R_REG_CMD_KEY,
> >  			   AD74413R_CMD_KEY_RESET1);
> >  	if (ret)
> > @@ -1316,6 +1324,10 @@ static int ad74413r_probe(struct spi_device *spi)
> >  	if (IS_ERR(st->regmap))
> >  		return PTR_ERR(st->regmap);
> >
> > +	st->reset_gpio = devm_gpiod_get_optional(st->dev, "reset",
> GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->reset_gpio))
> > +		return PTR_ERR(st->reset_gpio);
> > +
> >  	st->refin_reg = devm_regulator_get_optional(st->dev, "refin");
> >  	if (IS_ERR(st->refin_reg)) {
> >  		ret = PTR_ERR(st->refin_reg);