Re: [PATCH v7 1/3] backlight: Add IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE)

["Noralf Trønnes" <noralf@tronnes.org>]

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@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.


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);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel