From: Uwe Kleine-König <mailto:u.kleine-koenig@xxxxxxxxxxxxxx> Sent: Tuesday, October 13, 2015 3:07 PM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan > Fugang-B38611; kernel@xxxxxxxxxxxxxx; hkallweit1@xxxxxxxxx > 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 <B38611@xxxxxxxxxxxxx> > > Signed-off-by: Gao Pan <b54642@xxxxxxxxxxxxx> > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
