On Tue, Oct 03, 2017 at 07:38:29PM +0200, Noralf Trønnes wrote:
Den 03.10.2017 18.02, skrev Noralf Trønnes:
Den 03.10.2017 16.58, skrev Noralf Trønnes:
Den 02.10.2017 10.13, skrev Jani Nikula:
On Mon, 02 Oct 2017, Daniel Vetter daniel@ffwll.ch wrote:
Also adding Jani, who looked at the backlight Kconfig mess in the past.
I've even sent patches to fix some of the dependency mess, but the problem is social not technical. The problem is that people think "select" is more convenient than "depends" because they can just enable a config that way, while "depends" would require finding and enabling all the dependencies before the menu option even shows up.
I don't deny, that's annoying. But it's also abuse of select, see Documentation/kbuild/kconfig-language.txt:
Note: select should be used with care. select will force a symbol to a value without visiting the dependencies. By abusing select you are able to select a symbol FOO even if FOO depends on BAR that is not set. In general use select only for non-visible symbols (no prompts anywhere) and for symbols with no dependencies. That will limit the usefulness but on the other hand avoid the illegal configurations all over.
The real fix would be making finding and enabling dependencies recursively more convenient, but I don't see that happening anytime soon.
If we don't select BACKLIGHT in drivers, we can end up with CONFIG_DRM=y and CONFIG_BACKLIGHT_CLASS_DEVICE=m.
So we can either do:
menuconfig DRM depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
No, this was the thing that didn't work:
drivers/gpu/drm/Kconfig:7: symbol DRM depends on BACKLIGHT_CLASS_DEVICE drivers/video/backlight/Kconfig:158: symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_RADEON drivers/gpu/drm/Kconfig:164: symbol DRM_RADEON depends on DRM
How about putting the dependency in the driver combined with IS_REACHABLE, but not in backlight.h as I first proposed, that won't work:
menuconfig DRM_TINYDRM - select BACKLIGHT_LCD_SUPPORT - select BACKLIGHT_CLASS_DEVICE + depends on (BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n)
struct backlight_device *drm_of_find_backlight(struct device *dev) { struct backlight_device *backlight; struct device_node *np;
if (!IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)) return NULL; [...] }
But the IS_REACHABLE doesn't feel right.
Maybe the problem is putting a function in DRM that really doesn't belong there. How about calling it of_find_backlight() instead and put it in backlight.[hc]?
Wouldn't it make more sense to put the function in DRM ? Because all of the callers are located in DRM and so I'd think it makes more sense to put it in DRM along with the other _of functions.
Noralf.
Or we can:
-#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device *
I have one question. Why isn't your previous solution a good solution? i.e IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) which was part of v7
Using the second one it won't be obvious to users why backlight doesn't work, they have after all enabled backlight (but as module and DRM builtin).
So I suppose the first one is preferred. What effect will such a change have on an existing configuration where DRM=y and BACKLIGHT_CLASS_DEVICE=m? Will DRM become a module or will it be disabled?
Why would it be disabled in this case ?
Thanks and regards, Meghana
Noralf.
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel