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

  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.

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@rasmusvillemoes.dk>
---
 drivers/iio/addac/ad74413r.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
  

On Fri, 11 Nov 2022 15:39:21 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> 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.

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@rasmusvillemoes.dk>
> ---
>  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);
  

On 12/11/2022 18.07, Jonathan Cameron wrote:
> On Fri, 11 Nov 2022 15:39:21 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> 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?

Yes. I actually thought that was pretty standard. I do indeed have
something like

  reset-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;

in my .dts, so setting the gpio value to 1 (logically asserting its
function) will end up driving the signal low, and setting it to 0
(de-asserting reset) will set the signal high. I will add that line to
the example in the binding.

> For a pin documented as !reset that seems backwards 

Well, it depends on where the knowledge of the pin being active low
belongs. In this case, the driver itself handles the gpio so it could be
done both ways.

But if, for example, the iio framework would handle an optional
reset-gpio for each device, it couldn't possibly know whether to set it
to 1 or 0 for a given device, it could only set it logic 1 to assert
reset and then rely on DT gpio descriptor to include the active low/high
info.

Also, see the "The active low and open drain semantics" section in
Documentation/driver-api/gpio/consumer.rst.

Rasmus
  

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, November 12, 2022 7:07 PM
> To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@rasmusvillemoes.dk> 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@rasmusvillemoes.dk>
> > ---
> >  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);
  

On Mon, 14 Nov 2022 09:37:59 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 12/11/2022 18.07, Jonathan Cameron wrote:
> > On Fri, 11 Nov 2022 15:39:21 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> 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?  
> 
> Yes. I actually thought that was pretty standard. I do indeed have
> something like
> 
>   reset-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
> 
> in my .dts, so setting the gpio value to 1 (logically asserting its
> function) will end up driving the signal low, and setting it to 0
> (de-asserting reset) will set the signal high. I will add that line to
> the example in the binding.
> 
> > For a pin documented as !reset that seems backwards   
> 
> Well, it depends on where the knowledge of the pin being active low
> belongs. In this case, the driver itself handles the gpio so it could be
> done both ways.
> 
> But if, for example, the iio framework would handle an optional
> reset-gpio for each device, it couldn't possibly know whether to set it
> to 1 or 0 for a given device, it could only set it logic 1 to assert
> reset and then rely on DT gpio descriptor to include the active low/high
> info.
> 
> Also, see the "The active low and open drain semantics" section in
> Documentation/driver-api/gpio/consumer.rst.

Throw in an example in the dt-binding and I'm fine with this as it
stands.

Jonathan

> 
> Rasmus
>
  

On Mon, 14 Nov 2022 13:52:26 +0000
"Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, November 12, 2022 7:07 PM
> > To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter Clausen
> > <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> > devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > 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@rasmusvillemoes.dk> 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.

Agreed. I'd just like a comment + example in the dt-binding to make the point
that the pin is !reset.

Preferably with an example in the dt binding of the common case of it being wired
up to an active low pin.

The main oddity here is the need to pulse it rather than request it directly as
in the reset state and then just set that to off.

Jonathan
> 
> > 
> > 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@rasmusvillemoes.dk>
> > > ---
> > >  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);  
>
  

On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:
> On Mon, 14 Nov 2022 13:52:26 +0000
> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Jonathan Cameron <jic23@kernel.org>
> > > Sent: Saturday, November 12, 2022 7:07 PM
> > > To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > > Cc: Tanislav, Cosmin <Cosmin.Tanislav@analog.com>; Lars-Peter
> > > Clausen
> > > <lars@metafoo.de>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>;
> > > devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>;
> > > linux-
> > > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > 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@rasmusvillemoes.dk> 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.
> 
> Agreed. I'd just like a comment + example in the dt-binding to make
> the point
> that the pin is !reset.
> 
> Preferably with an example in the dt binding of the common case of it
> being wired
> up to an active low pin.
> 
> The main oddity here is the need to pulse it rather than request it
> directly as
> in the reset state and then just set that to off.
> 
> 

