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

[Nam Tran <trannamatk@gmail.com>]

On Fri, 18 Jul 2025, Krzysztof Kozlowski wrote:

> On 14/07/2025 19:23, Nam Tran wrote:
> >> +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.

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

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

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

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

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