Re: [PATCH] macintosh/via-pmu: Fix build failure when CONFIG_INPUT is disabled

[Finn Thain <fthain@linux-m68k.org>]

[-- Attachment #1: Type: text/plain, Size: 4110 bytes --]

On Mon, 21 Mar 2022, Geert Uytterhoeven wrote:

> On Mon, Mar 21, 2022 at 9:29 AM Finn Thain <fthain@linux-m68k.org> wrote:
> > On Mon, 21 Mar 2022, Christophe Leroy wrote:
> > > Le 21/03/2022 à 05:30, Finn Thain a écrit :
> > > > drivers/macintosh/via-pmu-event.o: In function `via_pmu_event':
> > > > via-pmu-event.c:(.text+0x44): undefined reference to `input_event'
> > > > via-pmu-event.c:(.text+0x68): undefined reference to `input_event'
> > > > via-pmu-event.c:(.text+0x94): undefined reference to `input_event'
> > > > via-pmu-event.c:(.text+0xb8): undefined reference to `input_event'
> > > > drivers/macintosh/via-pmu-event.o: In function `via_pmu_event_init':
> > > > via-pmu-event.c:(.init.text+0x20): undefined reference to `input_allocate_device'
> > > > via-pmu-event.c:(.init.text+0xc4): undefined reference to `input_register_device'
> > > > via-pmu-event.c:(.init.text+0xd4): undefined reference to `input_free_device'
> > > > make[1]: *** [Makefile:1155: vmlinux] Error 1
> > > > make: *** [Makefile:350: __build_one_by_one] Error 2
> > > >
> > > > Don't call into the input subsystem unless CONFIG_INPUT is built-in.
> > > >
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Cc: Randy Dunlap <rdunlap@infradead.org>
> > > > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > > > ---
> > > > This is equivalent to the patch I sent a couple of days ago. This one
> > > > is slightly longer and adds a new symbol so that Kconfig logic can been
> > > > used instead of Makefile logic in case reviewers prefer that.
> > > > ---
> > > >   drivers/macintosh/Kconfig   | 5 +++++
> > > >   drivers/macintosh/Makefile  | 3 ++-
> > > >   drivers/macintosh/via-pmu.c | 2 ++
> > > >   3 files changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
> > > > index 5cdc361da37c..b9102f051bbb 100644
> > > > --- a/drivers/macintosh/Kconfig
> > > > +++ b/drivers/macintosh/Kconfig
> > > > @@ -67,6 +67,11 @@ config ADB_PMU
> > > >       this device; you should do so if your machine is one of those
> > > >       mentioned above.
> > > >
> > > > +config ADB_PMU_EVENT
> > > > +   bool
> > > > +   depends on ADB_PMU && INPUT=y
> > > > +   default y
> > >
> > > Could be reduced to
> > >
> > > config ADB_PMU_EVENT
> > >       def_bool y if ADB_PMU && INPUT=y
> >
> > That's great but my question remains unanswered: why the aversion to
> > conditionals in Makefiles, when that would be simpler (no new symbol)?
> 
> While conditionals in Makefiles do exist, they are far less common, 

Perhaps the Kconfig solution is more common. I did look for it but didn't 
find it in recent commits. Maybe I didn't look hard enough. Mostly I see 
invisible Kconfig getting used for things that are hard to do in any 
simpler way.

> and can be confusing.

Kconfig can be confusing too. I don't think your approach reduces 
complexity, I think it increases it.

> They're also harder to grep for.
> E.g. "git grep via-pmu-event.o" after your alternative patch would
> give:
> 
>     obj-$(CONFIG_ADB_PMU)          += via-pmu-event.o
> 
> but would miss the important surrounding part:
> 
>     ifeq ($(CONFIG_INPUT), y)
>     obj-$(CONFIG_ADB_PMU)          += via-pmu-event.o
>     endif
> 
> Keeping configuration logic in a single place (the Kconfig file)
> avoids that.  The extra symbol is invisible, so it doesn't hurt much.
> 

The same argument applies to Kconfig. "git grep CONFIG_ADB_PMU_EVENT" 
doesn't even mention drivers/macintosh/Kconfig.

If you do "git grep ADB_PMU_EVENT" you get:
	drivers/macintosh/Kconfig:config ADB_PMU_EVENT
with no mention of the "important surrounding part" that you were 
concerned about:
	config ADB_PMU_EVENT
        	def_bool y if ADB_PMU && INPUT=y

Anyway, we don't have to debate this, as drivers/macintosh already has a 
maintainer who can decide the question. Both patches are available.