[PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[haibo.chen]

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

Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Alexander Kochetkov]

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
> 

Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Patrick DELAUNAY]

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

RE: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Bough Chen]

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

Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Patrick DELAUNAY]

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