[PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[Ulf Hansson]

Here's a couple changes for the i2c-designware driver. Most of them a related to
the support for runtime PM and system PM, but there's also a few that improves
some error handling.

I have tested these on Hisilicon Linaro 96-board (hi6220). I used a couple local
changes to enable the power-key to act as a wakeup in system PM suspend state.
If anyone are interested about those as well, I am happy to share them.

Ulf Hansson (10):
  i2c: designware-platdrv: Return error in ->probe() when clk ungate
    fails
  i2c: designware-platdrv: Gate clk in error path in ->probe()
  i2c: designware-platdrv: Unconditionally enable runtime PM
  i2c: designware-platdrv: Disable autosuspend in error path in
    ->probe()
  i2c: designware-platdrv: Fix clk gating in ->remove()
  i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()
  i2c: designware-platdrv: Re-init the HW when resuming
  i2c: designware-platdrv: Check return value from clk_prepare_enable()
  i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
  i2c: designware-platdrv: Rework system PM support

 drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++---------------
 1 file changed, 50 insertions(+), 56 deletions(-)

-- 
1.9.1

[PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[Ulf Hansson]

Here's a couple changes for the i2c-designware driver. Most of them a related to
the support for runtime PM and system PM, but there's also a few that improves
some error handling.

I have tested these on Hisilicon Linaro 96-board (hi6220). I used a couple local
changes to enable the power-key to act as a wakeup in system PM suspend state.
If anyone are interested about those as well, I am happy to share them.

Ulf Hansson (10):
  i2c: designware-platdrv: Return error in ->probe() when clk ungate
    fails
  i2c: designware-platdrv: Gate clk in error path in ->probe()
  i2c: designware-platdrv: Unconditionally enable runtime PM
  i2c: designware-platdrv: Disable autosuspend in error path in
    ->probe()
  i2c: designware-platdrv: Fix clk gating in ->remove()
  i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()
  i2c: designware-platdrv: Re-init the HW when resuming
  i2c: designware-platdrv: Check return value from clk_prepare_enable()
  i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
  i2c: designware-platdrv: Rework system PM support

 drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++---------------
 1 file changed, 50 insertions(+), 56 deletions(-)

-- 
1.9.1

[PATCH 01/10] i2c: designware-platdrv: Return error in ->probe() when clk ungate fails

[Ulf Hansson]

The error code received from clk_prepare_enable() becomes ignored when
i2c_dw_plat_prepare_clk() is called in ->probe().

Fix this by invoking clk_prepare_enable() in ->probe() instead of
i2c_dw_plat_prepare_clk(), as it allows the error code to be properly
propagated.

A side-effect from this change, makes the i2c_dw_plat_prepare_clk() used
only when CONFIG_PM is set. Avoid the compiler warning by moving the
function within the corresponding #ifdef.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 30 ++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..e39962b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -136,18 +136,6 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 }
 #endif
 
-static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
-{
-	if (IS_ERR(i_dev->clk))
-		return PTR_ERR(i_dev->clk);
-
-	if (prepare)
-		return clk_prepare_enable(i_dev->clk);
-
-	clk_disable_unprepare(i_dev->clk);
-	return 0;
-}
-
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -220,7 +208,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
-	if (!i2c_dw_plat_prepare_clk(dev, true)) {
+	if (!IS_ERR(dev->clk)) {
+		r = clk_prepare_enable(dev->clk);
+		if (r)
+			return r;
+
 		dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 
 		if (!dev->sda_hold_time && ht)
@@ -302,6 +294,18 @@ static void dw_i2c_plat_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
+static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
+{
+	if (IS_ERR(i_dev->clk))
+		return PTR_ERR(i_dev->clk);
+
+	if (prepare)
+		return clk_prepare_enable(i_dev->clk);
+
+	clk_disable_unprepare(i_dev->clk);
+	return 0;
+}
+
 static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-- 
1.9.1

[PATCH 01/10] i2c: designware-platdrv: Return error in ->probe() when clk ungate fails

[Ulf Hansson]

The error code received from clk_prepare_enable() becomes ignored when
i2c_dw_plat_prepare_clk() is called in ->probe().

Fix this by invoking clk_prepare_enable() in ->probe() instead of
i2c_dw_plat_prepare_clk(), as it allows the error code to be properly
propagated.

A side-effect from this change, makes the i2c_dw_plat_prepare_clk() used
only when CONFIG_PM is set. Avoid the compiler warning by moving the
function within the corresponding #ifdef.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 30 ++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..e39962b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -136,18 +136,6 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 }
 #endif
 
-static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
-{
-	if (IS_ERR(i_dev->clk))
-		return PTR_ERR(i_dev->clk);
-
-	if (prepare)
-		return clk_prepare_enable(i_dev->clk);
-
-	clk_disable_unprepare(i_dev->clk);
-	return 0;
-}
-
 static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -220,7 +208,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
-	if (!i2c_dw_plat_prepare_clk(dev, true)) {
+	if (!IS_ERR(dev->clk)) {
+		r = clk_prepare_enable(dev->clk);
+		if (r)
+			return r;
+
 		dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 
 		if (!dev->sda_hold_time && ht)
@@ -302,6 +294,18 @@ static void dw_i2c_plat_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
+static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
+{
+	if (IS_ERR(i_dev->clk))
+		return PTR_ERR(i_dev->clk);
+
+	if (prepare)
+		return clk_prepare_enable(i_dev->clk);
+
+	clk_disable_unprepare(i_dev->clk);
+	return 0;
+}
+
 static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-- 
1.9.1

Re: [PATCH 01/10] i2c: designware-platdrv: Return error in ->probe() when clk ungate fails

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> The error code received from clk_prepare_enable() becomes ignored when
> i2c_dw_plat_prepare_clk() is called in ->probe().
>
> Fix this by invoking clk_prepare_enable() in ->probe() instead of
> i2c_dw_plat_prepare_clk(), as it allows the error code to be properly
> propagated.
>
> A side-effect from this change, makes the i2c_dw_plat_prepare_clk() used
> only when CONFIG_PM is set. Avoid the compiler warning by moving the
> function within the corresponding #ifdef.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 30 ++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d656657..e39962b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -136,18 +136,6 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
>  }
>  #endif
>
> -static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
> -{
> -	if (IS_ERR(i_dev->clk))
> -		return PTR_ERR(i_dev->clk);
> -
> -	if (prepare)
> -		return clk_prepare_enable(i_dev->clk);
> -
> -	clk_disable_unprepare(i_dev->clk);
> -	return 0;
> -}
> -
>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
>  	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -220,7 +208,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
>
>  	dev->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (!i2c_dw_plat_prepare_clk(dev, true)) {
> +	if (!IS_ERR(dev->clk)) {
> +		r = clk_prepare_enable(dev->clk);
> +		if (r)

Looks ok to me. My only worry was commit b33af11de236 ("i2c: designware: 
Do not require clock when SSCN and FFCN are provided") but I think it's 
the IS_ERR(dev->clk) that captures that case not the clk_prepare_enable().

Suravee: Am I right IS_ERR(dev->clk) is enough to handle your case?

-- 
Jarkko

[PATCH 01/10] i2c: designware-platdrv: Return error in ->probe() when clk ungate fails

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> The error code received from clk_prepare_enable() becomes ignored when
> i2c_dw_plat_prepare_clk() is called in ->probe().
>
> Fix this by invoking clk_prepare_enable() in ->probe() instead of
> i2c_dw_plat_prepare_clk(), as it allows the error code to be properly
> propagated.
>
> A side-effect from this change, makes the i2c_dw_plat_prepare_clk() used
> only when CONFIG_PM is set. Avoid the compiler warning by moving the
> function within the corresponding #ifdef.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 30 ++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d656657..e39962b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -136,18 +136,6 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
>  }
>  #endif
>
> -static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
> -{
> -	if (IS_ERR(i_dev->clk))
> -		return PTR_ERR(i_dev->clk);
> -
> -	if (prepare)
> -		return clk_prepare_enable(i_dev->clk);
> -
> -	clk_disable_unprepare(i_dev->clk);
> -	return 0;
> -}
> -
>  static int dw_i2c_plat_probe(struct platform_device *pdev)
>  {
>  	struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
> @@ -220,7 +208,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  			DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
>
>  	dev->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (!i2c_dw_plat_prepare_clk(dev, true)) {
> +	if (!IS_ERR(dev->clk)) {
> +		r = clk_prepare_enable(dev->clk);
> +		if (r)

Looks ok to me. My only worry was commit b33af11de236 ("i2c: designware: 
Do not require clock when SSCN and FFCN are provided") but I think it's 
the IS_ERR(dev->clk) that captures that case not the clk_prepare_enable().

Suravee: Am I right IS_ERR(dev->clk) is enough to handle your case?

-- 
Jarkko

[PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Ulf Hansson]

The call to i2c_dw_probe() may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index e39962b..19174e7 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
+	pm_runtime_get_noresume(&pdev->dev);
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
@@ -245,8 +246,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	if (r) {
+		if (!IS_ERR(dev->clk))
+			clk_disable_unprepare(dev->clk);
+		if (!dev->pm_runtime_disabled)
+			pm_runtime_disable(&pdev->dev);
+	}
+	pm_runtime_put(&pdev->dev);
 
 	return r;
 }
-- 
1.9.1

[PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Ulf Hansson]

The call to i2c_dw_probe() may fail in ->probe() in which case the clock
remains ungated. Fix the error path by gating the clock before the error
code is returned.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index e39962b..19174e7 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->dev.of_node = pdev->dev.of_node;
 
+	pm_runtime_get_noresume(&pdev->dev);
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
@@ -245,8 +246,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	if (r) {
+		if (!IS_ERR(dev->clk))
+			clk_disable_unprepare(dev->clk);
+		if (!dev->pm_runtime_disabled)
+			pm_runtime_disable(&pdev->dev);
+	}
+	pm_runtime_put(&pdev->dev);
 
 	return r;
 }
-- 
1.9.1

Re: [PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Andy Shevchenko]

On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> The call to i2c_dw_probe() may fail in ->probe() in which case the
> clock
> remains ungated. Fix the error path by gating the clock before the
> error
> code is returned.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index e39962b..19174e7 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>  	adap->dev.of_node = pdev->dev.of_node;
> 

 
> +	pm_runtime_get_noresume(&pdev->dev);

> +	pm_runtime_put(&pdev->dev);

I don't see an explanation of these add-ons.

-- 

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

[PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Andy Shevchenko]

On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> The call to i2c_dw_probe() may fail in ->probe() in which case the
> clock
> remains ungated. Fix the error path by gating the clock before the
> error
> code is returned.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> ?drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
> ?1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index e39962b..19174e7 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> ?	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
> ?	adap->dev.of_node = pdev->dev.of_node;
> 

?
> +	pm_runtime_get_noresume(&pdev->dev);

> +	pm_runtime_put(&pdev->dev);

I don't see an explanation of these add-ons.

-- 

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

Re: [PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Ulf Hansson]

On 14 June 2016 at 17:22, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>> clock
>> remains ungated. Fix the error path by gating the clock before the
>> error
>> code is returned.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index e39962b..19174e7 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>       adap->dev.of_node = pdev->dev.of_node;
>>
>
>
>> +     pm_runtime_get_noresume(&pdev->dev);
>
>> +     pm_runtime_put(&pdev->dev);
>
> I don't see an explanation of these add-ons.

As we explicitly do clk_disable_unprepare() in the error path, we must
prevent the ->runtime_suspend() callback to be called during this
period.

This is a common pattern used by many drivers deploying runtime PM
support, which is probably why I left out the explanation.

I can update the change log with this information, if you like.

Kind regards
Uffe

[PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Ulf Hansson]

On 14 June 2016 at 17:22, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>> clock
>> remains ungated. Fix the error path by gating the clock before the
>> error
>> code is returned.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index e39962b..19174e7 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>> platform_device *pdev)
>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>       adap->dev.of_node = pdev->dev.of_node;
>>
>
>
>> +     pm_runtime_get_noresume(&pdev->dev);
>
>> +     pm_runtime_put(&pdev->dev);
>
> I don't see an explanation of these add-ons.

As we explicitly do clk_disable_unprepare() in the error path, we must
prevent the ->runtime_suspend() callback to be called during this
period.

This is a common pattern used by many drivers deploying runtime PM
support, which is probably why I left out the explanation.

I can update the change log with this information, if you like.

Kind regards
Uffe

Re: [PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Jarkko Nikula]

On 06/15/2016 10:16 AM, Ulf Hansson wrote:
> On 14 June 2016 at 17:22, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>>> clock
>>> remains ungated. Fix the error path by gating the clock before the
>>> error
>>> code is returned.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index e39962b..19174e7 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>>> platform_device *pdev)
>>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>>       adap->dev.of_node = pdev->dev.of_node;
>>>
>>
>>
>>> +     pm_runtime_get_noresume(&pdev->dev);
>>
>>> +     pm_runtime_put(&pdev->dev);
>>
>> I don't see an explanation of these add-ons.
>
> As we explicitly do clk_disable_unprepare() in the error path, we must
> prevent the ->runtime_suspend() callback to be called during this
> period.
>
> This is a common pattern used by many drivers deploying runtime PM
> support, which is probably why I left out the explanation.
>
What would cause here runtime suspend during probe? Also pairing 
pm_runtime_get_noresume() with pm_runtime_put() instead of 
pm_runtime_put_noidle() looks suspicious to me.

-- 
Jarkko

[PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Jarkko Nikula]

On 06/15/2016 10:16 AM, Ulf Hansson wrote:
> On 14 June 2016 at 17:22, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>>> clock
>>> remains ungated. Fix the error path by gating the clock before the
>>> error
>>> code is returned.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index e39962b..19174e7 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>>> platform_device *pdev)
>>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>>       adap->dev.of_node = pdev->dev.of_node;
>>>
>>
>>
>>> +     pm_runtime_get_noresume(&pdev->dev);
>>
>>> +     pm_runtime_put(&pdev->dev);
>>
>> I don't see an explanation of these add-ons.
>
> As we explicitly do clk_disable_unprepare() in the error path, we must
> prevent the ->runtime_suspend() callback to be called during this
> period.
>
> This is a common pattern used by many drivers deploying runtime PM
> support, which is probably why I left out the explanation.
>
What would cause here runtime suspend during probe? Also pairing 
pm_runtime_get_noresume() with pm_runtime_put() instead of 
pm_runtime_put_noidle() looks suspicious to me.

-- 
Jarkko

Re: [PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Ulf Hansson]

On 15 June 2016 at 14:07, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/15/2016 10:16 AM, Ulf Hansson wrote:
>>
>> On 14 June 2016 at 17:22, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>>
>>> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>>>>
>>>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>>>> clock
>>>> remains ungated. Fix the error path by gating the clock before the
>>>> error
>>>> code is returned.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> index e39962b..19174e7 100644
>>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>>>> platform_device *pdev)
>>>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>>>       adap->dev.of_node = pdev->dev.of_node;
>>>>
>>>
>>>
>>>> +     pm_runtime_get_noresume(&pdev->dev);
>>>
>>>
>>>> +     pm_runtime_put(&pdev->dev);
>>>
>>>
>>> I don't see an explanation of these add-ons.
>>
>>
>> As we explicitly do clk_disable_unprepare() in the error path, we must
>> prevent the ->runtime_suspend() callback to be called during this
>> period.
>>
>> This is a common pattern used by many drivers deploying runtime PM
>> support, which is probably why I left out the explanation.
>>
> What would cause here runtime suspend during probe? Also pairing

In practice this might not happen, although in theory it could...

The struct device is accessible by others than the driver (userspace
through sysfs for example). So, if "someone" would invoke
pm_runtime_suspend() (or a similar runtime PM API) for the device,
that would cause the ->runtime_suspend() callback to be invoked within
this period.

I suggest we just make sure to protect from this to happen.

> pm_runtime_get_noresume() with pm_runtime_put() instead of
> pm_runtime_put_noidle() looks suspicious to me.
>

It's not a problem. There's two cases here:

*) Runtime PM is enabled.
In this case the pm_runtime_put() will decrease the runtime PM usage
count and likely cause the device to be runtime suspended. Earlier,
the similar would happen as the driver core do a pm_request_idle()
when the ->probe() has completed.

**) Runtime PM is disabled.
In this case, the pm_runtime_put() has the same effect as a
pm_runtime_put_noidle(). It simply decrease the runtime PM usage
count, that's it.

Following this reasoning, one could decide to replace the
pm_runtime_put() with pm_runtime_put_noidle(), the behaviour will be
very similar.

If you would like to change to pm_runtime_put_noidle(), that's
perfectly fine by me.

Kind regards
Uffe

[PATCH 02/10] i2c: designware-platdrv: Gate clk in error path in ->probe()

[Ulf Hansson]

On 15 June 2016 at 14:07, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/15/2016 10:16 AM, Ulf Hansson wrote:
>>
>> On 14 June 2016 at 17:22, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>>
>>> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>>>>
>>>> The call to i2c_dw_probe() may fail in ->probe() in which case the
>>>> clock
>>>> remains ungated. Fix the error path by gating the clock before the
>>>> error
>>>> code is returned.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> ---
>>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> index e39962b..19174e7 100644
>>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>>> @@ -235,6 +235,7 @@ static int dw_i2c_plat_probe(struct
>>>> platform_device *pdev)
>>>>       ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>>>>       adap->dev.of_node = pdev->dev.of_node;
>>>>
>>>
>>>
>>>> +     pm_runtime_get_noresume(&pdev->dev);
>>>
>>>
>>>> +     pm_runtime_put(&pdev->dev);
>>>
>>>
>>> I don't see an explanation of these add-ons.
>>
>>
>> As we explicitly do clk_disable_unprepare() in the error path, we must
>> prevent the ->runtime_suspend() callback to be called during this
>> period.
>>
>> This is a common pattern used by many drivers deploying runtime PM
>> support, which is probably why I left out the explanation.
>>
> What would cause here runtime suspend during probe? Also pairing

In practice this might not happen, although in theory it could...

The struct device is accessible by others than the driver (userspace
through sysfs for example). So, if "someone" would invoke
pm_runtime_suspend() (or a similar runtime PM API) for the device,
that would cause the ->runtime_suspend() callback to be invoked within
this period.

I suggest we just make sure to protect from this to happen.

> pm_runtime_get_noresume() with pm_runtime_put() instead of
> pm_runtime_put_noidle() looks suspicious to me.
>

It's not a problem. There's two cases here:

*) Runtime PM is enabled.
In this case the pm_runtime_put() will decrease the runtime PM usage
count and likely cause the device to be runtime suspended. Earlier,
the similar would happen as the driver core do a pm_request_idle()
when the ->probe() has completed.

**) Runtime PM is disabled.
In this case, the pm_runtime_put() has the same effect as a
pm_runtime_put_noidle(). It simply decrease the runtime PM usage
count, that's it.

Following this reasoning, one could decide to replace the
pm_runtime_put() with pm_runtime_put_noidle(), the behaviour will be
very similar.

If you would like to change to pm_runtime_put_noidle(), that's
perfectly fine by me.

Kind regards
Uffe

[PATCH 03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

[Ulf Hansson]

In cases when the designware specific flag "pm_runtime_disable" is set,
->probe() calls pm_runtime_forbid() for the device, but without enabling
runtime PM. This increases the runtime PM usage count for the device, but
as runtime PM is disabled it's pointless.

Let's instead convert to unconditionally enable runtime PM, which has the
effect of making a parent device aware of its child.

To also maintain the old behaviour when the "pm_runtime_disable" flag is
set, which prevents userspace to enable runtime PM suspend via sysfs,
switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
during ->probe().

While changing this, let's also also correct the error path in ->probe()
and fix ->remove(), as to decrease the runtime PM usage count when it has
been in increased by pm_runtime_forbid() (after this change, by
pm_runtime_get_noresume()).

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 19174e7..94ff953 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	adap->dev.of_node = pdev->dev.of_node;
 
 	pm_runtime_get_noresume(&pdev->dev);
-	if (dev->pm_runtime_disabled) {
-		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	if (dev->pm_runtime_disabled)
+		pm_runtime_get_noresume(&pdev->dev);
 
 	r = i2c_dw_probe(dev);
 	if (r) {
 		if (!IS_ERR(dev->clk))
 			clk_disable_unprepare(dev->clk);
-		if (!dev->pm_runtime_disabled)
-			pm_runtime_disable(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+		if (dev->pm_runtime_disabled)
+			pm_runtime_put_noidle(&pdev->dev);
 	}
 	pm_runtime_put(&pdev->dev);
 
@@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	i2c_dw_disable(dev);
 
+	if (dev->pm_runtime_disabled)
+		pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

[Ulf Hansson]

In cases when the designware specific flag "pm_runtime_disable" is set,
->probe() calls pm_runtime_forbid() for the device, but without enabling
runtime PM. This increases the runtime PM usage count for the device, but
as runtime PM is disabled it's pointless.

Let's instead convert to unconditionally enable runtime PM, which has the
effect of making a parent device aware of its child.

To also maintain the old behaviour when the "pm_runtime_disable" flag is
set, which prevents userspace to enable runtime PM suspend via sysfs,
switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
during ->probe().

While changing this, let's also also correct the error path in ->probe()
and fix ->remove(), as to decrease the runtime PM usage count when it has
been in increased by pm_runtime_forbid() (after this change, by
pm_runtime_get_noresume()).

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 19174e7..94ff953 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	adap->dev.of_node = pdev->dev.of_node;
 
 	pm_runtime_get_noresume(&pdev->dev);
-	if (dev->pm_runtime_disabled) {
-		pm_runtime_forbid(&pdev->dev);
-	} else {
-		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-		pm_runtime_use_autosuspend(&pdev->dev);
-		pm_runtime_set_active(&pdev->dev);
-		pm_runtime_enable(&pdev->dev);
-	}
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	if (dev->pm_runtime_disabled)
+		pm_runtime_get_noresume(&pdev->dev);
 
 	r = i2c_dw_probe(dev);
 	if (r) {
 		if (!IS_ERR(dev->clk))
 			clk_disable_unprepare(dev->clk);
-		if (!dev->pm_runtime_disabled)
-			pm_runtime_disable(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+		if (dev->pm_runtime_disabled)
+			pm_runtime_put_noidle(&pdev->dev);
 	}
 	pm_runtime_put(&pdev->dev);
 
@@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	i2c_dw_disable(dev);
 
+	if (dev->pm_runtime_disabled)
+		pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
-	if (!dev->pm_runtime_disabled)
-		pm_runtime_disable(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> In cases when the designware specific flag "pm_runtime_disable" is set,
> ->probe() calls pm_runtime_forbid() for the device, but without enabling
> runtime PM. This increases the runtime PM usage count for the device, but
> as runtime PM is disabled it's pointless.
>
As far as I remember reason for that was to prevent suspending the 
device when unloading the driver.

> Let's instead convert to unconditionally enable runtime PM, which has the
> effect of making a parent device aware of its child.
>
> To also maintain the old behaviour when the "pm_runtime_disable" flag is
> set, which prevents userspace to enable runtime PM suspend via sysfs,
> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
> during ->probe().
>
> While changing this, let's also also correct the error path in ->probe()
> and fix ->remove(), as to decrease the runtime PM usage count when it has
> been in increased by pm_runtime_forbid() (after this change, by
> pm_runtime_get_noresume()).
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 19174e7..94ff953 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  	adap->dev.of_node = pdev->dev.of_node;
...
> +	if (dev->pm_runtime_disabled)
> +		pm_runtime_get_noresume(&pdev->dev);

> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
>
>  	i2c_dw_disable(dev);
>
> +	if (dev->pm_runtime_disabled)
> +		pm_runtime_put_noidle(&pdev->dev);

That will trigger power down after returning from dw_i2c_plat_remove() 
by looking at debug print from acpi_device_set_power() but now I don't 
find what call chain is actually causing it.

Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls 
from drivers/base/dd.c. Also I was wondering why RPM usage counts don't 
increase monotonically over repeated module load/unload cycle.

I had a few paper notes about these when I last time looked and debugged 
PM in i2c-designware but throw away them :-(

-- 
Jarkko

[PATCH 03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> In cases when the designware specific flag "pm_runtime_disable" is set,
> ->probe() calls pm_runtime_forbid() for the device, but without enabling
> runtime PM. This increases the runtime PM usage count for the device, but
> as runtime PM is disabled it's pointless.
>
As far as I remember reason for that was to prevent suspending the 
device when unloading the driver.

> Let's instead convert to unconditionally enable runtime PM, which has the
> effect of making a parent device aware of its child.
>
> To also maintain the old behaviour when the "pm_runtime_disable" flag is
> set, which prevents userspace to enable runtime PM suspend via sysfs,
> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
> during ->probe().
>
> While changing this, let's also also correct the error path in ->probe()
> and fix ->remove(), as to decrease the runtime PM usage count when it has
> been in increased by pm_runtime_forbid() (after this change, by
> pm_runtime_get_noresume()).
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 19174e7..94ff953 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  	adap->dev.of_node = pdev->dev.of_node;
...
> +	if (dev->pm_runtime_disabled)
> +		pm_runtime_get_noresume(&pdev->dev);

> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
>
>  	i2c_dw_disable(dev);
>
> +	if (dev->pm_runtime_disabled)
> +		pm_runtime_put_noidle(&pdev->dev);

That will trigger power down after returning from dw_i2c_plat_remove() 
by looking at debug print from acpi_device_set_power() but now I don't 
find what call chain is actually causing it.

Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls 
from drivers/base/dd.c. Also I was wondering why RPM usage counts don't 
increase monotonically over repeated module load/unload cycle.

I had a few paper notes about these when I last time looked and debugged 
PM in i2c-designware but throw away them :-(

-- 
Jarkko

Re: [PATCH 03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

[Ulf Hansson]

On 15 June 2016 at 14:47, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/14/2016 06:07 PM, Ulf Hansson wrote:
>>
>> In cases when the designware specific flag "pm_runtime_disable" is set,
>> ->probe() calls pm_runtime_forbid() for the device, but without enabling
>> runtime PM. This increases the runtime PM usage count for the device, but
>> as runtime PM is disabled it's pointless.
>>
> As far as I remember reason for that was to prevent suspending the device
> when unloading the driver.

Hmm. To me that's a weird way of implementing this.

Moreover, as i2c_dw_disable() indeed is called in ->remove() how will
the pm_runtime_forbid() help to avoid that from happen?

>
>> Let's instead convert to unconditionally enable runtime PM, which has the
>> effect of making a parent device aware of its child.
>>
>> To also maintain the old behaviour when the "pm_runtime_disable" flag is
>> set, which prevents userspace to enable runtime PM suspend via sysfs,
>> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
>> during ->probe().
>>
>> While changing this, let's also also correct the error path in ->probe()
>> and fix ->remove(), as to decrease the runtime PM usage count when it has
>> been in increased by pm_runtime_forbid() (after this change, by
>> pm_runtime_get_noresume()).
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 25
>> +++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 19174e7..94ff953 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device
>> *pdev)
>>         adap->dev.of_node = pdev->dev.of_node;
>
> ...
>>
>> +       if (dev->pm_runtime_disabled)
>> +               pm_runtime_get_noresume(&pdev->dev);
>
>
>> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device
>> *pdev)
>>
>>         i2c_dw_disable(dev);
>>
>> +       if (dev->pm_runtime_disabled)
>> +               pm_runtime_put_noidle(&pdev->dev);
>
>
> That will trigger power down after returning from dw_i2c_plat_remove() by
> looking at debug print from acpi_device_set_power() but now I don't find
> what call chain is actually causing it.

Oh, I didn't realize that by power down you meant the acpi power down.

Can you elaborate on why power down needs to be prevented?

Is there a problem to power on the device again in the next ->probe() attempt?

>
> Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls
> from drivers/base/dd.c. Also I was wondering why RPM usage counts don't
> increase monotonically over repeated module load/unload cycle.

$subject patch changes this, as I didn't think that was really the
wanted behaviour.

Perhaps acpi doesn't power down the device in case the runtime PM
usage count > 0. I will have look and see what I can dig out.

>
> I had a few paper notes about these when I last time looked and debugged PM
> in i2c-designware but throw away them :-(

Okay, I see.

Seems like we definitely need to do some more testing on acpi enabled platforms.

Kind regards
Uffe

[PATCH 03/10] i2c: designware-platdrv: Unconditionally enable runtime PM

[Ulf Hansson]

On 15 June 2016 at 14:47, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> On 06/14/2016 06:07 PM, Ulf Hansson wrote:
>>
>> In cases when the designware specific flag "pm_runtime_disable" is set,
>> ->probe() calls pm_runtime_forbid() for the device, but without enabling
>> runtime PM. This increases the runtime PM usage count for the device, but
>> as runtime PM is disabled it's pointless.
>>
> As far as I remember reason for that was to prevent suspending the device
> when unloading the driver.

Hmm. To me that's a weird way of implementing this.

Moreover, as i2c_dw_disable() indeed is called in ->remove() how will
the pm_runtime_forbid() help to avoid that from happen?

>
>> Let's instead convert to unconditionally enable runtime PM, which has the
>> effect of making a parent device aware of its child.
>>
>> To also maintain the old behaviour when the "pm_runtime_disable" flag is
>> set, which prevents userspace to enable runtime PM suspend via sysfs,
>> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume()
>> during ->probe().
>>
>> While changing this, let's also also correct the error path in ->probe()
>> and fix ->remove(), as to decrease the runtime PM usage count when it has
>> been in increased by pm_runtime_forbid() (after this change, by
>> pm_runtime_get_noresume()).
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 25
>> +++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index 19174e7..94ff953 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device
>> *pdev)
>>         adap->dev.of_node = pdev->dev.of_node;
>
> ...
>>
>> +       if (dev->pm_runtime_disabled)
>> +               pm_runtime_get_noresume(&pdev->dev);
>
>
>> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device
>> *pdev)
>>
>>         i2c_dw_disable(dev);
>>
>> +       if (dev->pm_runtime_disabled)
>> +               pm_runtime_put_noidle(&pdev->dev);
>
>
> That will trigger power down after returning from dw_i2c_plat_remove() by
> looking at debug print from acpi_device_set_power() but now I don't find
> what call chain is actually causing it.

Oh, I didn't realize that by power down you meant the acpi power down.

Can you elaborate on why power down needs to be prevented?

Is there a problem to power on the device again in the next ->probe() attempt?

>
> Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls
> from drivers/base/dd.c. Also I was wondering why RPM usage counts don't
> increase monotonically over repeated module load/unload cycle.

$subject patch changes this, as I didn't think that was really the
wanted behaviour.

Perhaps acpi doesn't power down the device in case the runtime PM
usage count > 0. I will have look and see what I can dig out.

>
> I had a few paper notes about these when I last time looked and debugged PM
> in i2c-designware but throw away them :-(

Okay, I see.

Seems like we definitely need to do some more testing on acpi enabled platforms.

Kind regards
Uffe

[PATCH 04/10] i2c: designware-platdrv: Disable autosuspend in error path in ->probe()

[Ulf Hansson]

Disable runtime PM autosuspend in the error path in ->probe() to make sure
it's disabled at the next probe attempt.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 94ff953..7f67d801 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -249,6 +249,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		if (!IS_ERR(dev->clk))
 			clk_disable_unprepare(dev->clk);
 		pm_runtime_disable(&pdev->dev);
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
 		if (dev->pm_runtime_disabled)
 			pm_runtime_put_noidle(&pdev->dev);
 	}
-- 
1.9.1

[PATCH 04/10] i2c: designware-platdrv: Disable autosuspend in error path in ->probe()

[Ulf Hansson]

Disable runtime PM autosuspend in the error path in ->probe() to make sure
it's disabled at the next probe attempt.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 94ff953..7f67d801 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -249,6 +249,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		if (!IS_ERR(dev->clk))
 			clk_disable_unprepare(dev->clk);
 		pm_runtime_disable(&pdev->dev);
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
 		if (dev->pm_runtime_disabled)
 			pm_runtime_put_noidle(&pdev->dev);
 	}
-- 
1.9.1

[PATCH 05/10] i2c: designware-platdrv: Fix clk gating in ->remove()

[Ulf Hansson]

The current approach to gate the clock in ->remove() relies on CONFIG_PM
to be set, as it's expected that the call to pm_runtime_put_sync()
triggers the ->runtime_suspend() callback to be invoked.

Even in the case when CONFIG_PM is set, userspace may prevent runtime PM
suspend via sysfs (pm_runtime_forbid()) for the device.

Fix the behaviour by converting from using pm_runtime_put_sync() into
directly using the clk API to manage clock gating. Let's also update the
runtime PM status for the device as to reflect the state of the HW.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 7f67d801..4083376 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,11 +268,16 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	i2c_dw_disable(dev);
 
+	if (!IS_ERR(dev->clk))
+		clk_disable_unprepare(dev->clk);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	if (dev->pm_runtime_disabled)
 		pm_runtime_put_noidle(&pdev->dev);
-	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_put_sync(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
+
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 05/10] i2c: designware-platdrv: Fix clk gating in ->remove()

[Ulf Hansson]

The current approach to gate the clock in ->remove() relies on CONFIG_PM
to be set, as it's expected that the call to pm_runtime_put_sync()
triggers the ->runtime_suspend() callback to be invoked.

Even in the case when CONFIG_PM is set, userspace may prevent runtime PM
suspend via sysfs (pm_runtime_forbid()) for the device.

Fix the behaviour by converting from using pm_runtime_put_sync() into
directly using the clk API to manage clock gating. Let's also update the
runtime PM status for the device as to reflect the state of the HW.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 7f67d801..4083376 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,11 +268,16 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	i2c_dw_disable(dev);
 
+	if (!IS_ERR(dev->clk))
+		clk_disable_unprepare(dev->clk);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	if (dev->pm_runtime_disabled)
 		pm_runtime_put_noidle(&pdev->dev);
-	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_put_sync(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
+
+	pm_runtime_put_noidle(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 06/10] i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()

[Ulf Hansson]

To avoid the device to be immediately runtime suspended when ->probe()
returns, update the last busy mark to use the delay from autosuspend.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4083376..106856f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -253,6 +253,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		if (dev->pm_runtime_disabled)
 			pm_runtime_put_noidle(&pdev->dev);
 	}
+	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put(&pdev->dev);
 
 	return r;
-- 
1.9.1

[PATCH 06/10] i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()

[Ulf Hansson]

To avoid the device to be immediately runtime suspended when ->probe()
returns, update the last busy mark to use the delay from autosuspend.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4083376..106856f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -253,6 +253,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		if (dev->pm_runtime_disabled)
 			pm_runtime_put_noidle(&pdev->dev);
 	}
+	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put(&pdev->dev);
 
 	return r;
-- 
1.9.1

Re: [PATCH 06/10] i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> To avoid the device to be immediately runtime suspended when ->probe()
> returns, update the last busy mark to use the delay from autosuspend.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 4083376..106856f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -253,6 +253,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  		if (dev->pm_runtime_disabled)
>  			pm_runtime_put_noidle(&pdev->dev);
>  	}
> +	pm_runtime_mark_last_busy(&pdev->dev);
>  	pm_runtime_put(&pdev->dev);
>
I was thinking this too earlier shortly while doing cd998ded5c12 ("i2c: 
designware: Prevent runtime suspend during adapter registration") but 
decided to keep suspending immediately after probe is better in cases 
when there are no slaves connected to a bus or if they start 
communicating after host auto-suspend timeout has already fired.

But I don't have strong opinion against this.

-- 
Jarkko

[PATCH 06/10] i2c: designware-platdrv: Update runtime PM last busy mark in ->probe()

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> To avoid the device to be immediately runtime suspended when ->probe()
> returns, update the last busy mark to use the delay from autosuspend.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 4083376..106856f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -253,6 +253,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>  		if (dev->pm_runtime_disabled)
>  			pm_runtime_put_noidle(&pdev->dev);
>  	}
> +	pm_runtime_mark_last_busy(&pdev->dev);
>  	pm_runtime_put(&pdev->dev);
>
I was thinking this too earlier shortly while doing cd998ded5c12 ("i2c: 
designware: Prevent runtime suspend during adapter registration") but 
decided to keep suspending immediately after probe is better in cases 
when there are no slaves connected to a bus or if they start 
communicating after host auto-suspend timeout has already fired.

But I don't have strong opinion against this.

-- 
Jarkko

[PATCH 07/10] i2c: designware-platdrv: Re-init the HW when resuming

[Ulf Hansson]

In cases when the designware specific flag "pm_runtime_disable" is set,
the HW becomes disabled during system PM suspend. Let's make sure it
becomes operational when resuming by re-initialize it again.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 106856f..95a9f4e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -337,9 +337,7 @@ static int dw_i2c_plat_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	i2c_dw_plat_prepare_clk(i_dev, true);
-
-	if (!i_dev->pm_runtime_disabled)
-		i2c_dw_init(i_dev);
+	i2c_dw_init(i_dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 07/10] i2c: designware-platdrv: Re-init the HW when resuming

[Ulf Hansson]

In cases when the designware specific flag "pm_runtime_disable" is set,
the HW becomes disabled during system PM suspend. Let's make sure it
becomes operational when resuming by re-initialize it again.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 106856f..95a9f4e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -337,9 +337,7 @@ static int dw_i2c_plat_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	i2c_dw_plat_prepare_clk(i_dev, true);
-
-	if (!i_dev->pm_runtime_disabled)
-		i2c_dw_init(i_dev);
+	i2c_dw_init(i_dev);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 08/10] i2c: designware-platdrv: Check return value from clk_prepare_enable()

[Ulf Hansson]

As clk_prepare_enable() can fail, let's not ignore the error in the
->runtime_resume() callback, but instead propagate it.

This change decreases the callers of i2c_dw_plat_prepare_clk() down to
one, which seems like a justification of removing it. Instead let's call
clk_disable_unprepare() directly when needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 95a9f4e..2b91af3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -308,25 +308,15 @@ static void dw_i2c_plat_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
-static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
-{
-	if (IS_ERR(i_dev->clk))
-		return PTR_ERR(i_dev->clk);
-
-	if (prepare)
-		return clk_prepare_enable(i_dev->clk);
-
-	clk_disable_unprepare(i_dev->clk);
-	return 0;
-}
-
 static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	i2c_dw_disable(i_dev);
-	i2c_dw_plat_prepare_clk(i_dev, false);
+
+	if (!IS_ERR(i_dev->clk))
+		clk_disable_unprepare(i_dev->clk);
 
 	return 0;
 }
@@ -335,10 +325,15 @@ static int dw_i2c_plat_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+	int ret;
 
-	i2c_dw_plat_prepare_clk(i_dev, true);
-	i2c_dw_init(i_dev);
+	if (!IS_ERR(i_dev->clk)) {
+		ret = clk_prepare_enable(i_dev->clk);
+		if (ret)
+			return ret;
+	}
 
+	i2c_dw_init(i_dev);
 	return 0;
 }
 
-- 
1.9.1

[PATCH 08/10] i2c: designware-platdrv: Check return value from clk_prepare_enable()

[Ulf Hansson]

As clk_prepare_enable() can fail, let's not ignore the error in the
->runtime_resume() callback, but instead propagate it.

This change decreases the callers of i2c_dw_plat_prepare_clk() down to
one, which seems like a justification of removing it. Instead let's call
clk_disable_unprepare() directly when needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 95a9f4e..2b91af3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -308,25 +308,15 @@ static void dw_i2c_plat_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
-static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
-{
-	if (IS_ERR(i_dev->clk))
-		return PTR_ERR(i_dev->clk);
-
-	if (prepare)
-		return clk_prepare_enable(i_dev->clk);
-
-	clk_disable_unprepare(i_dev->clk);
-	return 0;
-}
-
 static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
 	i2c_dw_disable(i_dev);
-	i2c_dw_plat_prepare_clk(i_dev, false);
+
+	if (!IS_ERR(i_dev->clk))
+		clk_disable_unprepare(i_dev->clk);
 
 	return 0;
 }
@@ -335,10 +325,15 @@ static int dw_i2c_plat_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+	int ret;
 
-	i2c_dw_plat_prepare_clk(i_dev, true);
-	i2c_dw_init(i_dev);
+	if (!IS_ERR(i_dev->clk)) {
+		ret = clk_prepare_enable(i_dev->clk);
+		if (ret)
+			return ret;
+	}
 
+	i2c_dw_init(i_dev);
 	return 0;
 }
 