Agreed... In theory we should be able to request the gpio with
GPIOD_OUT_HIGH and then just bring the device out of reset

- Nuno Sá
  

On Fri, 2022-11-11 at 15:39 +0100, Rasmus Villemoes 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.
> 
> 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@rasmusvillemoes.dk>
> ---
>  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);
> +

Minor thing but,

I would move this into ad74413r_reset() as there's no real need to have
the gpio_desc in struct ad74413r_state.

- Nuno Sá
  

On 15/11/2022 17.10, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 15:49:46 +0100
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
>> On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:
>>> On Mon, 14 Nov 2022 13:52:26 +0000
>>> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
>>>   
>>>>>
>>>>> 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.  
>>>
>>> Agreed. I'd just like a comment + example in the dt-binding to make
>>> the point
>>> that the pin is !reset.
>>>
>>> Preferably with an example in the dt binding of the common case of it
>>> being wired
>>> up to an active low pin.
>>>
>>> The main oddity here is the need to pulse it rather than request it
>>> directly as
>>> in the reset state and then just set that to off.
>>>
>>>   
>>
>> Agreed... In theory we should be able to request the gpio with
>> GPIOD_OUT_HIGH and then just bring the device out of reset
> 
> If I recall correctly the datasheet specifically calls out that a pulse
> should be used.  No idea if that's actually true, or if it was meant
> to be there just to say it needs to be set for X nsecs.

So the data sheet says

  The hardware reset is initiated by pulsing the RESET pin low. The
RESET pulse width must comply with the specifications in Table 11.

and table 11 says that the pulse must be min 50us, max 1ms. We don't
really have any way whatsoever to ensure that we're not rescheduled
right before pulling the gpio high again (deasserting the reset), so the
pulse could effectively be much more than 1ms. But I have a hard time
believing that that actually matters (i.e., what state would the chip be
in if we happen to make a pulse 1234us wide?). But what might be
relevant, and maybe where that 1ms figure really comes from, can perhaps
be read in table 10, which lists a "device reset time" of 1ms, with the
description

  Time taken for device reset and calibration memory upload to complete
hardware or software reset events after the device is powered up

