Re: [PATCH v9 2/4] leds: add TI/National Semiconductor LP5812 LED Driver

[Nam Tran <trannamatk@gmail.com>]

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