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

[Noralf Trønnes]

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 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?
>

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