-- 
1.9.1

[PATCH 09/10] i2c: designware-platdrv: Simplify code by using dev_get_drvdata()

[Ulf Hansson]

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 2b91af3..b2c6037 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -310,8 +310,7 @@ static void dw_i2c_plat_complete(struct device *dev)
 #ifdef CONFIG_PM
 static int dw_i2c_plat_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
 	i2c_dw_disable(i_dev);
 
@@ -323,8 +322,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 
 static int dw_i2c_plat_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 	int ret;
 
 	if (!IS_ERR(i_dev->clk)) {
-- 
1.9.1

[PATCH 09/10] i2c: designware-platdrv: Simplify code by using dev_get_drvdata()

[Ulf Hansson]

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 2b91af3..b2c6037 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -310,8 +310,7 @@ static void dw_i2c_plat_complete(struct device *dev)
 #ifdef CONFIG_PM
 static int dw_i2c_plat_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
 	i2c_dw_disable(i_dev);
 
@@ -323,8 +322,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 
 static int dw_i2c_plat_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 	int ret;
 
 	if (!IS_ERR(i_dev->clk)) {
-- 
1.9.1

Re: [PATCH 09/10] i2c: designware-platdrv: Simplify code by using dev_get_drvdata()

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 2b91af3..b2c6037 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -310,8 +310,7 @@ static void dw_i2c_plat_complete(struct device *dev)
>  #ifdef CONFIG_PM
>  static int dw_i2c_plat_suspend(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH 09/10] i2c: designware-platdrv: Simplify code by using dev_get_drvdata()

[Jarkko Nikula]

On 06/14/2016 06:07 PM, Ulf Hansson wrote:
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 2b91af3..b2c6037 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -310,8 +310,7 @@ static void dw_i2c_plat_complete(struct device *dev)
>  #ifdef CONFIG_PM
>  static int dw_i2c_plat_suspend(struct device *dev)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
> +	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[PATCH 10/10] i2c: designware-platdrv: Rework system PM support

[Ulf Hansson]

The current code that deploys the system PM support relies on the
"direct_complete" feature supported by the PM core. The goal is to avoid
performing unnecessary operations during the system PM sequence.

Unfortunate in this case there are some drawbacks with this solution as
explained below.

1)
In case of the ->prepare() callback find the device runtime resumed it
returns 0. The PM core will then run the regular set of the system PM
callbacks for the device, to allow it to be suspended.

Under these circumstances the device also becomes unconditionally resumed
during the system PM resume sequence. It would be better to postpone the
resume operations to be managed by runtime PM and thus only when actually
needed.

2)
It's good practice to keep the device's runtime PM status in sync with the
the current state of the HW. In the same scenario as described in 1), the
runtime PM status are RPM_ACTIVE the period in-between when the
->suspend() and ->resume() callbacks are invoked. This is wrong because
the device has actually been put into a low power state.

