[PATCH] i2c: correct I2C deblock logic

[Alexander Kochetkov]

> If use this code, must make sure dts use GPIO_ACTIVE_LOW, otherwise 
> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE) will config gpio output high, this is not what we want.

Patrik told that if we don’t have GPIO_ACTIVE_LOW  in the DTS, this is the case where hardware has some sort of
signal inversion inside. May be there is no such hardware now.
When we set gpio output active, it became high inside hardware, hardware set i2c line low.
If you think that previous version of my today’s patch is more correct, try it.

> 
> And here use  !dm_gpio_get_value(pin), this already hit use GPIO_ACTIVE_LOW, why not directly use GPIOD_ACTIVE_LOW
> when call dm_gpio_set_dir_flags, this make code more readable, and make sure the code logic can work and do not depends
> on dts setting.

Patric told, that we must not use GPIOD_ACTIVE_LOW in client code. So you must drop it. From any patch you send.
GPIOD_ACTIVE_LOW is not applied to gpio flags when you call dm_gpio_set_dir_flags(). It is configured from DTS.
You think it is applied, but actually not. It’s your finding. Here your words:

> i2c_gpio_set_pin-> dm_gpio_set_dir_flags-> dm_gpio_clrset_flags, it only clear GPIOD_MASK_DIR, this has no impact with GPIOD_ACTIVE_LOW. So this GPIOD_ACTIVE_LOW keep in flags.


> 23 марта 2023 г., в 13:41, Bough Chen <haibo.chen at nxp.com> написал(а):
> 
>> -----Original Message-----
>> From: Alexander Kochetkov <al.kochet at gmail.com <mailto:al.kochet at gmail.com>>
>> Sent: 2023年3月23日 17:34
>> To: Bough Chen <haibo.chen at nxp.com <mailto:haibo.chen at nxp.com>>
>> Cc: hs at denx.de <mailto:hs at denx.de>; marex at denx.de <mailto:marex at denx.de>; u-boot at lists.denx.de <mailto:u-boot at lists.denx.de>; dl-uboot-imx
>> <uboot-imx at nxp.com <mailto:uboot-imx at nxp.com>>; xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
>> Subject: Re: [PATCH] i2c: correct I2C deblock logic
>> 
>> Or even simpler. Like your original patch. If we take into accout Patrik’s
>> comment from another message:
>> 
>>> but if I assume that GPIO_ACTIVE_HIGH is NOT activated in DT
>> 
>> 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_IS_OUT_ACTIVE);
>>   }
>> }
>> 
>> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
>>    return !dm_gpio_get_value(pin);
>> }
> 
> 
> 
> And here use  !dm_gpio_get_value(pin), this already hit use GPIO_ACTIVE_LOW, why not directly use GPIOD_ACTIVE_LOW
> when call dm_gpio_set_dir_flags, this make code more readable, and make sure the code logic can work and do not depends
> on dts setting.
> 
> Best Regards
> Haibo Chen
>> 
>> 
>>> 23 марта 2023 г., в 11:43, Alexander Kochetkov <al.kochet at gmail.com>
>> написал(а):
>>> 
>>> Hello Haibo Chen!
>>> 
>>> Setting GPIOD_ACTIVE_LOW has no effect. It filtered out by
>> dm_gpio_set_dir_flags().
>>>> 
>>>> 
>>>> if (bit)
>>>> - dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>>>> + dm_gpio_set_dir_flags(pin, GPIOD_IS_IN |
>>>> +    GPIOD_ACTIVE_LOW);
>>> 
>>> Here in original code GPIOD_ACTIVE_LOW has not effect.
>>> 
>>> else
>>>   dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |
>>> 
>> GPIOD_ACTIVE_LOW |
>>> 
>> GPIOD_IS_OUT_ACTIVE);
>>> 
>>> 
>>> This code actually fix problem with your DTS-settings:
>>>> + return !dm_gpio_get_value(pin);
>>> 
>>> 
>>> Can you debug and check using oscilloscope the code like this:
>>> 
>>> static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
>>>   ulong flags;
>>>   ulong active;
>>> 
>>>   if (bit) {
>>>       dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>>>   } else {
>>>       dm_gpio_get_flags(pin, &flags);
>>>       active = (flags & GPIOD_ACTIVE_LOW) ? GPIOD_IS_OUT_ACTIVE :
>> 0;
>>>       dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT | active);
>>>   }
>>> }
>>> 
>>> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
>>>    ulong flags;
>>>    int value;
>>> 
>>>    value = dm_gpio_get_value(pin);
>>>    return (flags & GPIOD_ACTIVE_LOW) ? !value : value; }
>>> 
>>> 
>>>> 10 февр. 2023 г., в 12:27, haibo.chen at nxp.com написал(а):
>>>> 
>>>> From: Haibo Chen <haibo.chen at nxp.com>
>>>> 
>>>> Current code use dm_gpio_get_value() to get SDA and SCL value, and
>>>> the value depends on the flag GPIOD_ACTIVE_LOW. When toggle SCL to
>>>> wait slave release SDA, the SDA are config as GPIOD_IS_IN, and
>>>> whether contain the GPIOD_ACTIVE_LOW depends on the DTS setting.
>>>> Usually, for I2C GPIO, we use GPIOD_ACTIVE_LOW flag in DTS, so if the
>>>> SDA is in low level, then
>>>> dm_gpio_get_value() will return 1, current code logic will stop the
>>>> SCL toggle wrongly, cause the I2C deblock not work as expect.
>>>> 
>>>> This patch force set the GPIOD_ACTIVE_LOW for both GPIOD_IS_IN and
>>>> GPIOD_IS_OUT, and make the return value of i2c_gpio_get_pin() eaqual
>>>> to the physical voltage logic level.
>>>> 
>>>> Fixes: aa54192d4a87 ("dm: i2c: implement gpio-based I2C deblock")
>>>> Signed-off-by: Haibo Chen <haibo.chen at nxp.com>
>>>> ---
>>>> drivers/i2c/i2c-uclass.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
>>>> index 8d9a89ed89..4dc707c659 100644
>>>> --- a/drivers/i2c/i2c-uclass.c
>>>> +++ b/drivers/i2c/i2c-uclass.c
>>>> @@ -505,7 +505,8 @@ uint i2c_get_chip_addr_offset_mask(struct udevice
>>>> *dev) static void i2c_gpio_set_pin(struct gpio_desc *pin, int bit) {
>>>> if (bit)
>>>> - dm_gpio_set_dir_flags(pin, GPIOD_IS_IN);
>>>> + dm_gpio_set_dir_flags(pin, GPIOD_IS_IN |
>>>> +   GPIOD_ACTIVE_LOW);
>>>> else
>>>> dm_gpio_set_dir_flags(pin, GPIOD_IS_OUT |  GPIOD_ACTIVE_LOW | @@
>>>> -514,7 +515,7 @@ static void i2c_gpio_set_pin(struct gpio_desc *pin,
>>>> int bit)
>>>> 
>>>> static int i2c_gpio_get_pin(struct gpio_desc *pin) {
>>>> - return dm_gpio_get_value(pin);
>>>> + return !dm_gpio_get_value(pin);
>>>> }
>>>> 
>>>> int i2c_deblock_gpio_loop(struct gpio_desc *sda_pin,
>>>> --
>>>> 2.34.1