diff mbox series

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

Message ID 20210317065359.3109394-3-xiaoning.wang@nxp.com (mailing list archive)
State New, archived
Headers show
Series i2c: imx-lpi2c: New features and bug fixes | expand

Commit Message

Clark Wang March 17, 2021, 6:53 a.m. UTC 
From: Fugang Duan <fugang.duan@nxp.com>

- Add runtime pm support to dynamicly manage the clock.
- Put the suspend to suspend_noirq.
- Call .pm_runtime_force_suspend() to force runtime pm suspended
  in .suspend_noirq().

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 50 ++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 17 deletions(-)

Comments

Dong Aisheng March 19, 2021, 4:40 a.m. UTC | #1 
> From: Clark Wang <xiaoning.wang@nxp.com>
> Sent: Wednesday, March 17, 2021 2:54 PM
> Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
> 
> - Add runtime pm support to dynamicly manage the clock.
> - Put the suspend to suspend_noirq.
> - Call .pm_runtime_force_suspend() to force runtime pm suspended
>   in .suspend_noirq().
> 

The patch title needs to be improved as the driver already supports rpm.
And do one thing in one patch.

> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> Reviewed-by: Anson Huang <Anson.Huang@nxp.com>

Please add your sign-off.

> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 50 ++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index bbf44ac95021..1e920e7ac7c1 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device
> *pdev)
>  	if (ret)
>  		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
> 
> -	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> +	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
> +			       IRQF_NO_SUSPEND,

This belongs to a separate patch

>  			       pdev->name, lpi2c_imx);
>  	if (ret) {
>  		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -584,35
> +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
>  	platform_set_drvdata(pdev, lpi2c_imx);
> 
> -	ret = clk_prepare_enable(lpi2c_imx->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
> -		return ret;
> -	}
> -
>  	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.

> +
>  	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
>  	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
>  	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
> 
> +	pm_runtime_put(&pdev->dev);
> +
>  	ret = i2c_add_adapter(&lpi2c_imx->adapter);
>  	if (ret)
>  		goto rpm_disable;
> 
> -	pm_runtime_mark_last_busy(&pdev->dev);
> -	pm_runtime_put_autosuspend(&pdev->dev);
> -
>  	dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
> 
>  	return 0;
> 
>  rpm_disable:
> -	pm_runtime_put(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> 
> @@ -636,7 +634,7 @@ static int __maybe_unused
> lpi2c_runtime_suspend(struct device *dev)
>  	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
> 
>  	clk_disable_unprepare(lpi2c_imx->clk);
> -	pinctrl_pm_select_sleep_state(dev);
> +	pinctrl_pm_select_idle_state(dev);

This belongs to a separate patch

> 
>  	return 0;
>  }
> @@ -649,16 +647,34 @@ static int __maybe_unused
> lpi2c_runtime_resume(struct device *dev)
>  	pinctrl_pm_select_default_state(dev);
>  	ret = clk_prepare_enable(lpi2c_imx->clk);
>  	if (ret) {
> -		dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret);
> +		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);

Probably unnecessary change

>  		return ret;
>  	}
> 
> +	return ret;
> +}
> +
> +static int lpi2c_suspend_noirq(struct device *dev) {
> +	int ret;
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	pinctrl_pm_select_sleep_state(dev);
> +
>  	return 0;
>  }
> 
> +static int lpi2c_resume_noirq(struct device *dev) {
> +	return pm_runtime_force_resume(dev);
> +}
> +
>  static const struct dev_pm_ops lpi2c_pm_ops = {
> -	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				      pm_runtime_force_resume)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
> +				     lpi2c_resume_noirq)

Belongs to separate change and explain why need do this

>  	SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
>  			   lpi2c_runtime_resume, NULL)
>  };
> --
> 2.25.1
Clark Wang March 19, 2021, 5:33 a.m. UTC | #2 
> -----Original Message-----
> From: Aisheng Dong <aisheng.dong@nxp.com>
> Sent: Friday, March 19, 2021 12:40
> To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
> linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
> 
> > From: Clark Wang <xiaoning.wang@nxp.com>
> > Sent: Wednesday, March 17, 2021 2:54 PM
> > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
> >
> > - Add runtime pm support to dynamicly manage the clock.
> > - Put the suspend to suspend_noirq.
> > - Call .pm_runtime_force_suspend() to force runtime pm suspended
> >   in .suspend_noirq().
> >
> 
> The patch title needs to be improved as the driver already supports rpm.
> And do one thing in one patch.

Thanks. I will split this patch into several and resend.

