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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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.