RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[Aisheng Dong]

[...]

> > > >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> > > I2C_PM_TIMEOUT);
> > > >  	pm_runtime_use_autosuspend(&pdev->dev);
> > > > -	pm_runtime_get_noresume(&pdev->dev);
> > > > -	pm_runtime_set_active(&pdev->dev);
> > > >  	pm_runtime_enable(&pdev->dev);
> > > >
> > > > +	ret = pm_runtime_get_sync(&pdev->dev);
> > > > +	if (ret < 0) {
> > > > +		pm_runtime_put_noidle(&pdev->dev);
> > > > +		dev_err(&pdev->dev, "failed to enable clock\n");
> > > > +		return ret;
> > > > +	}
> > >
> > > Can't current clk control via rpm work well?
> > > Please describe why need change.
> >
> > I think the previous patch maker might want to use the return value of
> > pm_runtime_get_sync to check whether the clock has been turned on
> > correctly to
> > avoid the kernel panic.
> > Maybe I can change to the method like this.
> > 	pm_runtime_get_noresume(&pdev->dev);
> > 	ret = pm_runtime_set_active(&pdev->dev);
> > 	if (ret < 0)
> > 		goto out;
> > 	pm_runtime_enable(&pdev->dev);
> >
> > Best Regards,
> > Clark Wang
> 
> Sorry, I missed the point before.
> If we use pm_runtime_get_noresume(&pdev->dev); and
> pm_runtime_set_active(&pdev->dev); here, the clk should be enabled by using
> clk_prepare_enable() in the probe function. However, the call of
> clk_prepare_enable() is already in lpi2c_runtime_resume().
> Using get_sync() here can help to reduce the repetitive code, especially ipg
> clk will be added later.

Let's not do for this negligible improvement unless there're any other good benefits.
If really care, move clk operation into an inline function instead.
Another benefit if not doing that is the driver still can work even RPM not enabled.

Regards
Aisheng

> Shall we change to use pm_runtime_get_sync() here?
> 
> Regards,
> Clark Wang
>