[PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Bough Chen]

> -----Original Message-----
> From: Patrick DELAUNAY <patrick.delaunay at foss.st.com>
> Sent: 2023年3月23日 3:11
> To: Bough Chen <haibo.chen at nxp.com>; al.kochet at gmail.com; hs at denx.de;
> sjg at chromium.org; andrew at aj.id.au; patrice.chotard at foss.st.com;
> samuel at sholland.org; marex at denx.de
> Cc: dl-uboot-imx <uboot-imx at nxp.com>; u-boot at lists.denx.de
> Subject: Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR
> 
> Hi
> 
> On 3/22/23 12:26, haibo.chen at nxp.com wrote:
> > From: Haibo Chen <haibo.chen at 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 at 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