| Message ID | 20180529112754.13477-5-jarkko.nikula@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: designware: Improve debug prints | expand |
On Tue, 2018-05-29 at 14:27 +0300, Jarkko Nikula wrote: > SDA hold time is an important timing parameter and often reason for > arbitration lost errors if not set to a correct value. Add a debug > print > for it in order to see what value gets programmed to a HW. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-common.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c > b/drivers/i2c/busses/i2c-designware-common.c > index 4f951486f74a..cb55d7e3af8e 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -207,6 +207,10 @@ void i2c_dw_set_sda_hold(struct dw_i2c_dev *dev) > */ > if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) > dev->sda_hold_time |= 1 << > DW_IC_SDA_HOLD_RX_SHIFT; > + > + dev_dbg(dev->dev, "SDA Hold Time TX:RX = %d:%d\n", > + dev->sda_hold_time & > ~(u32)DW_IC_SDA_HOLD_RX_MASK, I'm not sure I understand why you need casting here. > + dev->sda_hold_time >> > DW_IC_SDA_HOLD_RX_SHIFT); > } else if (dev->sda_hold_time) { > dev_warn(dev->dev, > "Hardware too old to adjust SDA hold > time.\n");
On 05/29/2018 04:52 PM, Andy Shevchenko wrote: > On Tue, 2018-05-29 at 14:27 +0300, Jarkko Nikula wrote: >> SDA hold time is an important timing parameter and often reason for >> arbitration lost errors if not set to a correct value. Add a debug >> print >> for it in order to see what value gets programmed to a HW. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> --- >> drivers/i2c/busses/i2c-designware-common.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/i2c/busses/i2c-designware-common.c >> b/drivers/i2c/busses/i2c-designware-common.c >> index 4f951486f74a..cb55d7e3af8e 100644 >> --- a/drivers/i2c/busses/i2c-designware-common.c >> +++ b/drivers/i2c/busses/i2c-designware-common.c >> @@ -207,6 +207,10 @@ void i2c_dw_set_sda_hold(struct dw_i2c_dev *dev) >> */ >> if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) >> dev->sda_hold_time |= 1 << >> DW_IC_SDA_HOLD_RX_SHIFT; >> + >> + dev_dbg(dev->dev, "SDA Hold Time TX:RX = %d:%d\n", >> + dev->sda_hold_time & >> ~(u32)DW_IC_SDA_HOLD_RX_MASK, > > I'm not sure I understand why you need casting here. > Me neither but gcc thinks otherwise in 64-bit build. It appears to convert that "(u32) & ~FOO" as u64 and complains "format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘long unsigned int’". Sidenote, I need to change %d to %u.
On Wed, 2018-05-30 at 09:19 +0300, Jarkko Nikula wrote: > On 05/29/2018 04:52 PM, Andy Shevchenko wrote: > > On Tue, 2018-05-29 at 14:27 +0300, Jarkko Nikula wrote: > > > + dev_dbg(dev->dev, "SDA Hold Time TX:RX = > > > %d:%d\n", > > > + dev->sda_hold_time & > > > ~(u32)DW_IC_SDA_HOLD_RX_MASK, > > > > I'm not sure I understand why you need casting here. > Me neither but gcc thinks otherwise in 64-bit build. It appears to > convert that "(u32) & ~FOO" as u64 and complains "format ‘%d’ expects > argument of type ‘int’, but argument 4 has type ‘long unsigned int’". Okay, we have, I think, the following options - (u32) explicit casting - lower_32_bits() macro call - defining mask with U type (not sure if ~ doesn't promote to UL) - defining negative mask explicitly as a number All of them seems to me a compromise in one way or another (U suffix, for example, will probably require to change all such definitions for sake of consistency).
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 4f951486f74a..cb55d7e3af8e 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -207,6 +207,10 @@ void i2c_dw_set_sda_hold(struct dw_i2c_dev *dev) */ if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT; + + dev_dbg(dev->dev, "SDA Hold Time TX:RX = %d:%d\n", + dev->sda_hold_time & ~(u32)DW_IC_SDA_HOLD_RX_MASK, + dev->sda_hold_time >> DW_IC_SDA_HOLD_RX_SHIFT); } else if (dev->sda_hold_time) { dev_warn(dev->dev, "Hardware too old to adjust SDA hold time.\n");
SDA hold time is an important timing parameter and often reason for arbitration lost errors if not set to a correct value. Add a debug print for it in order to see what value gets programmed to a HW. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/i2c/busses/i2c-designware-common.c | 4 ++++ 1 file changed, 4 insertions(+)