Re: [PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4280 bytes --]

Hi,

On Wed, Aug 03, 2011 at 11:09:20AM -0700, Kevin Hilman wrote:
> Current usage of runtime PM is not quite correct.  The actual
> idle/unidle of the I2C hardware should not happen until the runtime PM
> callbacks are called.  Therefore, change omap_i2c_[un]idle() functions
> to only be called from the runtime PM callbacks (when usage count
> transitions to/from zero.)
> 
> Also, the runtime PM core does usage counting and replaces
> functionality currently managed by the dev->idle flag.  Remove usage
> of dev->idle in favor of using runtime PM, and checking status using
> pm_runtime_suspended().
> 
> Signed-off-by: Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |   58 ++++++++++++++++++++++++++--------------
>  1 files changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 12d0cbc..1b5325b 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -194,7 +194,6 @@ struct omap_i2c_dev {
>  						 */
>  	u8			rev;
>  	unsigned		b_hw:1;		/* bad h/w fixes */
> -	unsigned		idle:1;
>  	u16			iestate;	/* Saved interrupt register */
>  	u16			pscstate;
>  	u16			scllstate;
> @@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>  {
>  	struct omap_i2c_bus_platform_data *pdata;
>  
> -	WARN_ON(!dev->idle);
> -
>  	pdata = dev->dev->platform_data;
>  
> -	pm_runtime_get_sync(dev->dev);
> -
>  	if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>  		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
> @@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>  		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  	}
> -	dev->idle = 0;
>  
>  	/*
>  	 * Don't write to this register if the IE state is 0 as it can
> @@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
>  	struct omap_i2c_bus_platform_data *pdata;
>  	u16 iv;
>  
> -	WARN_ON(dev->idle);
> -
>  	pdata = dev->dev->platform_data;
>  
>  	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> @@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
>  	} else {
>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
>  
> -		/* Flush posted write before the dev->idle store occurs */
> +		/* Flush posted write */
>  		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>  	}
> -	dev->idle = 1;
> -
> -	pm_runtime_put_sync(dev->dev);
>  }
>  
>  static int omap_i2c_init(struct omap_i2c_dev *dev)
> @@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int i;
>  	int r;
>  
> -	omap_i2c_unidle(dev);
> +	pm_runtime_get_sync(dev->dev);
>  
>  	r = omap_i2c_wait_for_bb(dev);
>  	if (r < 0)
> @@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  	omap_i2c_wait_for_bb(dev);
>  out:
> -	omap_i2c_idle(dev);
> +	pm_runtime_put_sync(dev->dev);

I wonder if these pm_runtime_put need to be synchronous ? Could we just
call pm_runtime_put() instead ? Ditto to all other.

> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_RUNTIME
> +static int omap_i2c_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);

what happened to dev_get_drvdata(dev) ??

> +	omap_i2c_idle(_dev);
> +
> +	return 0;
> +}
> +
> +static int omap_i2c_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);

ditto

> +	omap_i2c_unidle(_dev);
> +
> +	return 0;
> +}
> +
> +static struct dev_pm_ops omap_i2c_pm_ops = {
> +	.runtime_suspend = omap_i2c_runtime_suspend,
> +	.runtime_resume = omap_i2c_runtime_resume,
> +};
> +#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> +#else
> +#define OMAP_I2C_PM_OPS NULL
> +#endif

OMAP_I2C_PM_OPS isn't used anywhere ??

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]