[PATCH V3] Set sda-hold-time based on ACPI *CNT value

[PATCH V3] Set sda-hold-time based on ACPI *CNT value

[Tan Chin Yew]

For I2c to operate correctly under all speed mode, sda-hold-time need to
be perfectly tuned. However, sda-hold-time is precalculated according to
circuit parameter which make it platform-specific.

In order to get accurate sda-hold-time for all platforms, pretuned
sda-hold-time for particular platform is stored in ACPI table and driver
to load the sda holding time from ACPI table.

This patch read the I2c sda-hold-time from ACPI table and assigned the
suitable hold time based on the i2c clock frequency.

Tested on Intel Apollo Lake.

Changes in V3:
	- Remove null handling in function dw_i2c_acpi_params.
	- Add descriptions to commit message.

Changes in V2:
	- The code is realigned according to suggestion.
	- "case 400000:" is added on top of "default:" for readability.

Tan Chin Yew (1):
  i2c: designware: Get selected speed mode sda-hold-time via ACPI

 drivers/i2c/busses/i2c-designware-platdrv.c | 31 ++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

-- 
1.9.1

[PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Tan Chin Yew]

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. Instead of loading all
speed mode settings, only selected speed mode settings are loaded.

Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 31 ++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index a597ba32..5a4eb6b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -85,8 +85,7 @@ static void dw_i2c_acpi_params(struct platform_device *pdev, char method[],
 
 		*hcnt = (u16)objs[0].integer.value;
 		*lcnt = (u16)objs[1].integer.value;
-		if (sda_hold)
-			*sda_hold = (u32)objs[2].integer.value;
+		*sda_hold = (u32)objs[2].integer.value;
 	}
 
 	kfree(buf.pointer);
@@ -105,14 +104,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)
-- 
1.9.1

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

On Tue, 2017-03-28 at 16:48 +0800, Tan Chin Yew wrote:
> 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. Instead of loading all
> speed mode settings, only selected speed mode settings are loaded.
> 
> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>


Looks good to me:

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 31
> ++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index a597ba32..5a4eb6b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -85,8 +85,7 @@ static void dw_i2c_acpi_params(struct
> platform_device *pdev, char method[],
>  
>  		*hcnt = (u16)objs[0].integer.value;
>  		*lcnt = (u16)objs[1].integer.value;
> -		if (sda_hold)
> -			*sda_hold = (u32)objs[2].integer.value;
> +		*sda_hold = (u32)objs[2].integer.value;
>  	}
>  
>  	kfree(buf.pointer);
> @@ -105,14 +104,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)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

On 03/28/2017 12:13 PM, Andy Shevchenko wrote:
> On Tue, 2017-03-28 at 16:48 +0800, Tan Chin Yew wrote:
>> 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. Instead of loading all
>> speed mode settings, only selected speed mode settings are loaded.
>>
>> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
>
>
> Looks good to me:
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

On Tue, Mar 28, 2017 at 04:48:02PM +0800, Tan Chin Yew wrote:
> 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. Instead of loading all
> speed mode settings, only selected speed mode settings are loaded.
> 
> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] Set sda-hold-time based on ACPI *CNT value

[chin.yew.tan]

From: Tan Chin Yew <chin.yew.tan@intel.com>

For I2c to operate correctly under all speed mode, sda-hold-time need to
be perfectly tuned. However, sda-hold-time is precalculated according to
circuit parameter which make it platform-specific.

In order to get accurate sda-hold-time for all platforms, pretuned
sda-hold-time for particular platform is stored in ACPI table and driver
to load the sda holding time from ACPI table.

This patch read the I2c sda-hold-time from ACPI table and assigned the
suitable hold time based on the i2c clock frequency.

Tested on Intel Apollo Lake.

Changes in V2:
	- The code is realigned according to suggestion.
	- "case 400000:" is added on top of "default:" for readability.

Tan Chin Yew (1):
  i2c: designware: Get selected speed mode sda-hold-time via ACPI

 drivers/i2c/busses/i2c-designware-platdrv.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[chin.yew.tan]

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(-)

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)
-- 
1.9.1

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

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>

RE: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Tan, Chin Yew]

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>

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

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)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]


> 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?



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Tan, Chin Yew]

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.

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]


> > > 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] Set sda-hold-time based on ACPI *CNT value

[chin.yew.tan]

From: Tan Chin Yew <chin.yew.tan@intel.com>

For I2c to operate correctly under all speed mode, sda-hold-time need to
be perfectly tuned. However, sda-hold-time is precalculated according to
circuit parameter which make it platform-specific.

In order to get accurate sda-hold-time for all platforms, pretuned
sda-hold-time for particular platform is stored in ACPI table and driver
to load the sda holding time from ACPI table.

This patch read the I2c sda-hold-time from ACPI table and assigned the
suitable hold time based on the i2c clock frequency.

Tested on Intel Apollo Lake.

Tan Chin Yew (1):
  i2c: designware: Get selected speed mode sda-hold-time via ACPI

 drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[chin.yew.tan]

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(-)

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;
-- 
1.9.1

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

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>

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

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;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

RE: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Tan, Chin Yew]

> 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.

> > +	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;
> 
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

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.

-- 
Jarkko

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy