All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND PATCH] iio: adis: set GPIO reset pin direction
@ 2021-07-06  9:29 Antti Keränen
  2021-07-07  8:18 ` Lars-Peter Clausen
  2021-07-07  8:36 ` Sa, Nuno
  0 siblings, 2 replies; 16+ messages in thread
From: Antti Keränen @ 2021-07-06  9:29 UTC (permalink / raw)
  To: linux-iio
  Cc: Antti Keränen, Hannu Hartikainen, Lars-Peter Clausen,
	Michael Hennerich, Nuno Sa, Jonathan Cameron

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-06  9:29 [RESEND PATCH] iio: adis: set GPIO reset pin direction Antti Keränen
@ 2021-07-07  8:18 ` Lars-Peter Clausen
  2021-07-07  8:36 ` Sa, Nuno
  1 sibling, 0 replies; 16+ messages in thread
From: Lars-Peter Clausen @ 2021-07-07  8:18 UTC (permalink / raw)
  To: Antti Keränen, linux-iio
  Cc: Hannu Hartikainen, Michael Hennerich, Nuno Sa, Jonathan Cameron

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-06  9:29 [RESEND PATCH] iio: adis: set GPIO reset pin direction Antti Keränen
  2021-07-07  8:18 ` Lars-Peter Clausen
@ 2021-07-07  8:36 ` Sa, Nuno
  2021-07-07 11:53   ` Hannu Hartikainen
  2021-07-07 12:32   ` Lars-Peter Clausen
  1 sibling, 2 replies; 16+ messages in thread
From: Sa, Nuno @ 2021-07-07  8:36 UTC (permalink / raw)
  To: Antti Keränen, linux-iio
  Cc: Hannu Hartikainen, Lars-Peter Clausen, Hennerich, Michael,
	Jonathan Cameron


> 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á


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-07  8:36 ` Sa, Nuno
@ 2021-07-07 11:53   ` Hannu Hartikainen
  2021-07-07 12:25     ` Lars-Peter Clausen
  2021-07-07 12:32     ` [RESEND PATCH] " Sa, Nuno
  2021-07-07 12:32   ` Lars-Peter Clausen
  1 sibling, 2 replies; 16+ messages in thread
From: Hannu Hartikainen @ 2021-07-07 11:53 UTC (permalink / raw)
  To: Sa, Nuno, Antti Keränen, linux-iio
  Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-07 11:53   ` Hannu Hartikainen
@ 2021-07-07 12:25     ` Lars-Peter Clausen
  2021-07-07 13:30       ` Hannu Hartikainen
  2021-07-07 12:32     ` [RESEND PATCH] " Sa, Nuno
  1 sibling, 1 reply; 16+ messages in thread
From: Lars-Peter Clausen @ 2021-07-07 12:25 UTC (permalink / raw)
  To: Hannu Hartikainen, Sa, Nuno, Antti Keränen, linux-iio
  Cc: Hennerich, Michael, Jonathan Cameron

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-07  8:36 ` Sa, Nuno
  2021-07-07 11:53   ` Hannu Hartikainen
@ 2021-07-07 12:32   ` Lars-Peter Clausen
  1 sibling, 0 replies; 16+ messages in thread
From: Lars-Peter Clausen @ 2021-07-07 12:32 UTC (permalink / raw)
  To: Sa, Nuno, Antti Keränen, linux-iio
  Cc: Hannu Hartikainen, Hennerich, Michael, Jonathan Cameron

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.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-07 11:53   ` Hannu Hartikainen
  2021-07-07 12:25     ` Lars-Peter Clausen
@ 2021-07-07 12:32     ` Sa, Nuno
  1 sibling, 0 replies; 16+ messages in thread
From: Sa, Nuno @ 2021-07-07 12:32 UTC (permalink / raw)
  To: Hannu Hartikainen, Antti Keränen, linux-iio
  Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron

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á

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RESEND PATCH] iio: adis: set GPIO reset pin direction
  2021-07-07 12:25     ` Lars-Peter Clausen
