[PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Patrick DELAUNAY]

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);
}

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