| Message ID | 20221111143921.742194-6-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: addac: ad74413r: various fixups | expand |
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 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: > > > > > > -----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 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. Jonathan > > - 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 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. Jonathan > > 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
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);
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(+)