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
