On Fri, 18 Jul 2025, Krzysztof Kozlowski wrote:

On 14/07/2025 19:23, Nam Tran wrote:

quoted

quoted

+static int lp5812_parse_led(struct device_node *np,

+			    struct lp5812_led_config *cfg,
+			    int led_index)
+{
+	int num_colors = 0, ret;
+
+	of_property_read_string(np, "label", &cfg[led_index].name);
+
+	ret = of_property_read_u32(np, "reg", &cfg[led_index].chan_nr);

You mix code for probe with code for regular operation. This is not
expected and confusing. All functions related to probe must be before
the probe function is defined.

I will restructured the code to move all probe-related helpers above the
lp5812_probe() function.

quoted

+
+static struct lp5812_led *lp5812_dev_to_led(struct device *dev)
+{
+	struct led_classdev *cdev = dev_get_drvdata(dev);
+	const char *name = dev->platform_data;
+
+	if (strcmp(name, LP5812_SC_LED) == 0)
+		return container_of(cdev, struct lp5812_led, cdev);
+
+	return container_of((struct led_classdev_mc *)cdev, struct lp5812_led, mc_cdev);

No, just pass correct pointer to platform data, no need with strcmp and
then different container of...

I will remove the string-based `platform_data` handling and now store a direct
pointer to `struct lp5812_led` in `dev->platform_data`.
This allows simplifying `lp5812_dev_to_led()` without the need for `strcmp()`
or multiple `container_of()` usages.

quoted

+static int lp5812_init_led(struct lp5812_led *led, struct lp5812_chip *chip, int chan)
+{
+	struct device *dev = &chip->client->dev;
+	struct mc_subled *mc_led_info;
+	struct led_classdev *led_cdev;
+	int i, ret = 0;
+
+	if (chip->led_config[chan].name) {
+		led->cdev.name = chip->led_config[chan].name;
+	} else {
+		led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s:channel%d",
+						chip->label ? : chip->client->name, chan);
+		if (!led->cdev.name)
+			return -ENOMEM;
+	}
+
+	if (!chip->led_config[chan].is_sc_led) {
+		mc_led_info = devm_kcalloc(dev,
+					   chip->led_config[chan].num_colors,
+					   sizeof(*mc_led_info), GFP_KERNEL);
+		if (!mc_led_info)
+			return -ENOMEM;
+
+		led_cdev = &led->mc_cdev.led_cdev;
+		led_cdev->name = led->cdev.name;
+		led_cdev->brightness_set_blocking = lp5812_set_mc_brightness;
+		led->mc_cdev.num_colors = chip->led_config[chan].num_colors;
+		for (i = 0; i < led->mc_cdev.num_colors; i++) {
+			mc_led_info[i].color_index =
+				chip->led_config[chan].color_id[i];
+			mc_led_info[i].channel =
+					chip->led_config[chan].led_id[i];
+		}
+
+		led->mc_cdev.subled_info = mc_led_info;
+	} else {
+		led->cdev.brightness_set_blocking = lp5812_set_brightness;
+	}
+
+	led->cdev.groups = lp5812_led_groups;
+	led->chan_nr = chan;
+
+	if (chip->led_config[chan].is_sc_led) {
+		ret = devm_led_classdev_register(dev, &led->cdev);
+		if (ret == 0) {
+			led->cdev.dev->platform_data = devm_kstrdup(dev, LP5812_SC_LED, GFP_KERNEL);
+			if (!led->cdev.dev->platform_data)
+				return -ENOMEM;
+		}
+	} else {
+		ret = devm_led_classdev_multicolor_register(dev, &led->mc_cdev);
+		if (ret == 0) {
+			led->mc_cdev.led_cdev.dev->platform_data =
+				devm_kstrdup(dev, LP5812_MC_LED, GFP_KERNEL);
+			if (!led->mc_cdev.led_cdev.dev->platform_data)
+				return -ENOMEM;
+
+			ret = sysfs_create_groups(&led->mc_cdev.led_cdev.dev->kobj,
+						  lp5812_led_groups);
+			if (ret)
+				dev_err(dev, "sysfs_create_groups failed\n");
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "led register err: %d\n", ret);

Why are you printing same error multiple times?

I will remove the redundant error message at the end of `lp5812_init_led()`
to avoid double-printing.
Now only the specific failure points log detailed errors.

quoted

+static int lp5812_probe(struct i2c_client *client)
+{
+	struct lp5812_chip *chip;
+	const struct i2c_device_id *id = i2c_client_get_device_id(client);
+	struct device_node *np = dev_of_node(&client->dev);
+	struct lp5812_led *led;
+	int ret;
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->cfg = i2c_get_match_data(client);
+
+	if (np) {
+		ret = lp5812_of_populate_pdata(&client->dev, np, chip);
+		if (ret)
+			goto err_init;
+	} else {
+		return dev_err_probe(&client->dev, -EINVAL, "No platform data\n");

This is confusing syntax. Expected is:
if (!missing something)
	return -EINVAL

I will update it.

The other problem is that you claim this can match and bind without OF,
but here you say it is a requirement. So either this code is wrong or
your I2C ID table should be removed.

I will update the probe logic for clarity and removed the i2c_device_id table
and .id_table field.

quoted

+	led = devm_kcalloc(&client->dev, chip->num_channels, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	chip->client = client;
+
+	mutex_init(&chip->lock);
+
+	i2c_set_clientdata(client, led);
+
+	ret = lp5812_init_device(chip);
+	if (ret)
+		goto err_init;
+
+	dev_info(&client->dev, "%s Programmable led chip found\n", id->name);

Drop. You have only one device type (look at your binding), so you
cannot "find" devices.

I will drop it.

quoted

+static void lp5812_remove(struct i2c_client *client)
+{
+	struct lp5812_led *led = i2c_get_clientdata(client);
+
+	lp5812_unregister_sysfs(led, led->chip);
+	lp5812_deinit_device(led->chip);
+
+	dev_info(&client->dev, "Removed driver\n");

No, drop, useless.

I will remove it.

Thank you for your thorough review and helpful suggestions.

Best regards,
Nam Tran