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

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Thu, Jun 11, 2015 at 09:50:04AM +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.
Which platform are you referring here? Looking at i.MX21 I cannot find a
delay for the i2c clock.

> To weigh the power consuption and i2c bus performance, runtime
consumption

> pm is the good solution for it. The clk is enabled when a i2c
> transfer starts, and disabled afer a specifically defined delay.
after

> [...]
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 785aa67..cc4b5d6 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> [...]
> @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>  
>  	i2c_imx_set_clk(i2c_imx);
>  
> -	result = clk_prepare_enable(i2c_imx->clk);
> -	if (result)
> -		return result;
you remove clk_prepare_enable enable and instead rely on
clk_prepare_enable in i2c_imx_runtime_resume, right?

If I understand correctly this results in never enabling the clock if
CONFIG_PM is disabled?!

> [...]
> @@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  	struct imx_i2c_struct *i2c_imx = dev_id;
>  	unsigned int temp;
>  
> +	if (pm_runtime_suspended(i2c_imx->adapter.dev.parent))
> +		return IRQ_NONE;
> +
I don't claim to understand the runtime pm stuff, but I agree to the
previous reviewer that this smells fishy. If this is required it needs a
comment why.

>  	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>  	if (temp & I2SR_IIF) {
>  		/* save status register */
> @@ -894,6 +896,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>  
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>  
> +	result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
> +	if (IS_ERR_VALUE(result))
Is this better than
+	if (result < 0)
?

> +		goto out;
> [...]
> @@ -1018,11 +1028,6 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  		return PTR_ERR(i2c_imx->clk);
>  	}
>  
> -	ret = clk_prepare_enable(i2c_imx->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "can't enable I2C clock\n");
> -		return ret;
> -	}
Is there a reason the clock was enabled *here* before. With the way you
rearranged the code the irq handler might be entered before the clock is
on. But if I'm not mistaken (which might very well be the case) there is
a problem also without the patch if the bootloader jumps to linux with a
pending i2c transfer that fires when the clk_disable is done below. IMHO
"Set up chip registers to defaults" should be done before "Request
IRQ"?!

> [...]
> @@ -1093,14 +1110,51 @@ static int i2c_imx_remove(struct platform_device *pdev)
> [...]
> +#ifdef CONFIG_PM
> +static int i2c_imx_runtime_suspend(struct device *dev)
> +{
> +	struct imx_i2c_struct *i2c_imx  = dev_get_drvdata(dev);
s/  / /
> [...]
> +static int i2c_imx_runtime_resume(struct device *dev)
> +{
> +	struct imx_i2c_struct *i2c_imx  = dev_get_drvdata(dev);
s/  / /
> +	int ret;
> +
> +	ret = clk_prepare_enable(i2c_imx->clk);
> +	if (ret) {
> +		dev_err(dev, "can't enable I2C clock\n");
> +		return ret;
> +	}
> +
> +	return 0;
this is equivalent to:

	if (ret)
		dev_err(dev, "can't enable I2C clock\n");

	return ret;

Probably a matter of taste. Also I'd suggest to add the value of ret to
the message to make the report more useful.

> +}
> +
> +static const struct dev_pm_ops i2c_imx_pm_ops = {
> +	SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend,
> +			   i2c_imx_runtime_resume, NULL)
> +};
> +#define I2C_IMX_PM_OPS (&i2c_imx_pm_ops)
> +#else
> +#define I2C_IMX_PM_OPS NULL
> +#endif /* CONFIG_PM */
> +
>  static struct platform_driver i2c_imx_driver = {
>  	.probe = i2c_imx_probe,
>  	.remove = i2c_imx_remove,
>  	.driver	= {
>  		.name	= DRIVER_NAME,
> +		.pm     = I2C_IMX_PM_OPS,
>  		.of_match_table = i2c_imx_dt_ids,
This increases the number of styles to place the = to three. .name uses
a tab, .of_match_table a space and you add .pm with >1 spaces.

>  	},
>  	.id_table	= imx_i2c_devtype,
unrelated to this patch: The = in the line above is not aligned
consistently compared to the other members at the same level like
.probe.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |