diff mbox series

[2/3] x86: apuv2: fix input dependencies

Message ID 20190304201930.1622839-2-arnd@arndb.de (mailing list archive)
State Deferred, archived
Delegated to: Andy Shevchenko
Headers show
Series [1/3] x86: apuv2: remove unused variable | expand

Commit Message

Arnd Bergmann March 4, 2019, 8:19 p.m. UTC
We cannot select KEYBOARD_GPIO_POLLED if CONFIG_INPUT or CONFIG_INPUT_KEYBOARD
are disabled:

WARNING: unmet direct dependencies detected for KEYBOARD_GPIO_POLLED
  Depends on [n]: !UML && INPUT [=y] && INPUT_KEYBOARD [=n] && GPIOLIB [=y]
  Selected by [y]:
  - PCENGINES_APU2 [=y] && X86 [=y] && X86_PLATFORM_DEVICES [=y]

This could be fixed using either a dependency or a 'select' statement.
I'm chosen 'depends on' here since it is simpler has a lower risk of
introducing circular dependencies.

Fixes: f8eb0235f659 ("x86: pcengines apuv2 gpio/leds/keys platform driver")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/platform/x86/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Comments

Enrico Weigelt, metux IT consult March 5, 2019, 12:18 a.m. UTC | #1
On 04.03.19 21:19, Arnd Bergmann wrote:

> This could be fixed using either a dependency or a 'select' statement.
> I'm chosen 'depends on' here since it is simpler has a lower risk of
> introducing circular dependencies.

I'd rather prefer using 'select'.

Otherwise the driver won't appear at all if INPUT or INPUT_KEYBOARD
aren't enabled (eg. when you start w/ minimal config - which I do
frequently), and people have a hard time actually finding/enabling it.


--mtx
Arnd Bergmann March 5, 2019, 8:23 a.m. UTC | #2
On Tue, Mar 5, 2019 at 1:18 AM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
>
> On 04.03.19 21:19, Arnd Bergmann wrote:
>
> > This could be fixed using either a dependency or a 'select' statement.
> > I'm chosen 'depends on' here since it is simpler has a lower risk of
> > introducing circular dependencies.
>
> I'd rather prefer using 'select'.
>
> Otherwise the driver won't appear at all if INPUT or INPUT_KEYBOARD
> aren't enabled (eg. when you start w/ minimal config - which I do
> frequently), and people have a hard time actually finding/enabling it.

No, that wouldn't be good here. In effect that means that with INPUT disabled,
most of the x86 platform drivers are disabled, until you enable the
PCENGINES_APU2 symbol, which then ends up showing all the other
symbols at once, and changing them to their default states.

Another problem is that you likely run into circular dependency chains
after trying that. The best practice for select vs. depends are

1. try to use 'depends on' if you can
2. use 'select' on hidden symbols, but not user visible ones
3. for each symbol, use either 'depends on'  or 'select' from all other
    drivers that need it, and do it consistently
4. never select entire subsystems, only helper code.

Those are sometimes at odds. I used 'select' for the LEDS code
because of 3. and 4, even though that goes against 2. For INPUT,
there is no real debate though.

   Arnd
Enrico Weigelt, metux IT consult March 5, 2019, 1:50 p.m. UTC | #3
On 05.03.19 09:23, Arnd Bergmann wrote:

