| Message ID | 1486726118-19447-2-git-send-email-chin.yew.tan@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On 10.02.2017 13:28, 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 | 27 ++++++++++++++++++++------- > 1 file changed, 20 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 Fri, 2017-02-10 at 19:28 +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> Couple of nitpicks below. > Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com> > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 27 > ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c > b/drivers/i2c/busses/i2c-designware-platdrv.c > index 6ce4313..aa33088 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -101,15 +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, > + switch (dev->clk_freq) { > + case 100000: > + dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev- > >ss_lcnt, > &dev->sda_hold_time); This indentation should go in a way that & character in the same column as p (in "p(s" context above). > - 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); > - > + 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; Can we prepend default with case 400000: here? > + 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) > dev->accessor_flags |= (u32)id->driver_data;
DQo+IE9uIEZyaSwgMjAxNy0wMi0xMCBhdCAxOToyOCArMDgwMCwgY2hpbi55ZXcudGFuQGludGVs LmNvbSB3cm90ZToNCj4gPiBGcm9tOiBUYW4gQ2hpbiBZZXcgPGNoaW4ueWV3LnRhbkBpbnRlbC5j b20+DQo+ID4NCj4gPiBTZGEtaG9sZC10aW1lIGlzIGFuIGltcG9ydGFudCBwYXJhbWV0ZXIgZm9y IHR1bmluZyBpMmMgdG8gbWVldCB0aGUNCj4gPiBlbGVjdHJpY2FsIHNwZWNpZmljYXRpb24gZXNw ZWNpYWxseSBmb3IgaGlnaCBzcGVlZC4gSTJDIHdpdGggaW5jb3JyZWN0DQo+ID4gc2RhLWhvbGQt dGltZSBtYXkgY2F1c2UgbG9zdCBhcmJpdHJhdGlvbiBlcnJvci4gTm93LCB0aGUgZHJpdmVyIGlz DQo+ID4gYWJsZSB0byBnZXQgc2RhLWhvbGQtdGltZSBmb3IgYWxsIHRoZSBzcGVlZCBzdXBwb3J0 ZWQuDQo+ID4NCj4gDQo+IFJldmlld2VkLWJ5OiBBbmR5IFNoZXZjaGVua28gPGFuZHJpeS5zaGV2 Y2hlbmtvQGxpbnV4LmludGVsLmNvbT4NCj4gDQo+IENvdXBsZSBvZiBuaXRwaWNrcyBiZWxvdy4N Cj4gDQo+ID4gU2lnbmVkLW9mZi1ieTogVGFuIENoaW4gWWV3IDxjaGluLnlldy50YW5AaW50ZWwu Y29tPg0KPiA+IC0tLQ0KPiA+IMKgZHJpdmVycy9pMmMvYnVzc2VzL2kyYy1kZXNpZ253YXJlLXBs YXRkcnYuYyB8IDI3DQo+ID4gKysrKysrKysrKysrKysrKysrKystLS0tLS0tDQo+ID4gwqAxIGZp bGUgY2hhbmdlZCwgMjAgaW5zZXJ0aW9ucygrKSwgNyBkZWxldGlvbnMoLSkNCj4gPg0KPiA+IGRp ZmYgLS1naXQgYS9kcml2ZXJzL2kyYy9idXNzZXMvaTJjLWRlc2lnbndhcmUtcGxhdGRydi5jDQo+ ID4gYi9kcml2ZXJzL2kyYy9idXNzZXMvaTJjLWRlc2lnbndhcmUtcGxhdGRydi5jDQo+ID4gaW5k ZXggNmNlNDMxMy4uYWEzMzA4OCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2kyYy9idXNzZXMv aTJjLWRlc2lnbndhcmUtcGxhdGRydi5jDQo+ID4gKysrIGIvZHJpdmVycy9pMmMvYnVzc2VzL2ky Yy1kZXNpZ253YXJlLXBsYXRkcnYuYw0KPiA+IEBAIC0xMDEsMTUgKzEwMSwyOCBAQCBzdGF0aWMg aW50IGR3X2kyY19hY3BpX2NvbmZpZ3VyZShzdHJ1Y3QNCj4gPiBwbGF0Zm9ybV9kZXZpY2UgKnBk ZXYpDQo+ID4gwqAJZGV2LT5yeF9maWZvX2RlcHRoID0gMzI7DQo+ID4NCj4gPiDCoAkvKg0KPiA+ IC0JwqAqIFRyeSB0byBnZXQgU0RBIGhvbGQgdGltZSBhbmQgKkNOVCB2YWx1ZXMgZnJvbSBhbiBB Q1BJDQo+ID4gbWV0aG9kIGlmDQo+ID4gLQnCoCogaXQgZXhpc3RzIGZvciBib3RoIHN1cHBvcnRl ZCBzcGVlZCBtb2Rlcy4NCj4gPiArCcKgKiBUcnkgdG8gZ2V0IFNEQSBob2xkIHRpbWUgYW5kICpD TlQgdmFsdWVzIGZyb20gYW4gQUNQSQ0KPiA+IG1ldGhvZCBmb3INCj4gPiArCcKgKiBzZWxlY3Rl ZCBzcGVlZCBtb2Rlcy4NCj4gPiDCoAnCoCovDQo+ID4gLQlkd19pMmNfYWNwaV9wYXJhbXMocGRl diwgIlNTQ04iLCAmZGV2LT5zc19oY250LCAmZGV2LQ0KPiA+ID5zc19sY250LCBOVUxMKTsNCj4g PiAtCWR3X2kyY19hY3BpX3BhcmFtcyhwZGV2LCAiRk1DTiIsICZkZXYtPmZzX2hjbnQsICZkZXYt DQo+ID4gPmZzX2xjbnQsDQo+ID4gKwlzd2l0Y2ggKGRldi0+Y2xrX2ZyZXEpIHsNCj4gPiArCWNh c2UgMTAwMDAwOg0KPiA+ICsJCWR3X2kyY19hY3BpX3BhcmFtcyhwZGV2LCAiU1NDTiIsICZkZXYt PnNzX2hjbnQsICZkZXYtDQo+ID4gPnNzX2xjbnQsDQo+IA0KPiA+IMKgCQkJwqDCoMKgJmRldi0+ c2RhX2hvbGRfdGltZSk7DQo+IA0KPiBUaGlzIGluZGVudGF0aW9uIHNob3VsZCBnbyBpbiBhIHdh eSB0aGF0ICYgY2hhcmFjdGVyIGluIHRoZSBzYW1lIGNvbHVtbiBhcyBwDQo+IChpbiAicChzIiBj b250ZXh0IGFib3ZlKS4NCkkgd2lsbCByZWFsaWduIHRoZSBjb2RlIGFjY29yZGluZyB0byBzdWdn ZXN0aW9uLg0KDQo+IA0KPiA+IC0JZHdfaTJjX2FjcGlfcGFyYW1zKHBkZXYsICJGUENOIiwgJmRl di0+ZnBfaGNudCwgJmRldi0NCj4gPiA+ZnBfbGNudCwgTlVMTCk7DQo+ID4gLQlkd19pMmNfYWNw aV9wYXJhbXMocGRldiwgIkhTQ04iLCAmZGV2LT5oc19oY250LCAmZGV2LQ0KPiA+ID5oc19sY250 LCBOVUxMKTsNCj4gPiAtDQo+ID4gKwkJYnJlYWs7DQo+ID4gKwljYXNlIDEwMDAwMDA6DQo+ID4g KwkJZHdfaTJjX2FjcGlfcGFyYW1zKHBkZXYsICJGUENOIiwgJmRldi0+ZnBfaGNudCwgJmRldi0N Cj4gPiA+ZnBfbGNudCwNCj4gPiArCQkJwqDCoMKgJmRldi0+c2RhX2hvbGRfdGltZSk7DQo+ID4g KwkJYnJlYWs7DQo+ID4gKwljYXNlIDM0MDAwMDA6DQo+ID4gKwkJZHdfaTJjX2FjcGlfcGFyYW1z KHBkZXYsICJIU0NOIiwgJmRldi0+aHNfaGNudCwgJmRldi0NCj4gPiA+aHNfbGNudCwNCj4gPiAr CQkJwqDCoMKgJmRldi0+c2RhX2hvbGRfdGltZSk7DQo+ID4gKwkJYnJlYWs7DQo+IA0KPiBDYW4g d2UgcHJlcGVuZCBkZWZhdWx0IHdpdGgNCj4gDQo+IGNhc2UgNDAwMDAwOg0KPiANCj4gaGVyZT8N Cj4gDQpZZXMsIHlvdSBhcmUgcmlnaHQsIGl0IGlzIGJlc3Qgbm90IHRvIGxvYWQgc2V0dGluZ3Mg Zm9yIHNwZWVkIG1vZGUgdGhhdCBpcyANCm5vdCBzdXBwb3J0ZWQuDQoNCj4gPiArCWRlZmF1bHQ6 DQo+IA0KPiANCj4gPiArCQlkd19pMmNfYWNwaV9wYXJhbXMocGRldiwgIkZNQ04iLCAmZGV2LT5m c19oY250LCAmZGV2LQ0KPiA+ID5mc19sY250LA0KPiA+ICsJCQnCoMKgwqAmZGV2LT5zZGFfaG9s ZF90aW1lKTsNCj4gPiArCQlicmVhazsNCj4gPiArCX0NCj4gPiArDQo+ID4gwqAJaWQgPSBhY3Bp X21hdGNoX2RldmljZShwZGV2LT5kZXYuZHJpdmVyLT5hY3BpX21hdGNoX3RhYmxlLA0KPiA+ICZw ZGV2LT5kZXYpOw0KPiA+IMKgCWlmIChpZCAmJiBpZC0+ZHJpdmVyX2RhdGEpDQo+ID4gwqAJCWRl di0+YWNjZXNzb3JfZmxhZ3MgfD0gKHUzMilpZC0+ZHJpdmVyX2RhdGE7DQo+IA0KPiAtLQ0KPiBB bmR5IFNoZXZjaGVua28gPGFuZHJpeS5zaGV2Y2hlbmtvQGxpbnV4LmludGVsLmNvbT4NCj4gSW50 ZWwgRmlubGFuZCBPeQ0K -- 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 13.02.2017 10:41, Tan, Chin Yew wrote: > >> On Fri, 2017-02-10 at 19:28 +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> >> >> Couple of nitpicks below. >> >>> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com> >>> --- >>> drivers/i2c/busses/i2c-designware-platdrv.c | 27 >>> ++++++++++++++++++++------- >>> 1 file changed, 20 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c >>> b/drivers/i2c/busses/i2c-designware-platdrv.c >>> index 6ce4313..aa33088 100644 >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >>> @@ -101,15 +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, >>> + switch (dev->clk_freq) { >>> + case 100000: >>> + dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev- >>>> ss_lcnt, >> >>> &dev->sda_hold_time); >> >> This indentation should go in a way that & character in the same column as p >> (in "p(s" context above). > I will realign the code according to suggestion. > >> >>> - 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); >>> - >>> + 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; >> >> Can we prepend default with >> >> case 400000: >> >> here? >> > Yes, you are right, it is best not to load settings for speed mode that is > not supported. > Andy: I guess you were looking for adding "case 400000:" for readability rather than removing the default case? I think it's best to keep fall back to 400 kHz speed that has been the default in this driver in case we get some not supported speed from ACPI.
On Mon, 2017-02-13 at 11:33 +0200, Jarkko Nikula wrote: > On 13.02.2017 10:41, Tan, Chin Yew wrote: > > > > > On Fri, 2017-02-10 at 19:28 +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. > > > > + 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; > > > > > > Can we prepend default with > > > > > > case 400000: > > > > > > here? > > > > > > > Yes, you are right, it is best not to load settings for speed mode > > that is > > not supported. > > > > Andy: I guess you were looking for adding "case 400000:" for > readability > rather than removing the default case? Correct. To explicitly show that default we rather assume 400000, but if it's not, still go that branch. > > I think it's best to keep fall back to 400 kHz speed that has been > the > default in this driver in case we get some not supported speed from > ACPI.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 6ce4313..aa33088 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -101,15 +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, + switch (dev->clk_freq) { + case 100000: + dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev->ss_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); - + 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; + 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) dev->accessor_flags |= (u32)id->driver_data;