Re: [PATCH v1 1/3] pinctrl: intel: Replace ifdeffery by pm_sleep_ptr() macro

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

On Wed, Sep 04, 2024 at 10:48:42AM +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2024 at 10:47 AM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg
> > <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> > > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote:
> > > > Explicit ifdeffery is ugly and theoretically might be not synchronised
> > > > with the rest of functions that are assigned via pm_sleep_ptr() macro.
> > > > Replace ifdeffery by pm_sleep_ptr() macro to improve this.
> 
> ...
> 
> > > Can't we make this a stub when !PM_SLEEP?
> > >
> > > #ifdef CONFIG_PM_SLEEP
> > > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > > {
> > > ...
> > > }
> > > #else
> > > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > > {
> > >         return 0;
> > > }
> > > #endif
> >
> > There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery.
> >
> > ...
> >
> > > > -     ret = intel_pinctrl_pm_init(pctrl);
> > > > +     ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
> > >
> > > Then this still looks like a function call and not like some weird
> > > conditional.
> >
> > I understand that, but the point is to make all PM callbacks use the
> > same approach against kernel configuration. Current state of affairs
> > is simple inconsistency, but it might, however quite unlikely, lead to
> > desynchronization between two pm_sleep_ptr() and ifdeffery approaches.
> >
> > Approach that I have before this one (and I kinda agree that ternary
> > here looks a bit weird) is to typedef the function and do something
> > like
> >
> > pinctrl-intel.h:
> 
> > typedef alloc_fn;
> 
> Actually typedef is not needed as it may be embedded in the below
> inline as it's used only once.
> 
> > static inline int ctx_alloc(pctrl, alloc_fn)
> > {
> >   if (alloc_fn)
> >     return alloc_fn(pctrl);
> >
> >   return 0;
> > }
> >
> > pinctrl-intel.c:
> >
> >   ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init))
> >   if (ret)
> >     return ret;

I don't think this makes it any better :( We want the driver to be
readable for anyone, not just for you.

I prefer the stub and ifdeffery.