@ 2021-07-07 13:30       ` Hannu Hartikainen
  2021-07-08  9:54         ` [PATCH v2] " Antti Keränen
  0 siblings, 1 reply; 16+ messages in thread
From: Hannu Hartikainen @ 2021-07-07 13:30 UTC (permalink / raw)
  To: Lars-Peter Clausen, Sa, Nuno, Antti Keränen, linux-iio
  Cc: Hennerich, Michael, Jonathan Cameron

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-07 13:30       ` Hannu Hartikainen
@ 2021-07-08  9:54         ` Antti Keränen
  2021-07-08 10:05           ` Sa, Nuno
                             ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Antti Keränen @ 2021-07-08  9:54 UTC (permalink / raw)
  To: linux-iio
  Cc: Antti Keränen, Hannu Hartikainen, Lars-Peter Clausen,
	Michael Hennerich, Nuno Sa, Jonathan Cameron, open list

Set reset pin direction to output as the reset pin needs to be an active
low output pin.

Co-developed-by: Hannu Hartikainen <hannu@hrtk.in>
Signed-off-by: Hannu Hartikainen <hannu@hrtk.in>
Signed-off-by: Antti Keränen <detegr@rbx.email>
---
Removed unnecessary toggling of the pin as requested by Lars-Peter. I
missed out on the conversation, but I agree this is better.

 drivers/iio/imu/adis.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 319b64b2fd88..f8b7837d8b8f 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -415,12 +415,11 @@ 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_HIGH);
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
 
 	if (gpio) {
-		gpiod_set_value_cansleep(gpio, 1);
 		msleep(10);
 		/* bring device out of reset */
 		gpiod_set_value_cansleep(gpio, 0);
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* RE: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-08  9:54         ` [PATCH v2] " Antti Keränen
@ 2021-07-08 10:05           ` Sa, Nuno
  2021-07-10 17:35           ` Jonathan Cameron
  2021-07-13 17:53           ` Jonathan Cameron
  2 siblings, 0 replies; 16+ messages in thread
From: Sa, Nuno @ 2021-07-08 10:05 UTC (permalink / raw)
  To: Antti Keränen, linux-iio
  Cc: Hannu Hartikainen, Lars-Peter Clausen, Hennerich, Michael,
	Jonathan Cameron, open list



> -----Original Message-----
> From: Antti Keränen <detegr@rbx.email>
> Sent: Thursday, July 8, 2021 11:54 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>;
> open list <linux-kernel@vger.kernel.org>
> Subject: [PATCH v2] iio: adis: set GPIO reset pin direction
> 
> Set reset pin direction to output as the reset pin needs to be an active
> low output pin.
> 
> Co-developed-by: Hannu Hartikainen <hannu@hrtk.in>
> Signed-off-by: Hannu Hartikainen <hannu@hrtk.in>
> Signed-off-by: Antti Keränen <detegr@rbx.email>
> ---
> Removed unnecessary toggling of the pin as requested by Lars-Peter. I
> missed out on the conversation, but I agree this is better.
> 
>  drivers/iio/imu/adis.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 319b64b2fd88..f8b7837d8b8f 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -415,12 +415,11 @@ 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_HIGH);
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
> 
>  	if (gpio) {
> -		gpiod_set_value_cansleep(gpio, 1);
>  		msleep(10);
>  		/* bring device out of reset */
>  		gpiod_set_value_cansleep(gpio, 0);
> --

Reviewed-by: Nuno Sá <nuno.sa@analog.com> 


Thanks!

- Nuno Sá

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-08  9:54         ` [PATCH v2] " Antti Keränen
  2021-07-08 10:05           ` Sa, Nuno
@ 2021-07-10 17:35           ` Jonathan Cameron
  2021-07-13 17:53           ` Jonathan Cameron
  2 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2021-07-10 17:35 UTC (permalink / raw)
  To: Antti Keränen
  Cc: linux-iio, Hannu Hartikainen, Lars-Peter Clausen,
	Michael Hennerich, Nuno Sa, open list

On Thu,  8 Jul 2021 12:54:29 +0300
Antti Keränen <detegr@rbx.email> wrote:

> Set reset pin direction to output as the reset pin needs to be an active
> low output pin.
> 
> Co-developed-by: Hannu Hartikainen <hannu@hrtk.in>
> Signed-off-by: Hannu Hartikainen <hannu@hrtk.in>
> Signed-off-by: Antti Keränen <detegr@rbx.email>

Hi Antti,

For future reference (in IIO anyway as not all maintainers agree on this!)
please don't send a v2 in reply to v1.  It gets lots if anyone is using
a threaded email client as it's buried deep.  If I'd not been following
the discussion I'd probably have missed this.

Seems correct to me, but will leave on list a few more days as rc1 isn't
out yet so I don't have a good based to start collecting fixes on a the
moment.

Jonathan

> ---
> Removed unnecessary toggling of the pin as requested by Lars-Peter. I
> missed out on the conversation, but I agree this is better.
> 
>  drivers/iio/imu/adis.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 319b64b2fd88..f8b7837d8b8f 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -415,12 +415,11 @@ 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_HIGH);
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
>  
>  	if (gpio) {
> -		gpiod_set_value_cansleep(gpio, 1);
>  		msleep(10);
>  		/* bring device out of reset */
>  		gpiod_set_value_cansleep(gpio, 0);


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-08  9:54         ` [PATCH v2] " Antti Keränen
  2021-07-08 10:05           ` Sa, Nuno
  2021-07-10 17:35           ` Jonathan Cameron
