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

[Noralf Trønnes]

Den 05.10.2017 05.53, skrev Meghana Madhyastha:
> 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 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]?
> 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.

I don't think it matters where the callers are, but the fact that the 
functions
are all about backlight and nothing about DRM, except the error messages.

I've made a suggestion in the last version where we have backlight 
maintainer attention.

Noralf.

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