Re: [PATCH v11 2/4] leds: add basic support for TI/National Semiconductor LP5812 LED Driver

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

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.

+	if (ret)
+		return ret;
+
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = lp5812_parse_led_channel(child, cfg, led_index, num_colors);
+		if (ret)
+			return ret;
+		num_colors++;
+	}
+
+	if (num_colors == 0) {
+		ret = lp5812_parse_led_channel(np, cfg, led_index, 0);
+		if (ret)
+			return ret;
+		num_colors = 1;
+		cfg[led_index].is_sc_led = true;
+	} else {
+		cfg[led_index].is_sc_led = false;
+	}
+
+	cfg[led_index].num_colors = num_colors;
+
+	return 0;
+}
+
+static int lp5812_of_populate_pdata(struct device *dev,
+				    struct device_node *np,
+				    struct lp5812_chip *chip)
+{
+	struct lp5812_led_config *cfg;
+	int num_channels, i = 0, ret;
+
+	num_channels = of_get_available_child_count(np);
+	if (num_channels == 0) {
+		dev_err(dev, "no LED channels\n");
+		return -EINVAL;
+	}
+
+	cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL);
+	if (!cfg)
+		return -ENOMEM;
+
+	chip->led_config = &cfg[0];
+	chip->num_channels = num_channels;
+
+	for_each_available_child_of_node_scoped(np, child) {
+		ret = lp5812_parse_led(child, cfg, i);
+		if (ret)
+			return -EINVAL;
+		i++;
+	}
+
+	of_property_read_string(np, "label", &chip->label);
+	return 0;
+}
+

This is a messy driver :/

....

+
+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...


...

+
+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?

+		return ret;
+	}
+
+	return 0;
+}
+
+static int lp5812_register_leds(struct lp5812_led *led, struct lp5812_chip *chip)
+{
+	struct lp5812_led *each;
+	int num_channels = chip->num_channels;
+	int ret, i, j;
+
+	for (i = 0; i < num_channels; i++) {
+		each = led + i;
+		ret = lp5812_init_led(each, chip, i);
+		if (ret)
+			goto err_init_led;
+
+		each->chip = chip;
+
+		for (j = 0; j < chip->led_config[i].num_colors; j++) {
+			ret = lp5812_auto_dc(chip, chip->led_config[i].led_id[j],
+					     chip->led_config[i].led_id[j]);
+			if (ret)
+				goto err_init_led;
+
+			ret = lp5812_set_led_mode(chip, chip->led_config[i].led_id[j],
+						  LP5812_MODE_MANUAL);
+			if (ret)
+				goto err_init_led;
+		}
+	}
+
+	return 0;
+
+err_init_led:
+	return ret;
+}
+
+static int lp5812_register_sysfs(struct lp5812_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	const struct lp5812_device_config *cfg = chip->cfg;
+	int ret;
+
+	ret = sysfs_create_group(&dev->kobj, cfg->dev_attr_group);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void lp5812_unregister_sysfs(struct lp5812_led *led, struct lp5812_chip *chip)
+{
+	struct device *dev = &chip->client->dev;
+	const struct lp5812_device_config *cfg = chip->cfg;
+	struct lp5812_led *each;
+	int i;
+
+	sysfs_remove_group(&dev->kobj, cfg->dev_attr_group);
+
+	for (i = 0; i < chip->num_channels; i++) {
+		if (!chip->led_config[i].is_sc_led) {
+			each = led + i;
+			sysfs_remove_groups(&each->mc_cdev.led_cdev.dev->kobj, lp5812_led_groups);
+		}
+	}
+}
+
+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

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.

+	}
+
+	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.

+
+	ret = lp5812_register_leds(led, chip);
+	if (ret)
+		goto err_out;
+
+	ret = lp5812_register_sysfs(chip);
+	if (ret) {
+		dev_err_probe(&client->dev, ret, "Registering sysfs failed\n");
+		goto err_out;
+	}
+
+	return 0;
+
+err_out:
+	lp5812_deinit_device(chip);
+err_init:
+	return ret;
+}
+
+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.


Best regards,
Krzysztof