linux-kernel - Re: [PATCH v3 3/3] leds: add new LED driver for TI LP5812

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20250318133508.4531-1-trannamatk@gmail.com>
Date: Tue, 18 Mar 2025 20:35:08 +0700
From: Nam Tran <trannamatk@...il.com>
To: krzk+dt@...nel.org
Cc: pavel@...nel.org,
	lee@...nel.org,
	robh@...nel.org,
	conor+dt@...nel.org,
	devicetree@...r.kernel.org,
	linux-leds@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] leds: add new LED driver for TI LP5812

From: Nam Tran <trannamatk@...il.com>
To: Krzysztof Kozlowski <krzk+dt@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org> Pavel Machek <pavel@...nel.org>, Lee Jones <lee@...nel.org>, linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org

I sincerely apologize for not addressing all of your previous comments earlier. That was not my intention, and I truly appreciate the time and effort you have put into reviewing my patch. Below, I would like to properly address your concerns.

On Fri, Mar 07, 2025 at 12:21:26AM +0700, Nam Tran wrote:
> The chip can drive LED matrix 4x3.
> This driver enables LED control via I2C.

You still did not respond to comments from v1. I don't see it being addressed.

Nam: I am sorry. This is my mistake. I think that I just need to update source code based on your comments and submit a new patch. This is the first time I try to update a new thing to the Linux Kernel. I will give answer inline your message for tracing easily.

In previous comment you have a question
 "Why none of the 10 existing drivers fit for your device?"

Nam: I have carefully reviewed the existing LED drivers in the kernel, but none of them fully support the advanced capabilities of the LP5812. Unlike basic LED controllers, the LP5812 has difference features and register
For more detail, please refer to this documentation https://www.ti.com/lit/ds/symlink/lp5812.pdf?ts=1741765622088&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FLP5812

>
> The driver is implemented in two parts:
> - Core driver logic in leds-lp5812.c
> - Common support functions in leds-lp5812-common.c

Why do you make two modules? This looks really unneccessary. Just compile them into one module. No, use proper kerneldoc Missing kerneldoc. Every exported symbol must have kerneldoc. Why this is not static?

Nam: I will merge source code into a file only. Therefore, I don’t need to export symbols here.

> +{
> +     int ret;
> +     u8 tmp;
> +
> +     ret = lp5812_read(chip, reg, &tmp);
> +     if (ret)
> +             return ret;
> +
> +     tmp &= ~mask;
> +     tmp |= val & mask;
> +
> +     return lp5812_write(chip, reg, tmp);
> +}
> +
> +/*
> + * Function: lp5812_read_tsd_config_status
> + * Description: read tsd config status register
> + * Param: chip --> struct lp5812_chip itself
> + *        reg_val
> + * Return: 0 if success
> + */
> +int lp5812_read_tsd_config_status(struct lp5812_chip *chip, u8 *reg_val)

Why this is not static?

Nam: I will change it to a static function.

