| 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: <42676fe3-8758-42ea-8e21-9c3955468c78@wanadoo.fr>
Date: Tue, 10 Jun 2025 23:07:59 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Nam Tran <trannamatk@...il.com>, lee@...nel.org
Cc: pavel@...nel.org, 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 v9 2/4] leds: add TI/National Semiconductor LP5812 LED
Driver
Le 10/06/2025 à 19:43, Nam Tran a écrit :
> The LP5812 is a 4×3 matrix RGB LED driver with an autonomous animation
> engine and time-cross-multiplexing (TCM) support for up to 12 LEDs or
> 4 RGB LEDs. Each LED can be configured through the related registers
> to realize vivid and fancy lighting effects.
Hi,
...
> +static struct lp5812_data *lp5812_of_populate_pdata(struct device *dev,
> + struct device_node *np,
> + struct lp5812_chip *chip)
> +{
> + struct device_node *child;
> + struct lp5812_data *pdata;
> + struct lp5812_led_config *cfg;
> + int num_channels, i = 0, ret;
> +
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + num_channels = of_get_available_child_count(np);
> + if (num_channels == 0) {
> + dev_err(dev, "no LED channels\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL);
> + if (!cfg)
> + return ERR_PTR(-ENOMEM);
> +
> + pdata->led_config = &cfg[0];
> + pdata->num_channels = num_channels;
> +
> + for_each_available_child_of_node(np, child) {
Maybe for_each_available_child_of_node_scoped() to slihtly simplify the
code?
> + ret = lp5812_parse_logical_led(child, cfg, i);
> + if (ret) {
> + of_node_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + i++;
> + }
> +
> + of_property_read_string(np, "label", &pdata->label);
> +
> + return pdata;
> +}
...
> +static ssize_t lp5812_aeu_slope_time(struct device *dev,
> + struct device_attribute *attr,
> + enum slope_time_num slope_chan,
> + const char *buf, size_t len)
> +{
> + struct lp5812_led *led;
> + struct lp5812_chip *chip;
> + struct lp5812_led_config *led_cfg;
> + const char *name = dev->platform_data;
> + int val[LED_COLOR_ID_MAX];
> + u8 chan_nr = 0;
> + char *sub_str, *str = (char *)buf;
> + int i, ret, aeu;
> + union slope_time slope_time_val;
> + u16 reg;
> +
> + if (strcmp(name, LP5812_SC_LED) == 0)
> + led = dev_to_lp5812_led(dev);
> + else
> + led = dev_to_lp5812_led_mc(dev);
> +
> + chan_nr = led->chan_nr;
> + chip = led->chip;
> + led_cfg = &chip->pdata->led_config[chan_nr];
> +
> + sub_str = strsep(&str, ":");
> + if (!sub_str)
> + return -EINVAL;
> + if (kstrtoint(&sub_str[3], 0, &aeu))
> + return -EINVAL;
> +
> + pr_info("AEU = %d", aeu);
> +
> + guard(mutex)(&chip->lock);
> + for (i = 0; i < led_cfg->num_colors; i++) {
> + sub_str = strsep(&str, " ");
> + if (!sub_str)
> + return -EINVAL;
> + if (kstrtoint(sub_str, 0, &val[i]))
> + return -EINVAL;
> + if (val[i] < 0 || val[i] > 15)
> + return -EINVAL;
> +
> + reg = LP5812_AEU_SLOPE_TIME_ADDR(led_cfg->led_id[i], aeu, slope_chan);
> +
> + /* get original value of slope time */
> + ret = lp5812_read(chip, reg, &slope_time_val.time_val);
> + if (ret)
> + return ret;
> +
> + /* Update new value for slope time*/
> + if (slope_chan == LP5812_SLOPE_TIME_T1 || slope_chan == LP5812_SLOPE_TIME_T3)
> + slope_time_val.s_time.first = val[i];
> + if (slope_chan == LP5812_SLOPE_TIME_T2 || slope_chan == LP5812_SLOPE_TIME_T4)
> + slope_time_val.s_time.second = val[i];
> +
> + /* Save updated value to hardware */
> + ret = lp5812_write(chip, reg, slope_time_val.time_val);
Should we do something if ret != 0?
> + }
> +
> + return len;
> +}
...
> +static struct attribute *lp5812_led_attrs[] = {
> + &dev_attr_led_current.attr,
> + &dev_attr_max_current.attr,
> + &dev_attr_mode.attr,
> + &dev_attr_activate.attr,
> + &dev_attr_pwm_dimming_scale.attr,
> + &dev_attr_pwm_phase_align.attr,
> + &dev_attr_auto_time_pause_at_start.attr,
> + &dev_attr_auto_time_pause_at_stop.attr,
> + &dev_attr_auto_playback_eau_number.attr,
> + &dev_attr_auto_playback_time.attr,
> + &dev_attr_aeu_playback_time.attr,
> + &dev_attr_aeu_pwm1.attr,
> + &dev_attr_aeu_pwm2.attr,
> + &dev_attr_aeu_pwm3.attr,
> + &dev_attr_aeu_pwm4.attr,
> + &dev_attr_aeu_pwm5.attr,
> + &dev_attr_aeu_slop_time_t1.attr,
> + &dev_attr_aeu_slop_time_t2.attr,
> + &dev_attr_aeu_slop_time_t3.attr,
> + &dev_attr_aeu_slop_time_t4.attr,
> + &dev_attr_lod_lsd.attr,
> + NULL,
Unneeded trailing comma after a terminator.
> +};
> +ATTRIBUTE_GROUPS(lp5812_led);
...
> +static int lp5812_init_led(struct lp5812_led *led, struct lp5812_chip *chip, int chan)
> +{
> + struct lp5812_data *pdata = chip->pdata;
> + struct device *dev = &chip->i2c_cl->dev;
> + struct mc_subled *mc_led_info;
> + struct led_classdev *led_cdev;
> + char name[32];
> + int i, ret = 0;
> +
> + if (pdata->led_config[chan].name) {
> + led->cdev.name = pdata->led_config[chan].name;
> + } else {
> + snprintf(name, sizeof(name), "%s:channel%d",
> + pdata->label ? : chip->i2c_cl->name, chan);
> + led->cdev.name = name;
Is it fine below when 'name' is defined on the stack and is used...
> + }
> +
> + if (pdata->led_config[chan].is_sc_led == 0) {
> + mc_led_info = devm_kcalloc(dev,
> + pdata->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;
...here?
> + led_cdev->brightness_set_blocking = lp5812_set_mc_brightness;
> + led->mc_cdev.num_colors = pdata->led_config[chan].num_colors;
> + for (i = 0; i < led->mc_cdev.num_colors; i++) {
> + mc_led_info[i].color_index =
> + pdata->led_config[chan].color_id[i];
> + mc_led_info[i].channel =
> + pdata->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 (pdata->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);
> + return ret;
> + }
> +
> + return 0;
> +}
...
CJ
Powered by blists - more mailing lists