Hi Wolfram,
On Thu, Apr 11, 2024 at 09:02:52AM +0200, Wolfram Sang wrote:
> On Wed, Apr 10, 2024 at 02:21:58PM +0200, Andi Shyti wrote:
> > On Wed, Apr 10, 2024 at 01:24:20PM +0200, Wolfram Sang wrote:
> > > I2C and SMBus timeouts are not something the user needs to be informed
> > > about on controller level. The client driver may know if that really is
> > > a problem and give more detailed information to the user. The controller
> > > should just pass this information upwards. Turn all timeout related
> > > printouts to debug level.
> > >
> > > Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >
> > > Here, I did not delete the printout to support checking the termination
> > > process. The other drivers in this series do not have this SMBus
> > > specific termination step.
> > >
> > > drivers/i2c/busses/i2c-i801.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > > index 4294c0c63cef..a42b5152f9bd 100644
> > > --- a/drivers/i2c/busses/i2c-i801.c
> > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > @@ -400,7 +400,7 @@ static int i801_check_post(struct i801_priv *priv, int status)
> > > * If the SMBus is still busy, we give up
> > > */
> > > if (unlikely(status < 0)) {
> > > - dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> > > + dev_dbg(&priv->pci_dev->dev, "Transaction timeout\n");
> >
> > why after 5 patches of removing dev_err's, here you are changing
> > them to dev_dbg?
>
> The reasoning was explained above:
>
> > > Here, I did not delete the printout to support checking the termination
> > > process. The other drivers in this series do not have this SMBus
> > > specific termination step.
>
> This is also why I converted two calls here to dev_dbg. But read on
> first.
It would make sense if the debug would give some more
information...
> > It's still good, but if we want to be strict, errors should
> > print errors: as we are returning -ETIMEDOUT, then we are
> > treating the case as an error and we should print error.
>
> I strongly disagree. While we use an errno, we don't know if this is a
> real error yet. It is more a return value saying that the transfer timed
> out. The client driver knows. For some I2C clients this may be an error,
> but for an EEPROM this might be an "oh, it might still be erasing a
> page, let's try again after some defined delay".
>
> Think of 'i2cdetect': If we printout something in the -ENXIO case (no
> device responded to the address), the log file would have more than 100
> entries on a typical I2C bus. Although we know that -ENXIO will be the
> majority of cases and are fine with it.
>
> > As you did before, I would just remove the printout here.
>
> Maybe we could because there is still the "Terminating the current
> operation" string as debug message making the code flow still clear.
... e.g. for me it's not totally right if we do:
dev_dbg("timed out")
return -ETIMEDOUT;
Considering that this might not be a real error I would let the
calling function decide and print. Indeed i801_access() is not
even checking the error but letting the caller of smbus_xfer()
decide.
It would make more sense if we provide more information like:
dev_dbg("Terminating current operation because the bus is busy and we timed out");
Just merged the two consecutive messages (we could still trim it
a bit and reduce dmesg spam).
The second /dev_err/dev_dbg/ in this file to me is fine (even
though it's not really self explaining).
> > I will wait a bit for more comments and take patches 1 to 5 so
> > that I can unburden you a little from them.
>
> The patches have no dependencies. To keep mail traffic low, I suggest
> you continue reviewing and I only resend the i801 patch?
Yeah... I'll wait a few more days and give more time for reviews
and comments. I checked the rest of the series and it's fine.
If you are willing to send a V2 you could send it as reply to
this mail instead of resending everything.
Thanks,
Andi
