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

[Gene Chen <gene.chen.richtek@gmail.com>]

Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月31日 週六 上午6:34寫道：
>
> On 10/30/20 9:51 AM, Gene Chen wrote:
> > Jacek Anaszewski <jacek.anaszewski@gmail.com> 於 2020年10月28日 週三 上午1:28寫道：
> >>
> >> 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).
> >>
> >
> > I think this case only register LED device, not LED "MC" device.
> > Because our FLASH is not a multicolor device.
>
> No, here I was describing following setup:
>
> - DT FLED node is present, CONFIG_LEDS_CLASS_FLASH is off
> - DT MC node is present, CONFIG_LEDS_CLASS_MULTICOLOR is on
>
> ML LED presence in DT is irrelevant in this case.
> It should be always registered if there is corresponding DT node
> and LEDS_CLASS is on.
>

As a long time discussion, we conclude some rules about MT6360 LED driver.
FLED is necessary, so Kconfig depends on LED_CLASS_FLASH
ML LED is optional, which is registered as led class device.
RGB LED can be either simple led class device or multicolor device,
which is decided in DT node
If Multicolor LED DT node is exist, it should be register multicolor
device success.
Maybe it is more specific to send a new patch?

Sample DT as below
LED "red" is simple led class device, LED "green&blue" is multicolor devices.
led@0 {
        reg = <0>;
        function = LED_FUNCTION_INDICATOR;
        color = <LED_COLOR_ID_RED>;
        led-max-microamp = <24000>;
};
led@6 {
        reg = <6>;
        function = LED_FUNCTION_INDICATOR;
        color = <LED_COLOR_ID_MULTI>;

        led@1 {
                reg = <1>;
                function = LED_FUNCTION_INDICATOR;
                color = <LED_COLOR_ID_GREEN>;
                led-max-microamp = <24000>;
        };
        led@2 {
                reg = <2>;
                function = LED_FUNCTION_INDICATOR;
                color = <LED_COLOR_ID_BLUE>;
                led-max-microamp = <24000>;
        };
};

> >
> >> 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
>
> s/LED_CLASS/LEDS_CLASS/
>
> And you have to remove LEDS_CLASS_FLASH from above line.
>
> > depends on LEDS_CLASS_MULTICOLOR || !!LEDS_CLASS_MULTICOLOR
>
> s/!!LEDS_CLASS_MULTICOLOR/!LEDS_CLASS_MULTICOLOR/
>
> > depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
> > depends on MFD_MT6360
>
> You will need V4L2_FLASH_LED_CLASS dependency as well, to avoid
> build break, when it is set to 'm'.
>
> To recap, following block of dependencies is required:
>
> depends on LEDS_CLASS && OF
> depends on LEDS_CLASS_MULTICOLOR || !LEDS_CLASS_MULTICOLOR
> depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
> depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
> depends on MFD_MT6360
>

LEDS_MT6360 depends on LEDS_CLASS_FLASH, and LEDS_CLASS_FLASH depends
on LEDS_CLASS
Is "depends on LEDS_CLASS" still needed?

> >
> > 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
>
> No, the guards should be in headers. That's why I recommended adding
> no ops for LED flash class registration functions in previous email.
>
> Please compare include/linux/led-class-multicolor.h and do similar
> changes in include/linux/led-class-flash.h.
>

ACK, I will submit a fixed patch about leds-class-flash.h.

By the way, if CONFIG_LED_CLASS_MULTICOLOR is not enabled and we don't
use #if IS_ENABLED,
according to led-class-multicolor.h return -EINVAL,
we will register multicolor device fail and cause probe fail.

> > =============
> >
> > Or Should I seperate two drivers?
> > one for RGB LED, one for ML LED and FLED
>
> This would incur unnecessary code duplication.
>
> --
> Best regards,
> Jacek Anaszewski