diff mbox series

gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

Message ID 20230322112604.33232-1-haibo.chen@nxp.com
State Rejected
Delegated to: Patrick Delaunay
Headers show
Series gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR | expand

Commit Message

Bough Chen March 22, 2023, 11:26 a.m. UTC 
From: Haibo Chen <haibo.chen@nxp.com>

dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags.
But there are cases like i2c_deblock_gpio_loop() will do like this:

-first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW
dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
			   GPIOD_ACTIVE_LOW |
			   GPIOD_IS_OUT_ACTIVE);

-then config GPIO input
dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);

-then get the GPIO input value:
dm_gpio_get_value(pin);

When config the GPIO input, only set GPIOD_IS_IN, but unfortunately
since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in
flags, make the value from dm_gpio_get_value() not logic correct.

So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 include/asm-generic/gpio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexander Kochetkov March 22, 2023, 11:30 a.m. UTC | #1 
Reviewed-by: Alexander Kochetkov <al.kochet@gmail.com>

> 22 марта 2023 г., в 14:26, haibo.chen@nxp.com написал(а):
> 
> From: Haibo Chen <haibo.chen@nxp.com>
> 
> dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags.
> But there are cases like i2c_deblock_gpio_loop() will do like this:
> 
> -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW
> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
>   GPIOD_ACTIVE_LOW |
>   GPIOD_IS_OUT_ACTIVE);
> 
> -then config GPIO input
> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> 
> -then get the GPIO input value:
> dm_gpio_get_value(pin);
> 
> When config the GPIO input, only set GPIOD_IS_IN, but unfortunately
> since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in
> flags, make the value from dm_gpio_get_value() not logic correct.
> 
> So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
> include/asm-generic/gpio.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index dd0bdf2315..903b237aac 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -131,7 +131,7 @@ struct gpio_desc {
> 
> /* Flags for updating the above */
> #define GPIOD_MASK_DIR (GPIOD_IS_OUT | GPIOD_IS_IN | \
> - GPIOD_IS_OUT_ACTIVE)
> + GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW)
> #define GPIOD_MASK_DSTYPE (GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE)
> #define GPIOD_MASK_PULL (GPIOD_PULL_UP | GPIOD_PULL_DOWN)
> 
> -- 
> 2.34.1
>
Patrick Delaunay March 22, 2023, 7:10 p.m. UTC | #2 
Hi

On 3/22/23 12:26, haibo.chen@nxp.com wrote:
> From: Haibo Chen <haibo.chen@nxp.com>
>
> dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags.
> But there are cases like i2c_deblock_gpio_loop() will do like this:
>
> -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW
> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
> 			   GPIOD_ACTIVE_LOW |
> 			   GPIOD_IS_OUT_ACTIVE);
>
> -then config GPIO input
> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>
> -then get the GPIO input value:
> dm_gpio_get_value(pin);
>
> When config the GPIO input, only set GPIOD_IS_IN, but unfortunately
> since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in
> flags, make the value from dm_gpio_get_value() not logic correct.
>
> So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue.
>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>   include/asm-generic/gpio.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index dd0bdf2315..903b237aac 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -131,7 +131,7 @@ struct gpio_desc {
>   
>   /* Flags for updating the above */
>   #define GPIOD_MASK_DIR		(GPIOD_IS_OUT | GPIOD_IS_IN | \
> -					GPIOD_IS_OUT_ACTIVE)
> +				 GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW)
>   #define GPIOD_MASK_DSTYPE	(GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE)
>   #define GPIOD_MASK_PULL		(GPIOD_PULL_UP | GPIOD_PULL_DOWN)
>   


I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by 
device tree in the GPIO uclass:

because the modified GPIOD_MASK_DIR is used in other location....


normally GPIOD_ACTIVE_LOW is saved in desc->flags

it is the "desciptor flags" and must be not cleary by normal API


see gpio_xlate_offs_flags() => gpio_flags_xlate()



For example in  gpio_request_tail(), in the line:

/* Keep any direction flags provided by the devicetree */
ret = dm_gpio_set_dir_flags(desc,
flags | (desc->flags& GPIOD_MASK_DIR));
With your patch the descriptor flags is cleared / so DT information is lost.
For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect.
and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it 
is normal to inverse it for INPUT and OUTPUT
it is managed in GPIO U-Class
=> dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW
you can change the caller ?
static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) { if (bit) 
dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); else dm_gpio_set_dir_flags(pin, 
GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); }
=>

static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit)
{
	if (bit)
		dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
	else
		dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT);
}

