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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

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.

> +	{
> +		"direct_mode",
> +		(const char *[]){LED0, LED1, LED2, LED3, NULL}
> +	},

...

> +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.

This could be done in a more consistent way.

> +	u16 reg;
> +	u8 reg_val;
> +
> +	if (led_number < 0x8)
> +		reg = DEV_CONFIG3;
> +	else
> +		reg = DEV_CONFIG4;
> +
> +	ret = lp5812_read(chip, reg, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	*mode = (reg_val & (1 << (led_number % 8))) ? AUTONOMOUS : MANUAL;
> +
> +	return 0;
> +}

...

> +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;
> +	}
> +}

...

> +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;
> +}

...

> +static ssize_t led_auto_animation_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	int ret;
> +	char tmp_str[256] = {};
> +	char usage[128] = {};
> +	char *aeu_select = "AEU Select: ";
> +	char *start_pause_time = "Start pause time: ";
> +	char *stop_pause_time = "; Stop pause time: ";
> +	char *led_playback_time = "; LED Playback time: ";
> +	int aeu_selection, playback_time, start_pause, stop_pause;
> +	struct lp5812_led *led = to_lp5812_led(kobj);
> +	struct lp5812_chip *chip = led->priv;
> +
> +	sprintf(usage, "%s%s",
> +	"Command usage: echo (aeu number):(start pause time):",
> +	"(stop pause time):(playback time) > autonomous_animation");
> +
> +	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;
> +	if (aeu_selection == ONLY_AEU1) {
> +		sprintf(tmp_str, "%s%s%s%s%s%s%s%s\n", aeu_select,
> +		"Only use AEU1; ", start_pause_time,
> +		time_name_array[start_pause], stop_pause_time,
> +		time_name_array[stop_pause], led_playback_time,
> +		led_playback_time_arr[playback_time]);
> +	} else if (aeu_selection == AEU1_AEU2) {
> +		sprintf(tmp_str, "%s%s%s%s%s%s%s%s\n", aeu_select,
> +		"Use AEU1 and AEU2; ", start_pause_time,
> +		time_name_array[start_pause], stop_pause_time,
> +		time_name_array[stop_pause], led_playback_time,
> +		led_playback_time_arr[playback_time]);
> +	} else {
> +		sprintf(tmp_str, "%s%s%s%s%s%s%s%s\n", aeu_select,
> +		"Use AEU1,AEU2 and AEU3; ", start_pause_time,

Space missing after the , ?

> +		time_name_array[start_pause], stop_pause_time,
> +		time_name_array[stop_pause], led_playback_time,
> +		led_playback_time_arr[playback_time]);
> +	}
> +	strcat(tmp_str, usage);
> +	mutex_unlock(&chip->lock);
> +	return sysfs_emit(buf, "%s\n", tmp_str);
> +
> +out:
> +	mutex_unlock(&chip->lock);
> +	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;
}

...

> +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);
> +}

...

> +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?

> +	union scan_order                      u_scan_order;
> +	union drive_mode_info                 u_drive_mode;
> +
> +	struct lp5812_led                     leds[MAX_LEDS]; /* MAX 16 LEDs */

This comment looks useless, IMHO.

> +};
> +
> +#endif /*_LEDS_LP5812_H_*/