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

From: Jacek Anaszewski
Date: Mon Nov 16 2020 - 13:25:58 EST

Next message: isaku . yamahata: "[RFC PATCH 02/67] x86/msr-index: Define MSR_IA32_MKTME_KEYID_PART used by TDX"
Previous message: Verma, Vishal L: "Re: [RFC PATCH 1/9] cxl/acpi: Add an acpi_cxl module for the CXL interconnect"
In reply to: Gene Chen: "Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360"
Next in thread: Gene Chen: "Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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.

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.

--
Best regards,
Jacek Anaszewski

Next message: isaku . yamahata: "[RFC PATCH 02/67] x86/msr-index: Define MSR_IA32_MKTME_KEYID_PART used by TDX"
Previous message: Verma, Vishal L: "Re: [RFC PATCH 1/9] cxl/acpi: Add an acpi_cxl module for the CXL interconnect"
In reply to: Gene Chen: "Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360"
Next in thread: Gene Chen: "Re: [PATCH v5 2/2] leds: mt6360: Add LED driver for MT6360"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]