diff mbox series

[13/18] i2c: rk3x: remove printout on handled timeouts

Message ID 20240410112418.6400-33-wsa+renesas@sang-engineering.com
State New
Headers show
Series i2c: remove printout on handled timeouts | expand

Commit Message

Wolfram Sang April 10, 2024, 11:24 a.m. UTC
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(-)

Comments

Heiko Stübner April 11, 2024, 6:59 p.m. UTC | #1
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;
>
Dragan Simic April 12, 2024, 9:12 p.m. UTC | #2
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;
Wolfram Sang April 13, 2024, 6:38 a.m. UTC | #3
> 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?
Dragan Simic April 13, 2024, 6:44 a.m. UTC | #4
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.
Wolfram Sang April 13, 2024, 7:10 a.m. UTC | #5
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
Dragan Simic April 13, 2024, 8:07 a.m. UTC | #6
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.
diff mbox series

Patch

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;