The output value is the same => GPIOD_ACTIVE_LOW and GPIOD_IS_OUT_ACTIVE 
not active
but you don't need to modify GPIOD_ACTIVE_LOW outside the GPIO uclass.
Patrick
Bough Chen March 23, 2023, 8:17 a.m. UTC | #3 
> -----Original Message-----
> From: Patrick DELAUNAY <patrick.delaunay@foss.st.com>
> Sent: 2023年3月23日 3:11
> To: Bough Chen <haibo.chen@nxp.com>; al.kochet@gmail.com; hs@denx.de;
> sjg@chromium.org; andrew@aj.id.au; patrice.chotard@foss.st.com;
> samuel@sholland.org; marex@denx.de
> Cc: dl-uboot-imx <uboot-imx@nxp.com>; u-boot@lists.denx.de
> Subject: Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR
> 
> Hi
> 
> On 3/22/23 12:26, haibo.chen@nxp.com wrote:
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags.
> > But there are cases like i2c_deblock_gpio_loop() will do like this:
> >
> > -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW
> > dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
> > 			   GPIOD_ACTIVE_LOW |
> > 			   GPIOD_IS_OUT_ACTIVE);
> >
> > -then config GPIO input
> > dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> >
> > -then get the GPIO input value:
> > dm_gpio_get_value(pin);
> >
> > When config the GPIO input, only set GPIOD_IS_IN, but unfortunately
> > since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in
> > flags, make the value from dm_gpio_get_value() not logic correct.
> >
> > So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >   include/asm-generic/gpio.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > index dd0bdf2315..903b237aac 100644
> > --- a/include/asm-generic/gpio.h
> > +++ b/include/asm-generic/gpio.h
> > @@ -131,7 +131,7 @@ struct gpio_desc {
> >
> >   /* Flags for updating the above */
> >   #define GPIOD_MASK_DIR		(GPIOD_IS_OUT | GPIOD_IS_IN | \
> > -					GPIOD_IS_OUT_ACTIVE)
> > +				 GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW)
> >   #define GPIOD_MASK_DSTYPE	(GPIOD_OPEN_DRAIN |
> GPIOD_OPEN_SOURCE)
> >   #define GPIOD_MASK_PULL		(GPIOD_PULL_UP |
> GPIOD_PULL_DOWN)
> >
> 
> 
> I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by
> device tree in the GPIO uclass:
> 
> because the modified GPIOD_MASK_DIR is used in other location....
> 
> 
> normally GPIOD_ACTIVE_LOW is saved in desc->flags
> 
> it is the "desciptor flags" and must be not cleary by normal API
> 
> 
> see gpio_xlate_offs_flags() => gpio_flags_xlate()
> 
> 
> 
> For example in  gpio_request_tail(), in the line:
> 
> /* Keep any direction flags provided by the devicetree */ ret =
> dm_gpio_set_dir_flags(desc, flags | (desc->flags& GPIOD_MASK_DIR)); With
> your patch the descriptor flags is cleared / so DT information is lost.
> For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect.
> and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it is
> normal to inverse it for INPUT and OUTPUT it is managed in GPIO U-Class =>
> dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW you can
> change the caller ?
> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) { if (bit)
> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); else dm_gpio_set_dir_flags(pin,
> GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); } =>
> 
> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
> 	if (bit)
> 		dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
> 	else
> 		dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT); }

Here, for i2c-deblock, when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), the software actually want to config the gpio at output, and config the output to low level.
This means in dts, need to config the i2c gpio as GPIO_ACTIVE_HIGH, then when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), it finally config the value to 0, which means low level.

But from user point, we usually take the i2c gpio as GPIO_ACTIVE_LOW, seems a bit conflict.

Any thoughts? Or just use my first patch?

Best Regards
Haibo Chen

> 
> The output value is the same => GPIOD_ACTIVE_LOW and
> GPIOD_IS_OUT_ACTIVE not active but you don't need to modify
> GPIOD_ACTIVE_LOW outside the GPIO uclass.
> Patrick
Patrick Delaunay March 23, 2023, 9:05 a.m. UTC | #4 
Hi,