To address the limitation in 2) and to simplify the system PM code, let's
convert to deploy the so called runtime PM centric approach.

This is done by assigning the system PM ->suspend|resume() callbacks to
the pm_runtime_force_suspend|resume() helper functions. In this way, the
->prepare() and ->complete() callbacks can also be removed.

Currently pm_runtime_force_resume() is also unconditionally resuming the
device, which is due to legacy reasons when CONFIG_PM_RUNTIME and
CONFIG_PM_SLEEP co-existed. Changing that behaviour is a reasonable
improvement to make and it would also resolve the limitation in 1).

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index b2c6037..4c31ff3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -291,24 +291,8 @@ static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_prepare(struct device *dev)
-{
-	return pm_runtime_suspended(dev);
-}
-
-static void dw_i2c_plat_complete(struct device *dev)
-{
-	if (dev->power.direct_complete)
-		pm_request_resume(dev);
-}
-#else
-#define dw_i2c_plat_prepare	NULL
-#define dw_i2c_plat_complete	NULL
-#endif
-
 #ifdef CONFIG_PM
-static int dw_i2c_plat_suspend(struct device *dev)
+static int dw_i2c_plat_runtime_suspend(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
@@ -320,7 +304,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	return 0;
 }
 
-static int dw_i2c_plat_resume(struct device *dev)
+static int dw_i2c_plat_runtime_resume(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 	int ret;
@@ -336,10 +320,11 @@ static int dw_i2c_plat_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
-	.prepare = dw_i2c_plat_prepare,
-	.complete = dw_i2c_plat_complete,
-	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
-	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
+			dw_i2c_plat_runtime_resume,
+			NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-- 
1.9.1

[PATCH 10/10] i2c: designware-platdrv: Rework system PM support

[Ulf Hansson]

The current code that deploys the system PM support relies on the
"direct_complete" feature supported by the PM core. The goal is to avoid
performing unnecessary operations during the system PM sequence.

Unfortunate in this case there are some drawbacks with this solution as
explained below.

1)
In case of the ->prepare() callback find the device runtime resumed it
returns 0. The PM core will then run the regular set of the system PM
callbacks for the device, to allow it to be suspended.

