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

From: Gene Chen
Date: Tue Nov 17 2020 - 04:55:22 EST

Next message: Eric Dumazet: "Re: [PATCH v6] tcp: fix race condition when creating child sockets from syncookies"
Previous message: Borislav Petkov: "Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message"
In reply to: Jacek Anaszewski: "Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 於 2020年11月17日週二上午2:25寫道：
>
> On 11/16/20 11:01 AM, Gene Chen wrote:
> > Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 於 2020年10月31日週六上午6:34寫道：
> >>
> >> On 10/30/20 9:51 AM, Gene Chen wrote:
> >>> 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.
> >>
> >> 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
>
> Maybe it is necessary in your use case, but probably it is possible to
> use the device without FLED. If so, then you should allow users
> disabling it. Therefore you should have:
>
> depends on LEDS_CLASS_FLASH || !LEDS_CLASS_FLASH
>

ACK

> > 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.
>
> But only if CONFIG_LEDS_CLASS_MULTICOLOR is enabled.
>
> > 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>;
> > };
> > };
>
> It looks OK.
>
> >>>> 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?
>
> Yes, because you should allow disabling CONFIG_LEDS_CLASS_FLASH.
> In such a case driver should still compile and register ML LED class
> device when it has corresponding DT node.
>

ACK

> >>> 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.
>
> Good point. So led-class-multicolor.h no-ops need to be fixed to return
> 0 instead of -EINVAL.
>
> And no-ops in include/linux/led-class-flash.h should also return 0.

DT node is first priority to decide how leds work.
If RGB LED use multicolor form in DT, CONFIG_LEDS_CLASS_FLASH should be defined.
Otherwise It is right action to probe fail.

>
> --
> Best regards,
> Jacek Anaszewski

Next message: Eric Dumazet: "Re: [PATCH v6] tcp: fix race condition when creating child sockets from syncookies"
Previous message: Borislav Petkov: "Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message"
In reply to: Jacek Anaszewski: "Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]