Message-ID: <544E44E9.4040208@ti.com>
Date:	Mon, 27 Oct 2014 15:13:13 +0200
From:	Tomi Valkeinen <tomi.valkeinen@...com>
To:	Jani Nikula <jani.nikula@...el.com>
CC:	<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>,
	Randy Dunlap <rdunlap@...radead.org>,
	David Airlie <airlied@...ux.ie>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Darren Hart <dvhart@...radead.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Jens Frederich <jfrederich@...il.com>,
	Daniel Drake <dsd@...top.org>,
	Jon Nettleton <jon.nettleton@...il.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Jingoo Han <jg1.han@...sung.com>,
	Bryan Wu <cooloney@...il.com>, Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE
 and ACPI_VIDEO

On 27/10/14 13:59, Jani Nikula wrote:

>> 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.
> 
> Agreed, but I don't think that's specific to this patch.

Well, no, the generic problem is not specific to this patch, but we can
avoid the issue with proper use of 'select' (at least in some cases),
which is specific to this patch.

>> So, not a NACK, but a "isn't there an another way to fix this?".
> 
> I think the real answer would be to fix kconfig to also show menu items
> whose dependencies are not met, and then recursively enabling the
> dependencies when the item is enabled. Beyond my scope.
> 
>> 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.
> 
> I think it should be possible to choose between y and m when it's

If I'm not mistaken, if CONFIG_FOO is 'm', and it 'select's CONFIG_BAR,
and CONFIG_BAR is tristate, then CONFIG_BAR will be set to 'm'.

> selected, and it should be possible to enable it when it's not selected
> by any drivers. I'm not sure a hidden option is good for that.

Why would you want to enable it if no one uses it? Does
BACKLIGHT_CLASS_DEVICE enable something even if no driver uses it?

>> 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.
> 
> At least for drm/i915 BACKLIGHT_CLASS_DEVICE is "an option". We use it
> if it's enabled, but we are just fine if it's not. I've learned the way
> to express that is
> 
> 	depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n
> 
> but I don't think there's a way to express that in terms of select, is
> there? The dependency above guarantees there's no DRM_I915=y and
> BACKLIGHT_CLASS_DEVICE=m combo which would fail. And this, btw, is where
> this whole patch got started, as select didn't handle that properly.

If backlight support is considered an option for drm/i915, then I think
there should be a Kconfig option for i915 to enable backlight support,
which in turn selects BACKLIGHT_CLASS_DEVICE. And that select will force
BACKLIGHT_CLASS_DEVICE to be built-in if drm/i915 is built-in.

Oh, but it doesn't work optimally with modules. The new option needed
for that would be boolean, so BACKLIGHT_CLASS_DEVICE would always be
either y or n. Sigh...

>> I didn't look at the ACPI_VIDEO side, so no idea how messy that is.
> 
> Basically it's another dependency on BACKLIGHT_CLASS_DEVICE. I can only
> imagine trying to solve this problem with select is going to end up in
> recursive dependencies that spread out and need changing about as wide
> as this patch.

If ACPI_VIDEO uses select to enable BACKLIGHT_CLASS_DEVICE, then, I
think, selecting ACPI_VIDEO will also select BACKLIGHT_CLASS_DEVICE. So
I don't right away see any recursive dependencies. Or what did you have
in mind?

> In the end, I agree with the problem you have with this patch, but yet I
> think it's the right thing to do in terms of expressing the
> dependencies.

Well, dri/i915 doesn't exactly depend on backlight, if I understood you
correctly. Instead, backlight is an option for dri/i915, and you kind of
hack it to be implemented with that 'depends on BACKLIGHT_CLASS_DEVICE
|| BACKLIGHT_CLASS_DEVICE=n'.

I guess it's debatable whether drivers should automatically use features
in the kernel if they happen to be enabled in the Kconfig, or should
they be individually enabled for that driver. I personally like the
latter option, as it allows more precise control, but it probably also
depends on the feature in question.

I also think the 'depends on BACKLIGHT_CLASS_DEVICE ||
BACKLIGHT_CLASS_DEVICE=n' pattern is quite... interesting (i.e. sounds
like a hack to me =).

 Tomi



Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists