On Tue, 10 Jun 2025, Christophe JAILLET wrote:
> > +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?
Thanks, I'll switch to for_each_available_child_of_node_scoped().
> > +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?
Yes. I'll add a return check to handle possible write errors.
> > +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.
I'll remove it.
> > +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?
You're right, name was stack-allocated and unsafe to use after the function returns.
I'll replace it with a devm_kasprintf() allocation.
Appreciate your time and feedback.
Best regards,
Nam Tran
