[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Michael Ellerman]

On Wed, 2014-10-29 at 09:54 +0200, Jani Nikula wrote:
> On Wed, 29 Oct 2014, Michael Ellerman  wrote:
> > On Tue, 2014-10-28 at 13:29 -0700, Randy Dunlap wrote:
> >> On 10/27/14 06:13, Tomi Valkeinen wrote:
> >> > 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 =).
> >> 
> >> It does exactly what is needed and it is used in many places in kernel
> >> Kconfig files.
> >
> > Is there any reason you can't do:
> >
> >   depends on BACKLIGHT_CLASS_DEVICE != m
> 
> That's not the same thing. The FOO || FOO=n allows for all options, but
> forbids it being a module when the option depending on it is
> built-in.

OK right. Because "BAR depends on FOO" is short for "depends on FOO=y || FOO=m",
but also adds the implicit condition that if FOO=m then BAR must also be m.

Thanks for clueing me in.

cheers

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Michael Ellerman]

On Tue, 2014-10-28 at 13:29 -0700, Randy Dunlap wrote:
> On 10/27/14 06:13, Tomi Valkeinen wrote:
> > 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 =).
> 
> It does exactly what is needed and it is used in many places in kernel
> Kconfig files.

Is there any reason you can't do:

  depends on BACKLIGHT_CLASS_DEVICE != m

cheers

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Jani Nikula]

On Wed, 29 Oct 2014, Michael Ellerman  wrote:
> On Tue, 2014-10-28 at 13:29 -0700, Randy Dunlap wrote:
>> On 10/27/14 06:13, Tomi Valkeinen wrote:
>> > 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 =).
>> 
>> It does exactly what is needed and it is used in many places in kernel
>> Kconfig files.
>
> Is there any reason you can't do:
>
>   depends on BACKLIGHT_CLASS_DEVICE != m

That's not the same thing. The FOO || FOO=n allows for all options, but
forbids it being a module when the option depending on it is
built-in. Obviously something that's built-in can't depend on something
built as a module.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Randy Dunlap]

On 10/27/14 06:13, Tomi Valkeinen wrote:
> 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 ||
> 

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Tomi Valkeinen]

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


-- next part --
A non-text attachment was scrubbed...
Name: 

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Jani Nikula]

On Wed, 22 Oct 2014, 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.

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

> 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.

> 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 

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Tomi Valkeinen]

On 23/10/14 11:10, Daniel Vetter wrote:

> 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

BACKLIGHT_CLASS_DEVICE only depends on HAS_IOMEM and
BACKLIGHT_LCD_SUPPORT so there are no other meta options to avoid.
HAS_IOMEM comes from drivers/video/Kconfig's "Graphics support", and I
guess we can ignore it.

> the entire BACKLIGHT_LCD_SUPPORT meta option. And then everyone could
> select it.

I don't quite understand what purpose does BACKLIGHT_LCD_SUPPORT serve.
It doesn't enable any code, it just opens up new Kconfig options. Why
can't the Kconfig options be always available? It's just another option
to 'select', without any reason I can see.

 Tomi


-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Daniel Vetter]

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

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Tomi Valkeinen]

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.

 Tomi


-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Darren Hart]

On Sat, Oct 18, 2014 at 12:13:23AM +0300, 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.
> 
> Reference: 
> http://lkml.kernel.org/r/CA+r1ZhhmT4DrWtf6MbRQo5EqXwx+LxCqh15Vsu_d9WpftLhnxw 
> at mail.gmail.com
> Reported-by: Jim Davis 
> Cc: Randy Dunlap 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Greg Kroah-Hartman 
> Cc: Darren Hart 
> Cc: Laurent Pinchart 
> Cc: Benjamin Herrenschmidt 
> Cc: Jens Frederich 
> Cc: Daniel Drake 
> Cc: Jon Nettleton 
> Cc: Jean-Christophe Plagniol-Villard 
> Cc: Tomi Valkeinen 
> Signed-off-by: Jani Nikula 

As for the drivers/platform/x86/Kconfig:

Acked-by: Darren Hart 

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

[Jani Nikula]

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.

Reference: 
http://lkml.kernel.org/r/CA+r1ZhhmT4DrWtf6MbRQo5EqXwx+LxCqh15Vsu_d9WpftLhnxw at 
mail.gmail.com
Reported-by: Jim Davis 
Cc: Randy Dunlap 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Greg Kroah-Hartman 
Cc: Darren Hart 
Cc: Laurent Pinchart 
Cc: Benjamin Herrenschmidt 
Cc: Jens Frederich 
Cc: Daniel Drake 
Cc: Jon Nettleton 
Cc: Jean-Christophe Plagniol-Villard 
Cc: Tomi Valkeinen 
Signed-off-by: Jani Nikula 

---

Sorry for the huge distribution; this is really quite hard to split up
sensibly without breaking the build!
---
 drivers/gpu/drm/Kconfig|  2 +-
 drivers/gpu/drm/gma500/Kconfig |  4 +---
 drivers/gpu/drm/i915/Kconfig   |  9 ++---
 drivers/gpu/drm/nouveau/Kconfig| 10 ++
 drivers/gpu/drm/shmobile/Kconfig   |  2 +-
 drivers/gpu/drm/tilcdc/Kconfig |  3 +--
 drivers/macintosh/Kconfig  |  2 +-
 drivers/platform/x86/Kconfig   | 19 ---
 drivers/staging/olpc_dcon/Kconfig  |  2 +-
 drivers/usb/misc/Kconfig   |  3 +--
 drivers/video/fbdev/Kconfig| 29 +++--
 drivers/video/fbdev/core/fbsysfs.c |  8 
 include/linux/fb.h |  2 +-
 include/uapi/linux/fb.h|  2 +-
 14 files changed, 36 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e3b4b0f02b3d..dc789d0e293c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -99,6 +99,7 @@ config DRM_R128
 config DRM_RADEON
tristate "ATI Radeon"
depends on DRM && PCI
+   depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
@@ -108,7 +109,6 @@ config DRM_RADEON
 select DRM_TTM
select POWER_SUPPLY
select HWMON
-   select BACKLIGHT_CLASS_DEVICE
select INTERVAL_TREE
select MMU_NOTIFIER
help
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index 17f928ec84ea..a84d0a4fcc58 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -8,9 +8,7 @@ config DRM_GMA500
select DRM_KMS_FB_HELPER
select DRM_TTM
# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
-   select ACPI_VIDEO if ACPI
-   select BACKLIGHT_CLASS_DEVICE if ACPI
-   select INPUT if ACPI
+   depends on (ACPI && ACPI_VIDEO) || ACPI=n
help
  Say yes for an experimental 2D KMS framebuffer driver for the
  Intel GMA500 ('Poulsbo') and other Intel IMG based graphics
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 4e39ab34eb1c..75d4c52c0971 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -3,6 +3,8 @@ config DRM_I915
depends on DRM
depends on X86 && PCI
depends on (AGP || AGP=n)
+   depends on (ACPI &&