On 3/23/23 09:17, Bough Chen wrote:
>> -----Original Message-----
>> From: Patrick DELAUNAY <patrick.delaunay@foss.st.com>
>> Sent: 2023年3月23日 3:11
>> To: Bough Chen <haibo.chen@nxp.com>; al.kochet@gmail.com; hs@denx.de;
>> sjg@chromium.org; andrew@aj.id.au; patrice.chotard@foss.st.com;
>> samuel@sholland.org; marex@denx.de
>> Cc: dl-uboot-imx <uboot-imx@nxp.com>; u-boot@lists.denx.de
>> Subject: Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR
>>
>> Hi
>>
>> On 3/22/23 12:26, haibo.chen@nxp.com wrote:
>>> From: Haibo Chen <haibo.chen@nxp.com>
>>>
>>> dm_gpio_set_dir_flags() will clear GPIOD_MASK_DIR and set new flags.
>>> But there are cases like i2c_deblock_gpio_loop() will do like this:
>>>
>>> -first conifg GPIO(SDA) output with GPIOD_ACTIVE_LOW
>>> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
>>> 			   GPIOD_ACTIVE_LOW |
>>> 			   GPIOD_IS_OUT_ACTIVE);
>>>
>>> -then config GPIO input
>>> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>>>
>>> -then get the GPIO input value:
>>> dm_gpio_get_value(pin);
>>>
>>> When config the GPIO input, only set GPIOD_IS_IN, but unfortunately
>>> since the previous GPIOD_ACTIVE_LOW is not cleared, still keep in
>>> flags, make the value from dm_gpio_get_value() not logic correct.
>>>
>>> So add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR to avoid this issue.
>>>
>>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
>>> ---
>>>    include/asm-generic/gpio.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
>>> index dd0bdf2315..903b237aac 100644
>>> --- a/include/asm-generic/gpio.h
>>> +++ b/include/asm-generic/gpio.h
>>> @@ -131,7 +131,7 @@ struct gpio_desc {
>>>
>>>    /* Flags for updating the above */
>>>    #define GPIOD_MASK_DIR		(GPIOD_IS_OUT | GPIOD_IS_IN | \
>>> -					GPIOD_IS_OUT_ACTIVE)
>>> +				 GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW)
>>>    #define GPIOD_MASK_DSTYPE	(GPIOD_OPEN_DRAIN |
>> GPIOD_OPEN_SOURCE)
>>>    #define GPIOD_MASK_PULL		(GPIOD_PULL_UP |
>> GPIOD_PULL_DOWN)
>>
>> I think you are breaking the management of GPIOD_ACTIVE_LOW, provided by
>> device tree in the GPIO uclass:
>>
>> because the modified GPIOD_MASK_DIR is used in other location....
>>
>>
>> normally GPIOD_ACTIVE_LOW is saved in desc->flags
>>
>> it is the "desciptor flags" and must be not cleary by normal API
>>
>>
>> see gpio_xlate_offs_flags() => gpio_flags_xlate()
>>
>>
>>
>> For example in  gpio_request_tail(), in the line:
>>
>> /* Keep any direction flags provided by the devicetree */ ret =
>> dm_gpio_set_dir_flags(desc, flags | (desc->flags& GPIOD_MASK_DIR)); With
>> your patch the descriptor flags is cleared / so DT information is lost.
>> For me GPIOD_ACTIVE_LOW must be managed carefully to avoid side effect.
>> and if you inverse the PIN logical in device tree (GPIOD_ACTIVE_LOW) it is
>> normal to inverse it for INPUT and OUTPUT it is managed in GPIO U-Class =>
>> dm_gpio_set_dir_flagsshould not cleared this flag GPIOD_ACTIVE_LOW you can
>> change the caller ?
>> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) { if (bit)
>> dm_gpio_set_dir_flags(pin, GPIOD_IS_IN); else dm_gpio_set_dir_flags(pin,
>> GPIOD_IS_OUT | GPIOD_ACTIVE_LOW | GPIOD_IS_OUT_ACTIVE); } =>
>>
>> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
>> 	if (bit)
>> 		dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>> 	else
>> 		dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT); }
> Here, for i2c-deblock, when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), the software actually want to config the gpio at output, and config the output to low level.
> This means in dts, need to config the i2c gpio as GPIO_ACTIVE_HIGH, then when call dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT), it finally config the value to 0, which means low level.
>
> But from user point, we usually take the i2c gpio as GPIO_ACTIVE_LOW, seems a bit conflict.
>
> Any thoughts? Or just use my first patch?

I am lost (I am not dig in I2C GPIO part)

but if I assume that GPIO_ACTIVE_HIGH is NOT activated in DT

(because the GPIO line is directly connected to the I2C device)


Then GPIO line = HIGH  => GPIO value is 1 for uclass (input or output)


=> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT);

       if you want to have output  LOW


=> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);

        if you want to have output  HIGH


You can  select what it is expected.... in I2C

	input => GPIO input selection

	output => ouput value selected by bit

for example

i2c_gpio_set_pin(struct gpio_desc *pin, int input, int bit) {

	if (input)
		dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
	else if (bit)
		dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
	else
		dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT);
}


if the GPIO is inverted in DT (that means some inverse logic exist on 
hardware), with GPIO_ACTIVE_LOW

=> the result is inverted as expected for INPUT and OUTPUT


if the logic is always inverted => you need to change the caller 
(request 1 instead 0)


For me the SW side in U-boot should be not take care of GPIO_ACTIVE_LOW, 
except the GPIO uclass.


for me your patch is not acceptable

it solves the I2C GPIO issue but break ALL the other users of GPIO uclass.


Regards

Patrick
diff mbox series

Patch

diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index dd0bdf2315..903b237aac 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -131,7 +131,7 @@  struct gpio_desc {
 
 /* Flags for updating the above */
 #define GPIOD_MASK_DIR		(GPIOD_IS_OUT | GPIOD_IS_IN | \
-					GPIOD_IS_OUT_ACTIVE)
+				 GPIOD_IS_OUT_ACTIVE | GPIOD_ACTIVE_LOW)
 #define GPIOD_MASK_DSTYPE	(GPIOD_OPEN_DRAIN | GPIOD_OPEN_SOURCE)
 #define GPIOD_MASK_PULL		(GPIOD_PULL_UP | GPIOD_PULL_DOWN)