@ 2021-07-13 17:53           ` Jonathan Cameron
  2021-07-14 10:04             ` Antti Keränen
  2 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2021-07-13 17:53 UTC (permalink / raw)
  To: Antti Keränen
  Cc: linux-iio, Hannu Hartikainen, Lars-Peter Clausen,
	Michael Hennerich, Nuno Sa, open list

On Thu,  8 Jul 2021 12:54:29 +0300
Antti Keränen <detegr@rbx.email> wrote:

> Set reset pin direction to output as the reset pin needs to be an active
> low output pin.
> 
> Co-developed-by: Hannu Hartikainen <hannu@hrtk.in>
> Signed-off-by: Hannu Hartikainen <hannu@hrtk.in>
> Signed-off-by: Antti Keränen <detegr@rbx.email>

So this sits on the boundary of whether it is a fix or not.
Do we want this to go into rc1 + stable?

If so a fixes tag would be great.

Thanks,

Jonathan

> ---
> Removed unnecessary toggling of the pin as requested by Lars-Peter. I
> missed out on the conversation, but I agree this is better.
> 
>  drivers/iio/imu/adis.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 319b64b2fd88..f8b7837d8b8f 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -415,12 +415,11 @@ 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_HIGH);
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
>  
>  	if (gpio) {
> -		gpiod_set_value_cansleep(gpio, 1);
>  		msleep(10);
>  		/* bring device out of reset */
>  		gpiod_set_value_cansleep(gpio, 0);


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-13 17:53           ` Jonathan Cameron
@ 2021-07-14 10:04             ` Antti Keränen
  2021-07-14 12:40               ` Jonathan Cameron
  0 siblings, 1 reply; 16+ messages in thread
From: Antti Keränen @ 2021-07-14 10:04 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, Hannu Hartikainen, Lars-Peter Clausen,
	Michael Hennerich, Nuno Sa, open list

On Tue, Jul 13, 2021 at 06:53:11PM +0100, Jonathan Cameron wrote:
> So this sits on the boundary of whether it is a fix or not.
> Do we want this to go into rc1 + stable?

I'm not familiar enough with kernel development to give any opinions of
where this should go, but I would say this is a fix as the current code
won't work with certain hardware configurations.

> If so a fixes tag would be great.

This would mean a v3 patch with fixes tag included into the commit
message, right?

-- 
Antti

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-14 10:04             ` Antti Keränen
@ 2021-07-14 12:40               ` Jonathan Cameron
  2021-07-14 18:25                 ` Antti Keränen
  0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2021-07-14 12:40 UTC (permalink / raw)
  To: Antti Keränen
  Cc: Jonathan Cameron, linux-iio, Hannu Hartikainen,
	Lars-Peter Clausen, Michael Hennerich, Nuno Sa, open list

On Wed, 14 Jul 2021 13:04:45 +0300
Antti Keränen <detegr@rbx.email> wrote:

> On Tue, Jul 13, 2021 at 06:53:11PM +0100, Jonathan Cameron wrote:
> > So this sits on the boundary of whether it is a fix or not.
> > Do we want this to go into rc1 + stable?  
> 
> I'm not familiar enough with kernel development to give any opinions of
> where this should go, but I would say this is a fix as the current code
> won't work with certain hardware configurations.
> 
> > If so a fixes tag would be great.  
> 
> This would mean a v3 patch with fixes tag included into the commit
> message, right?
> 

Send the fixes tag in reply to this and I'll make sure it's added.
No need to bother with a v3 for just that.

Thanks,

Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-14 12:40               ` Jonathan Cameron
@ 2021-07-14 18:25                 ` Antti Keränen
  2021-07-17 17:41                   ` Jonathan Cameron
  0 siblings, 1 reply; 16+ messages in thread
From: Antti Keränen @ 2021-07-14 18:25 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Jonathan Cameron, linux-iio, Hannu Hartikainen,
	Lars-Peter Clausen, Michael Hennerich, Nuno Sa, open list

On Wed, Jul 14, 2021 at 01:40:05PM +0100, Jonathan Cameron wrote:
> Send the fixes tag in reply to this and I'll make sure it's added.
> No need to bother with a v3 for just that.

Fixes: ecb010d44108 ("iio: imu: adis: Refactor adis_initial_startup")

-- 
Antti

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] iio: adis: set GPIO reset pin direction
  2021-07-14 18:25                 ` Antti Keränen
@ 2021-07-17 17:41                   ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2021-07-17 17:41 UTC (permalink / raw)
  To: Antti Keränen
  Cc: Jonathan Cameron, linux-iio, Hannu Hartikainen,
	Lars-Peter Clausen, Michael Hennerich, Nuno Sa, open list

On Wed, 14 Jul 2021 21:25:07 +0300
Antti Keränen <detegr@rbx.email> wrote:

> On Wed, Jul 14, 2021 at 01:40:05PM +0100, Jonathan Cameron wrote:
> > Send the fixes tag in reply to this and I'll make sure it's added.
> > No need to bother with a v3 for just that.  
> 
> Fixes: ecb010d44108 ("iio: imu: adis: Refactor adis_initial_startup")
Great.

Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-07-17 17:39 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-06  9:29 [RESEND PATCH] iio: adis: set GPIO reset pin direction Antti Keränen
2021-07-07  8:18 ` Lars-Peter Clausen
2021-07-07  8:36 ` Sa, Nuno
2021-07-07 11:53   ` Hannu Hartikainen
2021-07-07 12:25     ` Lars-Peter Clausen
2021-07-07 13:30       ` Hannu Hartikainen
2021-07-08  9:54         ` [PATCH v2] " Antti Keränen
2021-07-08 10:05           ` Sa, Nuno
2021-07-10 17:35           ` Jonathan Cameron
2021-07-13 17:53           ` Jonathan Cameron
2021-07-14 10:04             ` Antti Keränen
2021-07-14 12:40               ` Jonathan Cameron
2021-07-14 18:25                 ` Antti Keränen
2021-07-17 17:41                   ` Jonathan Cameron
2021-07-07 12:32     ` [RESEND PATCH] " Sa, Nuno
2021-07-07 12:32   ` Lars-Peter Clausen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.