| Message ID | 20231030120734.2831419-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | b10a74b5c0c1497f510f91333652fbb060060b69 |
| Headers | show |
| Series | pinctrl: intel: Use NOIRQ PM helper | expand |
On Mon, 30 Oct 2023 14:07:18 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > With the help of EXPORT_NS_GPL_DEV_PM_OPS() and > NOIRQ_SYSTEM_SLEEP_PM_OPS() we may convert PM ops functions to become > static. This also takes into account the PM configuration options such > as CONFIG_PM and CONFIG_PM_SLEEP. Hence the first step is to provide > a generic PM ops structure that can be used by drivers directly. It's a good thing to do, but if rolling a v3, should mention that the export moves into the namespace. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Other than that, LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 8 +++++--- > drivers/pinctrl/intel/pinctrl-intel.h | 4 ++-- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index b19527a8728e..9fbdc7bfa65c 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -899,7 +899,7 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset, > * > * Return: a GPIO offset, or negative error code if translation can't be done. > */ > -static __maybe_unused int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) > +static int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) > { > const struct intel_community *community; > const struct intel_padgroup *padgrp; > @@ -1682,7 +1682,6 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_ > } > EXPORT_SYMBOL_NS_GPL(intel_pinctrl_get_soc_data, PINCTRL_INTEL); > > -#ifdef CONFIG_PM_SLEEP > static bool __intel_gpio_is_direct_irq(u32 value) > { > return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) && > @@ -1883,7 +1882,10 @@ int intel_pinctrl_resume_noirq(struct device *dev) > return 0; > } > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); > -#endif > + > +EXPORT_NS_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops, PINCTRL_INTEL) = { > + NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, intel_pinctrl_resume_noirq) > +}; > > MODULE_AUTHOR("Mathias Nyman <mathias.nyman@linux.intel.com>"); > MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h > index 2bb553598e8b..d3f511f97ed9 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.h > +++ b/drivers/pinctrl/intel/pinctrl-intel.h > @@ -255,10 +255,8 @@ struct intel_pinctrl { > int intel_pinctrl_probe_by_hid(struct platform_device *pdev); > int intel_pinctrl_probe_by_uid(struct platform_device *pdev); > > -#ifdef CONFIG_PM_SLEEP > int intel_pinctrl_suspend_noirq(struct device *dev); > int intel_pinctrl_resume_noirq(struct device *dev); > -#endif > > #define INTEL_PINCTRL_PM_OPS(_name) \ > const struct dev_pm_ops _name = { \ > @@ -266,6 +264,8 @@ const struct dev_pm_ops _name = { \ > intel_pinctrl_resume_noirq) \ > } > > +extern const struct dev_pm_ops intel_pinctrl_pm_ops; > + > struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin); > > int intel_get_groups_count(struct pinctrl_dev *pctldev);
On Mon, 30 Oct 2023 19:41:12 +0000 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Mon, 30 Oct 2023 14:07:18 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > With the help of EXPORT_NS_GPL_DEV_PM_OPS() and > > NOIRQ_SYSTEM_SLEEP_PM_OPS() we may convert PM ops functions to become > > static. This also takes into account the PM configuration options such > > as CONFIG_PM and CONFIG_PM_SLEEP. Hence the first step is to provide > > a generic PM ops structure that can be used by drivers directly. > > It's a good thing to do, but if rolling a v3, should mention that the export > moves into the namespace. > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Other than that, > LGTM > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Actually looking at usecase, why isn't the absence of an EXPORT in the !CONFIG_PM_SLEEP path not a problem for bisection of this series given you haven't yet protected the users? Jonathan > > --- > > drivers/pinctrl/intel/pinctrl-intel.c | 8 +++++--- > > drivers/pinctrl/intel/pinctrl-intel.h | 4 ++-- > > 2 files changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > > index b19527a8728e..9fbdc7bfa65c 100644 > > --- a/drivers/pinctrl/intel/pinctrl-intel.c > > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > > @@ -899,7 +899,7 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset, > > * > > * Return: a GPIO offset, or negative error code if translation can't be done. > > */ > > -static __maybe_unused int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) > > +static int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) > > { > > const struct intel_community *community; > > const struct intel_padgroup *padgrp; > > @@ -1682,7 +1682,6 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_ > > } > > EXPORT_SYMBOL_NS_GPL(intel_pinctrl_get_soc_data, PINCTRL_INTEL); > > > > -#ifdef CONFIG_PM_SLEEP > > static bool __intel_gpio_is_direct_irq(u32 value) > > { > > return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) && > > @@ -1883,7 +1882,10 @@ int intel_pinctrl_resume_noirq(struct device *dev) > > return 0; > > } > > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); > > -#endif > > + > > +EXPORT_NS_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops, PINCTRL_INTEL) = { > > + NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, intel_pinctrl_resume_noirq) > > +}; > > > > MODULE_AUTHOR("Mathias Nyman <mathias.nyman@linux.intel.com>"); > > MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h > > index 2bb553598e8b..d3f511f97ed9 100644 > > --- a/drivers/pinctrl/intel/pinctrl-intel.h > > +++ b/drivers/pinctrl/intel/pinctrl-intel.h > > @@ -255,10 +255,8 @@ struct intel_pinctrl { > > int intel_pinctrl_probe_by_hid(struct platform_device *pdev); > > int intel_pinctrl_probe_by_uid(struct platform_device *pdev); > > > > -#ifdef CONFIG_PM_SLEEP > > int intel_pinctrl_suspend_noirq(struct device *dev); > > int intel_pinctrl_resume_noirq(struct device *dev); > > -#endif > > > > #define INTEL_PINCTRL_PM_OPS(_name) \ > > const struct dev_pm_ops _name = { \ > > @@ -266,6 +264,8 @@ const struct dev_pm_ops _name = { \ > > intel_pinctrl_resume_noirq) \ > > } > > > > +extern const struct dev_pm_ops intel_pinctrl_pm_ops; > > + > > struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin); > > > > int intel_get_groups_count(struct pinctrl_dev *pctldev); >
On Mon, Oct 30, 2023 at 07:43:50PM +0000, Jonathan Cameron wrote: > On Mon, 30 Oct 2023 19:41:12 +0000 > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: ... > Actually looking at usecase, why isn't the absence of an EXPORT in > the !CONFIG_PM_SLEEP path not a problem for bisection of this series given > you haven't yet protected the users? I'm not sure I got the issue you are trying to point out. Between first and last patches the main driver exports two things: the PM ops functions, which are _always_ been exported and PM ops structure, which is exported only when CONFIG_PM_SLEEP=y. Every converted user has pm_sleep_ptr() guard added so it shouldn't be a problem. What exactly did I miss?
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index b19527a8728e..9fbdc7bfa65c 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -899,7 +899,7 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset, * * Return: a GPIO offset, or negative error code if translation can't be done. */ -static __maybe_unused int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) +static int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) { const struct intel_community *community; const struct intel_padgroup *padgrp; @@ -1682,7 +1682,6 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_ } EXPORT_SYMBOL_NS_GPL(intel_pinctrl_get_soc_data, PINCTRL_INTEL); -#ifdef CONFIG_PM_SLEEP static bool __intel_gpio_is_direct_irq(u32 value) { return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) && @@ -1883,7 +1882,10 @@ int intel_pinctrl_resume_noirq(struct device *dev) return 0; } EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); -#endif + +EXPORT_NS_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops, PINCTRL_INTEL) = { + NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, intel_pinctrl_resume_noirq) +}; MODULE_AUTHOR("Mathias Nyman <mathias.nyman@linux.intel.com>"); MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h index 2bb553598e8b..d3f511f97ed9 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.h +++ b/drivers/pinctrl/intel/pinctrl-intel.h @@ -255,10 +255,8 @@ struct intel_pinctrl { int intel_pinctrl_probe_by_hid(struct platform_device *pdev); int intel_pinctrl_probe_by_uid(struct platform_device *pdev); -#ifdef CONFIG_PM_SLEEP int intel_pinctrl_suspend_noirq(struct device *dev); int intel_pinctrl_resume_noirq(struct device *dev); -#endif #define INTEL_PINCTRL_PM_OPS(_name) \ const struct dev_pm_ops _name = { \ @@ -266,6 +264,8 @@ const struct dev_pm_ops _name = { \ intel_pinctrl_resume_noirq) \ } +extern const struct dev_pm_ops intel_pinctrl_pm_ops; + struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin); int intel_get_groups_count(struct pinctrl_dev *pctldev);
With the help of EXPORT_NS_GPL_DEV_PM_OPS() and NOIRQ_SYSTEM_SLEEP_PM_OPS() we may convert PM ops functions to become static. This also takes into account the PM configuration options such as CONFIG_PM and CONFIG_PM_SLEEP. Hence the first step is to provide a generic PM ops structure that can be used by drivers directly. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pinctrl/intel/pinctrl-intel.c | 8 +++++--- drivers/pinctrl/intel/pinctrl-intel.h | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-)