[PATCH 1/2] i2c-s3c2410: Convert to devm_kzalloc()

[PATCH 1/2] i2c-s3c2410: Convert to devm_kzalloc()

[Mark Brown]

Saves remembering to call kfree(). There's some kfree()s used by the
resource still, these will be removed in 3.3 using the newly added
devm_request_and_ioremap().

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 2754cef..ed62a7f 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -889,7 +889,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
-	i2c = kzalloc(sizeof(struct s3c24xx_i2c), GFP_KERNEL);
+	i2c = devm_kzalloc(&pdev->dev, sizeof(struct s3c24xx_i2c), GFP_KERNEL);
 	if (!i2c) {
 		dev_err(&pdev->dev, "no memory for state\n");
 		return -ENOMEM;
@@ -1034,7 +1034,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	clk_put(i2c->clk);
 
  err_noclk:
-	kfree(i2c);
 	return ret;
 }
 
@@ -1060,7 +1059,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 	release_resource(i2c->ioarea);
 	s3c24xx_i2c_dt_gpio_free(i2c);
 	kfree(i2c->ioarea);
-	kfree(i2c);
 
 	return 0;
 }
-- 
1.7.7.3

[PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

Add stub runtime_pm calls which go through the flow of enabling and
disabling but don't actually do anything with the device itself as
there's nothing useful we can do. This provides the core PM framework
with information about when the device is idle, enabling chip wide
power savings.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index ed62a7f..065c316 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -31,6 +31,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/slab.h>
@@ -563,6 +564,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	int retry;
 	int ret;
 
+	pm_runtime_get_sync(&adap->dev);
 	clk_enable(i2c->clk);
 
 	for (retry = 0; retry < adap->retries; retry++) {
@@ -571,6 +573,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 
 		if (ret != -EAGAIN) {
 			clk_disable(i2c->clk);
+			pm_runtime_put_sync(&adap->dev);
 			return ret;
 		}
 
@@ -580,6 +583,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	}
 
 	clk_disable(i2c->clk);
+	pm_runtime_put_sync(&adap->dev);
 	return -EREMOTEIO;
 }
 
@@ -1012,6 +1016,9 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	of_i2c_register_devices(&i2c->adap);
 	platform_set_drvdata(pdev, i2c);
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(&i2c->adap.dev);
+
 	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
 	clk_disable(i2c->clk);
 	return 0;
@@ -1046,6 +1053,9 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 {
 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&i2c->adap.dev);
+	pm_runtime_disable(&pdev->dev);
+
 	s3c24xx_i2c_deregister_cpufreq(i2c);
 
 	i2c_del_adapter(&i2c->adap);
-- 
1.7.7.3

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Heiko Stübner]

Am Montag, 5. Dezember 2011, 15:06:06 schrieb Mark Brown:
> Add stub runtime_pm calls which go through the flow of enabling and
> disabling but don't actually do anything with the device itself as
> there's nothing useful we can do. This provides the core PM framework
> with information about when the device is idle, enabling chip wide
> power savings.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Heiko Stuebner <heiko@sntech.de>

Kgene, Jean, Ben: could we move this forward please?

Thanks
Heiko

> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c
> b/drivers/i2c/busses/i2c-s3c2410.c index ed62a7f..065c316 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -31,6 +31,7 @@
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/slab.h>
> @@ -563,6 +564,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>  	int retry;
>  	int ret;
> 
> +	pm_runtime_get_sync(&adap->dev);
>  	clk_enable(i2c->clk);
> 
>  	for (retry = 0; retry < adap->retries; retry++) {
> @@ -571,6 +573,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
> 
>  		if (ret != -EAGAIN) {
>  			clk_disable(i2c->clk);
> +			pm_runtime_put_sync(&adap->dev);
>  			return ret;
>  		}
> 
> @@ -580,6 +583,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>  	}
> 
>  	clk_disable(i2c->clk);
> +	pm_runtime_put_sync(&adap->dev);
>  	return -EREMOTEIO;
>  }
> 
> @@ -1012,6 +1016,9 @@ static int s3c24xx_i2c_probe(struct platform_device
> *pdev) of_i2c_register_devices(&i2c->adap);
>  	platform_set_drvdata(pdev, i2c);
> 
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_enable(&i2c->adap.dev);
> +
>  	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
>  	clk_disable(i2c->clk);
>  	return 0;
> @@ -1046,6 +1053,9 @@ static int s3c24xx_i2c_remove(struct platform_device
> *pdev) {
>  	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
> 
> +	pm_runtime_disable(&i2c->adap.dev);
> +	pm_runtime_disable(&pdev->dev);
> +
>  	s3c24xx_i2c_deregister_cpufreq(i2c);
> 
>  	i2c_del_adapter(&i2c->adap);

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Jean Delvare]

On Tue, 13 Dec 2011 15:53:48 +0100, Heiko Stübner wrote:
> Am Montag, 5. Dezember 2011, 15:06:06 schrieb Mark Brown:
> > Add stub runtime_pm calls which go through the flow of enabling and
> > disabling but don't actually do anything with the device itself as
> > there's nothing useful we can do. This provides the core PM framework
> > with information about when the device is idle, enabling chip wide
> > power savings.
> > 
> > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> 
> Kgene, Jean, Ben: could we move this forward please?

The i2c-s3c2410 driver is in Ben's garden, not mine.

-- 
Jean Delvare

[PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

Add stub runtime_pm calls which go through the flow of enabling and
disabling but don't actually do anything with the device itself as
there's nothing useful we can do. This provides the core PM framework
with information about when the device is idle, enabling chip wide
power savings.

Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
Acked-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index e6f982b..737f721 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -31,6 +31,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/slab.h>
@@ -564,6 +565,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	int retry;
 	int ret;
 
+	pm_runtime_get_sync(&adap->dev);
 	clk_enable(i2c->clk);
 
 	for (retry = 0; retry < adap->retries; retry++) {
@@ -572,6 +574,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 
 		if (ret != -EAGAIN) {
 			clk_disable(i2c->clk);
+			pm_runtime_put_sync(&adap->dev);
 			return ret;
 		}
 
@@ -581,6 +584,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	}
 
 	clk_disable(i2c->clk);
+	pm_runtime_put_sync(&adap->dev);
 	return -EREMOTEIO;
 }
 
@@ -1013,6 +1017,9 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	of_i2c_register_devices(&i2c->adap);
 	platform_set_drvdata(pdev, i2c);
 
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(&i2c->adap.dev);
+
 	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
 	clk_disable(i2c->clk);
 	return 0;
@@ -1047,6 +1054,9 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 {
 	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
 
+	pm_runtime_disable(&i2c->adap.dev);
+	pm_runtime_disable(&pdev->dev);
+
 	s3c24xx_i2c_deregister_cpufreq(i2c);
 
 	i2c_del_adapter(&i2c->adap);
-- 
1.7.7.3

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/21/2012 02:28 PM, Mark Brown wrote:
> Add stub runtime_pm calls which go through the flow of enabling and
> disabling but don't actually do anything with the device itself as
> there's nothing useful we can do. This provides the core PM framework
> with information about when the device is idle, enabling chip wide
> power savings.
> 
> Signed-off-by: Mark Brown<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> Acked-by: Heiko Stuebner<heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-s3c2410.c |   10 ++++++++++
>   1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index e6f982b..737f721 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -31,6 +31,7 @@
>   #include<linux/errno.h>
>   #include<linux/err.h>
>   #include<linux/platform_device.h>
> +#include<linux/pm_runtime.h>
>   #include<linux/clk.h>
>   #include<linux/cpufreq.h>
>   #include<linux/slab.h>
> @@ -564,6 +565,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>   	int retry;
>   	int ret;
> 
> +	pm_runtime_get_sync(&adap->dev);
>   	clk_enable(i2c->clk);

It looks a bit strange to have pm_runtime* and manual clock control side
by side. How about implementing proper runtime_suspend/resume calls and
moving clk_enable/disable to these handlers ?

It might also make sense to check return value of pm_runtime_get_sync().
 
>   	for (retry = 0; retry<  adap->retries; retry++) {
> @@ -572,6 +574,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
> 
>   		if (ret != -EAGAIN) {
>   			clk_disable(i2c->clk);
> +			pm_runtime_put_sync(&adap->dev);

I would go for just pm_runtime_put() here...
	
>   			return ret;
>   		}
> 
> @@ -581,6 +584,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>   	}
> 
>   	clk_disable(i2c->clk);
> +	pm_runtime_put_sync(&adap->dev);

.. and here as well.

>   	return -EREMOTEIO;
>   }
> 
> @@ -1013,6 +1017,9 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>   	of_i2c_register_devices(&i2c->adap);
>   	platform_set_drvdata(pdev, i2c);
> 
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_enable(&i2c->adap.dev);

