| Message ID | 1492203192-28025-1-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */
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
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>
> 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.
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
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
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 */
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(-)