Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for MT6360 LED

From: Jacek Anaszewski
Date: Tue Nov 17 2020 - 17:24:59 EST


Hi Gene,

Thank you for the patch.

On 11/17/20 11:55 AM, Gene Chen wrote:
From: Gene Chen <gene_chen@xxxxxxxxxxx>

Add bindings document for LED support on MT6360 PMIC

Signed-off-by: Gene Chen <gene_chen@xxxxxxxxxxx>
---
.../devicetree/bindings/leds/leds-mt6360.yaml | 114 +++++++++++++++++++++
1 file changed, 114 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-mt6360.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-mt6360.yaml b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
new file mode 100644
index 0000000..871db4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-mt6360.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-mt6360.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LED driver for MT6360 PMIC from MediaTek Integrated.
+
+maintainers:
+ - Gene Chen <gene_chen@xxxxxxxxxxx>
+
+description: |
+ This module is part of the MT6360 MFD device.
+ see Documentation/devicetree/bindings/mfd/mt6360.yaml
+ Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
+ and 4-channel RGB LED support Register/Flash/Breath Mode

What actually is the Register mode?

+
+properties:
+ compatible:
+ const: mediatek,mt6360-led
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+patternProperties:
+ "^led@[0-6]$":
+ type: object
+ $ref: common.yaml#
+ description:
+ Properties for a single LED.
+
+ properties:
+ reg:
+ description: Index of the LED.
+ enum:
+ - 0 # LED output INDICATOR1_RED
+ - 1 # LED output INDICATOR1_GREEN
+ - 2 # LED output INDICATOR1_BLUE
+ - 3 # LED output INDICATOR2_ML
+ - 4 # LED output FLED1
+ - 5 # LED output FLED2
+ - 6 # LED output MULTICOLOR
+
+unevaluatedProperties: false
+
+required:
+ - compatible
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+ led-controller {
+ compatible = "mediatek,mt6360-led";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_RED>;
+ led-max-microamp = <24000>;
+ };
+ led@3 {
+ reg = <3>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_AMBER>;

You should really have here LED_COLOR_ID_MOONLIGHT if this is
a moonlight LED. You'll need to add it to dt-bindings/leds/common.h.

+ led-max-microamp = <150000>;
+ };
+ led@4 {
+ reg = <4>;
+ function = LED_FUNCTION_FLASH;
+ color = <LED_COLOR_ID_WHITE>;
+ function-enumerator = <1>;
+ led-max-microamp = <200000>;
+ flash-max-microamp = <500000>;
+ flash-max-timeout-us = <1024000>;
+ };
+ led@5 {
+ reg = <5>;
+ function = LED_FUNCTION_FLASH;
+ color = <LED_COLOR_ID_WHITE>;
+ function-enumerator = <2>;
+ led-max-microamp = <200000>;
+ flash-max-microamp = <500000>;
+ flash-max-timeout-us = <1024000>;
+ };
+ led@6 {
+ reg = <6>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_MULTI>;
+ led-max-microamp = <24000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@1 {
+ reg = <1>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_GREEN>;
+ };
+ led@2 {
+ reg = <2>;
+ function = LED_FUNCTION_INDICATOR;
+ color = <LED_COLOR_ID_BLUE>;
+ };
+ };

It is of little avail to have multicolor LED with only two LEDs.
I propose not to allow such setup - i.e. you should have either
one multicolor LED comprising three sub-LEDs (regs: 0, 1, 2),
and with main color property set to LED_COLOR_ID_RGB, or three separate
LEDs.

Effectively, you should have two separate DT examples here to make it
clear: one for the case with three LED class devices and one with
LED multicolor class device.

+ };
+...


--
Best regards,
Jacek Anaszewski