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

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

[Thomas Petazzoni]

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(-)

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 */
-- 
2.7.4

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

[Thomas Petazzoni]

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(-)

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 */
-- 
2.7.4

Re: [PATCH] i2c: designware: do not show SDA hold time warning when not needed

[Andy Shevchenko]

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 */

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

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 */

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c: designware: do not show SDA hold time warning when not needed

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] i2c: designware: do not show SDA hold time warning when not needed

[Jarkko Nikula]

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>

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

[Jarkko Nikula]

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>

Re: [PATCH] i2c: designware: do not show SDA hold time warning when not needed

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]


> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

> 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170602/93213df3/attachment.sig>

Re: [PATCH] i2c: designware: do not show SDA hold time warning when not needed

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] i2c: designware: do not show SDA hold time warning when not needed

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170603/96a6744b/attachment.sig>