(CC'ing kbuild maintainer + list, hoping for better ideas :)


> No, that wouldn't be good here. In effect that means that with INPUT disabled,
> most of the x86 platform drivers are disabled, until you enable the
> PCENGINES_APU2 symbol, which then ends up showing all the other
> symbols at once, and changing them to their default states.

Okay, I didn't consider the defaults.

Maybe we should talk about step by step getting away from these defaults
(perhaps move them to the defconfigs ?) on a broader scale ... but yeah,
that's far out of scope now.

So, you've conviced me.
Add me to Reviewed-By to your patches and forget about mine.

> Another problem is that you likely run into circular dependency chains
> after trying that. The best practice for select vs. depends are

hmm, if circular deps happen, wouldn't that mean we've got some deeper
problems in here ? IMHO, dependencies should always form a DAG (except
for some really rare cases).

Do you recall any actual problem w/ input vs gpio vs. drivers ?
I'd like to have a closer look at it.

> 1. try to use 'depends on' if you can

Well, this has the unpleasant side effect that we often have to enable
a lot of things, just to even see the individual driver. For people who
just wanna configure a kernel for their board, this is a bit nasty.

But, okay, I'm going to do this w/ my own tool - I've written a small
tool that allows easy kernel reconfiguration on a higher level: you
can just pick some board templates and enable high level features like
eth, gpu, etc - it automatically creates a .config for you. I'm going
announce it on lkml soon.


--mtx
Andy Shevchenko March 5, 2019, 1:56 p.m. UTC | #4
On Tue, Mar 5, 2019 at 3:50 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
> On 05.03.19 09:23, Arnd Bergmann wrote:

> > 1. try to use 'depends on' if you can
>
> Well, this has the unpleasant side effect that we often have to enable
> a lot of things, just to even see the individual driver. For people who
> just wanna configure a kernel for their board, this is a bit nasty.
>
> But, okay, I'm going to do this w/ my own tool - I've written a small
> tool that allows easy kernel reconfiguration on a higher level: you
> can just pick some board templates and enable high level features like
> eth, gpu, etc - it automatically creates a .config for you. I'm going
> announce it on lkml soon.

Darren gave a talk about merging kernel configs to get something like
you want to.
This tool is quite long already lying around. merge_config.sh in your
kernel source tree.
Arnd Bergmann March 5, 2019, 4:46 p.m. UTC | #5
On Tue, Mar 5, 2019 at 2:50 PM Enrico Weigelt, metux IT consult
<lkml@metux.net> wrote:
>
> On 05.03.19 09:23, Arnd Bergmann wrote:
>
> hmm, if circular deps happen, wouldn't that mean we've got some deeper
> problems in here ? IMHO, dependencies should always form a DAG (except
> for some really rare cases).
>
> Do you recall any actual problem w/ input vs gpio vs. drivers ?
> I'd like to have a closer look at it.

Not with gpio. Most of the circular dependencies I've seen tend to
revolve around drivers/gpu/drm, which uses more 'select' statements
than most other subsystems. Those dependencies often touch
backlight, acpi, fbdev and other things then.

       Arnd
Enrico Weigelt, metux IT consult March 7, 2019, 12:10 a.m. UTC | #6
On 05.03.19 14:56, Andy Shevchenko wrote:
> 
> Darren gave a talk about merging kernel configs to get something like
> you want to.
> This tool is quite long already lying around. merge_config.sh in your
> kernel source tree.

Yes, that's similar to how some distros (eg. yocto) do it.

But my requirements are a bit more complex:

In my final meta-config, I just wanna say:

* i have board A (possibly multiple boards)
* i need features X, Y, Z (eg. eth, display, can, ext4, acl, ...)

And that shall be all to generate a minimal config for exactly those
requirements.

Doing that by just putting config snippets together, quickly turns into
a maintenance hell. At least you'd need recursive dependencies and some
if/else logic.

That's why I've written kmct:

https://github.com/metux/kmct


--mtx
Darren Hart March 7, 2019, 7:03 a.m. UTC | #7
On Thu, Mar 07, 2019 at 01:10:13AM +0100, Enrico Weigelt, metux IT consult wrote:
> On 05.03.19 14:56, Andy Shevchenko wrote:
> > 
> > Darren gave a talk about merging kernel configs to get something like
> > you want to.
> > This tool is quite long already lying around. merge_config.sh in your
> > kernel source tree.
> 
> Yes, that's similar to how some distros (eg. yocto) do it.
> 

I wrote merge_config.sh to replace and simplify some of the yocto
tooling. With merge_config upstream, Yocto now uses it directly.

> But my requirements are a bit more complex:
> 
> In my final meta-config, I just wanna say:
> 
> * i have board A (possibly multiple boards)
> * i need features X, Y, Z (eg. eth, display, can, ext4, acl, ...)
> 
> And that shall be all to generate a minimal config for exactly those
> requirements.

That's also the goal of the Yocto configuration fragments, and is
possible with merge_config with a set of defined fragments.

> 
> Doing that by just putting config snippets together, quickly turns into
> a maintenance hell. At least you'd need recursive dependencies and some
> if/else logic.
> 
> That's why I've written kmct:
> 
> https://github.com/metux/kmct

I had a look, the README could benefit from a basic usage example.
Digging through it further, it appears that you are creating yaml files
which contain CONFIGs. The problem with this in my opinion is these are
kernel version specific, so you know have a lot of boiler plate yaml
wrapping kernel version specific CONFIG options which will slowly get
out of sync over time. This is the usual argument for keeping config
fragments together with the kernel - and why we do that in arch/*/config
for example.

Perhaps I'm missing your desired workflow though. I tend to find the
options I need and record them in a fragment, and save it off for later
to quickly be able to "make defconfig fragA.config fragB.config" etc. Is
what you're trying to do different?
diff mbox series

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 851ea921a58d..4d65d37b0c86 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1305,6 +1305,7 @@  config HUAWEI_WMI
 
 config PCENGINES_APU2
 	tristate "PC Engines APUv2/3 front button and LEDs driver"
+	depends on INPUT && INPUT_KEYBOARD
 	select GPIO_AMD_FCH
 	select KEYBOARD_GPIO_POLLED
 	select LEDS_GPIO