Under these circumstances the device also becomes unconditionally resumed
during the system PM resume sequence. It would be better to postpone the
resume operations to be managed by runtime PM and thus only when actually
needed.

2)
It's good practice to keep the device's runtime PM status in sync with the
the current state of the HW. In the same scenario as described in 1), the
runtime PM status are RPM_ACTIVE the period in-between when the
->suspend() and ->resume() callbacks are invoked. This is wrong because
the device has actually been put into a low power state.

To address the limitation in 2) and to simplify the system PM code, let's
convert to deploy the so called runtime PM centric approach.

This is done by assigning the system PM ->suspend|resume() callbacks to
the pm_runtime_force_suspend|resume() helper functions. In this way, the
->prepare() and ->complete() callbacks can also be removed.

Currently pm_runtime_force_resume() is also unconditionally resuming the
device, which is due to legacy reasons when CONFIG_PM_RUNTIME and
CONFIG_PM_SLEEP co-existed. Changing that behaviour is a reasonable
improvement to make and it would also resolve the limitation in 1).

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm at vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index b2c6037..4c31ff3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -291,24 +291,8 @@ static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM_SLEEP
-static int dw_i2c_plat_prepare(struct device *dev)
-{
-	return pm_runtime_suspended(dev);
-}
-
-static void dw_i2c_plat_complete(struct device *dev)
-{
-	if (dev->power.direct_complete)
-		pm_request_resume(dev);
-}
-#else
-#define dw_i2c_plat_prepare	NULL
-#define dw_i2c_plat_complete	NULL
-#endif
-
 #ifdef CONFIG_PM
