| Message ID | 20210706092922.v555jjvxbyv52ifw@haukka.localdomain (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jonathan Cameron |
| Headers | show |
| Series | [RESEND] iio: adis: set GPIO reset pin direction | expand |
On 7/6/21 11:29 AM, Antti Keränen wrote: > Use GPIOD_OUT_LOW instead of GPIOD_ASIS as the reset pin needs to be an > active low output pin. > > Suggested-by: Hannu Hartikainen <hannu@hrtk.in> > Signed-off-by: Antti Keränen <detegr@rbx.email> Hi, Thanks for the patch, this looks good. How about requesting it as GPIOD_OUT_HIGH and removing the gpiod_set_value_cansleep(gpio, 1) to avoid unnecessary toggling of the pin. > --- > The documentation of GPIO consumer interface states: > > Be aware that there is no default direction for GPIOs. Therefore, > **using a GPIO without setting its direction first is illegal and will > result in undefined behavior!** > > Therefore the direction of the reset GPIO pin should be set as output. > > drivers/iio/imu/adis.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > index 319b64b2fd88..7f13b3763732 100644 > --- a/drivers/iio/imu/adis.c > +++ b/drivers/iio/imu/adis.c > @@ -415,7 +415,7 @@ int __adis_initial_startup(struct adis *adis) > int ret; > > /* check if the device has rst pin low */ > - gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", GPIOD_ASIS); > + gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(gpio)) > return PTR_ERR(gpio); >
> From: Antti Keränen <detegr@rbx.email> > Sent: Tuesday, July 6, 2021 11:29 AM > To: linux-iio@vger.kernel.org > Cc: Antti Keränen <detegr@rbx.email>; Hannu Hartikainen > <hannu@hrtk.in>; Lars-Peter Clausen <lars@metafoo.de>; Hennerich, > Michael <Michael.Hennerich@analog.com>; Sa, Nuno > <Nuno.Sa@analog.com>; Jonathan Cameron <jic23@kernel.org> > Subject: [RESEND PATCH] iio: adis: set GPIO reset pin direction > > Use GPIOD_OUT_LOW instead of GPIOD_ASIS as the reset pin needs > to be an > active low output pin. > > Suggested-by: Hannu Hartikainen <hannu@hrtk.in> > Signed-off-by: Antti Keränen <detegr@rbx.email> > --- > The documentation of GPIO consumer interface states: > > Be aware that there is no default direction for GPIOs. Therefore, > **using a GPIO without setting its direction first is illegal and will > result in undefined behavior!** > > Therefore the direction of the reset GPIO pin should be set as output. > > drivers/iio/imu/adis.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c > index 319b64b2fd88..7f13b3763732 100644 > --- a/drivers/iio/imu/adis.c > +++ b/drivers/iio/imu/adis.c > @@ -415,7 +415,7 @@ int __adis_initial_startup(struct adis *adis) > int ret; > > /* check if the device has rst pin low */ > - gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", > GPIOD_ASIS); > + gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", > GPIOD_OUT_LOW); > if (IS_ERR(gpio)) > return PTR_ERR(gpio); > Hi, Thanks for the patch. Forcing the device reset was intentional (thus the GPIO_ASIS). But what Lars is suggesting is a good idea and a neat improvement here. - Nuno Sá
Hi! Thanks for reviewing the patch. I'll also chime in. We were working on an ADIS device together with Antti so I've read some of the relevant code, documentation and datasheets. On Wed, 7 Jul 2021 08:36:47 +0000, "Sa, Nuno" <Nuno.Sa@analog.com> wrote: > Thanks for the patch. Forcing the device reset was intentional > (thus the GPIO_ASIS). But what Lars is suggesting is a good idea > and a neat improvement here. I don't understand what you mean. The GPIO consumer documentation[0] states that if GPIO_ASIS is used, the pin direction must be set in driver code later. AFAICT that doesn't happen. If a pin was defined as input by default by the manufacturer, I don't think there's a way to make an ADIS device work with RST on it without patching the driver. Device trees couldn't be used to do that IIRC. I.e. this patch is needed so the device reset works. On Wed, 7 Jul 2021 10:18:57 +0200, Lars-Peter Clausen <lars@metafoo.de> wrote: > How about requesting it as GPIOD_OUT_HIGH and removing the > gpiod_set_value_cansleep(gpio, 1) to avoid unnecessary toggling of the pin. GPIOD_OUT_LOW and GPIOD_OUT_HIGH have different semantics.[1] Is setting the pin to use wrong semantics to save one line of code and possibly toggle the pin one time less worth it? (The ADIS devices whose datasheets I've read have the RST pin as active low, ie. GPIOD_OUT_LOW is semantically correct.) If we really want that, I think the better choice is to use GPIO_ASIS, gpiod_direction_output and gpiod_set_raw_value_cansleep and skip semantically describing the pin altogether. Hannu [0]: https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html > GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must > be set later with one of the dedicated functions. [1]: https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html#the-active-low-and-open-drain-semantics
On 7/7/21 1:53 PM, Hannu Hartikainen wrote: > [...] > GPIOD_OUT_LOW and GPIOD_OUT_HIGH have different semantics.[1] Is setting > the pin to use wrong semantics to save one line of code and possibly > toggle the pin one time less worth it? (The ADIS devices whose > datasheets I've read have the RST pin as active low, ie. GPIOD_OUT_LOW > is semantically correct.) GPIOD_OUT_LOW does not mean that the GPIO is active low. OUT_LOW just means that the GPIO is configured as output in the non-asserted state[1]. Whether it is active low or active high is configured through the flags associated with the GPIO descriptor. E.g. when using devicetree this is typically the field after the GPIO offset. > > If we really want that, I think the better choice is to use GPIO_ASIS, > gpiod_direction_output and gpiod_set_raw_value_cansleep and skip > semantically describing the pin altogether. Requesting as output is the right solution. [1] https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html#obtaining-and-disposing-gpios
On 7/7/21 10:36 AM, Sa, Nuno wrote: >> From: Antti Keränen <detegr@rbx.email> >> Sent: Tuesday, July 6, 2021 11:29 AM >> To: linux-iio@vger.kernel.org >> Cc: Antti Keränen <detegr@rbx.email>; Hannu Hartikainen >> <hannu@hrtk.in>; Lars-Peter Clausen <lars@metafoo.de>; Hennerich, >> Michael <Michael.Hennerich@analog.com>; Sa, Nuno >> <Nuno.Sa@analog.com>; Jonathan Cameron <jic23@kernel.org> >> Subject: [RESEND PATCH] iio: adis: set GPIO reset pin direction >> >> Use GPIOD_OUT_LOW instead of GPIOD_ASIS as the reset pin needs >> to be an >> active low output pin. >> >> Suggested-by: Hannu Hartikainen <hannu@hrtk.in> >> Signed-off-by: Antti Keränen <detegr@rbx.email> >> --- >> The documentation of GPIO consumer interface states: >> >> Be aware that there is no default direction for GPIOs. Therefore, >> **using a GPIO without setting its direction first is illegal and will >> result in undefined behavior!** >> >> Therefore the direction of the reset GPIO pin should be set as output. >> >> drivers/iio/imu/adis.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c >> index 319b64b2fd88..7f13b3763732 100644 >> --- a/drivers/iio/imu/adis.c >> +++ b/drivers/iio/imu/adis.c >> @@ -415,7 +415,7 @@ int __adis_initial_startup(struct adis *adis) >> int ret; >> >> /* check if the device has rst pin low */ >> - gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", >> GPIOD_ASIS); >> + gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", >> GPIOD_OUT_LOW); >> if (IS_ERR(gpio)) >> return PTR_ERR(gpio); >> > Hi, > > Thanks for the patch. Forcing the device reset was intentional > (thus the GPIO_ASIS). But what Lars is suggesting is a good idea > and a neat improvement here. > GPIO_ASIS leaves the direction of the GPIO untouched. If the default is input, the GPIO will stay as an input, in which case triggering the reset does not work.
Hi, > From: Hannu Hartikainen <hannu@hrtk.in> > Sent: Wednesday, July 7, 2021 1:53 PM > To: Sa, Nuno <Nuno.Sa@analog.com>; Antti Keränen > <detegr@rbx.email>; linux-iio@vger.kernel.org > Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael > <Michael.Hennerich@analog.com>; Jonathan Cameron > <jic23@kernel.org> > Subject: RE: [RESEND PATCH] iio: adis: set GPIO reset pin direction > > Hi! > > Thanks for reviewing the patch. I'll also chime in. We were working on > an ADIS device together with Antti so I've read some of the relevant > code, documentation and datasheets. > > On Wed, 7 Jul 2021 08:36:47 +0000, "Sa, Nuno" > <Nuno.Sa@analog.com> wrote: > > Thanks for the patch. Forcing the device reset was intentional > > (thus the GPIO_ASIS). But what Lars is suggesting is a good idea > > and a neat improvement here. > > I don't understand what you mean. The GPIO consumer > documentation[0] > states that if GPIO_ASIS is used, the pin direction must be set in > driver code later. AFAICT that doesn't happen. > > If a pin was defined as input by default by the manufacturer, I don't > think there's a way to make an ADIS device work with RST on it without > patching the driver. Device trees couldn't be used to do that IIRC. I.e. > this patch is needed so the device reset works. Yes, you're right. This is pretty much assuming that the pin is already an output one. Though you can typically make sure that the pin will be configured as an output one (through pinctrl) which is what we are doing in the devicetree overlays for the adis16475 device for example (in ADI trees). > On Wed, 7 Jul 2021 10:18:57 +0200, Lars-Peter Clausen > <lars@metafoo.de> wrote: > > How about requesting it as GPIOD_OUT_HIGH and removing the > > gpiod_set_value_cansleep(gpio, 1) to avoid unnecessary toggling of > the pin. > > GPIOD_OUT_LOW and GPIOD_OUT_HIGH have different > semantics.[1] Is setting > the pin to use wrong semantics to save one line of code and possibly > toggle the pin one time less worth it? (The ADIS devices whose > datasheets I've read have the RST pin as active low, ie. > GPIOD_OUT_LOW > is semantically correct.) Well, AFAIK all the ADIS devices so far have the RST pin active low so this a very fair assumption to make and use GPIOD_OUT_HIGH. It can always be changed afterwards or even add a flag to the adis_data structure... > If we really want that, I think the better choice is to use GPIO_ASIS, > gpiod_direction_output and gpiod_set_raw_value_cansleep and skip > semantically describing the pin altogether. Yes, we want to make sure a reset is done on the device. So, personally, I'm fine with either approach. Either using GPIOD_OUT_HIGH or GPIO_ASIS with the extra 'gpiod_direction_output()'... - Nuno Sá
On Wed, 7 Jul 2021 14:25:06 +0200, Lars-Peter Clausen <lars@metafoo.de> wrote: > GPIOD_OUT_LOW does not mean that the GPIO is active low. OUT_LOW just > means that the GPIO is configured as output in the non-asserted > state[1]. Whether it is active low or active high is configured through > the flags associated with the GPIO descriptor. E.g. when using > devicetree this is typically the field after the GPIO offset. Oh, I misunderstood that. Thanks for pointing it out and sorry for the confusion! Your original suggestion of using GPIOD_OUT_HIGH sounds good to me, then. Hannu
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c index 319b64b2fd88..7f13b3763732 100644 --- a/drivers/iio/imu/adis.c +++ b/drivers/iio/imu/adis.c @@ -415,7 +415,7 @@ int __adis_initial_startup(struct adis *adis) int ret; /* check if the device has rst pin low */ - gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", GPIOD_ASIS); + gpio = devm_gpiod_get_optional(&adis->spi->dev, "reset", GPIOD_OUT_LOW); if (IS_ERR(gpio)) return PTR_ERR(gpio);
Use GPIOD_OUT_LOW instead of GPIOD_ASIS as the reset pin needs to be an active low output pin. Suggested-by: Hannu Hartikainen <hannu@hrtk.in> Signed-off-by: Antti Keränen <detegr@rbx.email> --- The documentation of GPIO consumer interface states: Be aware that there is no default direction for GPIOs. Therefore, **using a GPIO without setting its direction first is illegal and will result in undefined behavior!** Therefore the direction of the reset GPIO pin should be set as output. drivers/iio/imu/adis.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)