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

[Christophe JAILLET]

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