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