[PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)
Daniel Thompson
daniel.thompson at linaro.org
Mon Oct 9 09:06:50 UTC 2017
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 Thompson <daniel.thompson at linaro.org>
>>>>> wrote:
>>>>>> 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;
> }
>
> 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 property named 'backlight' on the DT node
> * connected to @dev and looks up the backlight device.
> *
> * Call backlight_put() to drop the reference on the backlight device.
> *
> * Returns:
> * A pointer to the backlight device if found.
> * Error pointer -EPROBE_DEFER if the DT property is set, but no backlight
> * device is found.
> * NULL if there's no backlight property.
> */
> struct backlight_device *backlight_get(struct device *dev)
> {
> struct backlight_device *bd = NULL;
> struct device_node *np;
>
> if (!dev)
> return NULL;
>
> if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> np = of_parse_phandle(dev->of_node, 'backlight', 0);
> if (np) {
> bd = of_find_backlight_by_node(np);
> of_node_put(np);
> if (!bd)
> return ERR_PTR(-EPROBE_DEFER);
> }
> }
>
> return bd;
> }
> EXPORT_SYMBOL(backlight_get);
>
> static void devm_backlight_get_release(void *data)
> {
> backlight_put(data);
> }
>
> /**
> * devm_backlight_get - Resource-managed backlight_get()
> * @dev: Device
> *
> * Device managed version of backlight_get(). The reference on the
> backlight
> * device is automatically dropped on driver detach.
> */
> struct backlight_device *devm_backlight_get(struct device *dev)
> {
> struct backlight_device *bd;
> int ret;
>
> bd = backlight_get(dev);
> if (!bd)
> return NULL;
>
> ret = devm_add_action(dev, devm_backlight_get_release, bd);
> if (ret) {
> backlight_put(bd);
> return ERR_PTR(ret);
> }
>
> return bd;
> }
> EXPORT_SYMBOL(devm_backlight_get);
>
>
More information about the dri-devel
mailing list