> -----Original Message----- > From: Alexander Kochetkov <al.koc...@gmail.com> > Sent: 2023年3月23日 17:34 > To: Bough Chen <haibo.c...@nxp.com> > Cc: h...@denx.de; ma...@denx.de; u-boot@lists.denx.de; dl-uboot-imx > <uboot-...@nxp.com>; xypron.g...@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.koc...@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.c...@nxp.com написал(а): > >> > >> From: Haibo Chen <haibo.c...@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.c...@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 > >> > >