i2c: designware: do not show SDA hold time warning when not needed

Submitted by Thomas Petazzoni on April 14, 2017, 8:53 p.m.

Details

Message ID 1492203192-28025-1-git-send-email-thomas.petazzoni@free-electrons.com
State Changes Requested
Headers show

Commit Message

Thomas Petazzoni April 14, 2017, 8:53 p.m.
When the I2C controller IP block has a revision too old to be able to
configure the SDA hold time, the driver currently displays a
warning. However, it does so unconditionally, even if no SDA hold time
has been configured through the Device Tree. This causes useless
warnings when running the system, so only show the warning if a SDA
hold time was specified.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko April 18, 2017, 7:54 a.m.
On Fri, 2017-04-14 at 22:53 +0200, Thomas Petazzoni wrote:
> When the I2C controller IP block has a revision too old to be able to
> configure the SDA hold time, the driver currently displays a
> warning. However, it does so unconditionally, even if no SDA hold time
> has been configured through the Device Tree. This causes useless
> warnings when running the system, so only show the warning if a SDA
> hold time was specified.

As far as I understand the warning it would be better to keep it in
either way, though you may shift it to debug level.

Wolfram, Jarkko, thoughts?

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c
> index 7a3faa5..7f9da6e 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -436,8 +436,9 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  			dev->sda_hold_time |= 1 <<
> DW_IC_SDA_HOLD_RX_SHIFT;
>  		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
>  	} else {
> -		dev_warn(dev->dev,
> -			"Hardware too old to adjust SDA hold
> time.\n");
> +		if (dev->sda_hold_time)
> +			dev_warn(dev->dev,
> +				 "Hardware too old to adjust SDA hold
> time.\n");
>  	}
>  
>  	/* Configure Tx/Rx FIFO threshold levels */
Thomas Petazzoni April 18, 2017, 7:59 a.m.
Hello,

On Tue, 18 Apr 2017 10:54:47 +0300, Andy Shevchenko wrote:
> On Fri, 2017-04-14 at 22:53 +0200, Thomas Petazzoni wrote:
> > When the I2C controller IP block has a revision too old to be able to
> > configure the SDA hold time, the driver currently displays a
> > warning. However, it does so unconditionally, even if no SDA hold time
> > has been configured through the Device Tree. This causes useless
> > warnings when running the system, so only show the warning if a SDA
> > hold time was specified.  
> 
> As far as I understand the warning it would be better to keep it in
> either way, though you may shift it to debug level.
> 
> Wolfram, Jarkko, thoughts?

Why show a message when the user has not requested a custom SDA hold
time? Getting a warning about something you haven't requested seems
really odd.

I think it makes a lot more sense to keep it at the warning level
(because it's important to get this message if you configure a custom
SDA hold time), but only show it when appropriate.

Best regards,

Thomas
Jarkko Nikula April 18, 2017, 10:23 a.m.
On 04/18/2017 10:59 AM, Thomas Petazzoni wrote:
> Hello,
>
> On Tue, 18 Apr 2017 10:54:47 +0300, Andy Shevchenko wrote:
>> On Fri, 2017-04-14 at 22:53 +0200, Thomas Petazzoni wrote:
>>> When the I2C controller IP block has a revision too old to be able to
>>> configure the SDA hold time, the driver currently displays a
>>> warning. However, it does so unconditionally, even if no SDA hold time
>>> has been configured through the Device Tree. This causes useless
>>> warnings when running the system, so only show the warning if a SDA
>>> hold time was specified.
>>
>> As far as I understand the warning it would be better to keep it in
>> either way, though you may shift it to debug level.
>>
>> Wolfram, Jarkko, thoughts?
>
> Why show a message when the user has not requested a custom SDA hold
> time? Getting a warning about something you haven't requested seems
> really odd.
>
> I think it makes a lot more sense to keep it at the warning level
> (because it's important to get this message if you configure a custom
> SDA hold time), but only show it when appropriate.
>
I guess warning over debug level could have slightly better chance to 
prevent someone not adding needless "i2c-sda-hold-time-ns" property in a 
hardware that doesn't support SDA hold time. But needless spamming have 
negative value so this is worth to fix. (I would do this as a single 
liner by else if ()).

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang June 2, 2017, 8:15 p.m.
> hardware that doesn't support SDA hold time. But needless spamming have
> negative value so this is worth to fix. (I would do this as a single liner
> by else if ()).
> 
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

I agree with Jarkko on everything, including the 'else if' fix.
Thomas Petazzoni June 3, 2017, 1:18 p.m.
Hello,

On Fri, 2 Jun 2017 22:15:02 +0200, Wolfram Sang wrote:
> > hardware that doesn't support SDA hold time. But needless spamming have
> > negative value so this is worth to fix. (I would do this as a single liner
> > by else if ()).
> > 
> > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>  
> 
> I agree with Jarkko on everything, including the 'else if' fix.

I am not sure to understand what Jarkko suggested to fix, besides using
"else if" in one line. Could you be more specific about the changes you
expect me to do?

Andy suggested to turn the warning into a debug message, to which I
objected, and my understanding was that Jarkko agreed with me that a
warning message was better, and only made the suggestion of the "else
if".

Thanks,

Thomas
Wolfram Sang June 3, 2017, 9:25 p.m.
Hi Thomas,

On Sat, Jun 03, 2017 at 03:18:21PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Fri, 2 Jun 2017 22:15:02 +0200, Wolfram Sang wrote:
> > > hardware that doesn't support SDA hold time. But needless spamming have
> > > negative value so this is worth to fix. (I would do this as a single liner
> > > by else if ()).
> > > 
> > > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>  
> > 
> > I agree with Jarkko on everything, including the 'else if' fix.
> 
> I am not sure to understand what Jarkko suggested to fix, besides using
> "else if" in one line. Could you be more specific about the changes you
> expect me to do?

I agree with Jarkko that needless spamming is worth fixing. Yet, it
should be done as a single line using "else if". So, basically the
paragraph I quoted from him. More clear now?

Regards,

   Wolfram

Patch hide | download patch | download mbox

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 7a3faa5..7f9da6e 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -436,8 +436,9 @@  int i2c_dw_init(struct dw_i2c_dev *dev)
 			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
 		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
 	} else {
-		dev_warn(dev->dev,
-			"Hardware too old to adjust SDA hold time.\n");
+		if (dev->sda_hold_time)
+			dev_warn(dev->dev,
+				 "Hardware too old to adjust SDA hold time.\n");
 	}
 
 	/* Configure Tx/Rx FIFO threshold levels */