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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
