| Message ID | 20230717172821.62827-8-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: Provide NOIRQ PM helper and use it | expand |
Le lundi 17 juillet 2023 à 22:36 +0300, Andy Shevchenko a écrit : > On Mon, Jul 17, 2023 at 10:07 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > > ... > > > > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, > > > mtk_paris_suspend, > > > mtk_paris_resume); > > > > It's a bit more work, but I think you should use > > EXPORT_GPL_DEV_PM_OPS > > (or even better, EXPORT_NS_GPL_DEV_PM_OPS) so that the dev_pm_ops > > is > > conditionally exported. All callers would have to be updated to use > > pm_ptr(). > > Why pm_ptr()? What did I miss? > The rest is OK. > Or pm_sleep_ptr(). As I said in my answer to the other patch, EXPORT_*_DEV_PM_OPS() currently only exports on CONFIG_PM, so it doesn't really matter which one you use. Cheers, -Paul
Il 17/07/23 19:28, Andy Shevchenko ha scritto: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 5 +---- > drivers/pinctrl/mediatek/pinctrl-paris.c | 9 +++------ > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > index 665dec419e7c..2bf5082d3aa9 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > @@ -922,10 +922,7 @@ static int mtk_eint_resume(struct device *device) > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_eint_pm_ops = { > - .suspend_noirq = mtk_eint_suspend, > - .resume_noirq = mtk_eint_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_eint_pm_ops, mtk_eint_suspend, mtk_eint_resume); > > static int mtk_pctrl_build_state(struct platform_device *pdev) > { > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index 33d6c3fb7908..b1cbd5bafa2e 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -1119,24 +1119,21 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev) > } > EXPORT_SYMBOL_GPL(mtk_paris_pinctrl_probe); > > -static int mtk_paris_pinctrl_suspend(struct device *device) > +static int mtk_paris_suspend(struct device *device) > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_suspend(pctl->eint); > } > > -static int mtk_paris_pinctrl_resume(struct device *device) > +static int mtk_paris_resume(struct device *device) What's the reason why you changed the suspend/resume function names? I don't really mind, but please at least mention that in the commit description. Thanks, Angelo > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_paris_pinctrl_pm_ops = { > - .suspend_noirq = mtk_paris_pinctrl_suspend, > - .resume_noirq = mtk_paris_pinctrl_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); > > MODULE_LICENSE("GPL v2"); > MODULE_DESCRIPTION("MediaTek Pinctrl Common Driver V2 Paris");
On Mon, 17 Jul 2023 20:28:18 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. Good to mention the renames as well. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 5 +---- > drivers/pinctrl/mediatek/pinctrl-paris.c | 9 +++------ > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > index 665dec419e7c..2bf5082d3aa9 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > @@ -922,10 +922,7 @@ static int mtk_eint_resume(struct device *device) > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_eint_pm_ops = { > - .suspend_noirq = mtk_eint_suspend, > - .resume_noirq = mtk_eint_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_eint_pm_ops, mtk_eint_suspend, mtk_eint_resume); > > static int mtk_pctrl_build_state(struct platform_device *pdev) > { > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index 33d6c3fb7908..b1cbd5bafa2e 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -1119,24 +1119,21 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev) > } > EXPORT_SYMBOL_GPL(mtk_paris_pinctrl_probe); > > -static int mtk_paris_pinctrl_suspend(struct device *device) > +static int mtk_paris_suspend(struct device *device) > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_suspend(pctl->eint); > } > > -static int mtk_paris_pinctrl_resume(struct device *device) > +static int mtk_paris_resume(struct device *device) > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_paris_pinctrl_pm_ops = { > - .suspend_noirq = mtk_paris_pinctrl_suspend, > - .resume_noirq = mtk_paris_pinctrl_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); > > MODULE_LICENSE("GPL v2"); > MODULE_DESCRIPTION("MediaTek Pinctrl Common Driver V2 Paris");
On Tue, Jul 18, 2023 at 12:47 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 17/07/23 19:28, Andy Shevchenko ha scritto: ... > > -static int mtk_paris_pinctrl_suspend(struct device *device) > > +static int mtk_paris_suspend(struct device *device) > > -static int mtk_paris_pinctrl_resume(struct device *device) > > +static int mtk_paris_resume(struct device *device) > > What's the reason why you changed the suspend/resume function names? > I don't really mind, but please at least mention that in the commit description. To put it on a single line. I will amend the commit message, thank you for review! ... > > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); ...here ^^^
On Mon, Jul 17, 2023 at 10:57 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 22:36 +0300, Andy Shevchenko a écrit : > > On Mon, Jul 17, 2023 at 10:07 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > > > > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, > > > > mtk_paris_suspend, > > > > mtk_paris_resume); > > > > > > It's a bit more work, but I think you should use > > > EXPORT_GPL_DEV_PM_OPS > > > (or even better, EXPORT_NS_GPL_DEV_PM_OPS) so that the dev_pm_ops > > > is > > > conditionally exported. All callers would have to be updated to use > > > pm_ptr(). > > > > Why pm_ptr()? What did I miss? > > The rest is OK. > > Or pm_sleep_ptr(). As I said in my answer to the other patch, > EXPORT_*_DEV_PM_OPS() currently only exports on CONFIG_PM, so it > doesn't really matter which one you use. Yes, I need to think about it. I don't like the inconsistency the EXPORT*PM_OPS() brings in this case.
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c index 665dec419e7c..2bf5082d3aa9 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c @@ -922,10 +922,7 @@ static int mtk_eint_resume(struct device *device) return mtk_eint_do_resume(pctl->eint); } -const struct dev_pm_ops mtk_eint_pm_ops = { - .suspend_noirq = mtk_eint_suspend, - .resume_noirq = mtk_eint_resume, -}; +DEFINE_NOIRQ_DEV_PM_OPS(mtk_eint_pm_ops, mtk_eint_suspend, mtk_eint_resume); static int mtk_pctrl_build_state(struct platform_device *pdev) { diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c index 33d6c3fb7908..b1cbd5bafa2e 100644 --- a/drivers/pinctrl/mediatek/pinctrl-paris.c +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c @@ -1119,24 +1119,21 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev) } EXPORT_SYMBOL_GPL(mtk_paris_pinctrl_probe); -static int mtk_paris_pinctrl_suspend(struct device *device) +static int mtk_paris_suspend(struct device *device) { struct mtk_pinctrl *pctl = dev_get_drvdata(device); return mtk_eint_do_suspend(pctl->eint); } -static int mtk_paris_pinctrl_resume(struct device *device) +static int mtk_paris_resume(struct device *device) { struct mtk_pinctrl *pctl = dev_get_drvdata(device); return mtk_eint_do_resume(pctl->eint); } -const struct dev_pm_ops mtk_paris_pinctrl_pm_ops = { - .suspend_noirq = mtk_paris_pinctrl_suspend, - .resume_noirq = mtk_paris_pinctrl_resume, -}; +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("MediaTek Pinctrl Common Driver V2 Paris");
Since pm.h provides a helper for system no-IRQ PM callbacks, switch the driver to use it instead of open coded variant. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 5 +---- drivers/pinctrl/mediatek/pinctrl-paris.c | 9 +++------ 2 files changed, 4 insertions(+), 10 deletions(-)