so perhaps we should ensure a 1ms delay after the reset (whether we used
the software or gpio method). But that would be a separate fix IMO (and
I'm not sure we actually need it).

I don't mind requesting the gpio with GPIOD_OUT_HIGH, but I'd still keep
the gpiod_set_value(, 1) in the reset function, otherwise it's a bit too
magic for my taste.

Rasmus
  

On Wed, 2022-11-16 at 10:22 +0000, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 20:10:53 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > On 15/11/2022 17.10, Jonathan Cameron wrote:
> > > On Tue, 15 Nov 2022 15:49:46 +0100
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >   
> > > > On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:  
> > > > > On Mon, 14 Nov 2022 13:52:26 +0000
> > > > > "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
> > > > >     
> > > > > > > 
> > > > > > > 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.    
> > > > > 
> > > > > Agreed. I'd just like a comment + example in the dt-binding
> > > > > to make
> > > > > the point
> > > > > that the pin is !reset.
> > > > > 
> > > > > Preferably with an example in the dt binding of the common
> > > > > case of it
> > > > > being wired
> > > > > up to an active low pin.
> > > > > 
> > > > > The main oddity here is the need to pulse it rather than
> > > > > request it
> > > > > directly as
> > > > > in the reset state and then just set that to off.
> > > > > 
> > > > >     
> > > > 
> > > > Agreed... In theory we should be able to request the gpio with
> > > > GPIOD_OUT_HIGH and then just bring the device out of reset  
> > > 
> > > If I recall correctly the datasheet specifically calls out that a
> > > pulse
> > > should be used.  No idea if that's actually true, or if it was
> > > meant
> > > to be there just to say it needs to be set for X nsecs.  
> > 
> > So the data sheet says
> > 
> >   The hardware reset is initiated by pulsing the RESET pin low. The
> > RESET pulse width must comply with the specifications in Table 11.
> > 
> > and table 11 says that the pulse must be min 50us, max 1ms. We
> > don't
> > really have any way whatsoever to ensure that we're not rescheduled
> > right before pulling the gpio high again (deasserting the reset),
> > so the
> > pulse could effectively be much more than 1ms. But I have a hard
> > time
> > believing that that actually matters (i.e., what state would the
> > chip be
> > in if we happen to make a pulse 1234us wide?).
> 
> Test it maybe?  Otherwise we'd have to play games to do it again if
> the
> timing was too long to ensure after a couple of goes we do get a
> suitable
> width pulse.
> 
> > But what might be
> > relevant, and maybe where that 1ms figure really comes from, can
> > perhaps
> > be read in table 10, which lists a "device reset time" of 1ms, with
> > the
> > description
> > 
> >   Time taken for device reset and calibration memory upload to
> > complete
> > hardware or software reset events after the device is powered up
> > 
> > so perhaps we should ensure a 1ms delay after the reset (whether we
> > used
> > the software or gpio method). But that would be a separate fix IMO
> > (and
> > I'm not sure we actually need it).
> > 
> > I don't mind requesting the gpio with GPIOD_OUT_HIGH, but I'd still
> > keep
> > the gpiod_set_value(, 1) in the reset function, otherwise it's a
> > bit too
> > magic for my taste.
> 
> Without testing I'd worry that it really does need a pulse so
> probably better
> to leave it doing so. 
> 

Yeah, without testing maybe better to play safe. But, FWIW, what I read
from the datasheet is just another typical reset gpio usage with more
(fancy/confusing) description.

- Nuno Sá
  

On 16/11/2022 11.22, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 20:10:53 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> On 15/11/2022 17.10, Jonathan Cameron wrote:
>>> On Tue, 15 Nov 2022 15:49:46 +0100
>>> Nuno Sá <noname.nuno@gmail.com> wrote:
>>>   
>>>> On Mon, 2022-11-14 at 19:44 +0000, Jonathan Cameron wrote:  
>>>>> On Mon, 14 Nov 2022 13:52:26 +0000
>>>>> "Tanislav, Cosmin" <Cosmin.Tanislav@analog.com> wrote:
>>>>>     
>>>>>>>
>>>>>>> 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.    
>>>>>
>>>>> Agreed. I'd just like a comment + example in the dt-binding to make
>>>>> the point
>>>>> that the pin is !reset.
>>>>>
>>>>> Preferably with an example in the dt binding of the common case of it
>>>>> being wired
>>>>> up to an active low pin.
>>>>>
>>>>> The main oddity here is the need to pulse it rather than request it
>>>>> directly as
>>>>> in the reset state and then just set that to off.
>>>>>
>>>>>     
>>>>
>>>> Agreed... In theory we should be able to request the gpio with
>>>> GPIOD_OUT_HIGH and then just bring the device out of reset  
>>>
>>> If I recall correctly the datasheet specifically calls out that a pulse
>>> should be used.  No idea if that's actually true, or if it was meant
>>> to be there just to say it needs to be set for X nsecs.  
>>
>> So the data sheet says
>>
>>   The hardware reset is initiated by pulsing the RESET pin low. The
>> RESET pulse width must comply with the specifications in Table 11.
>>
>> and table 11 says that the pulse must be min 50us, max 1ms. We don't
>> really have any way whatsoever to ensure that we're not rescheduled
>> right before pulling the gpio high again (deasserting the reset), so the
>> pulse could effectively be much more than 1ms. But I have a hard time
>> believing that that actually matters (i.e., what state would the chip be
>> in if we happen to make a pulse 1234us wide?).
> 
> Test it maybe?  Otherwise we'd have to play games to do it again if the
> timing was too long to ensure after a couple of goes we do get a suitable
> width pulse.

So I've booted quite a number of times with various large sleep values
(between 1 and 10ms), and never seen a problem. Our hardware guys also
confirm that there should be no such thing as a "too long" pulse.

So do you want me to respin, moving the gpio request into the reset
function (i.e. not storing the descriptor in the ad74413r_state as Nuno
pointed out), requesting it in asserted state, and then, if the gpio was
found, just do the fsleep(50) and then deassert it?

Rasmus