Re: [PATCH v6 2/5] leds: add TI/National Semiconductor LP5812 LED Driver

[Nam Tran <trannamatk@gmail.com>]

On Sun, 20 Apr 2025, Christophe JAILLET wrote:

> Le 19/04/2025 à 20: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.
> > Each LED can be configured through the related registers
> > to realize vivid and fancy lighting effects.
> 
> ...
> 
> > +static int lp5812_init_dev_config(struct lp5812_chip *chip,
> > +		const char *drive_mode, int rm_led_sysfs);
> > +
> > +static struct drive_mode_led_map chip_leds_map[] = {
> 
> I think this could be const.

I’ll update chip_leds_map to be const.

> > +static int lp5812_get_phase_align(struct lp5812_chip *chip, int led_number,
> > +		int *phase_align_val)
> > +{
> > +	int ret;
> > +	int bit_pos;
> > +	u16 reg;
> > +	u8 reg_val;
> > +
> > +	reg = DEV_CONFIG7 + (led_number / 4);
> > +	bit_pos = (led_number % 4) * 2;
> > +
> > +	ret = lp5812_read(chip, reg, &reg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*phase_align_val = (reg_val >> bit_pos) & 0x03;
> > +
> > +	return ret;
> > +}
> > +
> > +static int lp5812_get_led_mode(struct lp5812_chip *chip,
> > +		int led_number, enum control_mode *mode)
> > +{
> > +	int ret = 0;
> 
> In several function, sometimes ret is initialized, sometimes it is not.
> See lp5812_get_led_mode() and lp5812_get_phase_align() just above.

Agreed, I’ll go through and update these functions to follow a consistent pattern.

> > +static void set_mix_sel_led(struct lp5812_chip *chip, int mix_sel_led)
> > +{
> 
> Maybe init the 4 values at 0 first, then set to 1 only what is needed 
> below? This would save a few lines of code.
> 
> > +	if (mix_sel_led == 0) {
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = 1;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = 0;
> > +	}
> > +	if (mix_sel_led == 1) {
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = 1;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = 0;
> > +	}
> > +	if (mix_sel_led == 2) {
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = 1;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = 0;
> > +	}
> > +	if (mix_sel_led == 3) {
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = 0;
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = 1;
> > +	}
> > +}

Yep, that’s a cleaner approach. I’ll update it accordingly.

> > +static ssize_t dev_config_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> 
> The whole function could be simplified with sysfs_emit_at().
> This avoids temp buffer, malloc/free and some copies.
> 
> See led_auto_animation_show() below.
> 
> > +	int i;
> > +	int num_drive_mode;
> > +	char *mode_info;
> > +	char *total_str;
> > +	size_t total_length;
> > +	char *const_str = "\nPlease select below valid drive mode:\n";
> > +	char *const_ex_str = "For Ex: echo tcmscan:1:0 > dev_config\n";
> > +	int ret = 0;
> > +	struct lp5812_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
> > +
> > +	/* get drive mode and scan order */
> > +	mutex_lock(&chip->lock);
> > +	ret = lp5812_get_drive_mode_scan_order(chip);
> > +	mutex_unlock(&chip->lock);
> > +	if (ret)
> > +		return -EIO;
> > +
> > +	mode_info = parse_dev_config_info(chip);
> > +	if (!mode_info)
> > +		return -ENOMEM;
> > +
> > +	num_drive_mode = ARRAY_SIZE(chip_leds_map);
> > +	total_length = strlen(mode_info) + strlen(const_str) +
> > +			strlen(const_ex_str) + 1;
> > +	for (i = 0; i < num_drive_mode; ++i) {
> > +		total_length += strlen(chip_leds_map[i].drive_mode) +
> > +					strlen("\n");
> > +	}
> > +
> > +	total_str = kmalloc(total_length, GFP_KERNEL);
> > +	if (!total_str)
> > +		return -ENOMEM;
> > +
> > +	sprintf(total_str, "%s%s%s", mode_info, const_str, const_ex_str);
> > +	for (i = 0; i < num_drive_mode; ++i) {
> > +		strcat(total_str, chip_leds_map[i].drive_mode);
> > +		strcat(total_str, "\n");
> > +	}
> > +
> > +	ret = sysfs_emit(buf, "%s", total_str);
> > +	kfree(mode_info);
> > +	kfree(total_str);
> > +
> > +	return ret;
> > +}

...

> In order to have it more readable (IMHO), use less buffers, make less 
> copies, reduce code duplication and reduce the locking section, maybe 
> something like (un-tested):
> 
> static ssize_t led_auto_animation_show(struct kobject *kobj,
> 		struct kobj_attribute *attr, char *buf)
> {
> 	int aeu_selection, playback_time, start_pause, stop_pause;
> 	struct lp5812_led *led = to_lp5812_led(kobj);
> 	struct lp5812_chip *chip = led->priv;
> 	int pos = 0;
> 	int ret;
> 
> 	mutex_lock(&chip->lock);
> 	ret = led_get_autonomous_animation_config(led);
> 	if (ret) {
> 		ret = -EIO;
> 		goto out;
> 	}
> 
> 	/* parse config and feedback to userspace */
> 	aeu_selection = led->led_playback.s_led_playback.aeu_selection;
> 	playback_time = led->led_playback.s_led_playback.led_playback_time;
> 	start_pause = led->start_stop_pause_time.s_time.second;
> 	stop_pause = led->start_stop_pause_time.s_time.first;
> 
> 	mutex_unlock(&chip->lock);
> 
> 	pos += sysfs_emit_at(buf, pos, "AEU Select: ");
> 	if (aeu_selection == ONLY_AEU1)
> 		pos += sysfs_emit_at(buf, pos, "Only use AEU1");
> 	else if (aeu_selection == AEU1_AEU2)
> 		pos += sysfs_emit_at(buf, pos, "Use AEU1 and AEU2");
> 	else
> 		pos += sysfs_emit_at(buf, pos, "Use AEU1, AEU2 and AEU3");
> 
> 	pos += sysfs_emit_at(buf, pos, "; Start pause time: %s",
> 			     time_name_array[start_pause]);
> 	pos += sysfs_emit_at(buf, pos, "; Start pause time: %s",
> 			     time_name_array[start_pause]);
> 	pos += sysfs_emit_at(buf, pos, "; LED Playback time: %s",
> 			     led_playback_time_arr[playback_time]);
> 
> 	pos += sysfs_emit_at(buf, pos, "\n");
> 	pos += sysfs_emit_at(buf, pos, "Command usage: echo (aeu number):(start 
> pause time):(stop pause time):(playback time) > autonomous_animation\n");
> 
> 	return pos;
> 
> out:
> 	mutex_unlock(&chip->lock);
> 	return ret;
> }

Great point! I'll refactor these functions as suggested.

> > +static ssize_t aeu_playback_time_show(struct kobject *kobj,
> > +		struct kobj_attribute *attr, char *buf)
> > +{
> > +	int ret = 0;
> > +	u8 val = 0;
> > +	struct anim_engine_unit *aeu = to_anim_engine_unit(kobj);
> > +	struct lp5812_chip *chip = aeu->led->priv;
> > +
> > +	mutex_lock(&chip->lock);
> > +	ret = led_aeu_playback_time_get_val(aeu, &val);
> 
> Maybe unlock here, to simplify code and be consistent with some other 
> functions above? (led_pwm_dimming_scale_show(), ...)
> 
> Several other show/store function could be slightly simplified the same way.
> 
> > +	if (ret != 0) {
> > +		mutex_unlock(&chip->lock);
> > +		return -EIO;
> > +	}
> > +	mutex_unlock(&chip->lock);
> > +
> > +	return sysfs_emit(buf, "%d\n", val);
> > +}

Agreed, I’ll unlock earlier to simplify the code and maintain consistency with other functions.

> > +struct lp5812_led {
> > +	struct kobject                        kobj;
> > +	struct lp5812_chip                    *priv;
> > +	struct attribute_group                attr_group;
> > +	int                                   enable;
> > +	enum control_mode                     mode;
> > +	enum dimming_type                     dimming_type;
> > +	u8                                    lod_lsd;
> > +	u8                                    auto_pwm;
> > +	u8                                    aep_status;
> > +	u16                                   anim_base_addr;
> > +	int                                   led_number; /* start from 0 */
> > +	int                                   is_sysfs_created;
> > +	const char                            *led_name;
> > +
> > +	union led_playback                    led_playback;
> > +	union time                            start_stop_pause_time;
> > +
> > +	int                                   total_aeu;
> 
> What is the need to keeping it here?
> It is set to MAX_AEU. Why not just use it directly?
> 
> If needed for future use, maybe, 'aeu' below should be a flex array?
> 
> > +	struct anim_engine_unit               aeu[MAX_AEU];
> > +};
> > +
> > +struct lp5812_chip {
> > +	struct i2c_client                     *i2c_cl;
> > +	struct mutex                          lock; /* Protects access to device registers */
> > +	struct device                         *dev;
> > +	struct attribute_group                attr_group;
> > +	const struct lp5812_specific_regs     *regs;
> > +	const struct drive_mode_led_map       *chip_leds_map;
> > +	enum device_command                   command;
> > +	int                                   total_leds;
> 
> What is the need to keeping it here?
> It is set to MAX_LEDS. Why not just use it directly?
> 
> If needed for future use, maybe, 'leds' below should be a flex array?

You're right — since total_aeu and total_leds are always set to MAX_AEU and MAX_LEDS, respectively,
there's no need to store them separately. I'll remove those fields and use the constants directly.

We'll keep the static arrays as-is, given the fixed hardware limits.

Thanks for the helpful comments!

Best regards,
Nam Tran