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

[Gene Chen]

Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 於 2020年10月28日 週三 上午1:28寫道：
>
> On 10/27/20 10:28 AM, Gene Chen wrote:
> > Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 於 2020年10月21日 週三 上午5:47寫道：
> >>
> >> On 10/20/20 8:44 AM, Gene Chen wrote:
> >>> Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 於 2020年10月9日 週五 上午5:51寫道：
> >>>>
> >>>> Hi Gene,
> >>>>
> >>>> On 10/7/20 3:42 AM, Gene Chen wrote:
> >>>>> From: Gene Chen <gene_chen@xxxxxxxxxxx>
> >>>>>
> >>>>> 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@xxxxxxxxxxx>
> >>>>> ---
> >>>>>     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).
>

I think this case only register LED device, not LED "MC" device.
Because our FLASH is not a multicolor device.

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

According to your suggestion,
depends on LED_CLASS && LEDS_CLASS_FLASH && OF
depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR
depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
depends on MFD_MT6360

and source code add constraint

#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTICOLOR)
    ret = devm_led_classdev_multicolor_register_ext(parent, &led->rgb,
init_data);
#endif

#if IS_ENABLED(CONFIG_LEDS_CLASS_FLASH)
    ret = devm_led_classdev_flash_register_ext(parent, &led->flash, init_data);
#endif

=============

Or Should I seperate two drivers?
one for RGB LED, one for ML LED and FLED

> > I will remove "depends", use "select" instead.
>
> --
> Best regards,
> Jacek Anaszewski