From: Uwe Kleine-König <mailto:[email protected]> Sent: Tuesday,
October 13, 2015 3:07 PM
> To: Gao Pan-B54642
> Cc: [email protected]; [email protected]; Li Frank-B20596; Duan
> Fugang-B38611; [email protected]; [email protected]
> Subject: Re: [Patch V8] i2c: imx: add runtime pm support to improve the
> performance
>
> Hello,
>
> On Fri, Sep 18, 2015 at 05:51:09PM +0800, Gao Pan wrote:
> > In our former i2c driver, i2c clk is enabled and disabled in xfer
> > function, which contributes to power saving. However, the clk enable
> > process brings a busy wait delay until the core is stable. As a
> > result, the performance is sacrificed.
> >
> > To weigh the power consumption and i2c bus performance, runtime pm is
> > the good solution for it. The clk is enabled when a i2c transfer
> > starts, and disabled after a specifically defined delay.
> >
> > Without the patch the test case (many eeprom reads) executes with
> approx:
> > real 1m7.735s
> > user 0m0.488s
> > sys 0m20.040s
> >
> > With the patch the same test case (many eeprom reads) executes with
> approx:
> > real 0m54.241s
> > user 0m0.440s
> > sys 0m5.920s
> >
> > Signed-off-by: Fugang Duan <[email protected]>
> > Signed-off-by: Gao Pan <[email protected]>
>
> If runtime-pm is disabled the net result of this patch is that the clock
> is never disabled, right? I think this is ok, but I would have pointed
> that out in the commit log.
Yes, you are right, I will point that out in the commit log in next version.
> > @@ -1037,6 +1045,18 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> > /* Set up adapter data */
> > i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
> >
> > + /* Set up platform driver data */
> > + platform_set_drvdata(pdev, i2c_imx);
> > +
> > + pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
> > + pm_runtime_use_autosuspend(&pdev->dev);
> > + pm_runtime_set_active(&pdev->dev);
> > + pm_runtime_enable(&pdev->dev);
> > +
> > + ret = pm_runtime_get_sync(&pdev->dev);
> > + if (ret < 0)
> > + goto rpm_disable;
> > +
> > /* Set up clock divider */
> > i2c_imx->bitrate = IMX_I2C_BIT_RATE;
> > ret = of_property_read_u32(pdev->dev.of_node,
> > @@ -1053,12 +1073,11 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> > ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "registration failed\n");
> > - goto clk_disable;
> > + goto rpm_disable;
>
> Is it right, that here the same error path is taken as when
> pm_runtime_get_sync fails above? Even if it works now, not having the
> error cleanup path undo all things in reverse order is a danger to get it
> wrong in the next change. So if this is fixable, it would be nice to
> implement.
When i2c_add_numbered_adapter fails, the cleanup should include the runtime pm
stuff.
So it's ok to take the same error patch.
What do you mean by undo all things in reverse order, I think the cleanup is in
reverse order.
The calling sequence is:
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
pm_runtime_get_sync(&pdev->dev);
The cleanup order is:
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
Is it that I miss pm_runtime_dont_use_autosuspend(&pdev->dev);
Thank you!
Best Reguards
Gao Pan
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
