[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 4c17180..e6f982b 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -890,7 +890,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;
@@ -1035,7 +1035,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	clk_put(i2c->clk);
 
  err_noclk:
-	kfree(i2c);
 	return ret;
 }
 
@@ -1061,7 +1060,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>
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

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

[Sylwester Nawrocki]

On 01/21/2012 02:28 PM, Mark Brown wrote:
> 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 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 4c17180..e6f982b 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -890,7 +890,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);

Just in case you are resending the patch for any other reason, it might be worth
to change to sizeof(*i2c) for consistency.

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

[Mark Brown]

On Sat, Jan 21, 2012 at 05:10:22PM +0100, Sylwester Nawrocki wrote:
> > -	i2c = kzalloc(sizeof(struct s3c24xx_i2c), GFP_KERNEL);
> > +	i2c = devm_kzalloc(&pdev->dev, sizeof(struct s3c24xx_i2c), GFP_KERNEL);

> Just in case you are resending the patch for any other reason, it might be worth
> to change to sizeof(*i2c) for consistency.

Consistency with...?

TBH I'd rather not change the coding style as well as presumably Ben
likes that the way it is.

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

[Sylwester Nawrocki]

On 01/21/2012 06:57 PM, Mark Brown wrote:
> On Sat, Jan 21, 2012 at 05:10:22PM +0100, Sylwester Nawrocki wrote:
>>> -	i2c = kzalloc(sizeof(struct s3c24xx_i2c), GFP_KERNEL);
>>> +	i2c = devm_kzalloc(&pdev->dev, sizeof(struct s3c24xx_i2c), GFP_KERNEL);
> 
>> Just in case you are resending the patch for any other reason, it might be worth
>> to change to sizeof(*i2c) for consistency.
> 
> Consistency with...?

Consistency with the rest of code in i2c-s3c244.c, all other occurrences
of sizeof take name of a variable, rather than its type.
 
> TBH I'd rather not change the coding style as well as presumably Ben
> likes that the way it is.

If that's Ben's will then let it stay as it is. I don't even dare to propose
any change ;) It's rather a meaningless detail. I personally like more using 
variable's name, and I had a feeling it was preferred style in the kernel.

It's even mentioned in Documentation/CodingStyle:

"The kernel provides the following general purpose memory allocators:
kmalloc(), kzalloc(), kcalloc(), vmalloc(), and vzalloc().  Please refer to
the API documentation for further information about them.

The preferred form for passing a size of a struct is the following:

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not."

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

[Mark Brown]

On Sat, Jan 21, 2012 at 07:27:32PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 06:57 PM, Mark Brown wrote:

> > TBH I'd rather not change the coding style as well as presumably Ben
> > likes that the way it is.

> If that's Ben's will then let it stay as it is. I don't even dare to propose
> any change ;) It's rather a meaningless detail. I personally like more using 
> variable's name, and I had a feeling it was preferred style in the kernel.

I tend to use the variable myself, but when editing existing code I'm
going to stick with what's there.

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

[Ben Dooks]

On Sat, Jan 21, 2012 at 05:10:22PM +0100, Sylwester Nawrocki wrote:
> On 01/21/2012 02:28 PM, Mark Brown wrote:
> > 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 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 4c17180..e6f982b 100644
> > --- a/drivers/i2c/busses/i2c-s3c2410.c
> > +++ b/drivers/i2c/busses/i2c-s3c2410.c
> > @@ -890,7 +890,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);
> 
> Just in case you are resending the patch for any other reason, it might be worth
> to change to sizeof(*i2c) for consistency.

Yuck.

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

[Shubhrajyoti Datta]

Hi Mark,

The changes look good to me.
Reviewed-by: Datta Shubhrajyoti <shubhrajyoti-l0cyMroinI0@public.gmane.org>
For your changes.

Some other doubts though not related to your patch.
Just curious.

On Sat, Jan 21, 2012 at 6:58 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> 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 4c17180..e6f982b 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -890,7 +890,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");

The comment looks confusing as I am not sure what is meant here by state.


>                return -ENOMEM;
> @@ -1035,7 +1035,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>        clk_put(i2c->clk);
>
>  err_noclk:
> -       kfree(i2c);
>        return ret;
>  }
>
> @@ -1061,7 +1060,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
>        release_resource(i2c->ioarea);
>        s3c24xx_i2c_dt_gpio_free(i2c);
>        kfree(i2c->ioarea);
Do we need both  release_resource and kfree

> -       kfree(i2c);
>
>        return 0;
>  }
> --
> 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

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

[Sylwester Nawrocki]

Hi Shubhrajyoti,

On 02/14/2012 02:03 PM, Shubhrajyoti Datta wrote:
> Hi Mark,
> 
> The changes look good to me.
> Reviewed-by: Datta Shubhrajyoti <shubhrajyoti@ti.com>
> For your changes.
> 
> Some other doubts though not related to your patch.
> Just curious.
> 
...
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> index 4c17180..e6f982b 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
...
>> @@ -1061,7 +1060,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
>>        release_resource(i2c->ioarea);
>>        s3c24xx_i2c_dt_gpio_free(i2c);
>>        kfree(i2c->ioarea);
> Do we need both  release_resource and kfree

No, instead of release_resource() and kfree() you normally use
release_mem_region() to free resources acquired with request_mem_region().
See this patch for instance:

http://www.spinics.net/lists/linux-samsung-soc/msg08325.html

--
Thanks,
Sylwester

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

[Sylwester Nawrocki]

On 01/21/2012 02:28 PM, Mark Brown wrote:
> 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@opensource.wolfsonmicro.com>

Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

> ---
>   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 4c17180..e6f982b 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -890,7 +890,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;
> @@ -1035,7 +1035,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>   	clk_put(i2c->clk);
> 
>    err_noclk:
> -	kfree(i2c);
>   	return ret;
>   }
> 
> @@ -1061,7 +1060,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;
>   }

-- 

Thanks!
Sylwester

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

[Barry Song]

2012/1/21 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> 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@opensource.wolfsonmicro.com>

Reviewed-by: Barry Song <Baohua.Song@csr.com>

> ---
>  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 4c17180..e6f982b 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -890,7 +890,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;
> @@ -1035,7 +1035,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>        clk_put(i2c->clk);
>
>  err_noclk:
> -       kfree(i2c);
>        return ret;
>  }
>
> @@ -1061,7 +1060,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 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