Re: [PATCH v1 2/2] dt-bindings: add bindings for QCOM flash LED

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2022/9/28 16:21, Krzysztof Kozlowski wrote:
On 28/09/2022 04:42, Fenglin Wu wrote:
Add binding document for flash LED module inside Qualcomm Technologies,
Inc. PMICs.

Signed-off-by: Fenglin Wu <quic_fenglinw@xxxxxxxxxxx>

You did not Cc me on first patch, so difficult to say how much it
matches the driver... There is also no DTS.
Thanks for reviewing the binding change, I sent the driver changes in the same series and you can check it here:
https://lore.kernel.org/linux-leds/6c0e5083-baae-3ed3-5eed-e08bbb9e7576@xxxxxxxxxx/T/#m97f71ce3f291f62d65f8107352d8ab9507093ab2

I will add you in email to list when sending next patchset.

---
  .../bindings/leds/leds-qcom-flash.yaml        | 108 ++++++++++++++++++
  1 file changed, 108 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-flash.yaml

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-flash.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-flash.yaml
new file mode 100644
index 000000000000..52a99182961b
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-flash.yaml


Filename matching compatible if there is one fallback (e.g.
qcom,spmi-flash-led.yaml).

Sure, I will update the file name to match with the fallback compatible string.
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/leds-qcom-flash.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Flash LED device inside Qualcomm Technologies, Inc. PMICs
+
+maintainers:
+  - Fenglin Wu <quic_fenglinw@xxxxxxxxxxx>
+
+description: |
+  Flash LED controller is present inside some Qualcomm Technologies, Inc. PMICs.
+  The flash LED module can have different number of LED channels supported
+  e.g. 3 or 4. There are some different registers between them but they can
+  both support maximum current up to 1.5 A per channel and they can also support
+  ganging 2 channels together to supply maximum current up to 2 A. The current
+  will be split symmetrically on each channel and they will be enabled and
+  disabled at the same time.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,spmi-flash-led
+          - qcom,pm8150c-flash-led
+          - qcom,pm8150l-flash-led
+          - qcom,pm8350c-flash-led

I doubt these are all different. You should use fallback, which also
will make use of the "items" you used...
pm8150c and pm8150l are different PMIC variants which have same flash LED module with 3 flash LED channels, while pm8350c has a different flash LED module with 4 flash LED channels. They can all use "qcom,spmi-flash-led" as the fallback because the driver has code logic to detect HW sub-types. But I was thinking to give out the PMIC names here so anyone who is using the driver could easily identify if the driver is suitable for the HW that he/she is using.

+
+  reg:
+    description: address offset of the flash LED controller
+    maxItems: 1
+
+patternProperties:
+  "^led@[0-3]$":
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+    description:
+      Represents the physical LED components which are connected to the flash LED channels' output.

Does not look like wrapped at 80.

Other places as well.
Sure, will wrap the lines at 80, I thought not exceeding 110 is also
acceptable.
+
+    properties:

Does not look like you tested the bindings...

You miss here reg.

will update the node name without using unit name.

+      led-sources:
+        description: The HW indices of the flash LED channels that connect to the physical LED
+        allOf:
+          - minItems: 1
+            maxItems: 2
+            items:
+              enum: [1, 2, 3, 4]
+
+      led-max-microamp:
+        description: |
+          The maximum current value when LED is not operating in flash mode (i.e. torch mode)
+          Valid values when an LED is connected to one flash LED channel:
+            5000 - 500000, step by 5000> +          Valid values when an LED is connected to two flash LED
channels:
+            10000 - 1000000, step by 10000

You need minimum and maximum.

Sure, I will add them
+
+      flash-max-microamp:
+        description: |
+          The maximum current value when LED is operating in flash mode.
+          Valid values when an LED is connected to one flash LED channel:
+            12500 - 1500000, step by 12500
+          Valid values when an LED is connected to two flash LED channels:
+            25000 - 2000000, step by 12500

You need minimum and maximum.
Sure, I will add them


+
+      flash-max-timeout-us:
+        description: |
+          The maximum timeout value when LED is operating in flash mode.
+          Valid values: 10000 - 1280000, step by 10000

You need minimum and maximum.

+
+    required:
+      - led-sources
+      - led-max-microamp

reg.

+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+    flash-led@ee00 {

Node name: led-controller

+            compatible = "qcom,spmi-flash-led";
+            reg = <0xee00>;
+
+            led@0 {

Test your bindings...

+                    function = LED_FUNCTION_FLASH;

Use 4 spaces for indentation of example.

sure, I will update it.
+                    color = <LED_COLOR_ID_WHITE>;
+                    led-sources = <1>, <4>;
+                    led-max-microamp = <300000>;
+                    flash-max-microamp = <2000000>;
+                    flash-max-timeout-us = <1280000>;
+                    function-enumerator = <0>;
+            };
+

Best regards,
Krzysztof




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux