Hi,
On 3/23/23 09:17, Bough Chen wrote:
-----Original Message-----
From: Patrick DELAUNAY <patrick.delau...@foss.st.com>
Sent: 2023年3月23日 3:11
To: Bough Chen <haibo.c...@nxp.com>; 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 <uboot-...@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.c...@nxp.com wrote:
From: Haibo Chen <haibo.c...@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.c...@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
