Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
Den 09.10.2017 11.06, skrev Daniel Thompson: On 06/10/17 19:01, Noralf Trønnes wrote: Den 03.10.2017 11.04, skrev Daniel Vetter: On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote: On 03/10/17 09:03, Daniel Vetter wrote: On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote: On Mon, 02 Oct 2017, Daniel Thompsonwrote: So I'm coming to this patchset cold but can you explain *why* something wants to call of_find_backlight_by_node() when there is no backlight support enabled. Why isn't the code that called is conditional on BACKLIGHT_CLASS_DEVICE? The undefined symbol issue is a pain but to be honest I'd rather solve the use of undefined symbols by avoiding declaring them; this making them into compile errors rather than link errors. Typically the kernel header files define static inline stubs of the functions when the actual functions aren't configured/built. The code using the functions looks the same regardless of the config option, and handles the -ENODEV or NULL or whatever returns from the stubs gracefully. With the inlines, the compiler can usually throw out much of the code anyway. In this regard, the backlight interface is an exception, forcing the callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not the rule. fwiw, I think it'd be great if we can move backlight over to the common pattern, like everyone else. It's pretty big pain as-is ... For sure, after Jani's mail yesterday I looked at the GMA500 driver and concluded I didn't want code related to backlight having to look like that! Currently the three main patterns of use are: 1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE, 2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM driver is an example of this), 3. Other compound drivers perform heroics with the pre-processor. The main reason I'm not just agreeing straight away is that, to adopt the static inline pattern for the whole API, we have to believe that #3 above is desirable. Given the size and scope of the compound drivers in #3 above, I don't entirely understand why they want to avoid the select. People love to over-configure their kernels and shave off a few bytes. And big gpu drivers might have backlight support, but not always need it (e.g. if you run without a panel as e.g. a tv set-top-box). Same with e.g. a driver that supports both OF/DT and pci to find its devices. On the desktop gpu driver side we don't really subscribe to this idea of making everything optional, hence select (mostly), except select is a huge pain. On the mobile/soc gpu side, #3 seems to be the desired outcome. Doing static inline helpers for backlights would make both easier (since in the end for desktop you just get a distro kernel that enables everything anyway). So yeah, imo if you think backlight should be a Kconfig, then it should have static inline dummy functions to make life simpler for everyone. Only exception are pure driver-only subsystem code which aren't ever called by anything outside of your subsystem. But since the point of the backlight subsystem is to provide backlight support to other drivers (there's still the dream that we don't offload this onto userspace, that just doesn't work well) we really should have these static inline helpers. I suggest we put all the backlight subsystem only functionality we need, into the backlight subsystem :-) I've put together a suggestion below and I've deliberately dropped the of_ infix in backlight_get() to make room for possible future additions that can make it possible to set the connection between device and backlight using platform table or ACPI, fashioned after gpiolib. Looks good to me. If I've read this right, other sub-systems can use these symbols with or without BACKLIGHT_CLASS_DEVICE and still compile OK? The code haven't seen a compiler, but that's the idea. Noralf. Daniel. include/linux/backlight.h: /** * backlight_enable - Enable backlight * @bd: the backlight device to enable */ static inline int backlight_enable(struct backlight_device *bd) { if (!bd) return 0; bd->props.power = FB_BLANK_UNBLANK; bd->props.state &= ~BL_CORE_FBBLANK; return backlight_update_status(bd); } /** * backlight_disable - Disable backlight * @bd: the backlight device to disable */ static inline int backlight_disable(struct backlight_device *bd) { if (!bd) return 0; bd->props.power = FB_BLANK_POWERDOWN; bd->props.state |= BL_CORE_FBBLANK; return backlight_update_status(bd); } /** * backlight_put - Drop backlight reference * @bd: the backlight device to put */ static inline void backlight_put(struct backlight_device *bd) { if (bd) put_device(bd->dev); } #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *backlight_get(struct device *dev); struct backlight_device
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On 06/10/17 19:01, Noralf Trønnes wrote: Den 03.10.2017 11.04, skrev Daniel Vetter: On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote: On 03/10/17 09:03, Daniel Vetter wrote: On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote: On Mon, 02 Oct 2017, Daniel Thompsonwrote: So I'm coming to this patchset cold but can you explain *why* something wants to call of_find_backlight_by_node() when there is no backlight support enabled. Why isn't the code that called is conditional on BACKLIGHT_CLASS_DEVICE? The undefined symbol issue is a pain but to be honest I'd rather solve the use of undefined symbols by avoiding declaring them; this making them into compile errors rather than link errors. Typically the kernel header files define static inline stubs of the functions when the actual functions aren't configured/built. The code using the functions looks the same regardless of the config option, and handles the -ENODEV or NULL or whatever returns from the stubs gracefully. With the inlines, the compiler can usually throw out much of the code anyway. In this regard, the backlight interface is an exception, forcing the callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not the rule. fwiw, I think it'd be great if we can move backlight over to the common pattern, like everyone else. It's pretty big pain as-is ... For sure, after Jani's mail yesterday I looked at the GMA500 driver and concluded I didn't want code related to backlight having to look like that! Currently the three main patterns of use are: 1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE, 2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM driver is an example of this), 3. Other compound drivers perform heroics with the pre-processor. The main reason I'm not just agreeing straight away is that, to adopt the static inline pattern for the whole API, we have to believe that #3 above is desirable. Given the size and scope of the compound drivers in #3 above, I don't entirely understand why they want to avoid the select. People love to over-configure their kernels and shave off a few bytes. And big gpu drivers might have backlight support, but not always need it (e.g. if you run without a panel as e.g. a tv set-top-box). Same with e.g. a driver that supports both OF/DT and pci to find its devices. On the desktop gpu driver side we don't really subscribe to this idea of making everything optional, hence select (mostly), except select is a huge pain. On the mobile/soc gpu side, #3 seems to be the desired outcome. Doing static inline helpers for backlights would make both easier (since in the end for desktop you just get a distro kernel that enables everything anyway). So yeah, imo if you think backlight should be a Kconfig, then it should have static inline dummy functions to make life simpler for everyone. Only exception are pure driver-only subsystem code which aren't ever called by anything outside of your subsystem. But since the point of the backlight subsystem is to provide backlight support to other drivers (there's still the dream that we don't offload this onto userspace, that just doesn't work well) we really should have these static inline helpers. I suggest we put all the backlight subsystem only functionality we need, into the backlight subsystem :-) I've put together a suggestion below and I've deliberately dropped the of_ infix in backlight_get() to make room for possible future additions that can make it possible to set the connection between device and backlight using platform table or ACPI, fashioned after gpiolib. Looks good to me. If I've read this right, other sub-systems can use these symbols with or without BACKLIGHT_CLASS_DEVICE and still compile OK? Daniel. include/linux/backlight.h: /** * backlight_enable - Enable backlight * @bd: the backlight device to enable */ static inline int backlight_enable(struct backlight_device *bd) { if (!bd) return 0; bd->props.power = FB_BLANK_UNBLANK; bd->props.state &= ~BL_CORE_FBBLANK; return backlight_update_status(bd); } /** * backlight_disable - Disable backlight * @bd: the backlight device to disable */ static inline int backlight_disable(struct backlight_device *bd) { if (!bd) return 0; bd->props.power = FB_BLANK_POWERDOWN; bd->props.state |= BL_CORE_FBBLANK; return backlight_update_status(bd); } /** * backlight_put - Drop backlight reference * @bd: the backlight device to put */ static inline void backlight_put(struct backlight_device *bd) { if (bd) put_device(bd->dev); } #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *backlight_get(struct device *dev); struct backlight_device *devm_backlight_get(struct device *dev); #else static inline struct backlight_device *backlight_get(struct device *dev) { return NULL; }
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
Den 03.10.2017 11.04, skrev Daniel Vetter: On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote: On 03/10/17 09:03, Daniel Vetter wrote: On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote: On Mon, 02 Oct 2017, Daniel Thompsonwrote: So I'm coming to this patchset cold but can you explain *why* something wants to call of_find_backlight_by_node() when there is no backlight support enabled. Why isn't the code that called is conditional on BACKLIGHT_CLASS_DEVICE? The undefined symbol issue is a pain but to be honest I'd rather solve the use of undefined symbols by avoiding declaring them; this making them into compile errors rather than link errors. Typically the kernel header files define static inline stubs of the functions when the actual functions aren't configured/built. The code using the functions looks the same regardless of the config option, and handles the -ENODEV or NULL or whatever returns from the stubs gracefully. With the inlines, the compiler can usually throw out much of the code anyway. In this regard, the backlight interface is an exception, forcing the callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not the rule. fwiw, I think it'd be great if we can move backlight over to the common pattern, like everyone else. It's pretty big pain as-is ... For sure, after Jani's mail yesterday I looked at the GMA500 driver and concluded I didn't want code related to backlight having to look like that! Currently the three main patterns of use are: 1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE, 2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM driver is an example of this), 3. Other compound drivers perform heroics with the pre-processor. The main reason I'm not just agreeing straight away is that, to adopt the static inline pattern for the whole API, we have to believe that #3 above is desirable. Given the size and scope of the compound drivers in #3 above, I don't entirely understand why they want to avoid the select. People love to over-configure their kernels and shave off a few bytes. And big gpu drivers might have backlight support, but not always need it (e.g. if you run without a panel as e.g. a tv set-top-box). Same with e.g. a driver that supports both OF/DT and pci to find its devices. On the desktop gpu driver side we don't really subscribe to this idea of making everything optional, hence select (mostly), except select is a huge pain. On the mobile/soc gpu side, #3 seems to be the desired outcome. Doing static inline helpers for backlights would make both easier (since in the end for desktop you just get a distro kernel that enables everything anyway). So yeah, imo if you think backlight should be a Kconfig, then it should have static inline dummy functions to make life simpler for everyone. Only exception are pure driver-only subsystem code which aren't ever called by anything outside of your subsystem. But since the point of the backlight subsystem is to provide backlight support to other drivers (there's still the dream that we don't offload this onto userspace, that just doesn't work well) we really should have these static inline helpers. I suggest we put all the backlight subsystem only functionality we need, into the backlight subsystem :-) I've put together a suggestion below and I've deliberately dropped the of_ infix in backlight_get() to make room for possible future additions that can make it possible to set the connection between device and backlight using platform table or ACPI, fashioned after gpiolib. include/linux/backlight.h: /** * backlight_enable - Enable backlight * @bd: the backlight device to enable */ static inline int backlight_enable(struct backlight_device *bd) { if (!bd) return 0; bd->props.power = FB_BLANK_UNBLANK; bd->props.state &= ~BL_CORE_FBBLANK; return backlight_update_status(bd); } /** * backlight_disable - Disable backlight * @bd: the backlight device to disable */ static inline int backlight_disable(struct backlight_device *bd) { if (!bd) return 0; bd->props.power = FB_BLANK_POWERDOWN; bd->props.state |= BL_CORE_FBBLANK; return backlight_update_status(bd); } /** * backlight_put - Drop backlight reference * @bd: the backlight device to put */ static inline void backlight_put(struct backlight_device *bd) { if (bd) put_device(bd->dev); } #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) struct backlight_device *backlight_get(struct device *dev); struct backlight_device *devm_backlight_get(struct device *dev); #else static inline struct backlight_device *backlight_get(struct device *dev) { return NULL; } static inline struct backlight_device *devm_backlight_get(struct device *dev) { return NULL; } #endif drivers/video/backlight/backlight.c: /** * backlight_get - Get backlight device * @dev: Device * * This function looks for a
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On Tue, Oct 03, 2017 at 09:51:49AM +0100, Daniel Thompson wrote: > On 03/10/17 09:03, Daniel Vetter wrote: > > On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote: > > > On Mon, 02 Oct 2017, Daniel Thompsonwrote: > > > > So I'm coming to this patchset cold but can you explain *why* something > > > > wants to call of_find_backlight_by_node() when there is no backlight > > > > support enabled. Why isn't the code that called is conditional on > > > > BACKLIGHT_CLASS_DEVICE? > > > > > > > > The undefined symbol issue is a pain but to be honest I'd rather solve > > > > the use of undefined symbols by avoiding declaring them; this making > > > > them into compile errors rather than link errors. > > > > > > Typically the kernel header files define static inline stubs of the > > > functions when the actual functions aren't configured/built. The code > > > using the functions looks the same regardless of the config option, and > > > handles the -ENODEV or NULL or whatever returns from the stubs > > > gracefully. With the inlines, the compiler can usually throw out much of > > > the code anyway. > > > > > > In this regard, the backlight interface is an exception, forcing the > > > callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not > > > the rule. > > > > fwiw, I think it'd be great if we can move backlight over to the common > > pattern, like everyone else. It's pretty big pain as-is ... > > For sure, after Jani's mail yesterday I looked at the GMA500 driver and > concluded I didn't want code related to backlight having to look like that! > > Currently the three main patterns of use are: > > 1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE, > 2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM > driver is an example of this), > 3. Other compound drivers perform heroics with the pre-processor. > > The main reason I'm not just agreeing straight away is that, to adopt the > static inline pattern for the whole API, we have to believe that #3 above is > desirable. Given the size and scope of the compound drivers in #3 above, I > don't entirely understand why they want to avoid the select. People love to over-configure their kernels and shave off a few bytes. And big gpu drivers might have backlight support, but not always need it (e.g. if you run without a panel as e.g. a tv set-top-box). Same with e.g. a driver that supports both OF/DT and pci to find its devices. On the desktop gpu driver side we don't really subscribe to this idea of making everything optional, hence select (mostly), except select is a huge pain. On the mobile/soc gpu side, #3 seems to be the desired outcome. Doing static inline helpers for backlights would make both easier (since in the end for desktop you just get a distro kernel that enables everything anyway). So yeah, imo if you think backlight should be a Kconfig, then it should have static inline dummy functions to make life simpler for everyone. Only exception are pure driver-only subsystem code which aren't ever called by anything outside of your subsystem. But since the point of the backlight subsystem is to provide backlight support to other drivers (there's still the dream that we don't offload this onto userspace, that just doesn't work well) we really should have these static inline helpers. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On 03/10/17 09:03, Daniel Vetter wrote: On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote: On Mon, 02 Oct 2017, Daniel Thompsonwrote: So I'm coming to this patchset cold but can you explain *why* something wants to call of_find_backlight_by_node() when there is no backlight support enabled. Why isn't the code that called is conditional on BACKLIGHT_CLASS_DEVICE? The undefined symbol issue is a pain but to be honest I'd rather solve the use of undefined symbols by avoiding declaring them; this making them into compile errors rather than link errors. Typically the kernel header files define static inline stubs of the functions when the actual functions aren't configured/built. The code using the functions looks the same regardless of the config option, and handles the -ENODEV or NULL or whatever returns from the stubs gracefully. With the inlines, the compiler can usually throw out much of the code anyway. In this regard, the backlight interface is an exception, forcing the callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not the rule. fwiw, I think it'd be great if we can move backlight over to the common pattern, like everyone else. It's pretty big pain as-is ... For sure, after Jani's mail yesterday I looked at the GMA500 driver and concluded I didn't want code related to backlight having to look like that! Currently the three main patterns of use are: 1. Standalone drivers simple depend on BACKLIGHT_CLASS_DEVICE, 2. Some compound drivers select BACKLIGHT_CLASS_DEVICE (the AMD DRM driver is an example of this), 3. Other compound drivers perform heroics with the pre-processor. The main reason I'm not just agreeing straight away is that, to adopt the static inline pattern for the whole API, we have to believe that #3 above is desirable. Given the size and scope of the compound drivers in #3 above, I don't entirely understand why they want to avoid the select. Daniel. PS Whatever the outcome here, I do agree 100% that is wrong to prototype undefined symbols. That must be changed! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On Mon, Oct 02, 2017 at 12:00:54PM +0300, Jani Nikula wrote: > On Mon, 02 Oct 2017, Daniel Thompsonwrote: > > So I'm coming to this patchset cold but can you explain *why* something > > wants to call of_find_backlight_by_node() when there is no backlight > > support enabled. Why isn't the code that called is conditional on > > BACKLIGHT_CLASS_DEVICE? > > > > The undefined symbol issue is a pain but to be honest I'd rather solve > > the use of undefined symbols by avoiding declaring them; this making > > them into compile errors rather than link errors. > > Typically the kernel header files define static inline stubs of the > functions when the actual functions aren't configured/built. The code > using the functions looks the same regardless of the config option, and > handles the -ENODEV or NULL or whatever returns from the stubs > gracefully. With the inlines, the compiler can usually throw out much of > the code anyway. > > In this regard, the backlight interface is an exception, forcing the > callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not > the rule. fwiw, I think it'd be great if we can move backlight over to the common pattern, like everyone else. It's pretty big pain as-is ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On Mon, 02 Oct 2017, Daniel Thompsonwrote: > On 02/10/17 09:49, Jani Nikula wrote: >> On Mon, 02 Oct 2017, Daniel Thompson wrote: >>> On 01/10/17 18:26, Meghana Madhyastha wrote: -#ifdef CONFIG_OF +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) >>> >>> The above comments are more important but why does this mix defined and >>> IS_ENABLED? Couldn't they both use defined (and preferably with the >>> optional brackets around the CONFIG_ symbol). >> >> No, always use IS_ENABLED() for tristates when you want to match 'y' or >> 'm'. > > Oops. > > Actually it was the mis-match in the original code that attracted my > attention (defined on one side, IS_ENABLED() on the other)... I'd be > happier if both used the same approach. Agreed. BR, Jani. -- 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 v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On 02/10/17 09:49, Jani Nikula wrote: On Mon, 02 Oct 2017, Daniel Thompsonwrote: On 01/10/17 18:26, Meghana Madhyastha wrote: -#ifdef CONFIG_OF +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) The above comments are more important but why does this mix defined and IS_ENABLED? Couldn't they both use defined (and preferably with the optional brackets around the CONFIG_ symbol). No, always use IS_ENABLED() for tristates when you want to match 'y' or 'm'. Oops. Actually it was the mis-match in the original code that attracted my attention (defined on one side, IS_ENABLED() on the other)... I'd be happier if both used the same approach. Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On Mon, 02 Oct 2017, Daniel Thompsonwrote: > So I'm coming to this patchset cold but can you explain *why* something > wants to call of_find_backlight_by_node() when there is no backlight > support enabled. Why isn't the code that called is conditional on > BACKLIGHT_CLASS_DEVICE? > > The undefined symbol issue is a pain but to be honest I'd rather solve > the use of undefined symbols by avoiding declaring them; this making > them into compile errors rather than link errors. Typically the kernel header files define static inline stubs of the functions when the actual functions aren't configured/built. The code using the functions looks the same regardless of the config option, and handles the -ENODEV or NULL or whatever returns from the stubs gracefully. With the inlines, the compiler can usually throw out much of the code anyway. In this regard, the backlight interface is an exception, forcing the callers to wrap the code around IS_ENABLED(BACKLIGHT_CLASS_DEVICE), not the rule. BR, Jani. -- 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 v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On Mon, 02 Oct 2017, Daniel Thompsonwrote: > On 01/10/17 18:26, Meghana Madhyastha wrote: >> -#ifdef CONFIG_OF >> +#if defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > > The above comments are more important but why does this mix defined and > IS_ENABLED? Couldn't they both use defined (and preferably with the > optional brackets around the CONFIG_ symbol). No, always use IS_ENABLED() for tristates when you want to match 'y' or 'm'. BR, Jani. -- 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 v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On 02/10/17 06:58, Daniel Thompson wrote: On 01/10/17 18:26, Meghana Madhyastha wrote: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the if directive for the function declaration of of_find_backlight_by_node in order to avoid module dependency errors. Module dependency errors? Does you mean mean use of undefined symbols? Sorry, drafting error! Could you pretend I wrote "Do you mean use of..." instead of the nonsense above! Signed-off-by: Meghana Madhyastha--- Changes in v7: -This patch did not exist in v6. So I'm coming to this patchset cold but can you explain *why* something wants to call of_find_backlight_by_node() when there is no backlight support enabled. Why isn't the code that called is conditional on BACKLIGHT_CLASS_DEVICE? The undefined symbol issue is a pain but to be honest I'd rather solve the use of undefined symbols by avoiding declaring them; this making them into compile errors rather than link errors. 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 5f2fd61..a52ce82 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 defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) The above comments are more important but why does this mix defined and IS_ENABLED? Couldn't they both use defined (and preferably with the optional brackets around the CONFIG_ symbol). Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
On 01/10/17 18:26, Meghana Madhyastha wrote: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the if directive for the function declaration of of_find_backlight_by_node in order to avoid module dependency errors. Module dependency errors? Does you mean mean use of undefined symbols? Signed-off-by: Meghana Madhyastha--- Changes in v7: -This patch did not exist in v6. So I'm coming to this patchset cold but can you explain *why* something wants to call of_find_backlight_by_node() when there is no backlight support enabled. Why isn't the code that called is conditional on BACKLIGHT_CLASS_DEVICE? The undefined symbol issue is a pain but to be honest I'd rather solve the use of undefined symbols by avoiding declaring them; this making them into compile errors rather than link errors. 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 5f2fd61..a52ce82 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 defined CONFIG_OF && IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) The above comments are more important but why does this mix defined and IS_ENABLED? Couldn't they both use defined (and preferably with the optional brackets around the CONFIG_ symbol). Daniel. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) as part of the if directive for the function declaration of of_find_backlight_by_node in order to avoid module dependency errors. Signed-off-by: Meghana Madhyastha--- Changes in v7: -This patch did not exist in v6. 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 5f2fd61..a52ce82 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 defined 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 * -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel