Re: [RFC PATCH 1/2] backlight: Rename duplicated devices to support dual-backlight setups

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sun, 25 May 2025, Pengyu Luo <mitltlatltl@gmail.com> wrote:
> When registering a backlight device, if a device with the same name
> already exists, append "-secondary" to the new device's name. This is
> useful for platforms with dual backlight drivers (e.g. some panels use
> dual ktz8866), where both instances need to coexist.
>
> For now, only one secondary instance is supported. If more instances
> are needed, this logic can be extended with auto-increment or a more
> flexible naming scheme.

I think for both patches you should consider adding a new interface for
creating dual backlight scenarios.

For example, this patch turns a driver error (registering two or more
backlights with the same name) into a special use case, patch 2
magically connecting the two, and hiding the problem.

With i915, you could have multiple devices, each with multiple
independent panels with independent backlights. I think accidentally
trying to register more than one backlight with the same name should
remain an error, *unless* you want the special case of combined
backlights.

Similarly, what if you encounter a device with two panels, and two
*independent* ktz8866?

Please be explicit rather than implicit.


BR,
Jani.


>
> Suggested-by: Daniel Thompson <danielt@kernel.org>
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
>  drivers/video/backlight/backlight.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 9dc93c5e4..991702f5d 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -365,7 +365,8 @@ struct backlight_device *backlight_device_register(const char *name,
>  	struct device *parent, void *devdata, const struct backlight_ops *ops,
>  	const struct backlight_properties *props)
>  {
> -	struct backlight_device *new_bd;
> +	struct backlight_device *new_bd, *prev_bd;
> +	const char *new_name = NULL;
>  	int rc;
>  
>  	pr_debug("backlight_device_register: name=%s\n", name);
> @@ -377,10 +378,23 @@ struct backlight_device *backlight_device_register(const char *name,
>  	mutex_init(&new_bd->update_lock);
>  	mutex_init(&new_bd->ops_lock);
>  
> +	/*
> +	 * If there is an instance with the same name already, then rename it.
> +	 * We also can use an auto-increment field, but it seems that there is
> +	 * no triple or quad case.
> +	 */
> +	prev_bd = backlight_device_get_by_name(name);
> +	if (!IS_ERR_OR_NULL(prev_bd)) {
> +		new_name = kasprintf(GFP_KERNEL, "%s-secondary", name);
> +		if (!new_name)
> +			return ERR_PTR(-ENOMEM);
> +		put_device(&prev_bd->dev);
> +	}
> +
>  	new_bd->dev.class = &backlight_class;
>  	new_bd->dev.parent = parent;
>  	new_bd->dev.release = bl_device_release;
> -	dev_set_name(&new_bd->dev, "%s", name);
> +	dev_set_name(&new_bd->dev, "%s", new_name ? new_name : name);
>  	dev_set_drvdata(&new_bd->dev, devdata);
>  
>  	/* Set default properties */
> @@ -414,6 +428,8 @@ struct backlight_device *backlight_device_register(const char *name,
>  	list_add(&new_bd->entry, &backlight_dev_list);
>  	mutex_unlock(&backlight_dev_list_mutex);
>  
> +	kfree(new_name);
> +
>  	return new_bd;
>  }
>  EXPORT_SYMBOL(backlight_device_register);

-- 
Jani Nikula, Intel