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

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

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