| Message ID | 20250328193252.1570811-1-mohammed.0.elbadry@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] i2c: npcm: Add clock toggle recovery | expand |
Looks good to me. On 3/29/2025 1:02 AM, mohammed.0.elbadry@gmail.com wrote: > From: Tali Perry <tali.perry1@gmail.com> > > During init of the bus, the module checks that the bus is idle. > If one of the lines are stuck try to recover them first before failing. > Sometimes SDA and SCL are low if improper reset occurs (e.g., reboot). > > Signed-off-by: Tali Perry <tali.perry1@gmail.com> > Signed-off-by: Mohammed Elbadry <mohammed.0.elbadry@gmail.com> Reviewed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > drivers/i2c/busses/i2c-npcm7xx.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > index 2fe68615942e..caf9aa1e6319 100644 > --- a/drivers/i2c/busses/i2c-npcm7xx.c > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > @@ -1967,10 +1967,14 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode, > > /* Check HW is OK: SDA and SCL should be high at this point. */ > if ((npcm_i2c_get_SDA(&bus->adap) == 0) || (npcm_i2c_get_SCL(&bus->adap) == 0)) { > - dev_err(bus->dev, "I2C%d init fail: lines are low\n", bus->num); > - dev_err(bus->dev, "SDA=%d SCL=%d\n", npcm_i2c_get_SDA(&bus->adap), > - npcm_i2c_get_SCL(&bus->adap)); > - return -ENXIO; > + dev_warn(bus->dev, " I2C%d SDA=%d SCL=%d, attempting to recover\n", bus->num, > + npcm_i2c_get_SDA(&bus->adap), npcm_i2c_get_SCL(&bus->adap)); > + if (npcm_i2c_recovery_tgclk(&bus->adap)) { > + dev_err(bus->dev, "I2C%d init fail: SDA=%d SCL=%d\n", > + bus->num, npcm_i2c_get_SDA(&bus->adap), > + npcm_i2c_get_SCL(&bus->adap)); > + return -ENXIO; > + } > } > > npcm_i2c_int_enable(bus, true);
Hi Mohammed, On Fri, Mar 28, 2025 at 07:32:50PM +0000, mohammed.0.elbadry@gmail.com wrote: > From: Tali Perry <tali.perry1@gmail.com> > > During init of the bus, the module checks that the bus is idle. > If one of the lines are stuck try to recover them first before failing. > Sometimes SDA and SCL are low if improper reset occurs (e.g., reboot). > > Signed-off-by: Tali Perry <tali.perry1@gmail.com> > Signed-off-by: Mohammed Elbadry <mohammed.0.elbadry@gmail.com> > --- we are missing the changelog here. You are at v4 and I need to see the changes between the versions. For now it's OK, please, next time don't forget to add the changelog. One more thing, no need to send patches as --in-reply-to your previous patch. > drivers/i2c/busses/i2c-npcm7xx.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c > index 2fe68615942e..caf9aa1e6319 100644 > --- a/drivers/i2c/busses/i2c-npcm7xx.c > +++ b/drivers/i2c/busses/i2c-npcm7xx.c > @@ -1967,10 +1967,14 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode, > > /* Check HW is OK: SDA and SCL should be high at this point. */ > if ((npcm_i2c_get_SDA(&bus->adap) == 0) || (npcm_i2c_get_SCL(&bus->adap) == 0)) { > - dev_err(bus->dev, "I2C%d init fail: lines are low\n", bus->num); > - dev_err(bus->dev, "SDA=%d SCL=%d\n", npcm_i2c_get_SDA(&bus->adap), > - npcm_i2c_get_SCL(&bus->adap)); > - return -ENXIO; > + dev_warn(bus->dev, " I2C%d SDA=%d SCL=%d, attempting to recover\n", bus->num, the space at the beginning of the line should be removed. I will take care of it if you won't be asked to send a new version. > + npcm_i2c_get_SDA(&bus->adap), npcm_i2c_get_SCL(&bus->adap)); > + if (npcm_i2c_recovery_tgclk(&bus->adap)) { > + dev_err(bus->dev, "I2C%d init fail: SDA=%d SCL=%d\n", > + bus->num, npcm_i2c_get_SDA(&bus->adap), > + npcm_i2c_get_SCL(&bus->adap)); > + return -ENXIO; why don't we return the error coming from npcm_i2c_recovery_tgclk() instead of forcing it to ENXIO? Thanks, Andi > + } > } > > npcm_i2c_int_enable(bus, true); > -- > 2.49.0.472.ge94155a9ec-goog >
Hi Mohammed, ... > > > + npcm_i2c_get_SDA(&bus->adap), npcm_i2c_get_SCL(&bus->adap)); > > > + if (npcm_i2c_recovery_tgclk(&bus->adap)) { > > > + dev_err(bus->dev, "I2C%d init fail: SDA=%d SCL=%d\n", > > > + bus->num, npcm_i2c_get_SDA(&bus->adap), > > > + npcm_i2c_get_SCL(&bus->adap)); > > > + return -ENXIO; > > > > why don't we return the error coming from > > npcm_i2c_recovery_tgclk() instead of forcing it to ENXIO? > > The error that comes from the current driver is only -ENOTRECOVERABLE. > I do not see why we cannot return that error instead. I understand the > difference between both states; however, from the driver's > perspective, on initialization, ENXIO and ENOTRECOVERABLE behavior is > identical in that state. I think it makes more sense to return the error we receive from npcm_i2c_recovery_tgclk(). Then we can discuss whether that error is correct or not, but that's a different topic. > ENXIO was what the driver returned by default without trying to > recover. Do you recommend changing it? Let me know. I am happy to send > the v5 patch with the change and address the above comments, too. Yes, please do. Andi
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c index 2fe68615942e..caf9aa1e6319 100644 --- a/drivers/i2c/busses/i2c-npcm7xx.c +++ b/drivers/i2c/busses/i2c-npcm7xx.c @@ -1967,10 +1967,14 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode, /* Check HW is OK: SDA and SCL should be high at this point. */ if ((npcm_i2c_get_SDA(&bus->adap) == 0) || (npcm_i2c_get_SCL(&bus->adap) == 0)) { - dev_err(bus->dev, "I2C%d init fail: lines are low\n", bus->num); - dev_err(bus->dev, "SDA=%d SCL=%d\n", npcm_i2c_get_SDA(&bus->adap), - npcm_i2c_get_SCL(&bus->adap)); - return -ENXIO; + dev_warn(bus->dev, " I2C%d SDA=%d SCL=%d, attempting to recover\n", bus->num, + npcm_i2c_get_SDA(&bus->adap), npcm_i2c_get_SCL(&bus->adap)); + if (npcm_i2c_recovery_tgclk(&bus->adap)) { + dev_err(bus->dev, "I2C%d init fail: SDA=%d SCL=%d\n", + bus->num, npcm_i2c_get_SDA(&bus->adap), + npcm_i2c_get_SCL(&bus->adap)); + return -ENXIO; + } } npcm_i2c_int_enable(bus, true);