| Message ID | 1487051641-31927-2-git-send-email-chin.yew.tan@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 14.02.2017 07:54, chin.yew.tan@intel.com wrote: > From: Tan Chin Yew <chin.yew.tan@intel.com> > > Sda-hold-time is an important parameter for tuning i2c to meet the > electrical specification especially for high speed. I2C with incorrect > sda-hold-time may cause lost arbitration error. Now, the driver is able to > get sda-hold-time for all the speed supported. > > Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com> > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > 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
On Tue, 2017-02-14 at 13:54 +0800, chin.yew.tan@intel.com wrote: > From: Tan Chin Yew <chin.yew.tan@intel.com> > > Sda-hold-time is an important parameter for tuning i2c to meet the > electrical specification especially for high speed. I2C with incorrect > sda-hold-time may cause lost arbitration error. Now, the driver is > able to > get sda-hold-time for all the speed supported. > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com> > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 28 > +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c > b/drivers/i2c/busses/i2c-designware-platdrv.c > index 6ce4313..00c880a 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -101,14 +101,28 @@ static int dw_i2c_acpi_configure(struct > platform_device *pdev) > dev->rx_fifo_depth = 32; > > /* > - * Try to get SDA hold time and *CNT values from an ACPI > method if > - * it exists for both supported speed modes. > + * Try to get SDA hold time and *CNT values from an ACPI > method for > + * selected speed modes. > */ > - dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev- > >ss_lcnt, NULL); > - dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev- > >fs_lcnt, > - &dev->sda_hold_time); > - dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev- > >fp_lcnt, NULL); > - dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev- > >hs_lcnt, NULL); > + switch (dev->clk_freq) { > + case 100000: > + dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev- > >ss_lcnt, > + &dev->sda_hold_time); > + break; > + case 1000000: > + dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev- > >fp_lcnt, > + &dev->sda_hold_time); > + break; > + case 3400000: > + dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev- > >hs_lcnt, > + &dev->sda_hold_time); > + break; > + case 400000: > + default: > + dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev- > >fs_lcnt, > + &dev->sda_hold_time); > + break; > + } > > id = acpi_match_device(pdev->dev.driver->acpi_match_table, > &pdev->dev); > if (id && id->driver_data)
Hi Wolfram, Wondering if you manage to review this patch? It is about loading selected speed mode's sda-hold-time via ACPI. I would like to find out if there are any additional comments? Sincerely Tan Chin Yew > -----Original Message----- > From: Jarkko Nikula [mailto:jarkko.nikula@linux.intel.com] > Sent: Tuesday, February 14, 2017 3:01 PM > To: Tan, Chin Yew <chin.yew.tan@intel.com>; > andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com > Cc: linux-i2c@vger.kernel.org > Subject: Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time > via ACPI > > On 14.02.2017 07:54, chin.yew.tan@intel.com wrote: > > From: Tan Chin Yew <chin.yew.tan@intel.com> > > > > Sda-hold-time is an important parameter for tuning i2c to meet the > > electrical specification especially for high speed. I2C with incorrect > > sda-hold-time may cause lost arbitration error. Now, the driver is > > able to get sda-hold-time for all the speed supported. > > > > Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com> > > --- > > drivers/i2c/busses/i2c-designware-platdrv.c | 28 > > +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > 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
On Tue, Feb 14, 2017 at 01:54:01PM +0800, chin.yew.tan@intel.com wrote: > From: Tan Chin Yew <chin.yew.tan@intel.com> > > Sda-hold-time is an important parameter for tuning i2c to meet the > electrical specification especially for high speed. I2C with incorrect > sda-hold-time may cause lost arbitration error. Now, the driver is able to > get sda-hold-time for all the speed supported. This describes why you change NULL to dev->sda_hold_time. But it doesn't say why you introduce the switch-block instead of populating all fields like it was done before. Furthermore, since now there is no NULL case for dw_i2c_acpi_params() anymore, we can remove NULL handling in that function, or?
> This describes why you change NULL to dev->sda_hold_time. But it doesn't > say why you introduce the switch-block instead of populating all fields > like it was done before. > > Furthermore, since now there is no NULL case for dw_i2c_acpi_params() > anymore, we can remove NULL handling in that function, or? And since designware driver had a few changes meanwhile, can you base the next version on top of i2c/for-next?
Hi > -----Original Message----- > From: Wolfram Sang [mailto:wsa@the-dreams.de] > Sent: Friday, March 24, 2017 5:01 AM > To: Tan, Chin Yew <chin.yew.tan@intel.com> > Cc: jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; > mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org > Subject: Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time > via ACPI > > > > This describes why you change NULL to dev->sda_hold_time. But it > > doesn't say why you introduce the switch-block instead of populating > > all fields like it was done before. Instead of loading all speed mode settings, switch block is used to load the required settings for selected speed mode. Furthermore, using switch block re-use existing variable, Whereas, populating all fields introduce new variables. > > > > Furthermore, since now there is no NULL case for dw_i2c_acpi_params() > > anymore, we can remove NULL handling in that function, or? Ok, I will remove the null handling. > > And since designware driver had a few changes meanwhile, can you base the > next version on top of i2c/for-next? > Ok, I will base it to i2c/for-next. -- 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
> > > This describes why you change NULL to dev->sda_hold_time. But it > > > doesn't say why you introduce the switch-block instead of populating > > > all fields like it was done before. > Instead of loading all speed mode settings, switch block is used to load the > required settings for selected speed mode. Thanks. Please add this sentence to the commit message.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 6ce4313..00c880a 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -101,14 +101,28 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev) dev->rx_fifo_depth = 32; /* - * Try to get SDA hold time and *CNT values from an ACPI method if - * it exists for both supported speed modes. + * Try to get SDA hold time and *CNT values from an ACPI method for + * selected speed modes. */ - dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev->ss_lcnt, NULL); - dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt, - &dev->sda_hold_time); - dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev->fp_lcnt, NULL); - dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev->hs_lcnt, NULL); + switch (dev->clk_freq) { + case 100000: + dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev->ss_lcnt, + &dev->sda_hold_time); + break; + case 1000000: + dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev->fp_lcnt, + &dev->sda_hold_time); + break; + case 3400000: + dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev->hs_lcnt, + &dev->sda_hold_time); + break; + case 400000: + default: + dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt, + &dev->sda_hold_time); + break; + } id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); if (id && id->driver_data)