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 <[email protected]>
---
drivers/pinctrl/intel/pinctrl-intel.c | 5 +----
drivers/pinctrl/intel/pinctrl-intel.h | 9 ++-------
2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 64c3e62b4348..40e45c4889ee 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -929,7 +929,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;
@@ -1488,7 +1488,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
if (!communities)
return -ENOMEM;
-
for (i = 0; i < pctrl->ncommunities; i++) {
struct intel_community *community = &pctrl->communities[i];
u32 *intmask, *hostown;
@@ -1712,7 +1711,6 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_
}
EXPORT_SYMBOL_GPL(intel_pinctrl_get_soc_data);
-#ifdef CONFIG_PM_SLEEP
static bool __intel_gpio_is_direct_irq(u32 value)
{
return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) &&
@@ -1913,7 +1911,6 @@ int intel_pinctrl_resume_noirq(struct device *dev)
return 0;
}
EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq);
-#endif
MODULE_AUTHOR("Mathias Nyman <[email protected]>");
MODULE_AUTHOR("Mika Westerberg <[email protected]>");
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index 1faf2ada480a..65b1699a2ed5 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -255,15 +255,10 @@ 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 = { \
- SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, \
- intel_pinctrl_resume_noirq) \
-}
+#define INTEL_PINCTRL_PM_OPS(_name) \
+ DEFINE_NOIRQ_DEV_PM_OPS(_name, intel_pinctrl_suspend_noirq, intel_pinctrl_resume_noirq)
#endif /* PINCTRL_INTEL_H */
--
2.40.0.1.gaa8946217a0b
On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <[email protected]> wrote:
> Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :
...
> Unrelated change.
OK.
...
> So the correct way to update this driver would be to have a
> conditionally-exported dev_pm_ops structure:
>
> EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = {
> NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq,
> intel_pinctrl_resume_noirq),
> };
This looks ugly. I didn't know that EXPORT*PM_OPS designed that way,
but it seems pm.h in such case needs EXPORT for NOIRQ case as well.
> Then your two callbacks can be "static" and without #ifdef guards.
>
> The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in the
> pinctrl-intel.h without any guards, as long as it is only referenced
> with the pm_ptr() macro.
I'm not sure I got this. Currently drivers do not have any guards.
Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it?
--
With Best Regards,
Andy Shevchenko
Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit :
> On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <[email protected]>
> wrote:
> > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :
>
> ...
>
> > Unrelated change.
>
> OK.
>
> ...
>
> > So the correct way to update this driver would be to have a
> > conditionally-exported dev_pm_ops structure:
> >
> > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = {
> > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq,
> > intel_pinctrl_resume_noirq),
> > };
>
> This looks ugly. I didn't know that EXPORT*PM_OPS designed that way,
> but it seems pm.h in such case needs EXPORT for NOIRQ case as well.
It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is
garbage-collected along with all its callbacks.
I know it looks ugly, but we already have 4 variants (regular,
namespace, GPL, namespace + GPL), if we start to add macros for
specific use-cases then it will become bloated really quick.
And the "bloat" I'm trying to avoid here is the extreme expansion of
the API which makes it hard for people not familiar to the code to
understand what should be used and how.
> > Then your two callbacks can be "static" and without #ifdef guards.
> >
> > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in
> > the
> > pinctrl-intel.h without any guards, as long as it is only
> > referenced
> > with the pm_ptr() macro.
>
> I'm not sure I got this. Currently drivers do not have any guards.
> Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it?
>
The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops"
conditionally depending on CONFIG_PM. We could add variants that export
it conditionally depending on CONFIG_PM_SLEEP, but we're back at the
problem of adding bloat.
You could use pm_sleep_ptr() indeed, with the existing macros, with the
drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the
dev_pm_ops + callbacks are compiled in but never referenced.
Cheers,
-Paul
On Mon, Jul 17, 2023 at 10:56 PM Paul Cercueil <[email protected]> wrote:
> Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit :
> > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <[email protected]>
> > wrote:
> > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :
...
> > > So the correct way to update this driver would be to have a
> > > conditionally-exported dev_pm_ops structure:
> > >
> > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = {
> > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq,
> > > intel_pinctrl_resume_noirq),
> > > };
> >
> > This looks ugly. I didn't know that EXPORT*PM_OPS designed that way,
> > but it seems pm.h in such case needs EXPORT for NOIRQ case as well.
>
> It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is
> garbage-collected along with all its callbacks.
>
> I know it looks ugly, but we already have 4 variants (regular,
> namespace, GPL, namespace + GPL), if we start to add macros for
> specific use-cases then it will become bloated really quick.
Maybe macros can be replaced / changed to make it scale?
> And the "bloat" I'm trying to avoid here is the extreme expansion of
> the API which makes it hard for people not familiar to the code to
> understand what should be used and how.
So far, based on the rest of the messages in the thread the
EXPORT*PM_OPS() have the following issues:
1) do not scale (for variants with different scope we need new set of macros);
2) do not cover cases with pm_sleep_ptr();
3) export symbols in case when it's not needed.
Am I right?
> > > Then your two callbacks can be "static" and without #ifdef guards.
> > >
> > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in
> > > the
> > > pinctrl-intel.h without any guards, as long as it is only
> > > referenced
> > > with the pm_ptr() macro.
> >
> > I'm not sure I got this. Currently drivers do not have any guards.
> > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it?
>
> The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops"
> conditionally depending on CONFIG_PM. We could add variants that export
> it conditionally depending on CONFIG_PM_SLEEP, but we're back at the
> problem of adding bloat.
Exactly.
> You could use pm_sleep_ptr() indeed, with the existing macros, with the
> drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the
> dev_pm_ops + callbacks are compiled in but never referenced.
And exactly.
I don't think they are ready to use (in the current form). But let's
see what we may do better here...
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Le mardi 18 juillet 2023 à 15:57 +0300, Andy Shevchenko a écrit :
> On Mon, Jul 17, 2023 at 10:56 PM Paul Cercueil <[email protected]>
> wrote:
> > Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit :
> > > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil
> > > <[email protected]>
> > > wrote:
> > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit
> > > > :
>
> ...
>
> > > > So the correct way to update this driver would be to have a
> > > > conditionally-exported dev_pm_ops structure:
> > > >
> > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = {
> > > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq,
> > > > intel_pinctrl_resume_noirq),
> > > > };
> > >
> > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that
> > > way,
> > > but it seems pm.h in such case needs EXPORT for NOIRQ case as
> > > well.
> >
> > It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is
> > garbage-collected along with all its callbacks.
> >
> > I know it looks ugly, but we already have 4 variants (regular,
> > namespace, GPL, namespace + GPL), if we start to add macros for
> > specific use-cases then it will become bloated really quick.
>
> Maybe macros can be replaced / changed to make it scale?
If you have any ideas, then yes absolutely.
>
> > And the "bloat" I'm trying to avoid here is the extreme expansion
> > of
> > the API which makes it hard for people not familiar to the code to
> > understand what should be used and how.
>
> So far, based on the rest of the messages in the thread the
> EXPORT*PM_OPS() have the following issues:
> 1) do not scale (for variants with different scope we need new set of
> macros);
> 2) do not cover cases with pm_sleep_ptr();
> 3) export symbols in case when it's not needed.
>
> Am I right?
I think that's right yes.
>
> > > > Then your two callbacks can be "static" and without #ifdef
> > > > guards.
> > > >
> > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern"
> > > > in
> > > > the
> > > > pinctrl-intel.h without any guards, as long as it is only
> > > > referenced
> > > > with the pm_ptr() macro.
> > >
> > > I'm not sure I got this. Currently drivers do not have any
> > > guards.
> > > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it?
> >
> > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops"
> > conditionally depending on CONFIG_PM. We could add variants that
> > export
> > it conditionally depending on CONFIG_PM_SLEEP, but we're back at
> > the
> > problem of adding bloat.
>
> Exactly.
>
> > You could use pm_sleep_ptr() indeed, with the existing macros, with
> > the
> > drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the
> > dev_pm_ops + callbacks are compiled in but never referenced.
>
> And exactly.
>
> I don't think they are ready to use (in the current form). But let's
> see what we may do better here...
They were OK when Jonathan and myself were updating the IIO drivers -
but now they definitely show their limitations.
Cheers,
-Paul
On Tue, Jul 18, 2023 at 11:04:51AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Jul 2023 20:28:15 +0300
> Andy Shevchenko <[email protected]> wrote:
...
> > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq);
>
> Can you check if this is successfully removed? I think it won't be.
> Not immediately obvious how to tidy that up given these are used
> in a macro called from lots of drivers.
That's what Paul noticed I think with his proposal to export only the ops
variable and make these to be static.
> Maybe just leaving the ifdef is best we can do here.
See above.
--
With Best Regards,
Andy Shevchenko