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

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

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.