From: Arnd Bergmann <arnd@arndb.de> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>, David Airlie <airlied@linux.ie>, dri-devel <dri-devel@lists.freedesktop.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency Date: Wed, 19 Apr 2017 22:39:16 +0200 [thread overview] Message-ID: <CAK8P3a0rQqOZ-VJjmSR_MrRmKUK2ztASK2JTMe-NwV0TStXURA@mail.gmail.com> (raw) In-Reply-To: <2979804.i6EKSW6ohY@avalon> 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
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency Date: Wed, 19 Apr 2017 22:39:16 +0200 [thread overview] Message-ID: <CAK8P3a0rQqOZ-VJjmSR_MrRmKUK2ztASK2JTMe-NwV0TStXURA@mail.gmail.com> (raw) In-Reply-To: <2979804.i6EKSW6ohY@avalon> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-04-19 20:39 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-19 17:59 [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency Arnd Bergmann 2017-04-19 17:59 ` Arnd Bergmann 2017-04-19 17:59 ` [PATCH 2/3] drm/panel: S6E3HA2 needs backlight code Arnd Bergmann 2017-04-19 17:59 ` Arnd Bergmann 2017-04-19 18:03 ` [PATCH 3/3] drm/panel: add backlight dependency for sitronix-st7789v Arnd Bergmann 2017-04-20 1:37 ` Hoegeun Kwon 2017-06-14 18:13 ` Thierry Reding 2017-06-14 18:13 ` Thierry Reding 2017-04-20 1:34 ` [PATCH 2/3] drm/panel: S6E3HA2 needs backlight code Hoegeun Kwon 2017-06-14 18:13 ` Thierry Reding 2017-06-14 18:13 ` Thierry Reding 2017-04-19 20:21 ` [PATCH 1/3] drm/omap: displays: panel-dpi: add backlight dependency Laurent Pinchart 2017-04-19 20:21 ` Laurent Pinchart 2017-04-19 20:39 ` Arnd Bergmann [this message] 2017-04-19 20:39 ` Arnd Bergmann 2017-04-20 9:01 ` Jani Nikula
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAK8P3a0rQqOZ-VJjmSR_MrRmKUK2ztASK2JTMe-NwV0TStXURA@mail.gmail.com \ --to=arnd@arndb.de \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=tomi.valkeinen@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.