-static int dw_i2c_plat_suspend(struct device *dev)
+static int dw_i2c_plat_runtime_suspend(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
@@ -320,7 +304,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	return 0;
 }
 
-static int dw_i2c_plat_resume(struct device *dev)
+static int dw_i2c_plat_runtime_resume(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 	int ret;
@@ -336,10 +320,11 @@ static int dw_i2c_plat_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
-	.prepare = dw_i2c_plat_prepare,
-	.complete = dw_i2c_plat_complete,
-	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
-	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
+			dw_i2c_plat_runtime_resume,
+			NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-- 
1.9.1

Re: [PATCH 10/10] i2c: designware-platdrv: Rework system PM support

[Andy Shevchenko]

On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> The current code that deploys the system PM support relies on the
> "direct_complete" feature supported by the PM core. The goal is to
> avoid
> performing unnecessary operations during the system PM sequence.
> 

> 
>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> -	.prepare = dw_i2c_plat_prepare,
> -	.complete = dw_i2c_plat_complete,
> -	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
> dw_i2c_plat_resume)
> -	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume,
> NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
> +			dw_i2c_plat_runtime_resume,
> +			NULL)
>  };

UNIVERSAL_PM_OPS ?

-- 

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

[PATCH 10/10] i2c: designware-platdrv: Rework system PM support

