Re: [PATCH] i2c: cadence: Fix regression with bus recovery

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 11/28/22 06:24, Datta, Shubhrajyoti wrote:
[AMD Official Use Only - General]


Hi ,
Thanks for the patch

-----Original Message-----
From: carsten.haitzler@xxxxxxxxxxxx <carsten.haitzler@xxxxxxxxxxxx>
Sent: Friday, November 25, 2022 5:21 PM
To: michal.simek@xxxxxxxxxx; shubhrajyoti.datta@xxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx
Cc: torvalds@xxxxxxxxxxxxxxxxxxxx
Subject: [PATCH] i2c: cadence: Fix regression with bus recovery

CAUTION: This message has originated from an External Source. Please use
proper judgment and caution when opening attachments, clicking links, or
responding to this email.


From: Carsten Haitzler <carsten.haitzler@xxxxxxx>

Commit "i2c: cadence: Add standard bus recovery support" breaks for i2c
devices that have no pinctrl defined. There is no requirement for this to exist
in the DT. This has worked perfectly well without this before in at least 1 real
usage case on hardware (Mali Komeda DPU, Cadence i2c to talk to a tda99xx
phy). Adding the requirement to have pinctrl set up in the device tree (or
otherwise be found) is a regression where the whole i2c device is lost
entirely (in this case dropping entire devices which then leads to the drm
display stack unable to find the phy for display output, thus having no drm
display device and so on down the chain).

This converts the above commit to an enhancement if pinctrl can be found
for the i2c device, providing a timeout on read with recovery, but if not, do
what used to be done rather than a fatal loss of a device.

This restores the mentioned display devices to their working state again.

Fixes: 58b924241d0a ("i2c: cadence: Add standard bus recovery support")
Signed-off-by: Carsten Haitzler <carsten.haitzler@xxxxxxx>
---
Note: This issue was discovered during the porting of the linux kernel on
Morello [1].

[1]
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
morello-
project.org%2Fmorello%2Fkernel%2Flinux&amp;data=05%7C01%7Cshubhraj
yoti.datta%40amd.com%7C651e141296fb497b6c2e08dacedb65ad%7C3dd896
1fe4884e608e11a82d994e183d%7C0%7C0%7C638049738949855658%7CUnkn
own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ny08zTdgJKxXC
dwYaCWFS3mwd65LDqFAIYfjao6gO8Y%3D&amp;reserved=0
---
  drivers/i2c/busses/i2c-cadence.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-
cadence.c
index fe0cd205502d..40d0cc6bb996 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -852,7 +852,8 @@ static int cdns_i2c_master_xfer(struct i2c_adapter
*adap, struct i2c_msg *msgs,
                                          CDNS_I2C_POLL_US, CDNS_I2C_TIMEOUT_US);
         if (ret) {
                 ret = -EAGAIN;
-               i2c_recover_bus(adap);
+               if (id->adap.bus_recovery_info)
+                       i2c_recover_bus(adap);
                 goto out;
         }

@@ -1262,10 +1263,10 @@ static int cdns_i2c_probe(struct platform_device
*pdev)
         }

         id->rinfo.pinctrl = devm_pinctrl_get(&pdev->dev);
-       if (IS_ERR(id->rinfo.pinctrl)) {
+       if (IS_ERR(id->rinfo.pinctrl))
                 dev_info(&pdev->dev, "can't get pinctrl, bus recovery not
supported\n");
-               return PTR_ERR(id->rinfo.pinctrl);
-       }
+       else
+               id->adap.bus_recovery_info = &id->rinfo;

May be we could differentiate between  ENODEV and  other errors.
As we want to do this for the case when it is not provided.

We could, but just keep in mind this patch is essentially restoring behavior on any pinctrl error to previous behavior - so trying to undo the regression as strictly as possible.

Certainly checking specifically for ENODEV does fix it for me which is the case of no pinctrl. I'll send a v2.

         id->membase = devm_platform_get_and_ioremap_resource(pdev, 0,
&r_mem);
         if (IS_ERR(id->membase))
@@ -1283,7 +1284,6 @@ static int cdns_i2c_probe(struct platform_device
*pdev)
         id->adap.retries = 3;           /* Default retry value. */
         id->adap.algo_data = id;
         id->adap.dev.parent = &pdev->dev;
-       id->adap.bus_recovery_info = &id->rinfo;
         init_completion(&id->xfer_done);
         snprintf(id->adap.name, sizeof(id->adap.name),
                  "Cadence I2C at %08lx", (unsigned long)r_mem->start);
--
2.32.0



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux