Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

On Tue, Jul 18, 2023 at 01:55:05PM +0200, Paul Cercueil wrote:
> Le mardi 18 juillet 2023 à 13:41 +0200, Thierry Reding a écrit :
> > On Tue, Jul 18, 2023 at 10:42:47AM +0200, Paul Cercueil wrote:
> > > Hi Thierry,
> > > 
> > > Le mardi 18 juillet 2023 à 09:45 +0200, Thierry Reding a écrit :
> > > > On Mon, Jul 17, 2023 at 09:14:12PM +0200, Paul Cercueil wrote:
> > > > > Hi Andy,
> > > > > 
> > > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a
> > > > > écrit :
> > > > > > 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@xxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > >  drivers/pinctrl/tegra/pinctrl-tegra.c | 5 +----
> > > > > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > > b/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > > index 4547cf66d03b..734c71ef005b 100644
> > > > > > --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > > +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > > @@ -747,10 +747,7 @@ static int tegra_pinctrl_resume(struct
> > > > > > device
> > > > > > *dev)
> > > > > >         return 0;
> > > > > >  }
> > > > > >  
> > > > > > -const struct dev_pm_ops tegra_pinctrl_pm = {
> > > > > > -       .suspend_noirq = &tegra_pinctrl_suspend,
> > > > > > -       .resume_noirq = &tegra_pinctrl_resume
> > > > > > -};
> > > > > > +DEFINE_NOIRQ_DEV_PM_OPS(tegra_pinctrl_pm,
> > > > > > tegra_pinctrl_suspend,
> > > > > > tegra_pinctrl_resume);
> > > > > >  
> > > > > >  static bool tegra_pinctrl_gpio_node_has_range(struct
> > > > > > tegra_pmx
> > > > > > *pmx)
> > > > > >  {
> > > > > 
> > > > > Another driver where using EXPORT_GPL_DEV_PM_OPS() would make
> > > > > more
> > > > > sense.
> > > > 
> > > > We don't currently export these PM ops because none of the Tegra
> > > > pinctrl
> > > > drivers can be built as a module.
> > > 
> > > This doesn't change anything. You'd want to use
> > > EXPORT_GPL_DEV_PM_OPS
> > > (or better, the namespaced version) so that the PM ops can be
> > > defined
> > > in one file and referenced in another, while still having them
> > > garbage-
> > > collected when CONFIG_PM is disabled.
> > 
> > Looking at the definition of EXPORT_GPL_DEV_PM_OPS(), it will cause
> > an
> > EXPORT_SYMBOL_GPL() to be added. So there very well is a change. And
> > it's a completely bogus change because no module is ever going to use
> > that symbol. If we were to ever support building the pinctrl driver
> > as
> > a module, then this would perhaps make sense, but we don't.
> 
> In this particular case the EXPORT_SYMBOL_GPL() isn't really important,
> the rest of EXPORT_GPL_DEV_PM_OPS() is.
> 
> I don't think having a symbol exported it is a big deal, TBH, if you
> use the namespaced version. If you really don't want that, we need a
> version of EXPORT_GPL_DEV_PM_OPS() that doesn't export the symbol.

I do think it's a big deal to export a symbol if there's no reason to do
so.

And please, can we stop adding these macros for every possible scenario?
Maybe I'm just getting old, but I find it increasingly difficult to
understand what all of these are supposed to be. I get that people want
to get rid of boilerplate, but I think we need to more carefully balance
boilerplate vs. simplicity.

I'm seeing the same thing with stuff like those mass conversions to
atrocities like devm_platform_ioremap_resource() and
devm_platform_get_and_ioremap_resource(). There's so much churn involved
in getting those merged for usually saving a single line of code. And
it's not even mass conversions, but people tend to send these as one
patch per driver, which doesn't exactly help (except perhaps for patch
statistics).

Thierry

Attachment: signature.asc
Description: PGP signature