Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 10/27/20 10:28 AM, Gene Chen wrote:
> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月21日 週三 上午5:47寫道：
>>
>> On 10/20/20 8:44 AM, Gene Chen wrote:
>>> Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月9日 週五 上午5:51寫道：
>>>>
>>>> Hi Gene,
>>>>
>>>> On 10/7/20 3:42 AM, Gene Chen wrote:
>>>>> From: Gene Chen <gene_chen@richtek.com>
>>>>>
>>>>> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
>>>>> 3-channel RGB LED support Register/Flash/Breath Mode, and 1-channel for
>>>>> moonlight LED.
>>>>>
>>>>> Signed-off-by: Gene Chen <gene_chen@richtek.com>
>>>>> ---
>>>>>     drivers/leds/Kconfig       |  12 +
>>>>>     drivers/leds/Makefile      |   1 +
>>>>>     drivers/leds/leds-mt6360.c | 783 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>     3 files changed, 796 insertions(+)
>>>>>     create mode 100644 drivers/leds/leds-mt6360.c
>>>>>
>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>> index 1c181df..c7192dd 100644
>>>>> --- a/drivers/leds/Kconfig
>>>>> +++ b/drivers/leds/Kconfig
>>>>> @@ -271,6 +271,18 @@ config LEDS_MT6323
>>>>>           This option enables support for on-chip LED drivers found on
>>>>>           Mediatek MT6323 PMIC.
>>>>>
>>>>> +config LEDS_MT6360
>>>>> +     tristate "LED Support for Mediatek MT6360 PMIC"
>>>>> +     depends on LEDS_CLASS_FLASH && OF
>>>>> +     depends on LEDS_CLASS_MULTICOLOR
>>>>
>>>> Since CONFIG_LED_CLASS_MULTICOLOR can be turned off you need to have
>>>> below instead:
>>>>
>>>> depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR

My typo here, should be one "!":

depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR

And you should also have

depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH

But to make it work correctly you would have to add registration
stubs to include/linux/led-class-flash.h similarly to LED mc stubs
in include/linux/led-class-multicolor.h.

>>>>
>>>> Unless you want to prevent enabling the driver without RGB LED,
>>>> but that does not seem to be reasonable at first glance.
>>>>
>>>
>>> May I change to "select LEDS_CLASS_MULTICOLOR"?
>>> I suppose RGB always use multicolor mode.
>>
>> You will also have moonlight LED that will not need multicolor
>> framework. Is it somehow troublesome to keep "depends on"?
>>
> 
> If only use ML LED and FLED,  DTSI will only define ML LED and FLED.
> Therefore, the drivers probe will not register rgb multicolor device.

Please test your use case again with my fixed "depends on".

In case when there is only ML LED and FLED in the DT it should
register both devices if LEDS_CLASS_FLASH is turned on.
Multicolor framework has nothing to do in this case.

But if you additionally had MC LED node, then it should
be registered only if LEDS_CLASS_MULTICOLOR is enabled.

Similarly, when FLED node is present, but LEDS_CLASS_FLASH
is off, and LEDS_CLASS_MULTICOLOR is on, the driver should still
compile, but register only LED MC device (if its node is present).

Possible should be also the case when both LEDS_CLASS_FLASH
and LEDS_CLASS_MULTICOLOR are off. Then only LED class device
for ML LED will be registered (provided there is ML DT node).
But to make it possible you should have also "depends on LEDS_CLASS"
in the Kconfig entry.

> I will remove "depends", use "select" instead.

-- 
Best regards,
Jacek Anaszewski