Felipe Balbi <balbi@xxxxxx> writes:
> 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@xxxxxx>
>> ---
>> 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.
Yes, will use asynchronous versions.
>> @@ -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) ??
>
Yes, that would work too since:
static inline void *platform_get_drvdata(const struct platform_device *pdev)
{
return dev_get_drvdata(&pdev->dev);
}
but IMO, readability is better if the driver does platform_set_drvdata()
and then the corresponding platform_get_drvdata()
>> + 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 ??
doh
thanks for catching
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
