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