| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0c7e171d-056d-4c00-a30b-0fd39e25bf4c@kernel.org>
Date: Fri, 18 Jul 2025 10:04:34 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nam Tran <trannamatk@...il.com>, lee@...nel.org
Cc: pavel@...nel.org, rdunlap@...radead.org, christophe.jaillet@...adoo.fr,
krzk+dt@...nel.org, robh@...nel.org, conor+dt@...nel.org, corbet@....net,
linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-doc@...r.kernel.org
Subject: 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:
>> +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
Powered by blists - more mailing lists