[PATCH] i2c: correct I2C deblock logic

[Bough Chen]

> -----Original Message-----
> From: Alexander Kochetkov <al.kochet at gmail.com>
> Sent: 2023年3月23日 17:34
> To: Bough Chen <haibo.chen at nxp.com>
> Cc: hs at denx.de; marex at denx.de; u-boot at lists.denx.de; dl-uboot-imx
> <uboot-imx at nxp.com>; 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);
> }

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.

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