| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20250421022129.3384-1-trannamatk@gmail.com>
Date: Mon, 21 Apr 2025 09:21:29 +0700
From: Nam Tran <trannamatk@...il.com>
To: christophe.jaillet@...adoo.fr
Cc: pavel@...nel.org,
lee@...nel.org,
krzk+dt@...nel.org,
robh@...nel.org,
conor+dt@...nel.org,
corbet@....net,
devicetree@...r.kernel.org,
linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/5] leds: add TI/National Semiconductor LP5812 LED Driver
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, ®_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
Powered by blists - more mailing lists