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