> -----Original Message----- > From: Alexander Kochetkov <al.koc...@gmail.com> > Sent: 2023年3月20日 16:03 > To: h...@denx.de > Cc: Bough Chen <haibo.c...@nxp.com>; ma...@denx.de; > u-boot@lists.denx.de; dl-uboot-imx <uboot-...@nxp.com>; > xypron.g...@gmx.de; Simon Glass <s...@chromium.org> > Subject: Re: [PATCH] i2c: correct I2C deblock logic > > Hello! > > The patch doesn’t add new functionality to the code. > May be it makes code more readable. But in later case the patch description > should be corrected and Fixes tag removed.
Hi Alexander, This patch not only make the code more readable, but also really fix a bug. In our current code, we will call i2c_gpio_set_pin(scl_pin, 1) and then i2c_gpio_set_pin(scl_pin, 0). After the i2c_gpio_set_pin(scl_pin, 0), the flags contain GPIOD_ACTIVE_LOW. Though for SDA, it will first call i2c_gpio_set_pin(sda_pin, 0) then i2c_gpio_set_pin(sda_pin, 1), but 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. Then call i2c_gpio_get_pin by using dm_gpio_get_value(pin) will get the wrong data, let the function i2c_deblock_gpio_loop always return -EREMOTEIO Best Regards Haibo Chen > > The flag GPIOD_ACTIVE_LOW affects return value dm_gpio_get_value(). > And return value doesn’t depends on the DTS settings. It depends on the flag > passed to dm_gpio_set_dir_flags(). Usually the code parses DTS and provide the > correct flag to the dm_gpio_set_dir_flags(). > > So the patch adds flag GPIOD_ACTIVE_LOW to the dm_gpio_set_dir_flags() and > as expected this negates the return value of dm_gpio_get_value(). So in order > to > keep the code correct the patch also fixes adds negotiation to > dm_gpio_get_value(). > > Alexander. > > > > > 20 марта 2023 г., в 10:44, Heiko Schocher <h...@denx.de> написал(а): > > > > Hi! > > > > On 13.03.23 03:55, Bough Chen wrote: > >>> -----Original Message----- > >>> From: Bough Chen <haibo.c...@nxp.com> > >>> Sent: 2023年2月10日 17:27 > >>> To: h...@denx.de; al.koc...@gmail.com; ma...@denx.de > >>> Cc: u-boot@lists.denx.de; dl-uboot-imx <uboot-...@nxp.com>; > >>> xypron.g...@gmx.de; Bough Chen <haibo.c...@nxp.com> > >>> Subject: [PATCH] i2c: correct I2C deblock logic > >>> > >>> 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. > >>> > >> > >> Hi, > >> > >> Any comments for this patch, just a reminder in case you miss it. > > > > Indeed, I missed this patch, sorry! > > Your explanation sounds reasonable to me, but I wonder if nobody > > tapped into this problem... it would be good to have some test reports > > from others! Also added Simon, as he did a lot of work in this space. > > > > Thanks! > > > > bye, > > Heiko > >> > >> Best Regards > >> Haibo Chen > >> > >>> 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 > >> > > > > -- > > DENX Software Engineering GmbH, Managing Director: Erika Unter > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > > Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: h...@denx.de