[PATCH] i2c: correct I2C deblock logic

[Bough Chen]

> -----Original Message-----
> From: Alexander Kochetkov <al.kochet at gmail.com>
> Sent: 2023年3月20日 16:03
> To: hs at denx.de
> Cc: Bough Chen <haibo.chen at nxp.com>; marex at denx.de;
> u-boot at lists.denx.de; dl-uboot-imx <uboot-imx at nxp.com>;
> xypron.glpk at gmx.de; Simon Glass <sjg at 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 <hs at denx.de> написал(а):
> >
> > Hi!
> >
> > On 13.03.23 03:55, Bough Chen wrote:
> >>> -----Original Message-----
> >>> From: Bough Chen <haibo.chen at nxp.com>
> >>> Sent: 2023年2月10日 17:27
> >>> To: hs at denx.de; al.kochet at gmail.com; marex at denx.de
> >>> Cc: u-boot at lists.denx.de; dl-uboot-imx <uboot-imx at nxp.com>;
> >>> xypron.glpk at gmx.de; Bough Chen <haibo.chen at nxp.com>
> >>> Subject: [PATCH] i2c: correct I2C deblock logic
> >>>
> >>> 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.
> >>>
> >>
> >> 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.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
> >>
> >
> > --
> > 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: hs at denx.de