[PATCH] i2c: correct I2C deblock logic

[Alexander Kochetkov]

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.

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