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.
> @@ -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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
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
