Message ID | 2979804.i6EKSW6ohY@avalon (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Apr 19, 2017 at 10:21 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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
On Wed, 19 Apr 2017, Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Apr 19, 2017 at 10:21 PM, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> 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
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 *