i2c imx: suspicious use of runtime PM calls

i2c imx: suspicious use of runtime PM calls

[Heiner Kallweit]

Just by chance I stumbled across this driver and the recently added runtime PM code.
Two things in i2c_imx_probe look suspicious to me:

- You activate the I2C clock and then initialize runtime PM with

    pm_runtime_enable(&pdev->dev);
    pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
    pm_runtime_use_autosuspend(&pdev->dev);

  Seems like a call to pm_runtime_set_active is missing to tell runtime PM
  that the device is in status "active" now.

- You use pm_runtime_put_autosuspend (which decrements the ref count) but you don't
  get a reference before. I think you would have to use pm_runtime_get_... before
  or use pm_runtime_autosuspend.

Regards, Heiner

RE: i2c imx: suspicious use of runtime PM calls

[Gao Pandy]

From: Heiner Kallweit <mailto:hkallweit1@gmail.com> Sent: Sunday, August 23, 2015 5:42 AM
> To: Gao Pan-B54642
> Cc: linux-i2c@vger.kernel.org; Wolfram Sang
> Subject: i2c imx: suspicious use of runtime PM calls
> 
> Just by chance I stumbled across this driver and the recently added
> runtime PM code.
> Two things in i2c_imx_probe look suspicious to me:
> 
> - You activate the I2C clock and then initialize runtime PM with
> 
>     pm_runtime_enable(&pdev->dev);
>     pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
>     pm_runtime_use_autosuspend(&pdev->dev);
> 
>   Seems like a call to pm_runtime_set_active is missing to tell runtime
> PM that the device is in status "active" now.
 
Thanks, you are right. I will fix it in next patch.

> - You use pm_runtime_put_autosuspend (which decrements the ref count) but
> you don't
>   get a reference before. I think you would have to use pm_runtime_get_...
> before or use pm_runtime_autosuspend.

Thanks for your precise review. I will fix it in next patch.

Re: i2c imx: suspicious use of runtime PM calls

[Wolfram Sang]

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


> > Just by chance I stumbled across this driver and the recently added
> > runtime PM code.
> > Two things in i2c_imx_probe look suspicious to me:
> > 
> > - You activate the I2C clock and then initialize runtime PM with
> > 
> >     pm_runtime_enable(&pdev->dev);
> >     pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> >     pm_runtime_use_autosuspend(&pdev->dev);
> > 
> >   Seems like a call to pm_runtime_set_active is missing to tell runtime
> > PM that the device is in status "active" now.
>  
> Thanks, you are right. I will fix it in next patch.

Thanks Heiner for the report. I dropped this patch and will wait for a
new version.


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