Re: [PATCH 1/2] dell-wmi, dell-laptop: hide dell-smbios

["Pali Rohár" <pali.rohar@gmail.com>]

On Thursday 02 June 2016 13:03:33 Michał Kępień wrote:
> > Dell-smbios is a helper module, it serves no purpose on its own, so
> > do not present it as an option to the user. Instead, select it
> > automatically whenever a driver which needs it is selected.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Michał Kępień <kernel@kempniu.pl>
> > Cc: Pali Rohár <pali.rohar@gmail.com>
> > Cc: Darren Hart <dvhart@infradead.org>
> > ---
> >  drivers/platform/x86/Kconfig |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > --- linux-4.6.orig/drivers/platform/x86/Kconfig	2016-05-16 00:43:13.000000000 +0200
> > +++ linux-4.6/drivers/platform/x86/Kconfig	2016-05-26 11:18:50.029103790 +0200
> > @@ -92,7 +92,7 @@ config ASUS_LAPTOP
> >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> >  
> >  config DELL_SMBIOS
> > -	tristate "Dell SMBIOS Support"
> > +	tristate
> >  	depends on DCDBAS
> >  	default n
> >  	---help---
> > @@ -104,12 +104,13 @@ config DELL_SMBIOS
> >  config DELL_LAPTOP
> >  	tristate "Dell Laptop Extras"
> >  	depends on X86
> > -	depends on DELL_SMBIOS
> >  	depends on DMI
> >  	depends on BACKLIGHT_CLASS_DEVICE
> >  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> >  	depends on RFKILL || RFKILL = n
> >  	depends on SERIO_I8042
> > +	depends on DCDBAS
> > +	select DELL_SMBIOS
> >  	select POWER_SUPPLY
> >  	select LEDS_CLASS
> >  	select NEW_LEDS
> > @@ -124,7 +125,8 @@ config DELL_WMI
> >  	depends on DMI
> >  	depends on INPUT
> >  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> > -	depends on DELL_SMBIOS
> > +	depends on DCDBAS
> > +	select DELL_SMBIOS
> >  	select INPUT_SPARSEKMAP
> >  	---help---
> >  	  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
> 
> While I'm not a maintainer, I feel obliged to respond as I introduced
> the changes which this patch applies to.

Well, I'm on maintainer list of dell modules, but I do care about kbuild
configuration if it is working. So I let review of this change to other
people who understand kbuild better.

What I see in this change is adding "duplicate" or "redundant"
transitive dependency from DELL_WMI to DCDBAS. But DELL_WMI does not use
or need DCDBAS. It just needs DELL_SMIBIOS and basically DELL_WMI does
not care what DELL_SMBIOS is using (if DCDBAS, ACPI or any other thing).
So from my graph dependency point of view it is not correct, but I do
not know how kbuild is working...

So I think kbuild developers should review this change.

> I like this approach and all the way I've been thinking whether it's
> reasonable to hide configuration options that are pretty
> self-explanatory ("Dell Laptop Extras", "Dell WMI extras") behind a
> rather cryptic one ("Dell SMBIOS Support").  The only reason I didn't go
> the way you're suggesting in this patch is my lack of experience with
> kbuild and the warning in Documentation/kbuild/kconfig-language.txt that
> "select should be used with care".

-- 
Pali Rohár
pali.rohar@gmail.com