Re: [Patch V8] i2c: imx: add runtime pm support to improve the performance

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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@freescale.com>
> Signed-off-by: Gao Pan <b54642@freescale.com>

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/  |