Message-ID: <20141023081034.GA26941@phenom.ffwll.local>
Date:	Thu, 23 Oct 2014 10:10:34 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Tomi Valkeinen <tomi.valkeinen@...com>
Cc:	Jani Nikula <jani.nikula@...el.com>, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, linuxppc-dev@...ts.ozlabs.org,
	platform-driver-x86@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-fbdev@...r.kernel.org, Jingoo Han <jg1.han@...sung.com>,
	Daniel Drake <dsd@...top.org>,
	Jens Frederich <jfrederich@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jon Nettleton <jon.nettleton@...il.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Darren Hart <dvhart@...radead.org>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Bryan Wu <cooloney@...il.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH] drivers: depend on instead of select
 BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

On Wed, Oct 22, 2014 at 11:02:26AM +0300, Tomi Valkeinen wrote:
> On 18/10/14 00:13, Jani Nikula wrote:
> > Documentation/kbuild/kconfig-language.txt warns to use select with care,
> > and in general use select only for non-visible symbols and for symbols
> > with no dependencies, because select will force a symbol to a value
> > without visiting the dependencies.
> > 
> > Select has become particularly problematic, interdependently, with the
> > BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO configs. For example:
> > 
> > scripts/kconfig/conf --randconfig Kconfig
> > KCONFIG_SEED=0x48312B00
> > warning: (DRM_RADEON && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 &&
> > DRM_SHMOBILE && DRM_TILCDC && FB_BACKLIGHT && FB_MX3 && USB_APPLEDISPLAY
> > && FB_OLPC_DCON && ASUS_LAPTOP && SONY_LAPTOP && THINKPAD_ACPI &&
> > EEEPC_LAPTOP && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE
> > which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT)
> > warning: (DRM_RADEON && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 &&
> > DRM_SHMOBILE && DRM_TILCDC && FB_BACKLIGHT && FB_MX3 && USB_APPLEDISPLAY
> > && FB_OLPC_DCON && ASUS_LAPTOP && SONY_LAPTOP && THINKPAD_ACPI &&
> > EEEPC_LAPTOP && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE
> > which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT)
> > 
> > With tristates it's possible to end up selecting FOO=y depending on
> > BAR=m in the config, which gets discovered at build time, not config
> > time, like reported in the thread referenced below.
> > 
> > Do the following to fix the dependencies:
> > 
> > * Depend on instead of select BACKLIGHT_CLASS_DEVICE everywhere. Drop
> >   select BACKLIGHT_LCD_SUPPORT in such cases, as it's a dependency of
> >   BACKLIGHT_CLASS_DEVICE.
> > 
> > * Remove config FB_BACKLIGHT altogether, and replace it with a
> >   dependency on BACKLIGHT_CLASS_DEVICE. All configs selecting
> >   FB_BACKLIGHT select or depend on FB anyway, so we can simplify.
> > 
> > * Depend on (ACPI && ACPI_VIDEO) || ACPI=n in several places instead of
> >   selecting ACPI_VIDEO and a number of its dependencies if ACPI is
> >   enabled. This is tied to backlight, as ACPI_VIDEO depends on
> >   BACKLIGHT_CLASS_DEVICE.
> > 
> > * Replace a couple of select INPUT/VT with depends as it seemed to be
> >   necessary.
> 
> While doing 'depends on' instead of 'select' is an "easy" fix for this,
> I do dislike it quite a bit. It's a major pain to go around the kernel
> config, trying to find all the dependencies that a particular driver
> wants. If I need fb-foobar, I should just be able to enable it, instead
> of first searching and selecting its minor dependencies individually.
> 
> So, not a NACK, but a "isn't there an another way to fix this?".
> 
> Looking at backlight... BACKLIGHT_LCD_SUPPORT seems to be a "meta"
> option, it only enables a Kconfig submenu.
> 
> So I think we could just remove the whole BACKLIGHT_LCD_SUPPORT option.
> But if we do that, all the items in drivers/video/backlight/Kconfig with
> default 'y' or 'm' would get enabled by default, so I think we should
> remove the 'default's from that file. That makes sense in any case, as I
> don't see why "HP Jornada 700 series LCD Driver" should be "default y".
> 
> BACKLIGHT_CLASS_DEVICE doesn't depend on anything except
> BACKLIGHT_LCD_SUPPORT, so after removing BACKLIGHT_LCD_SUPPORT it should
> be safe to 'select' BACKLIGHT_CLASS_DEVICE.
> 
> BACKLIGHT_CLASS_DEVICE could be made a hidden option, and the drivers in
> drivers/video/backlight/Kconfig which are under BACKLIGHT_CLASS_DEVICE
> could be made to select BACKLIGHT_CLASS_DEVICE instead.
> 
> That doesn't exactly fix anything, but I think it makes sense as
> BACKLIGHT_CLASS_DEVICE is something that's selected from all around the
> kernel, so it should be a selectable "library" instead of a Kconfig menu
> option.
> 
> I didn't look at the ACPI_VIDEO side, so no idea how messy that is.

If we want to make BACKLIGHT_CLASS_DEVICE into a library thing then I
guess we could do that, but we must then also drag it out of all the other
meta options to make sure it's always available. No need I think to ditch
the entire BACKLIGHT_LCD_SUPPORT meta option. And then everyone could
select it.

The problem is if you mix depends and select Kconfig falls over and starts
to see loops where there are none. So you really can't ever mix them for
the same symbol.

I think if we get the BACKLIGHT_CLASS_DEVICE out of the picture with that
ACPI_VIDEO should be fixable with some

	select ACPI_VIDEO if ACPI

or so. Currently that doesn't work because of the backlight class knob and
select being broken.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists