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

[Nam Tran <trannamatk@gmail.com>]

On 18 Mar 2025 19:54:15, Krzysztof Kozlowski wrote:

>> 
>> 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
>
>So you read my question and decided to not give an answer. Great.

I am sorry, this was my mistake. I thought I only needed to update the source code based on your comments and submit a new patch.
I will make sure to respond to your comments more promptly in the future.

>The burden of proving this is on you. Do not expect me to read this and
>10 other datasheets for existing LP devices. Maybe they fit, maybe they
>don't but based on style of this submission and style of the code I feel
>you just want to push your patches instead of integrate.
>
>That's not how it works.

I appreciate your feedback. I will provide a more detailed explanation rather than just linking the datasheet.

LP5812 is a new LED driver family. LP5812 has different registers for new features such as run mode, config AEU module...
These are not supported in the existing drivers, which are tailored for different device architectures or use cases.

The LP5812 uses a register map and control scheme that differs significantly from other LPxxxx LED drivers.
This required a custom implementation to manage device mode, select LEDs in each mode, configure autonomous AEU...

Integrating the LP5812 into an existing driver would introduce significant complexity,
as it would require modifications that could compromise the performance or maintainability of the existing codebase.
Developing a dedicated driver ensures clean, maintainable, and optimized code.

By creating a separate driver for the LP5812, we ensure that it can fully exploit its capabilities while maintaining compatibility with the Linux driver framework.
This approach also facilitates scalability for future revisions of this or similar devices.

>> 
>>>
>>> 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.
>
> I cannot parse this. Use standard email quotes and replies. Like every
> email client since 30 years.

I apologize for the formatting issue in my previous response. I will ensure that my future responses follow standard email quoting for better readability.
For my remaining unreadable responses, I will correct them and reply to the corresponding emails.

Best regards,
Nam Tran