[Andy Shevchenko]

On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> The current code that deploys the system PM support relies on the
> "direct_complete" feature supported by the PM core. The goal is to
> avoid
> performing unnecessary operations during the system PM sequence.
> 

> 
> ?static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> -	.prepare = dw_i2c_plat_prepare,
> -	.complete = dw_i2c_plat_complete,
> -	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
> dw_i2c_plat_resume)
> -	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume,
> NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
> +			dw_i2c_plat_runtime_resume,
> +			NULL)
> ?};

UNIVERSAL_PM_OPS ?

-- 

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

Re: [PATCH 10/10] i2c: designware-platdrv: Rework system PM support

[Ulf Hansson]

On 14 June 2016 at 17:35, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> The current code that deploys the system PM support relies on the
>> "direct_complete" feature supported by the PM core. The goal is to
>> avoid
>> performing unnecessary operations during the system PM sequence.
>>
>
>>
>>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>> -     .prepare = dw_i2c_plat_prepare,
>> -     .complete = dw_i2c_plat_complete,
>> -     SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
>> dw_i2c_plat_resume)
>> -     SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume,
>> NULL)
>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                             pm_runtime_force_resume)
>> +     SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>> +                     dw_i2c_plat_runtime_resume,
>> +                     NULL)
>>  };
>
> UNIVERSAL_PM_OPS ?

