[PATCH v3 1/2] drm/tinydrm: Move tinydrm_of_find_backlight into drm_of.c

[Noralf Trønnes]

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 at 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]?

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 *
>>
>> 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?
>>
>> Noralf.
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>