Message-ID: <c1d2b2f5-1755-48f3-ac02-952bda718193@kernel.org>
Date: Thu, 9 Oct 2025 23:25:11 +0900
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jonathan Brophy <professorjonny98@...il.com>, lee Jones <lee@...nel.org>,
 Pavel Machek <pavel@...nel.org>,
 Jonathan Brophy <professor_jonny@...mail.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Radoslav Tsvetkov <rtsvetkov@...dotech.eu>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-leds@...r.kernel.org
Subject: Re: [PATCH 1/4] leds: Add Virtual Color LED Group driver

On 09/10/2025 17:43, Jonathan Brophy wrote:
> From: Jonathan Brophy <professor_jonny@...mail.com>
> 
> This commit introduces a new driver that implements virtual LED groups


Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94

> by aggregating multiple monochromatic LEDs. The driver provides
> priority-based control to manage concurrent LED activation requests,



> +
> +static int leds_virtualcolor_init_vled(struct device *dev, struct device_node *child,
> +				       struct virtual_led *vled, struct leds_virtualcolor *vc_data)
> +{
> +	struct fwnode_handle *child_fwnode = of_fwnode_handle(child);
> +	struct led_init_data init_data = {};
> +	u32 blink_interval;
> +	u32 phandle_count;
> +	u32 max_brightness;
> +	int ret, i;
> +
> +	ret = of_property_read_u32(child, "priority", &vled->priority);
> +	if (ret)
> +		vled->priority = 0;
> +
> +	ret = of_property_read_u32(child, "blink", &blink_interval);


Where is this ABI documented? I do not see.

> +	if (!ret) {
> +		vled->blink_delay_on = blink_interval;
> +		vled->blink_delay_off = blink_interval;
> +	}
> +
> +	phandle_count = fwnode_property_count_u32(child_fwnode, "leds");


No, don't mix OF and fwnode.

> +	if (phandle_count <= 0) {
> +		dev_err(dev, "No monochromatic LEDs specified for virtual LED %s\n",
> +			vled->cdev.name);
> +		return -EINVAL;
> +	}
> +
> +	vled->num_monochromatics = phandle_count;
> +	vled->monochromatics = devm_kcalloc(dev, vled->num_monochromatics,
> +					    sizeof(*vled->monochromatics), GFP_KERNEL);
> +	if (!vled->monochromatics)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < vled->num_monochromatics; i++) {
> +		struct led_classdev *led_cdev;
> +
> +		led_cdev = devm_of_led_get_optional(dev, i);
> +		if (IS_ERR(led_cdev)) {
> +			/*
> +			 * If the LED is not available yet, defer the probe.
> +			 * The probe will be retried when it becomes available.
> +			 */
> +			if (PTR_ERR(led_cdev) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;


Pointless...

> +
> +			ret = PTR_ERR(led_cdev);
> +			dev_err(dev, "Failed to get monochromatic LED for %s, error %d\n",
> +				vled->cdev.name, ret);
> +			return ret;


...just return dev_err_probe

> +		}
> +
> +		vled->monochromatics[i] = led_cdev;
> +	}
> +
> +	ret = of_property_read_u32(child, "max-brightness", &max_brightness);
> +	if (ret)
> +		vled->cdev.max_brightness = LED_FULL;
> +	else
> +		vled->cdev.max_brightness = max_brightness;
> +
> +	vled->cdev.brightness_set_blocking = virtual_led_brightness_set;
> +	vled->cdev.max_brightness = LED_FULL;
> +	vled->cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> +	init_data.fwnode = child_fwnode;
> +	ret = devm_led_classdev_register_ext(dev, &vled->cdev, &init_data);
> +	if (ret) {
> +		dev_err(dev, "Failed to register virtual LED %s\n", vled->cdev.name);
> +		return ret;
> +	}
> +
> +	ret = device_create_file(vled->cdev.dev, &dev_attr_priority);
> +	if (ret) {
> +		dev_err(dev, "Failed to create sysfs attribute for priority\n");
> +		return ret;
> +	}
> +
> +	ret = device_create_file(vled->cdev.dev, &dev_attr_blink_delay_on);
> +	if (ret) {
> +		dev_err(dev, "Failed to create sysfs attribute for blink_delay_on\n");
> +		return ret;
> +	}
> +
> +	ret = device_create_file(vled->cdev.dev, &dev_attr_blink_delay_off);
> +	if (ret) {
> +		dev_err(dev, "Failed to create sysfs attribute for blink_delay_off\n");
> +		return ret;
> +	}
> +
> +	vled->vc_data = vc_data;
> +
> +	return 0;
> +}
> +
> +static int leds_virtualcolor_disable_sysfs_access(struct device *dev, struct virtual_led *vled)
> +{
> +	int i;
> +
> +	for (i = 0; i < vled->num_monochromatics; i++) {
> +		struct led_classdev *led_cdev = vled->monochromatics[i];
> +
> +		mutex_lock(&led_cdev->led_access);
> +		led_sysfs_disable(led_cdev);
> +		mutex_unlock(&led_cdev->led_access);
> +
> +		devm_add_action_or_reset(dev, restore_sysfs_write_access, led_cdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static int leds_virtualcolor_probe(struct platform_device *pdev)
> +{
> +	struct leds_virtualcolor *vc_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *child;
> +	int count = 0;
> +	int ret;
> +
> +	vc_data = devm_kzalloc(dev, sizeof(*vc_data), GFP_KERNEL);
> +	if (!vc_data)
> +		return -ENOMEM;
> +
> +	mutex_init(&vc_data->lock);
> +	INIT_LIST_HEAD(&vc_data->active_leds);
> +
> +	vc_data->num_vleds = of_get_child_count(dev->of_node);
> +	if (vc_data->num_vleds == 0) {
> +		dev_err(dev, "No virtual LEDs defined in device tree\n");
> +		ret = -EINVAL;
> +		goto err_mutex_destroy;
> +	}
> +
> +	vc_data->vleds = devm_kcalloc(dev, vc_data->num_vleds, sizeof(*vc_data->vleds), GFP_KERNEL);
> +	if (!vc_data->vleds) {
> +		ret = -ENOMEM;
> +		goto err_mutex_destroy;
> +	}
> +
> +	for_each_child_of_node(dev->of_node, child) {
> +		struct virtual_led *vled = &vc_data->vleds[count];
> +
> +		ret = leds_virtualcolor_init_vled(dev, child, vled, vc_data);
> +		if (ret) {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to initialize virtual LED %d\n", count);
> +
> +			of_node_put(child);


Just use scoped loop.

> +			goto err_node_put;
> +		}
> +
> +		count++;
> +	}
> +
> +	platform_set_drvdata(pdev, vc_data);
> +
> +	if (of_property_read_bool(dev->of_node, "monochromatics-ro")) {
> +		int i;
> +
> +		for (i = 0; i < count; i++) {
> +			struct virtual_led *vled = &vc_data->vleds[i];
> +
> +			ret = leds_virtualcolor_disable_sysfs_access(dev, vled);
> +			if (ret)
> +				goto err_node_put;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_node_put:
> +	of_node_put(child);


Double of node release or your code is just confusing. Each functions
cleans up only pieces it allocates, not some other function resources.

> +err_mutex_destroy:
> +	mutex_destroy(&vc_data->lock);
> +
> +	return ret;
> +}
> +
> +static void leds_virtualcolor_remove(struct platform_device *pdev)
> +{
> +	struct leds_virtualcolor *vc_data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < vc_data->num_vleds; i++) {
> +		struct virtual_led *vled = &vc_data->vleds[i];
> +		int j;
> +
> +		device_remove_file(vled->cdev.dev, &dev_attr_priority);
> +		device_remove_file(vled->cdev.dev, &dev_attr_blink_delay_on);
> +		device_remove_file(vled->cdev.dev, &dev_attr_blink_delay_off);
> +
> +		for (j = 0; j < vled->num_monochromatics; j++) {
> +			if (vled->monochromatics[j]) {
> +				led_put(vled->monochromatics[j]);
> +				vled->monochromatics[j] = NULL;
> +			}
> +		}
> +	}
> +
> +	mutex_destroy(&vc_data->lock);
> +}
> +
> +static const struct of_device_id leds_virtualcolor_of_match[] = {
> +	{ .compatible = "leds-group-virtualcolor" },


Please organize the patch documenting compatible (DT bindings) before
their user.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, leds_virtualcolor_of_match);
> +
> +static struct platform_driver leds_virtualcolor_driver = {
> +	.probe  = leds_virtualcolor_probe,
> +	.remove = leds_virtualcolor_remove,
> +	.driver = {
> +		.name           = "leds_virtualcolor",
> +		.of_match_table = leds_virtualcolor_of_match,
> +	},
> +};
> +
> +module_platform_driver(leds_virtualcolor_driver);
> +
> +MODULE_AUTHOR("Radoslav Tsvetkov <rtsvetkov@...dotech.eu>");
> +MODULE_DESCRIPTION("LEDs Virtual Color Driver with Priority Handling");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-group-virtualcolor");


You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.




Best regards,
Krzysztof

Powered by blists - more mailing lists