Re: [PATCH] gpio: add GPIOD_ACTIVE_LOW into GPIOD_MASK_DIR

[Patrick DELAUNAY <patrick.delaunay@foss.st.com>]

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