Re: [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency
On Wed, 19 Apr 2017, Arnd Bergmann wrote: > On Wed, Apr 19, 2017 at 10:21 PM, Laurent Pinchart > wrote: >>> >>> This adds a dependency like we have for the other panel drivers. >> >> I believe the dependency should be made optional. DPI panels that don't need >> backlight control should be supported by a kernel that has backlight support >> compiled out. > > That would be nice in principle, but I fear this would cause additional > problems. > >> --- a/include/linux/backlight.h >> +++ b/include/linux/backlight.h >> @@ -162,7 +162,7 @@ struct generic_bl_info { >> void (*kick_battery)(void); >> }; >> >> -#ifdef CONFIG_OF >> +#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) >> struct backlight_device *of_find_backlight_by_node(struct device_node >> *node); >> #else >> static inline struct backlight_device * >> >> >> We might need to create stubs for backlight_force_update() and >> backlight_device_set_brightness() too. >> > > With BACKLIGHT_CLASS_DEVICE=m, you still get a link error when the user is > in a built-in driver. Using 'depends on' usually solves this (except for > drivers > that cannot be modules). > > There are three possible workarounds for this that I can think of: > > - Use 'depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n' > in each driver that implements optional backlight support. We do > this elsewhere, but > it's confusing and easy to get wrong. FWIW I think this is the fix, and not a workaround. "depends on FOO || FOO=n" is an expression used throughout the kernel, and it accurately describes the dependency here. Of course, all drivers implementing this must still wrap backlight class usage around IS_ENABLED(). BR, Jani. > > - use IS_REACHABLE() instead of IS_ENABLED() when testing for > backlight support. This will always result in a kernel that builds cleanly, > but can be surprising for users when backlight support is a module that > gets loaded at boot, but it is still not used. > > - Make BACKLIGHT_CLASS_DEVICE a 'bool' symbol instead, and force the > core API code to always be built-in or completely disabled. This makes > it really easy to use, at the expense of a larger kernel image for those > that > currently use a loadable module. > >Arnd > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency
On Wed, Apr 19, 2017 at 10:21 PM, Laurent Pinchart wrote: >> >> This adds a dependency like we have for the other panel drivers. > > I believe the dependency should be made optional. DPI panels that don't need > backlight control should be supported by a kernel that has backlight support > compiled out. That would be nice in principle, but I fear this would cause additional problems. > --- a/include/linux/backlight.h > +++ b/include/linux/backlight.h > @@ -162,7 +162,7 @@ struct generic_bl_info { > void (*kick_battery)(void); > }; > > -#ifdef CONFIG_OF > +#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > struct backlight_device *of_find_backlight_by_node(struct device_node *node); > #else > static inline struct backlight_device * > > > We might need to create stubs for backlight_force_update() and > backlight_device_set_brightness() too. > With BACKLIGHT_CLASS_DEVICE=m, you still get a link error when the user is in a built-in driver. Using 'depends on' usually solves this (except for drivers that cannot be modules). There are three possible workarounds for this that I can think of: - Use 'depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n' in each driver that implements optional backlight support. We do this elsewhere, but it's confusing and easy to get wrong. - use IS_REACHABLE() instead of IS_ENABLED() when testing for backlight support. This will always result in a kernel that builds cleanly, but can be surprising for users when backlight support is a module that gets loaded at boot, but it is still not used. - Make BACKLIGHT_CLASS_DEVICE a 'bool' symbol instead, and force the core API code to always be built-in or completely disabled. This makes it really easy to use, at the expense of a larger kernel image for those that currently use a loadable module. Arnd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency
Hi Arnd, Thank you for the patch. On Wednesday 19 Apr 2017 19:59:17 Arnd Bergmann wrote: > The panel driver gained support for backlight but fails to link now > when that is disabled: > > drivers/gpu/drm/omapdrm/displays/panel-dpi.o: In function > `panel_dpi_probe_of': panel-dpi.c:(.text.panel_dpi_probe_of+0xe8): > undefined reference to `of_find_backlight_by_node' > > This adds a dependency like we have for the other panel drivers. I believe the dependency should be made optional. DPI panels that don't need backlight control should be supported by a kernel that has backlight support compiled out. How about something like From 07a98ab23b2080c79abbf8b5e7479123c50e6be7 Mon Sep 17 00:00:00 2001 From: Laurent Pinchart Date: Wed, 19 Apr 2017 23:13:43 +0300 Subject: [PATCH] backlight: Define API stub when backlight support is disabled The of_find_backlight_by_node() function has a stubbed when CONFIG_OF is disabled, but drivers that use backlights optionally will still fail to link if backlight support is disabled. Fix it by defining the stub when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled. Signed-off-by: Laurent Pinchart --- include/linux/backlight.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/backlight.h b/include/linux/backlight.h index 5f2fd61ef4fb..fae0b189f7b4 100644 --- a/include/linux/backlight.h +++ b/include/linux/backlight.h @@ -162,7 +162,7 @@ struct generic_bl_info { void (*kick_battery)(void); }; -#ifdef CONFIG_OF +#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *of_find_backlight_by_node(struct device_node *node); #else static inline struct backlight_device * We might need to create stubs for backlight_force_update() and backlight_device_set_brightness() too. > Fixes: 39135a305a0f ("drm/omap: displays: panel-dpi: Support for handling > backlight devices") > Signed-off-by: Arnd Bergmann > --- > drivers/gpu/drm/omapdrm/displays/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/omapdrm/displays/Kconfig > b/drivers/gpu/drm/omapdrm/displays/Kconfig index c226da145fb3..a349cb61961e > 100644 > --- a/drivers/gpu/drm/omapdrm/displays/Kconfig > +++ b/drivers/gpu/drm/omapdrm/displays/Kconfig > @@ -35,6 +35,7 @@ config DRM_OMAP_CONNECTOR_ANALOG_TV > > config DRM_OMAP_PANEL_DPI > tristate "Generic DPI panel" > + depends on BACKLIGHT_CLASS_DEVICE > help > Driver for generic DPI panels. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel