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