Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

2023-03-23 Thread Patrick DELAUNAY 

Hi,

On 3/23/23 09:17, Bough Chen wrote:

-Original Message-
From: Patrick DELAUNAY 
Sent: 2023年3月23日 3:11
To: Bough Chen ; al.koc...@gmail.com; h...@denx.de;
s...@chromium.org; and...@aj.id.au; patrice.chot...@foss.st.com;
sam...@sholland.org; ma...@denx.de
Cc: dl-uboot-imx ; 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.c...@nxp.com wrote:

From: Haibo Chen 

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

RE: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

2023-03-23 Thread Bough Chen 
> -Original Message-
> From: Patrick DELAUNAY 
> Sent: 2023年3月23日 3:11
> To: Bough Chen ; al.koc...@gmail.com; h...@denx.de;
> s...@chromium.org; and...@aj.id.au; patrice.chot...@foss.st.com;
> sam...@sholland.org; ma...@denx.de
> Cc: dl-uboot-imx ; 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.c...@nxp.com wrote:
> > From: Haibo Chen 
> >
> > 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 
> > ---
> >   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

2023-03-22 Thread Patrick DELAUNAY 

Hi

On 3/22/23 12:26, haibo.c...@nxp.com wrote:

From: Haibo Chen 

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

2023-03-22 Thread Alexander Kochetkov 
Reviewed-by: Alexander Kochetkov 

> 22 марта 2023 г., в 14:26, haibo.c...@nxp.com написал(а):
> 
> From: Haibo Chen 
> 
> 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 
> ---
> 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
>