Best Regards,
Clark Wang
> 
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
> 
> Please add your sign-off.
> 
> > ---
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 50
> > ++++++++++++++++++++----------
> >  1 file changed, 33 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index bbf44ac95021..1e920e7ac7c1 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device
> > *pdev)
> >  	if (ret)
> >  		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
> >
> > -	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> > +	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
> > +			       IRQF_NO_SUSPEND,
> 
> This belongs to a separate patch
> 
> >  			       pdev->name, lpi2c_imx);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -
> 584,35
> > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> >  	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
> >  	platform_set_drvdata(pdev, lpi2c_imx);
> >
> > -	ret = clk_prepare_enable(lpi2c_imx->clk);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
> > -		return ret;
> > -	}
> > -
> >  	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.
> 
> > +
> >  	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
> >  	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
> >  	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
> >
> > +	pm_runtime_put(&pdev->dev);
> > +
> >  	ret = i2c_add_adapter(&lpi2c_imx->adapter);
> >  	if (ret)
> >  		goto rpm_disable;
> >
> > -	pm_runtime_mark_last_busy(&pdev->dev);
> > -	pm_runtime_put_autosuspend(&pdev->dev);
> > -
> >  	dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
> >
> >  	return 0;
> >
> >  rpm_disable:
> > -	pm_runtime_put(&pdev->dev);
> >  	pm_runtime_disable(&pdev->dev);
> >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> >
> > @@ -636,7 +634,7 @@ static int __maybe_unused
> > lpi2c_runtime_suspend(struct device *dev)
> >  	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
> >
> >  	clk_disable_unprepare(lpi2c_imx->clk);
> > -	pinctrl_pm_select_sleep_state(dev);
> > +	pinctrl_pm_select_idle_state(dev);
> 
> This belongs to a separate patch
> 
> >
> >  	return 0;
> >  }
> > @@ -649,16 +647,34 @@ static int __maybe_unused
> > lpi2c_runtime_resume(struct device *dev)
> >  	pinctrl_pm_select_default_state(dev);
> >  	ret = clk_prepare_enable(lpi2c_imx->clk);
> >  	if (ret) {
> > -		dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret);
> > +		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
> 
> Probably unnecessary change
> 
> >  		return ret;
> >  	}
> >
> > +	return ret;
> > +}
> > +
> > +static int lpi2c_suspend_noirq(struct device *dev) {
> > +	int ret;
> > +
> > +	ret = pm_runtime_force_suspend(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pinctrl_pm_select_sleep_state(dev);
> > +
> >  	return 0;
> >  }
> >
> > +static int lpi2c_resume_noirq(struct device *dev) {
> > +	return pm_runtime_force_resume(dev); }
> > +
> >  static const struct dev_pm_ops lpi2c_pm_ops = {
> > -	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > -				      pm_runtime_force_resume)
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
> > +				     lpi2c_resume_noirq)
> 
> Belongs to separate change and explain why need do this
> 
> >  	SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
> >  			   lpi2c_runtime_resume, NULL)
> >  };
> > --
> > 2.25.1
Clark Wang March 19, 2021, 6:16 a.m. UTC | #3 
> -----Original Message-----
> From: Aisheng Dong <aisheng.dong@nxp.com>
> Sent: Friday, March 19, 2021 12:40
> To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
> linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
>
> > From: Clark Wang <xiaoning.wang@nxp.com>
> > Sent: Wednesday, March 17, 2021 2:54 PM
> > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
> >
> > - Add runtime pm support to dynamicly manage the clock.
> > - Put the suspend to suspend_noirq.
> > - Call .pm_runtime_force_suspend() to force runtime pm suspended
> >   in .suspend_noirq().
> >
>
> The patch title needs to be improved as the driver already supports rpm.
> And do one thing in one patch.
>
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
>
> Please add your sign-off.
>
> > ---
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 50
> > ++++++++++++++++++++----------
> >  1 file changed, 33 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index bbf44ac95021..1e920e7ac7c1 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device
> > *pdev)
> >  	if (ret)
> >  		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
> >
> > -	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> > +	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
> > +			       IRQF_NO_SUSPEND,
>
> This belongs to a separate patch
>
> >  			       pdev->name, lpi2c_imx);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -
> 584,35
> > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> >  	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
> >  	platform_set_drvdata(pdev, lpi2c_imx);
> >
> > -	ret = clk_prepare_enable(lpi2c_imx->clk);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
> > -		return ret;
> > -	}
> > -
> >  	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
Clark Wang March 19, 2021, 8 a.m. UTC | #4 
> -----Original Message-----
> From: Clark Wang <xiaoning.wang@nxp.com>
> Sent: Friday, March 19, 2021 14:16
> To: Aisheng Dong <aisheng.dong@nxp.com>; shawnguo@kernel.org;
> s.hauer@pengutronix.de
> Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
> linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
>
>
> > -----Original Message-----
> > From: Aisheng Dong <aisheng.dong@nxp.com>
> > Sent: Friday, March 19, 2021 12:40
> > To: Clark Wang <xiaoning.wang@nxp.com>; shawnguo@kernel.org;
> > s.hauer@pengutronix.de
> > Cc: kernel@pengutronix.de; festevam@gmail.com; dl-linux-imx <linux-
> > imx@nxp.com>; sumit.semwal@linaro.org; christian.koenig@amd.com;
> > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > kernel@vger.kernel.org
> > Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
> >
> > > From: Clark Wang <xiaoning.wang@nxp.com>
> > > Sent: Wednesday, March 17, 2021 2:54 PM
> > > Subject: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support
> > >
> > > - Add runtime pm support to dynamicly manage the clock.
> > > - Put the suspend to suspend_noirq.
> > > - Call .pm_runtime_force_suspend() to force runtime pm suspended
> > >   in .suspend_noirq().
> > >
> >
> > The patch title needs to be improved as the driver already supports rpm.
> > And do one thing in one patch.
> >
> > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > > Reviewed-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > Please add your sign-off.
> >
> > > ---
> > >  drivers/i2c/busses/i2c-imx-lpi2c.c | 50
> > > ++++++++++++++++++++----------
> > >  1 file changed, 33 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > index bbf44ac95021..1e920e7ac7c1 100644
> > > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > > @@ -574,7 +574,8 @@ static int lpi2c_imx_probe(struct platform_device
> > > *pdev)
> > >  	if (ret)
> > >  		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
> > >
> > > -	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
> > > +	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
> > > +			       IRQF_NO_SUSPEND,
> >
> > This belongs to a separate patch
> >
> > >  			       pdev->name, lpi2c_imx);
> > >  	if (ret) {
> > >  		dev_err(&pdev->dev, "can't claim irq %d\n", irq); @@ -
> > 584,35
> > > +585,32 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
> > >  	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
> > >  	platform_set_drvdata(pdev, lpi2c_imx);
> > >
> > > -	ret = clk_prepare_enable(lpi2c_imx->clk);
> > > -	if (ret) {
> > > -		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
> > > -		return ret;
> > > -	}
> > > -
> > >  	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.
Shall we change to use pm_runtime_get_sync() here?

Regards,
Clark Wang
Dong Aisheng March 19, 2021, 10:26 a.m. UTC | #5 
[...]

> > > >  	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
>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index bbf44ac95021..1e920e7ac7c1 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -574,7 +574,8 @@  static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr, 0,
+	ret = devm_request_irq(&pdev->dev, irq, lpi2c_imx_isr,
+			       IRQF_NO_SUSPEND,
 			       pdev->name, lpi2c_imx);
 	if (ret) {
 		dev_err(&pdev->dev, "can't claim irq %d\n", irq);
@@ -584,35 +585,32 @@  static int lpi2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
 
-	ret = clk_prepare_enable(lpi2c_imx->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "clk enable failed %d\n", ret);
-		return ret;
-	}
-
 	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;
+	}
+
 	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
 	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
 	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
 