The UNIVERSAL_DEV_PM_OPS assigns the same callbacks for system PM as
runtime PM, that doesn't work here.

Kind regards
Uffe

[PATCH 10/10] i2c: designware-platdrv: Rework system PM support

[Ulf Hansson]

On 14 June 2016 at 17:35, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> The current code that deploys the system PM support relies on the
>> "direct_complete" feature supported by the PM core. The goal is to
>> avoid
>> performing unnecessary operations during the system PM sequence.
>>
>
>>
>>  static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
>> -     .prepare = dw_i2c_plat_prepare,
>> -     .complete = dw_i2c_plat_complete,
>> -     SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend,
>> dw_i2c_plat_resume)
>> -     SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume,
>> NULL)
>> +     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                             pm_runtime_force_resume)
>> +     SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
>> +                     dw_i2c_plat_runtime_resume,
>> +                     NULL)
>>  };
>
> UNIVERSAL_PM_OPS ?

The UNIVERSAL_DEV_PM_OPS assigns the same callbacks for system PM as
runtime PM, that doesn't work here.

Kind regards
Uffe

Re: [PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[Andy Shevchenko]

On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> Here's a couple changes for the i2c-designware driver. Most of them a
> related to
> the support for runtime PM and system PM, but there's also a few that
> improves
> some error handling.
> 
> I have tested these on Hisilicon Linaro 96-board (hi6220). I used a
> couple local
> changes to enable the power-key to act as a wakeup in system PM
> suspend state.
> If anyone are interested about those as well, I am happy to share
> them.

I know Jarkko spent a lot to understand PM flow in this driver.

My overall feelings after brief reading of the series you fixed a
particular problem with your device or flow, which might have broken the
half of current users. So, I wouldn't take this without Tested-by tags
of (almost) all active stakeholders.

> 
> Ulf Hansson (10):
>   i2c: designware-platdrv: Return error in ->probe() when clk ungate
>     fails
>   i2c: designware-platdrv: Gate clk in error path in ->probe()
>   i2c: designware-platdrv: Unconditionally enable runtime PM
>   i2c: designware-platdrv: Disable autosuspend in error path in
>     ->probe()
>   i2c: designware-platdrv: Fix clk gating in ->remove()
>   i2c: designware-platdrv: Update runtime PM last busy mark in
> ->probe()
>   i2c: designware-platdrv: Re-init the HW when resuming
>   i2c: designware-platdrv: Check return value from
> clk_prepare_enable()
>   i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
>   i2c: designware-platdrv: Rework system PM support
> 
>  drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++----
> -----------
>  1 file changed, 50 insertions(+), 56 deletions(-)
> 

-- 

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

[PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[Andy Shevchenko]

On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
> Here's a couple changes for the i2c-designware driver. Most of them a
> related to
> the support for runtime PM and system PM, but there's also a few that
> improves
> some error handling.
> 
> I have tested these on Hisilicon Linaro 96-board (hi6220). I used a
> couple local
> changes to enable the power-key to act as a wakeup in system PM
> suspend state.
> If anyone are interested about those as well, I am happy to share
> them.

I know Jarkko spent a lot to understand PM flow in this driver.

My overall feelings after brief reading of the series you fixed a
particular problem with your device or flow, which might have broken the
half of current users. So, I wouldn't take this without Tested-by tags
of (almost) all active stakeholders.

> 
> Ulf Hansson (10):
> ? i2c: designware-platdrv: Return error in ->probe() when clk ungate
> ????fails
> ? i2c: designware-platdrv: Gate clk in error path in ->probe()
> ? i2c: designware-platdrv: Unconditionally enable runtime PM
> ? i2c: designware-platdrv: Disable autosuspend in error path in
> ????->probe()
> ? i2c: designware-platdrv: Fix clk gating in ->remove()
> ? i2c: designware-platdrv: Update runtime PM last busy mark in
> ->probe()
> ? i2c: designware-platdrv: Re-init the HW when resuming
> ? i2c: designware-platdrv: Check return value from
> clk_prepare_enable()
> ? i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
> ? i2c: designware-platdrv: Rework system PM support
> 
> ?drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++----
> -----------
> ?1 file changed, 50 insertions(+), 56 deletions(-)
> 

-- 

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

Re: [PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[Ulf Hansson]

+ Rafael, Kevin, Tomeu

On 14 June 2016 at 17:39, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> Here's a couple changes for the i2c-designware driver. Most of them a
>> related to
>> the support for runtime PM and system PM, but there's also a few that
>> improves
>> some error handling.
>>
>> I have tested these on Hisilicon Linaro 96-board (hi6220). I used a
>> couple local
>> changes to enable the power-key to act as a wakeup in system PM
>> suspend state.
>> If anyone are interested about those as well, I am happy to share
>> them.
>
> I know Jarkko spent a lot to understand PM flow in this driver.
>
> My overall feelings after brief reading of the series you fixed a
> particular problem with your device or flow, which might have broken the
> half of current users. So, I wouldn't take this without Tested-by tags
> of (almost) all active stakeholders.

That makes sense to me. Should I resend to include some more people?

Let me also elaborate a bit more of the reason behind this series.

I was using a 4.4 kernel on my Hikey board and testing system PM
suspend/resume, when I found this driver to be broken. Not only broken
for the Hikey board but for *all* users of this driver!

Without going into too much details, the problem was because of how
the "direct_complete" feature was being used and supported by the PM
core. In the case when the driver's ->prepare() callback found the
device runtime suspended and thus returning '1' to allow the PM core
to try "direct_complete" for the device, it caused the driver's
->suspend() callback to be invoked twice in a row. This causes a clk
reference count imbalance issue and messing up system PM
suspend/resume.

Therefore I started piece by piece improving the error handling,
system PM code and runtime PM code to narrow down the problem.
This series solves the problem on a 4.4 based kernel, however I also
realized that the issue became resolved already in 4.5 kernel because
of the following commit:

commit aa8e54b559479d0cb7eb632ba443b8cacd20cd4b
Author: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Date:   Thu Jan 7 16:46:14 2016 +0100

    PM / sleep: Go direct_complete if driver has no callbacks

    If a suitable prepare callback cannot be found for a given device and
    its driver has no PM callbacks at all, assume that it can go direct to
    complete when the system goes to sleep.

    The reason for this is that there's lots of devices in a system that do
    no PM at all and there's no reason for them to prevent their ancestors
    to do direct_complete if they can support it.

    Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
    Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I believe this commit solved the problem more as of a co-incidence,
than actually intended to be a fix for a regression. Perhaps I should
send this patch to "stable" as well.

Anyway, I still think the series I have posted here make some nice
improvement to the driver.

Kind regards
Uffe

>
>>
>> Ulf Hansson (10):
>>   i2c: designware-platdrv: Return error in ->probe() when clk ungate
>>     fails
>>   i2c: designware-platdrv: Gate clk in error path in ->probe()
>>   i2c: designware-platdrv: Unconditionally enable runtime PM
>>   i2c: designware-platdrv: Disable autosuspend in error path in
>>     ->probe()
>>   i2c: designware-platdrv: Fix clk gating in ->remove()
>>   i2c: designware-platdrv: Update runtime PM last busy mark in
>> ->probe()
>>   i2c: designware-platdrv: Re-init the HW when resuming
>>   i2c: designware-platdrv: Check return value from
>> clk_prepare_enable()
>>   i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
>>   i2c: designware-platdrv: Rework system PM support
>>
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++----
>> -----------
>>  1 file changed, 50 insertions(+), 56 deletions(-)
>>
>
> --
>
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

[PATCH 00/10] i2c: designware-platdrv: Some improvments related to PM

[Ulf Hansson]

+ Rafael, Kevin, Tomeu

On 14 June 2016 at 17:39, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, 2016-06-14 at 17:07 +0200, Ulf Hansson wrote:
>> Here's a couple changes for the i2c-designware driver. Most of them a
>> related to
>> the support for runtime PM and system PM, but there's also a few that
>> improves
>> some error handling.
>>
>> I have tested these on Hisilicon Linaro 96-board (hi6220). I used a
>> couple local
>> changes to enable the power-key to act as a wakeup in system PM
>> suspend state.
>> If anyone are interested about those as well, I am happy to share
>> them.
>
> I know Jarkko spent a lot to understand PM flow in this driver.
>
> My overall feelings after brief reading of the series you fixed a
> particular problem with your device or flow, which might have broken the
> half of current users. So, I wouldn't take this without Tested-by tags
> of (almost) all active stakeholders.

That makes sense to me. Should I resend to include some more people?

Let me also elaborate a bit more of the reason behind this series.

I was using a 4.4 kernel on my Hikey board and testing system PM
suspend/resume, when I found this driver to be broken. Not only broken
for the Hikey board but for *all* users of this driver!

Without going into too much details, the problem was because of how
the "direct_complete" feature was being used and supported by the PM
core. In the case when the driver's ->prepare() callback found the
device runtime suspended and thus returning '1' to allow the PM core
to try "direct_complete" for the device, it caused the driver's
->suspend() callback to be invoked twice in a row. This causes a clk
reference count imbalance issue and messing up system PM
suspend/resume.

Therefore I started piece by piece improving the error handling,
system PM code and runtime PM code to narrow down the problem.
This series solves the problem on a 4.4 based kernel, however I also
realized that the issue became resolved already in 4.5 kernel because
of the following commit:

commit aa8e54b559479d0cb7eb632ba443b8cacd20cd4b
Author: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Date:   Thu Jan 7 16:46:14 2016 +0100

    PM / sleep: Go direct_complete if driver has no callbacks

    If a suitable prepare callback cannot be found for a given device and
    its driver has no PM callbacks at all, assume that it can go direct to
    complete when the system goes to sleep.

    The reason for this is that there's lots of devices in a system that do
    no PM at all and there's no reason for them to prevent their ancestors
    to do direct_complete if they can support it.

    Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
    Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I believe this commit solved the problem more as of a co-incidence,
than actually intended to be a fix for a regression. Perhaps I should
send this patch to "stable" as well.

Anyway, I still think the series I have posted here make some nice
improvement to the driver.

Kind regards
Uffe

>
>>
>> Ulf Hansson (10):
>>   i2c: designware-platdrv: Return error in ->probe() when clk ungate
>>     fails
>>   i2c: designware-platdrv: Gate clk in error path in ->probe()
>>   i2c: designware-platdrv: Unconditionally enable runtime PM
>>   i2c: designware-platdrv: Disable autosuspend in error path in
>>     ->probe()
>>   i2c: designware-platdrv: Fix clk gating in ->remove()
>>   i2c: designware-platdrv: Update runtime PM last busy mark in
>> ->probe()
>>   i2c: designware-platdrv: Re-init the HW when resuming
>>   i2c: designware-platdrv: Check return value from
>> clk_prepare_enable()
>>   i2c: designware-platdrv: Simplify code by using dev_get_drvdata()
>>   i2c: designware-platdrv: Rework system PM support
>>
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 106 +++++++++++++----
>> -----------
>>  1 file changed, 50 insertions(+), 56 deletions(-)
>>
>
> --
>
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy