| Message ID | 1617113966-40498-6-git-send-email-yangyicong@hisilicon.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for HiSilicon I2C controller | expand |
From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] Sent: Wednesday, March 31, 2021 10:57 AM To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> Cc: yangyicong <yangyicong@huawei.com>; wsa@kernel.org; andriy.shevchenko@linux.intel.com; linux-i2c@vger.kernel.org; Sergey.Semin@baikalelectronics.ru; linux-kernel@vger.kernel.org; digetx@gmail.com; treding@nvidia.com; jarkko.nikula@linux.intel.com; rmk+kernel@armlinux.org.uk; John Garry <john.garry@huawei.com>; mika.westerberg@linux.intel.com; Zengtao (B) <prime.zeng@hisilicon.com>; Linuxarm <linuxarm@huawei.com> Subject: Re: [PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string() On Wednesday, March 31, 2021, Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> wrote: > -----Original Message----- > From: yangyicong > Sent: Wednesday, March 31, 2021 3:19 AM > To: wsa@kernel.org; andriy.shevchenko@linux.intel.com; > linux-i2c@vger.kernel.org; Sergey.Semin@baikalelectronics.ru; > linux-kernel@vger.kernel.org > Cc: digetx@gmail.com; treding@nvidia.com; jarkko.nikula@linux.intel.com; > rmk+kernel@armlinux.org.uk; Song Bao Hua (Barry Song) > <song.bao.hua@hisilicon.com>; John Garry <john.garry@huawei.com>; > mika.westerberg@linux.intel.com; yangyicong <yangyicong@huawei.com>; Zengtao > (B) <prime.zeng@hisilicon.com>; Linuxarm <linuxarm@huawei.com> > Subject: [PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string() > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Use generic i2c_freq_mode_string() helper to print chosen bus speed. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/i2c/busses/i2c-designware-master.c | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-master.c > b/drivers/i2c/busses/i2c-designware-master.c > index dd27b9d..b64c4c8 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -35,10 +35,10 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev > *dev) > > static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) > { > - const char *mode_str, *fp_str = ""; > u32 comp_param1; > u32 sda_falling_time, scl_falling_time; > struct i2c_timings *t = &dev->timings; > + const char *fp_str = ""; > u32 ic_clk; > int ret; > > @@ -153,22 +153,10 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev > *dev) > > ret = i2c_dw_set_sda_hold(dev); > if (ret) > - goto out; > - > - switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) { > - case DW_IC_CON_SPEED_STD: > - mode_str = "Standard Mode"; > - break; > - case DW_IC_CON_SPEED_HIGH: > - mode_str = "High Speed Mode"; > - break; > - default: > - mode_str = "Fast Mode"; > - } > - dev_dbg(dev->dev, "Bus speed: %s%s\n", mode_str, fp_str); > + return ret; > > -out: > - return ret; > + dev_dbg(dev->dev, "Bus speed: %s\n", > i2c_freq_mode_string(t->bus_freq_hz)); > Weird the original code was printing both mode and fp. > And you are printing mode only. >> Sorry, but I didn’t get what you mean here. The code is equivalent, and actually it will print even more. The original code will print the string fp_str: %s%s\n", mode_str, fp_str The new code is printing mode_str only: %s > + return 0; > } > > /** > -- > 2.8.1 -- With Best Regards, Andy Shevchenko
> -----Original Message----- > From: Song Bao Hua (Barry Song) > Sent: Wednesday, March 31, 2021 10:54 AM > To: 'Andy Shevchenko' <andy.shevchenko@gmail.com> > Cc: yangyicong <yangyicong@huawei.com>; wsa@kernel.org; > andriy.shevchenko@linux.intel.com; linux-i2c@vger.kernel.org; > Sergey.Semin@baikalelectronics.ru; linux-kernel@vger.kernel.org; > digetx@gmail.com; treding@nvidia.com; jarkko.nikula@linux.intel.com; > rmk+kernel@armlinux.org.uk; John Garry <john.garry@huawei.com>; > mika.westerberg@linux.intel.com; Zengtao (B) <prime.zeng@hisilicon.com>; > Linuxarm <linuxarm@huawei.com> > Subject: RE: [PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string() > > > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > Sent: Wednesday, March 31, 2021 10:57 AM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com> > Cc: yangyicong <yangyicong@huawei.com>; wsa@kernel.org; > andriy.shevchenko@linux.intel.com; linux-i2c@vger.kernel.org; > Sergey.Semin@baikalelectronics.ru; linux-kernel@vger.kernel.org; > digetx@gmail.com; treding@nvidia.com; jarkko.nikula@linux.intel.com; > rmk+kernel@armlinux.org.uk; John Garry <john.garry@huawei.com>; > mika.westerberg@linux.intel.com; Zengtao (B) <prime.zeng@hisilicon.com>; > Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string() > > > > On Wednesday, March 31, 2021, Song Bao Hua (Barry Song) > <song.bao.hua@hisilicon.com> wrote: > > > > -----Original Message----- > > From: yangyicong > > Sent: Wednesday, March 31, 2021 3:19 AM > > To: wsa@kernel.org; andriy.shevchenko@linux.intel.com; > > linux-i2c@vger.kernel.org; Sergey.Semin@baikalelectronics.ru; > > linux-kernel@vger.kernel.org > > Cc: digetx@gmail.com; treding@nvidia.com; jarkko.nikula@linux.intel.com; > > rmk+kernel@armlinux.org.uk; Song Bao Hua (Barry Song) > > <song.bao.hua@hisilicon.com>; John Garry <john.garry@huawei.com>; > > mika.westerberg@linux.intel.com; yangyicong <yangyicong@huawei.com>; > Zengtao > > (B) <prime.zeng@hisilicon.com>; Linuxarm <linuxarm@huawei.com> > > Subject: [PATCH 5/5] i2c: designware: Switch over to i2c_freq_mode_string() > > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Use generic i2c_freq_mode_string() helper to print chosen bus speed. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > > --- > > drivers/i2c/busses/i2c-designware-master.c | 20 ++++---------------- > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-designware-master.c > > b/drivers/i2c/busses/i2c-designware-master.c > > index dd27b9d..b64c4c8 100644 > > --- a/drivers/i2c/busses/i2c-designware-master.c > > +++ b/drivers/i2c/busses/i2c-designware-master.c > > @@ -35,10 +35,10 @@ static void i2c_dw_configure_fifo_master(struct > dw_i2c_dev > > *dev) > > > > static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) > > { > > - const char *mode_str, *fp_str = ""; > > u32 comp_param1; > > u32 sda_falling_time, scl_falling_time; > > struct i2c_timings *t = &dev->timings; > > + const char *fp_str = ""; > > u32 ic_clk; > > int ret; > > > > @@ -153,22 +153,10 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev > > *dev) > > > > ret = i2c_dw_set_sda_hold(dev); > > if (ret) > > - goto out; > > - > > - switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) { > > - case DW_IC_CON_SPEED_STD: > > - mode_str = "Standard Mode"; > > - break; > > - case DW_IC_CON_SPEED_HIGH: > > - mode_str = "High Speed Mode"; > > - break; > > - default: > > - mode_str = "Fast Mode"; > > - } > > - dev_dbg(dev->dev, "Bus speed: %s%s\n", mode_str, fp_str); > > + return ret; > > > > -out: > > - return ret; > > + dev_dbg(dev->dev, "Bus speed: %s\n", > > i2c_freq_mode_string(t->bus_freq_hz)); > > > Weird the original code was printing both mode and fp. > > And you are printing mode only. > > >> Sorry, but I didn’t get what you mean here. The code is equivalent, and actually > it will print even more. > > The original code will print the string fp_str: > %s%s\n", mode_str, fp_str > > The new code is printing mode_str only: > %s > Isn't fp_str redundant? Do we need to change dev_dbg(dev->dev, "Fast Mode:%s HCNT:LCNT = %d:%d\n", fp_str...) > > + return 0; > > } > > > > /** > > -- > > 2.8.1 > > > -- > With Best Regards, > Andy Shevchenko
> No, please read the code carefully. > We can duplicate conditional, but it brings a bit of inconsistency to how the counters are printed. Thanks for clarification, I am still confused as the original code print the real mode based on dev->master_cfg, the new code is printing mode based on frequency. My understanding is the original code could fall back to a lower speed when higher speed modes were not set successfully. For example, high speed mode falls back to fast mode: if ((dev->master_cfg & DW_IC_CON_SPEED_MASK) == DW_IC_CON_SPEED_HIGH) { if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK) != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) { dev_err(dev->dev, "High Speed not supported!\n"); dev->master_cfg &= ~DW_IC_CON_SPEED_MASK; dev->master_cfg |= DW_IC_CON_SPEED_FAST; dev->hs_hcnt = 0; dev->hs_lcnt = 0; } the original code was printing the mode based on the new fallback dev->master_cfg but not the mode calculated from frequency: switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) { case DW_IC_CON_SPEED_STD: mode_str = "Standard Mode"; break; case DW_IC_CON_SPEED_HIGH: mode_str = "High Speed Mode"; break; default: mode_str = "Fast Mode"; } > > + return 0; > > } > > > > /** > > -- > > 2.8.1 > > > -- > With Best Regards, > Andy Shevchenko -- With Best Regards, Andy Shevchenko
On Wed, Mar 31, 2021 at 08:53:02AM +0000, Song Bao Hua (Barry Song) wrote: > > > No, please read the code carefully. > > We can duplicate conditional, but it brings a bit of inconsistency to how the counters are printed. > > Thanks for clarification, I am still confused as the original > code print the real mode based on dev->master_cfg, the new > code is printing mode based on frequency. > > My understanding is the original code could fall back to a lower > speed when higher speed modes were not set successfully. For > example, high speed mode falls back to fast mode: This is a good catch! I should be fixed by a separate patch I assume. > if ((dev->master_cfg & DW_IC_CON_SPEED_MASK) == > DW_IC_CON_SPEED_HIGH) { > if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK) > != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) { > dev_err(dev->dev, "High Speed not supported!\n"); > dev->master_cfg &= ~DW_IC_CON_SPEED_MASK; > dev->master_cfg |= DW_IC_CON_SPEED_FAST; Basically we have to adjust timings here to reflect this change. > dev->hs_hcnt = 0; > dev->hs_lcnt = 0; > } -- With Best Regards, Andy Shevchenko
On Tue, Mar 30, 2021 at 10:19:26PM +0800, Yicong Yang wrote: > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Use generic i2c_freq_mode_string() helper to print chosen bus speed. Since it will be a new version (based on Jarkko's comments), I guess you may add his Ack here that he gave against standalone submission of this patch. What Bary reported I will fix separately. -- With Best Regards, Andy Shevchenko
On 2021/3/31 18:37, Andy Shevchenko wrote: > On Tue, Mar 30, 2021 at 10:19:26PM +0800, Yicong Yang wrote: >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> >> Use generic i2c_freq_mode_string() helper to print chosen bus speed. > > Since it will be a new version (based on Jarkko's comments), I guess you may > add his Ack here that he gave against standalone submission of this patch. > > What Bary reported I will fix separately. > i'll addresse the comments and update the series with Jarkko's acked-by added. Thanks for reminding me!
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index dd27b9d..b64c4c8 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -35,10 +35,10 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev) static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) { - const char *mode_str, *fp_str = ""; u32 comp_param1; u32 sda_falling_time, scl_falling_time; struct i2c_timings *t = &dev->timings; + const char *fp_str = ""; u32 ic_clk; int ret; @@ -153,22 +153,10 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev) ret = i2c_dw_set_sda_hold(dev); if (ret) - goto out; - - switch (dev->master_cfg & DW_IC_CON_SPEED_MASK) { - case DW_IC_CON_SPEED_STD: - mode_str = "Standard Mode"; - break; - case DW_IC_CON_SPEED_HIGH: - mode_str = "High Speed Mode"; - break; - default: - mode_str = "Fast Mode"; - } - dev_dbg(dev->dev, "Bus speed: %s%s\n", mode_str, fp_str); + return ret; -out: - return ret; + dev_dbg(dev->dev, "Bus speed: %s\n", i2c_freq_mode_string(t->bus_freq_hz)); + return 0; } /**