[PATCH] i2c: correct I2C deblock logic

[Alexander Kochetkov]

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


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