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

[Daniel Thompson <daniel.thompson@linaro.org>]

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

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