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