Why do we need pm_runtime_enable() on i2c->adap.dev ?
AFAIK enabling runtime PM on the platform device only should do.

> +
>   	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
>   	clk_disable(i2c->clk);
>   	return 0;
> @@ -1047,6 +1054,9 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
>   {
>   	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
> 
> +	pm_runtime_disable(&i2c->adap.dev);
> +	pm_runtime_disable(&pdev->dev);

Ditto.

> +
>   	s3c24xx_i2c_deregister_cpufreq(i2c);
> 
>   	i2c_del_adapter(&i2c->adap);

--

Thanks,
Sylwester

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

On Sat, Jan 21, 2012 at 04:52:58PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 02:28 PM, Mark Brown wrote:

> > +	pm_runtime_get_sync(&adap->dev);
> >   	clk_enable(i2c->clk);

> It looks a bit strange to have pm_runtime* and manual clock control side
> by side. How about implementing proper runtime_suspend/resume calls and
> moving clk_enable/disable to these handlers ?

I'd rather not do that in a single patch, and really we ought to be
doing more than just stopping and starting the clocks anyway.  Given
that the rather serious problems with getting I2C patches applied I'd
rather do as little as possible in individual patches.

It would also introduce a regression for systems that don't use runtime
PM (probably most s3c24xx ones).

> It might also make sense to check return value of pm_runtime_get_sync().

This is fairly idiomatic for the API, the error handling gets a bit
complicated if you do it properly.

> >   	for (retry = 0; retry<  adap->retries; retry++) {
> > @@ -572,6 +574,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
> > 
> >   		if (ret != -EAGAIN) {
> >   			clk_disable(i2c->clk);
> > +			pm_runtime_put_sync(&adap->dev);

> I would go for just pm_runtime_put() here...

Yeah, me too but it seemed idiomatic.  Frankly I don't really understand
why there's a synchronous version of put.

> > +	pm_runtime_enable(&pdev->dev);
> > +	pm_runtime_enable(&i2c->adap.dev);

> Why do we need pm_runtime_enable() on i2c->adap.dev ?
> AFAIK enabling runtime PM on the platform device only should do.

We probably don't need to, no.

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/21/2012 07:31 PM, Mark Brown wrote:
> On Sat, Jan 21, 2012 at 04:52:58PM +0100, Sylwester Nawrocki wrote:
>> On 01/21/2012 02:28 PM, Mark Brown wrote:
> 
>>> +	pm_runtime_get_sync(&adap->dev);
>>>    	clk_enable(i2c->clk);
> 
>> It looks a bit strange to have pm_runtime* and manual clock control side
>> by side. How about implementing proper runtime_suspend/resume calls and
>> moving clk_enable/disable to these handlers ?
> 
> I'd rather not do that in a single patch, and really we ought to be
> doing more than just stopping and starting the clocks anyway.  Given

Agreed.
> that the rather serious problems with getting I2C patches applied I'd
> rather do as little as possible in individual patches.
> 
> It would also introduce a regression for systems that don't use runtime
> PM (probably most s3c24xx ones).

Indeed, I'd forgotten about this fact for a while. I suspect the Run-time PM
core functionality could be enabled on those platform, allowing to make the
common driver dependant on PM_RUNTIME. Not sure if that's acceptable for
the smaller SoCs though. It rather sounds more like lots of trouble for 
little benefits. Maybe it's desirable to do that at some point anyway.
 
>> It might also make sense to check return value of pm_runtime_get_sync().
> 
> This is fairly idiomatic for the API, the error handling gets a bit
> complicated if you do it properly.

OK.

>>>    	for (retry = 0; retry<   adap->retries; retry++) {
>>> @@ -572,6 +574,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>>>
>>>    		if (ret != -EAGAIN) {
>>>    			clk_disable(i2c->clk);
>>> +			pm_runtime_put_sync(&adap->dev);
> 
>> I would go for just pm_runtime_put() here...
> 
> Yeah, me too but it seemed idiomatic.  Frankly I don't really understand
> why there's a synchronous version of put.

The synchronous put callbacks might be needed for instance in a bus core code
to make sure the child device drivers aren't accessing resources while
bus adapter (USB/PCI) is being unplugged for example. In cases like platform 
I2C bus controller it might be more optimal to use asynchronous put versions.
As there are usually performed fast multiple adjacent transfers and multiple
put runtime get/put might be merged to one get/put due to scheduling latency.
However I've never really measured that. Implementing autosuspend might give
some insights about that.

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

On Sat, Jan 21, 2012 at 09:38:13PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 07:31 PM, Mark Brown wrote:

> > Yeah, me too but it seemed idiomatic.  Frankly I don't really understand
> > why there's a synchronous version of put.

> The synchronous put callbacks might be needed for instance in a bus core code
> to make sure the child device drivers aren't accessing resources while
> bus adapter (USB/PCI) is being unplugged for example. In cases like platform 

In those cases it feels like we ought to either do what we do with
suspend and make sure the device is resumed on the exit path (like we do
when we're entering system suspend) or just disable the PM operations,
especially given that we're reference counting.

> I2C bus controller it might be more optimal to use asynchronous put versions.
> As there are usually performed fast multiple adjacent transfers and multiple
> put runtime get/put might be merged to one get/put due to scheduling latency.
> However I've never really measured that. Implementing autosuspend might give
> some insights about that.

As I was just saying in another thread I do think it's probably worth
considering adding some small amount of autosuspend by default in order
to try to deal with that sort of pattern.  Of course for I2C devices I
rather suspect the bus is sufficiently slow that any performance work we
do in the host is immaterial.  

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Heiko Stübner]

Am Samstag 21 Januar 2012, 21:38:13 schrieb Sylwester Nawrocki:
> On 01/21/2012 07:31 PM, Mark Brown wrote:
> > On Sat, Jan 21, 2012 at 04:52:58PM +0100, Sylwester Nawrocki wrote:
> >> On 01/21/2012 02:28 PM, Mark Brown wrote:
> > that the rather serious problems with getting I2C patches applied I'd
> > rather do as little as possible in individual patches.
> > 
> > It would also introduce a regression for systems that don't use runtime
> > PM (probably most s3c24xx ones).
> 
> Indeed, I'd forgotten about this fact for a while. I suspect the Run-time
> PM core functionality could be enabled on those platform, allowing to make
> the common driver dependant on PM_RUNTIME. Not sure if that's acceptable
> for the smaller SoCs though. It rather sounds more like lots of trouble
> for little benefits. Maybe it's desirable to do that at some point anyway.

At least S3C2416/S3C2450 and S3C2412 (i.e. the ARMv5 SoCs) might profit from 
it, as they also support the idle modes (stop modes) that Mark is targetting 
with his patches in the long run.

Not sure about the 2410, 2440 and 2443 currently

Heiko

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/21/2012 10:23 PM, Heiko Stübner wrote:
> Am Samstag 21 Januar 2012, 21:38:13 schrieb Sylwester Nawrocki:
>> On 01/21/2012 07:31 PM, Mark Brown wrote:
>>> On Sat, Jan 21, 2012 at 04:52:58PM +0100, Sylwester Nawrocki wrote:
>>>> On 01/21/2012 02:28 PM, Mark Brown wrote:
>>> that the rather serious problems with getting I2C patches applied I'd
>>> rather do as little as possible in individual patches.
>>>
>>> It would also introduce a regression for systems that don't use runtime
>>> PM (probably most s3c24xx ones).
>>
>> Indeed, I'd forgotten about this fact for a while. I suspect the Run-time
>> PM core functionality could be enabled on those platform, allowing to make
>> the common driver dependant on PM_RUNTIME. Not sure if that's acceptable
>> for the smaller SoCs though. It rather sounds more like lots of trouble
>> for little benefits. Maybe it's desirable to do that at some point anyway.
> 
> At least S3C2416/S3C2450 and S3C2412 (i.e. the ARMv5 SoCs) might profit from
> it, as they also support the idle modes (stop modes) that Mark is targetting
> with his patches in the long run.

Ah, I was just about to ask whether this patch is a part of some wider plan.
It would be much better to enable core runtime PM support on all platforms
that use particular driver, even though there is no any drivers adapted 
runtime PM on some of them yet.

> 
> Not sure about the 2410, 2440 and 2443 currently

But would just enabling RUNTIME_PM make any harm to those platforms ?

-- 

Regards,
Sylwester

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

On Sat, Jan 21, 2012 at 10:33:44PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 10:23 PM, Heiko Stübner wrote:

> > At least S3C2416/S3C2450 and S3C2412 (i.e. the ARMv5 SoCs) might profit from
> > it, as they also support the idle modes (stop modes) that Mark is targetting
> > with his patches in the long run.

> It would be much better to enable core runtime PM support on all platforms
> that use particular driver, even though there is no any drivers adapted 
> runtime PM on some of them yet.

It's just a Kconfig switch, the only issue is that users might not turn
it on and for platforms where there's not much driver support they're
more likely to not have done so.

> > Not sure about the 2410, 2440 and 2443 currently

> But would just enabling RUNTIME_PM make any harm to those platforms ?

It really shouldn't cause any issues but it seems better to not push
people towards it too much when there's not much win yet.  What I've
been doing with all these patches is leaving any PM that already exists
untouched (in so far as it's not buggy).  Where there's nothing already
and it's all new code I've been using a more modern idiom.  I hope that
this minimise any impact on existing systems.

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Sylwester Nawrocki]

On 01/21/2012 10:57 PM, Mark Brown wrote:
> On Sat, Jan 21, 2012 at 10:33:44PM +0100, Sylwester Nawrocki wrote:
>> On 01/21/2012 10:23 PM, Heiko Stübner wrote:
> 
>>> At least S3C2416/S3C2450 and S3C2412 (i.e. the ARMv5 SoCs) might profit from
>>> it, as they also support the idle modes (stop modes) that Mark is targetting
>>> with his patches in the long run.
> 
>> It would be much better to enable core runtime PM support on all platforms
>> that use particular driver, even though there is no any drivers adapted
>> runtime PM on some of them yet.
>
> It's just a Kconfig switch, the only issue is that users might not turn
> it on and for platforms where there's not much driver support they're
> more likely to not have done so.

Yes, and some simple SoCs might probably never benefit from runtime PM due to
their limited power modes. Hence enforcing RUNTIME_PM dependency on some 
common drivers might not be sane. But it would be ideal not to work around
things too much, and reimplement in drivers functionality that involves
upper layers.
 
>>> Not sure about the 2410, 2440 and 2443 currently
> 
>> But would just enabling RUNTIME_PM make any harm to those platforms ?
> 
> It really shouldn't cause any issues but it seems better to not push
> people towards it too much when there's not much win yet.  What I've
> been doing with all these patches is leaving any PM that already exists
> untouched (in so far as it's not buggy).  Where there's nothing already
> and it's all new code I've been using a more modern idiom.  I hope that
> this minimise any impact on existing systems.

Sounds good. The idea of enforcing runtime PM seems to be good for /dev/null
only.. I noticed recently that you did a nice work correcting broken s3c-fb 
driver PM code. I've done similar corrections but the patches never left the 
internal trees due to limited time for this. And there is also a DRM driver 
for FIMD on Exynos now.

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Mark Brown]

On Sat, Jan 21, 2012 at 11:49:27PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 10:57 PM, Mark Brown wrote:

> > It's just a Kconfig switch, the only issue is that users might not turn
> > it on and for platforms where there's not much driver support they're
> > more likely to not have done so.

> Yes, and some simple SoCs might probably never benefit from runtime PM due to
> their limited power modes. Hence enforcing RUNTIME_PM dependency on some 
> common drivers might not be sane. But it would be ideal not to work around
> things too much, and reimplement in drivers functionality that involves
> upper layers.

Yes, that's the thinking behind my approach of not touching existing
code - I don't feel too bad about making new stuff depend on new
features but for older stuff I'd rather leave it alone for now.  Once we
can manage to deliver the WFI states there would be a very useful win
there so there's going to be a benefit to users from turning it on.

> only.. I noticed recently that you did a nice work correcting broken s3c-fb 
> driver PM code. I've done similar corrections but the patches never left the 

Thanks.

> internal trees due to limited time for this. And there is also a DRM driver 
> for FIMD on Exynos now.

Yeah, I don't really have access to too much of the shiny new stuff
right now so I've not been able to take advantage of a lot of the stuff
you guys are doing.  My primary development platform is S3C6410 based at
the minute.

Re: [PATCH 2/2] i2c-s3c2410: Add stub runtime power management

[Ben Dooks]

On Sat, Jan 21, 2012 at 01:28:47PM +0000, Mark Brown wrote:
> Add stub runtime_pm calls which go through the flow of enabling and
> disabling but don't actually do anything with the device itself as
> there's nothing useful we can do. This provides the core PM framework
> with information about when the device is idle, enabling chip wide
> power savings.
> 
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> Acked-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>

I've applied these two to for-34/i2c/i2c-samsung and will start a -next
branch later in the week

> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index e6f982b..737f721 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -31,6 +31,7 @@
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/slab.h>
> @@ -564,6 +565,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>  	int retry;
>  	int ret;
>  
> +	pm_runtime_get_sync(&adap->dev);
>  	clk_enable(i2c->clk);
>  
>  	for (retry = 0; retry < adap->retries; retry++) {
> @@ -572,6 +574,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>  
>  		if (ret != -EAGAIN) {
>  			clk_disable(i2c->clk);
> +			pm_runtime_put_sync(&adap->dev);
>  			return ret;
>  		}
>  
> @@ -581,6 +584,7 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
>  	}
>  
>  	clk_disable(i2c->clk);
> +	pm_runtime_put_sync(&adap->dev);
>  	return -EREMOTEIO;
>  }
>  
> @@ -1013,6 +1017,9 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>  	of_i2c_register_devices(&i2c->adap);
>  	platform_set_drvdata(pdev, i2c);
>  
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_enable(&i2c->adap.dev);
> +
>  	dev_info(&pdev->dev, "%s: S3C I2C adapter\n", dev_name(&i2c->adap.dev));
>  	clk_disable(i2c->clk);
>  	return 0;
> @@ -1047,6 +1054,9 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
>  {
>  	struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
>  
> +	pm_runtime_disable(&i2c->adap.dev);
> +	pm_runtime_disable(&pdev->dev);
> +
>  	s3c24xx_i2c_deregister_cpufreq(i2c);
>  
>  	i2c_del_adapter(&i2c->adap);
> -- 
> 1.7.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html