+	pm_runtime_put(&pdev->dev);
+
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
 		goto rpm_disable;
 
-	pm_runtime_mark_last_busy(&pdev->dev);
-	pm_runtime_put_autosuspend(&pdev->dev);
-
 	dev_info(&lpi2c_imx->adapter.dev, "LPI2C adapter registered\n");
 
 	return 0;
 
 rpm_disable:
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 
@@ -636,7 +634,7 @@  static int __maybe_unused lpi2c_runtime_suspend(struct device *dev)
 	struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
 
 	clk_disable_unprepare(lpi2c_imx->clk);
-	pinctrl_pm_select_sleep_state(dev);
+	pinctrl_pm_select_idle_state(dev);
 
 	return 0;
 }
@@ -649,16 +647,34 @@  static int __maybe_unused lpi2c_runtime_resume(struct device *dev)
 	pinctrl_pm_select_default_state(dev);
 	ret = clk_prepare_enable(lpi2c_imx->clk);
 	if (ret) {
-		dev_err(dev, "failed to enable I2C clock, ret=%d\n", ret);
+		dev_err(dev, "can't enable I2C clock, ret=%d\n", ret);
 		return ret;
 	}
 
+	return ret;
+}
+
+static int lpi2c_suspend_noirq(struct device *dev)
+{
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	pinctrl_pm_select_sleep_state(dev);
+
 	return 0;
 }
 
+static int lpi2c_resume_noirq(struct device *dev)
+{
+	return pm_runtime_force_resume(dev);
+}
+
 static const struct dev_pm_ops lpi2c_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				      pm_runtime_force_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
+				     lpi2c_resume_noirq)
 	SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
 			   lpi2c_runtime_resume, NULL)
 };