> +{
> +     int ret = 0;
> +
> +     if (!reg_val)
> +             return -1;
> +
> +     ret = lp5812_read(chip, chip->regs->tsd_config_status_reg, reg_val);
> +
> +     return ret;
> +}
> +
> +/*
> + * Function: lp5812_update_regs_config

Missing kerneldoc

Nam: If I merge leds-lp5812-common.c into leds-lp5812.c, the functions will no longer need to be exported, as they will only be used internally within a single compilation unit. As a result, kernel doc for these symbols will no longer be mandatory. Would you agree that this approach eliminates the need for separate kernel doc entries for these functions?

> + * Description: update reg config register
> + * Param: chip --> struct lp5812_chip itself
> + * Return: 0 if success
> + */
> +int lp5812_update_regs_config(struct lp5812_chip *chip)
> +{
> +     int ret;
> +     u8 reg_val; /* save register value */
> +

...

> +static struct drive_mode_led_map chip_leds_map[] = {
> +     {
> +             "direct_mode",
> +             (const char *[]){LED0, LED1, LED2, LED3, NULL},

Drop the cast.
Nam: I will drop them.

> +     },
> +     {
> +             "tcmscan:1:0", /* tcm 1 scan; scan order 0 out0 */
> +             (const char *[]){LED_A0, LED_A1, LED_A2, NULL},
> +     },
> +     {
> +             "tcmscan:1:1", /* tcm 1 scan; scan order 0 out1 */
> +             (const char *[]){LED_B0, LED_B1, LED_B2, NULL},
> +     },
> +     {
> +             "tcmscan:1:2", /* tcm 1 scan; scan order 0 out2 */
> +             (const char *[]){LED_C0, LED_C1, LED_C2, NULL},
> +     },
> +     {
> +             "tcmscan:1:3", /* tcm 1 scan; scan order 0 out3 */
> +             (const char *[]){LED_D0, LED_D1, LED_D2, NULL},


What is all this here?

How LED controller with so little properties have so complicated driver?

Nam: This hardware has 7 modes. Each mode has many options. My idea is to create a driver that supports all capabilities of the hardware. Therefore, when the user updates the mode of hardware, it will create specific device files.
Refer: https://dev.ti.com/gallery/view/LED/LP581x/ver/0.16.0/

> +     },
> +     { /* tcm 2 scan, scan order 0 out0; scan order 1 out1 */
> +             "tcmscan:2:0:1",
> +             (const char *[]){LED_A0, LED_A1, LED_A2, LED_B0, LED_B1, LED_B2,
> +             NULL},
> +     },
> +     { /* tcm 2 scan, scan order 0 out0; scan order 1 out2 */
> +             "tcmscan:2:0:2",
> +             (const char *[]){LED_A0, LED_A1, LED_A2, LED_C0, LED_C1, LED_C2,
> +             NULL},


> +static void led_kobj_release(struct kobject *kobj)
> +{
> +     kfree(kobj);
> +}
> +
> +static void aeu_kobj_release(struct kobject *kobj)
> +{
> +     kfree(kobj);
> +}

What is all this? Why do you create your own kobjects?

Nam: Same as previous answer, I need to create kobjects to support many interfaces.

> +
> +static const struct kobj_type led_ktype = {
> +     .release = led_kobj_release,
> +     .sysfs_ops = &kobj_sysfs_ops,
> +};
> +
> +static const struct kobj_type aeu_ktype = {
> +     .release = aeu_kobj_release,
> +     .sysfs_ops = &kobj_sysfs_ops,
> +};


> +static ssize_t device_reset_store(struct device *dev,
> +             struct device_attribute *attr,
> +             const char *buf, size_t len)

NAK.

Sorry, you just cannot create whatever interfaces you want. You must use
standard LED interfaces for normal LED operations. None of these sysfs
are needed for device control.

Respond to this comment that you understood it (you ignored all previous
comments). …

Nam: based on LP5812 datasheet, it is not normal LED operations. It supports many modes, each mode we can control specific leds (user can enable/disable leds). Each led has normal mode, autonomous mode … That is why I want to create interfaces.

Based on your concern, I think that my driver may not be compatible with normal LED operations. Given that the LP5812 driver has advanced functionality beyond basic LED control, would it be acceptable to place the driver in a different location instead of drivers/led/? If so, could you suggest a more suitable location?

I am open to discussions on how best to integrate these features while maintaining compliance with kernel standards. Please let me know your thoughts on how we can refine the approach.

> +static LP5812_KOBJ_ATTR_RW(pwm1, aeu_pwm1_show, aeu_pwm1_store);
> +static LP5812_KOBJ_ATTR_RW(pwm2, aeu_pwm2_show, aeu_pwm2_store);
> +static LP5812_KOBJ_ATTR_RW(pwm3, aeu_pwm3_show, aeu_pwm3_store);
> +static LP5812_KOBJ_ATTR_RW(pwm4, aeu_pwm4_show, aeu_pwm4_store);
> +static LP5812_KOBJ_ATTR_RW(pwm5, aeu_pwm5_show, aeu_pwm5_store);
> +static LP5812_KOBJ_ATTR_RW(slope_time_t1, aeu_slope_time_t1_show,
> +             aeu_slope_time_t1_store);
> +static LP5812_KOBJ_ATTR_RW(slope_time_t2, aeu_slope_time_t2_show,
> +             aeu_slope_time_t2_store);
> +static LP5812_KOBJ_ATTR_RW(slope_time_t3, aeu_slope_time_t3_show,
> +             aeu_slope_time_t3_store);
> +static LP5812_KOBJ_ATTR_RW(slope_time_t4, aeu_slope_time_t4_show,
> +             aeu_slope_time_t4_store);
> +static LP5812_KOBJ_ATTR_RW(playback_time, aeu_playback_time_show,
> +             aeu_playback_time_store);


What is all this?

Nam: In autonomous mode, we can blink leds with specific PWM, delay, slope …
Refer: chapter 7.3.4 from https://www.ti.com/lit/ds/symlink/lp5812.pdf?ts=1741765622088&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FLP5812

> +static int lp5812_probe(struct i2c_client *client)
> +{
> +     struct lp5812_chip *chip;
> +     int i; > +             aeu_init_properties(&chip->leds[i]);
> +
> +             /* set autonomous animation config as default for all LEDs */
> +             led_set_autonomous_animation_config(&chip->leds[i]);
> +     }
> +
> +     i2c_set_clientdata(client, chip);
> +
> +     ret = sysfs_create_group(&chip->dev->kobj, &chip->attr_group);


You need sysfs ABI documentation.

Nam: I understand that since the driver creates a sysfs attribute group using sysfs_create_group(), corresponding sysfs ABI documentation is required.
I will add a new entry under Documentation/ABI/testing/ to document the exposed sysfs attributes for the LP5812 driver in the next patch.

Best regards,
Nam