Re: [PATCH v3 1/8] backlight: add kernel-internal backlight API

[Louis Chauvet <louis.chauvet@bootlin.com>]

On 4/25/26 00:09, Mario Limonciello wrote:
> From: David Herrmann <dh.herrmann@gmail.com>
> 
> So far backlights have only been controlled via sysfs. However, sysfs is
> not a proper user-space API for runtime modifications, and never was
> intended to provide such. The DRM drivers are now prepared to provide
> such a backlight link so user-space can control backlight via DRM
> connector properties. This allows us to employ the same access-management
> we use for mode-setting.
> 
> This patch adds few kernel-internal backlight helpers so we can modify
> backlights from within DRM.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> 
> V2: Marta Lofstedt <marta.lofstedt@intel.com>
> - rebase
> - minor edit for checkpatch warning
> 
> Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com>
> 
> V3: Mario Limonciello <mario.limonciello@amd.com>
>   - rebase
>   - Use guard(mutex)
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/video/backlight/backlight.c | 60 +++++++++++++++++++++++++++++
>   include/linux/backlight.h           | 16 ++++++++
>   2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index ab87a5e3dbf70..c3673bee6d9cf 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -513,6 +513,66 @@ static int devm_backlight_device_match(struct device *dev, void *res,
>   	return *r == data;
>   }
>   
> +/**
> + * backlight_device_lookup - find a backlight device
> + * @name: sysname of the backlight device
> + *
> + * @return Reference to the backlight device, NULL if not found.
> + *
> + * This searches through all registered backlight devices for a device with the
> + * given device name. In case none is found, NULL is returned, otherwise a
> + * new reference to the backlight device is returned. You must drop this
> + * reference via backlight_device_unref() once done.
> + * Note that the devices might get unregistered at any time. You need to lock
> + * around this lookup and inside of your backlight-notifier if you need to know
> + * when a device gets unregistered.
> + *
> + * This function can be safely called from IRQ context.
> + */
> +struct backlight_device *backlight_device_lookup(const char *name)
> +{
> +	struct backlight_device *bd;
> +	const char *t;
> +
> +	guard(mutex)(&backlight_dev_list_mutex);
> +	list_for_each_entry(bd, &backlight_dev_list, entry) {
> +		t = dev_name(&bd->dev);
> +		if (t && !strcmp(t, name)) {
> +			backlight_device_ref(bd);
> +			return bd;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(backlight_device_lookup);
> 

Hello,

I think this function can be repalced with backlight_device_get_by_name.

> +/**
> + * backlight_set_brightness - set brightness on a backlight device
> + * @bd: backlight device to operate on
> + * @value: brightness value to set on the device
> + * @reason: backlight-change reason to use for notifications
> + *
> + * This is the in-kernel API equivalent of writing into the 'brightness' sysfs
> + * file. It calls into the underlying backlight driver to change the brightness
> + * value. The value is clamped according to device bounds.
> + * A uevent notification is sent with the reason set to @reason.
> + */
> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> +			      enum backlight_update_reason reason)
> +{
> +	guard(mutex)(&bd->ops_lock);
> +	if (bd->ops) {
> +		value = clamp(value, 0U,
> +			      (unsigned int)bd->props.max_brightness);

Why did you use a clamping here? I think it is better to return error 
instead.

> +		dev_dbg(&bd->dev, "set brightness to %u\n", value);
> +		bd->props.brightness = value;
> +		backlight_update_status(bd);
> +	}
> +	backlight_generate_event(bd, reason);
> +}
> +EXPORT_SYMBOL_GPL(backlight_set_brightness);
> 

I think this could be nice to update backlight_device_set_brightness to 
avoid code duplication:

int backlight_device_set_brightness(...) {
	return backlight_set_brightness(..., BACKLIGHT_UPDATE_SYSFS);
}

>   /**
>    * backlight_register_notifier - get notified of backlight (un)registration
>    * @nb: notifier block with the notifier to call on backlight (un)registration
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index d905173c7f73c..7e4fee65fddd9 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -429,6 +429,22 @@ static inline void backlight_notify_blank_all(struct device *display_dev,
>   { }
>   #endif
>   
> +struct backlight_device *backlight_device_lookup(const char *name);
> +void backlight_set_brightness(struct backlight_device *bd, unsigned int value,
> +			      enum backlight_update_reason reason);
> +
> +static inline void backlight_device_ref(struct backlight_device *bd)
> +{
> +	if (bd)
> +		get_device(&bd->dev);
> +}
> +
> +static inline void backlight_device_unref(struct backlight_device *bd)
> +{
> +	if (bd)
> +		put_device(&bd->dev);
> +}
> +
Most of the kernel use _put and _get functions, I think it could be nice 
to keep the same naming.

Thanks,
Louis Chauvet

>   #define to_backlight_device(obj) container_of(obj, struct backlight_device, dev)
>   
>   /**