| Message ID | 20240410112418.6400-33-wsa+renesas@sang-engineering.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: remove printout on handled timeouts | expand |
Am Mittwoch, 10. April 2024, 13:24:27 CEST schrieb Wolfram Sang: > 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. Remove the printout. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Acked-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/i2c/busses/i2c-rk3x.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c > index 086fdf262e7b..8c7367f289d3 100644 > --- a/drivers/i2c/busses/i2c-rk3x.c > +++ b/drivers/i2c/busses/i2c-rk3x.c > @@ -1106,9 +1106,6 @@ static int rk3x_i2c_xfer_common(struct i2c_adapter *adap, > spin_lock_irqsave(&i2c->lock, flags); > > if (timeout == 0) { > - dev_err(i2c->dev, "timeout, ipd: 0x%02x, state: %d\n", > - i2c_readl(i2c, REG_IPD), i2c->state); > - > /* Force a STOP condition without interrupt */ > i2c_writel(i2c, 0, REG_IEN); > val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK; >
Hello Wolfram, On 2024-04-10 13:24, 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. Remove the printout. Maybe it would be good to turn it into a debug message, instead of simply removing it? Maybe not all client drivers handle it correctly, in which case having an easy way for debugging would be beneficial. > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-rk3x.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rk3x.c > b/drivers/i2c/busses/i2c-rk3x.c > index 086fdf262e7b..8c7367f289d3 100644 > --- a/drivers/i2c/busses/i2c-rk3x.c > +++ b/drivers/i2c/busses/i2c-rk3x.c > @@ -1106,9 +1106,6 @@ static int rk3x_i2c_xfer_common(struct > i2c_adapter *adap, > spin_lock_irqsave(&i2c->lock, flags); > > if (timeout == 0) { > - dev_err(i2c->dev, "timeout, ipd: 0x%02x, state: %d\n", > - i2c_readl(i2c, REG_IPD), i2c->state); > - > /* Force a STOP condition without interrupt */ > i2c_writel(i2c, 0, REG_IEN); > val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
> Maybe it would be good to turn it into a debug message, instead of > simply removing it? Maybe not all client drivers handle it correctly, > in which case having an easy way for debugging would be beneficial. Hmm, but it still returns -ETIMEDOUT to distinguish cases?
On 2024-04-13 08:38, Wolfram Sang wrote: >> Maybe it would be good to turn it into a debug message, instead of >> simply removing it? Maybe not all client drivers handle it correctly, >> in which case having an easy way for debugging would be beneficial. > > Hmm, but it still returns -ETIMEDOUT to distinguish cases? Sure, but I think that having such an additional debug facility can only help and save the people from adding temporary printk()s while debugging.
Hi Dragan, > Sure, but I think that having such an additional debug facility > can only help and save the people from adding temporary printk()s > while debugging. Mileages, I guess. I like temporary printouts for temporary debug sessions. This way the upstream source code stays slim. In my experience with I2C over the years, debugging happens with developers anyhow. Logfiles from users which help developers create patches are very rare. And those users are usually capable enough to add debug patches. Given these experiences (which may be different in other subsystems), I don't see why we should carry the bloat. That said, I'll leave the final decision to the driver maintainers. Happy hacking, Wolfram
Am Samstag, 13. April 2024, 08:44:41 CEST schrieb Dragan Simic: > On 2024-04-13 08:38, Wolfram Sang wrote: > >> Maybe it would be good to turn it into a debug message, instead of > >> simply removing it? Maybe not all client drivers handle it correctly, > >> in which case having an easy way for debugging would be beneficial. > > > > Hmm, but it still returns -ETIMEDOUT to distinguish cases? > > Sure, but I think that having such an additional debug facility > can only help and save the people from adding temporary printk()s > while debugging. Also we're talking about two lines of code, I wouldn't call that bloat ;-) I was thinking about dev_dbg vs. removal too, but hadn't a clear favorite. So essentially Dragan is tipping the scale and I guess dev_dbg might be the nicer way to go. Heiko
Hello Wolfram, On 2024-04-13 09:10, Wolfram Sang wrote: > Hi Dragan, > >> Sure, but I think that having such an additional debug facility >> can only help and save the people from adding temporary printk()s >> while debugging. > > Mileages, I guess. I like temporary printouts for temporary debug > sessions. This way the upstream source code stays slim. In my > experience > with I2C over the years, debugging happens with developers anyhow. > Logfiles from users which help developers create patches are very rare. > And those users are usually capable enough to add debug patches. > Given these experiences (which may be different in other subsystems), I > don't see why we should carry the bloat. Yup, adding some printk()s while debugging is pretty much inevitable, and having full debugging available would add a lot of code, regardless of the subsystem. > That said, I'll leave the final decision to the driver maintainers. Agreed.
Hello Heiko, On 2024-04-13 09:58, Heiko Stübner wrote: > Am Samstag, 13. April 2024, 08:44:41 CEST schrieb Dragan Simic: >> On 2024-04-13 08:38, Wolfram Sang wrote: >> >> Maybe it would be good to turn it into a debug message, instead of >> >> simply removing it? Maybe not all client drivers handle it correctly, >> >> in which case having an easy way for debugging would be beneficial. >> > >> > Hmm, but it still returns -ETIMEDOUT to distinguish cases? >> >> Sure, but I think that having such an additional debug facility >> can only help and save the people from adding temporary printk()s >> while debugging. > > Also we're talking about two lines of code, I wouldn't call that bloat > ;-) > I was thinking about dev_dbg vs. removal too, but hadn't a clear > favorite. > > So essentially Dragan is tipping the scale and I guess dev_dbg might be > the nicer way to go. Yes, the code for printing the message is already there and it's only a couple of lines, so it might be a good idea to recycle it. :)
> Also we're talking about two lines of code, I wouldn't call that bloat ;-)
With this patch, yes. But once you allow debug code, it is hard to draw
a line which debug is still okay and which is too fine-grained. And then
you end up with a lot. Over the years, I developed the tendency to try
to have less but meaningful error printouts. But I don't enforce it
strictly because it is too much bike-shedding discussion.
In case of this error printout here, it is just wrong. But, see, it also
came from this tendency I don't like to have printouts for every error.
Hi, On Sat, Apr 13, 2024 at 04:35:06PM +0200, Wolfram Sang wrote: > > Also we're talking about two lines of code, I wouldn't call that bloat ;-) > > With this patch, yes. But once you allow debug code, it is hard to draw > a line which debug is still okay and which is too fine-grained. And then > you end up with a lot. Over the years, I developed the tendency to try > to have less but meaningful error printouts. But I don't enforce it > strictly because it is too much bike-shedding discussion. > > In case of this error printout here, it is just wrong. But, see, it also > came from this tendency I don't like to have printouts for every error. I agree with Wolfram here. Debug messages are OK if they are providing real useful information to a final product. Besides, as I explained earlier, the patter: dev_dbg("timed out") return -ETIMEDOUT; (print a debug but return error) doesn't make much sense. But, I this is all personal preference. So that I can also leave it to the specific maintainer. From my side all patches in this series are r-b'ed, except for patch 6 where there are 3 dev_dbg in a row stating the same thing. Thanks Dragan and Heiko for your feedback. Andi
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 086fdf262e7b..8c7367f289d3 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -1106,9 +1106,6 @@ static int rk3x_i2c_xfer_common(struct i2c_adapter *adap, spin_lock_irqsave(&i2c->lock, flags); if (timeout == 0) { - dev_err(i2c->dev, "timeout, ipd: 0x%02x, state: %d\n", - i2c_readl(i2c, REG_IPD), i2c->state); - /* Force a STOP condition without interrupt */ i2c_writel(i2c, 0, REG_IEN); val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
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. Remove the printout. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rk3x.c | 3 --- 1 file changed, 3 deletions(-)