diff mbox series

[2/7] dt-bindings: leds: leds-qcom-lpg: Add support for LUT through NVMEM devices

Message ID 20230621185949.2068-3-quic_amelende@quicinc.com
State New
Headers show
Series Add support for LUT PPG | expand

Commit Message

Anjelique Melendez June 21, 2023, 6:59 p.m. UTC
Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
devices.

Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
---
 .../bindings/leds/leds-qcom-lpg.yaml          | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)

Comments

Krzysztof Kozlowski June 24, 2023, 9:42 a.m. UTC | #1
On 21/06/2023 20:59, Anjelique Melendez wrote:
> Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
> devices.
> 
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> ---
>  .../bindings/leds/leds-qcom-lpg.yaml          | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> index e6f1999cb22f..c9d53820bf83 100644
> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> @@ -63,6 +63,31 @@ properties:
>          - description: dtest line to attach
>          - description: flags for the attachment
>  
> +  nvmem:
> +    description: >
> +      Phandle(s) of the nvmem device(s) to access the LUT stored in the SDAM module(s).

It's the first time in this binding the "LUT" appears. Drop redundant
parts like "Phandle(s) of ...." and describe what do you expect there
and why the hardware needs it.

> +      This property is required only when LUT mode is supported and the LUT pattern is
> +      stored in SDAM modules instead of a LUT module.
> +    minItems: 1
> +    maxItems: 2
> +
> +  nvmem-names:
> +    description: >
> +      The nvmem device name(s) for the SDAM module(s) where the LUT pattern data is stored.
> +      This property is required only when LUT mode is supported with SDAM module instead of
> +      LUT module.
> +    minItems: 1
> +    items:
> +      - const: lpg_chan_sdam
> +      - const: lut_sdam
> +
> +  qcom,pbs-client:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: >
> +      Phandle of the PBS client used for sending the PBS trigger. This property is


You need to explain what is PBS and what this property is doing.

Phandle of PBS client? This is the PBS client! It does not make sense.



> +      required when LUT mode is supported and the LUT pattern is stored in a single
> +      SDAM module instead of a LUT module.

Which devices support LUT? Why this is not constrained per variant?

> +
>    multi-led:
>      type: object
>      $ref: leds-class-multicolor.yaml#
> @@ -191,4 +216,64 @@ examples:
>        compatible = "qcom,pm8916-pwm";
>        #pwm-cells = <2>;
>      };
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    led-controller {
> +      compatible = "qcom,pm8350c-pwm";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      #pwm-cells = <2>;
> +      nvmem-names = "lpg_chan_sdam" , "lut_sdam";

Fix your whitespaces.

> +      nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;

Two entries, not one.

Anyway, adding one property does not justify new example. Integrate it
into existing one.

> +
> +      led@1 {
> +        reg = <1>;
> +        color = <LED_COLOR_ID_RED>;
> +        label = "red";
> +      };
> +
> +      led@2 {
> +        reg = <2>;
> +        color = <LED_COLOR_ID_GREEN>;
> +        label = "green";
> +      };
> +
> +      led@3 {
> +        reg = <3>;
> +        color = <LED_COLOR_ID_BLUE>;
> +        label = "blue";
> +      };
> +    };
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    led-controller {
> +      compatible = "qcom,pmi632-lpg";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      #pwm-cells = <2>;
> +      nvmem-names = "lpg_chan_sdam";
> +      nvmem = <&pmi632_sdam7>;
> +      qcom,pbs-client = <&pmi632_pbs_client3>;

One more example? Why?

Why do you have here only one NVMEM cell? Aren't you missing constraints
in the binding?

Best regards,
Krzysztof
Anjelique Melendez June 29, 2023, 12:12 a.m. UTC | #2
On 6/24/2023 2:42 AM, Krzysztof Kozlowski wrote:
> On 21/06/2023 20:59, Anjelique Melendez wrote:
>> Update leds-qcom-lpg bindings to support LUT patterns through NVMEM
>> devices.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
>> ---
>>  .../bindings/leds/leds-qcom-lpg.yaml          | 85 +++++++++++++++++++
>>  1 file changed, 85 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> index e6f1999cb22f..c9d53820bf83 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
>> @@ -63,6 +63,31 @@ properties:
>>          - description: dtest line to attach
>>          - description: flags for the attachment
>>  
>> +  nvmem:
>> +    description: >
>> +      Phandle(s) of the nvmem device(s) to access the LUT stored in the SDAM module(s).
> 
> It's the first time in this binding the "LUT" appears. Drop redundant
> parts like "Phandle(s) of ...." and describe what do you expect there
> and why the hardware needs it. 

LUT is a "lookup table"(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml?h=v6.4#n14)
and in this case holds pattern data i.e LUT patterns.

Currently, qcom,pm660l-lpg, qcom,pm8150b-lpg, qcom,pm8150l-lpg, qcom,pm8941-lpg, qcom,pm8994-lpg,
qcom,pmc8180c-lpg, qcom,pmi8994-lpg, and qcom,pmi8998-lpg all have an a specific LUT peripheral
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.4-rc7#n1382)
and this is already being handled in leds-qcom-lpg.

What is new with this patch set is that instead of having a LUT peripheral, some PMICs use nvmem to 
hold LUT pattern and other lpg per channel data.
The use of nvmems to store LUT pattern and lpg data is called PPG.
You can either have a single nvmem PPG (a single nvmem device that holds LUT pattern 
and lpg per channel data) or two-nvmem PPG(1 nvmem for LUT pattern and 1 nvmem for lpg per channel data)

I can update the descritpion for the entire binding to mention PPG and LUT so this is more clear. 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml?h=v6.4#n12

 
>> +      This property is required only when LUT mode is supported and the LUT pattern is
>> +      stored in SDAM modules instead of a LUT module.
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  nvmem-names:
>> +    description: >
>> +      The nvmem device name(s) for the SDAM module(s) where the LUT pattern data is stored.
>> +      This property is required only when LUT mode is supported with SDAM module instead of
>> +      LUT module.
>> +    minItems: 1
>> +    items:
>> +      - const: lpg_chan_sdam
>> +      - const: lut_sdam
>> +
>> +  qcom,pbs-client:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: >
>> +      Phandle of the PBS client used for sending the PBS trigger. This property is
> 
> 
> You need to explain what is PBS and what this property is doing.
> 
> Phandle of PBS client? This is the PBS client! It does not make sense.
Ack
> 
> 
> 
>> +      required when LUT mode is supported and the LUT pattern is stored in a single
>> +      SDAM module instead of a LUT module.
> 
> Which devices support LUT? Why this is not constrained per variant?
When you say constrained per variant, are you looking for something more like this?
i.e. 
allOf:
  - if: 
      properties:
        compatible:
          contains:
            const: qcom,pmi632-lpg
    then:
      properties:
        nvmem:
          maxItems: 1
        nvmem-names:
          items:
            - const: lpg_chan_sdam
      required:
        - nvmem
        - qcom,pbs-client
  - if: 
      properties:
        compatible:
          contains:
            const: qcom,pm8350c-pwm
    then:
      properties:
        nvmem:
          maxItems: 2
        nvmem-names:
          items:
            - const: lpg_chan_sdam
            - const: lut_sdam
      required:
       - nvmem

> 
>> +
>>    multi-led:
>>      type: object
>>      $ref: leds-class-multicolor.yaml#
>> @@ -191,4 +216,64 @@ examples:
>>        compatible = "qcom,pm8916-pwm";
>>        #pwm-cells = <2>;
>>      };
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +
>> +    led-controller {
>> +      compatible = "qcom,pm8350c-pwm";
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      #pwm-cells = <2>;
>> +      nvmem-names = "lpg_chan_sdam" , "lut_sdam";
> 
> Fix your whitespaces.
Ack
> 
>> +      nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
> 
> Two entries, not one> 
> Anyway, adding one property does not justify new example. Integrate it
> into existing one.

So we actually cannot integrate these properties into existing examples.
The current examples are for PMICs that use LUT peripherals (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.4#n1417).
This patch series is adding support for PMICs that do not have a LUT peripheral
and instead store LUT patterns and LPG configurations in either 1 or 2 NVMEM(s). 
> 
>> +
>> +      led@1 {
>> +        reg = <1>;
>> +        color = <LED_COLOR_ID_RED>;
>> +        label = "red";
>> +      };
>> +
>> +      led@2 {
>> +        reg = <2>;
>> +        color = <LED_COLOR_ID_GREEN>;
>> +        label = "green";
>> +      };
>> +
>> +      led@3 {
>> +        reg = <3>;
>> +        color = <LED_COLOR_ID_BLUE>;
>> +        label = "blue";
>> +      };
>> +    };
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +
>> +    led-controller {
>> +      compatible = "qcom,pmi632-lpg";
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      #pwm-cells = <2>;
>> +      nvmem-names = "lpg_chan_sdam";
>> +      nvmem = <&pmi632_sdam7>;
>> +      qcom,pbs-client = <&pmi632_pbs_client3>;
> 
> One more example? Why?
> 
> Why do you have here only one NVMEM cell? Aren't you missing constraints
> in the binding?The use of the qcom,pbs-client is only used when we have a PMIC device that has a single PPG NVMEM, 
which is why this was not included in the above 2 nvmem PPG example. I see how these two PPG examples
are repetitive so I am ok with getting rid of one of them but I do think we should have at least one PPG example.

> 
> Best regards,
> Krzysztof
> 
Thanks,
Anjelique
Krzysztof Kozlowski July 1, 2023, 11:01 a.m. UTC | #3
On 29/06/2023 02:12, Anjelique Melendez wrote:
>>
>>
>>
>>> +      required when LUT mode is supported and the LUT pattern is stored in a single
>>> +      SDAM module instead of a LUT module.
>>
>> Which devices support LUT? Why this is not constrained per variant?
> When you say constrained per variant, are you looking for something more like this?
> i.e. 
> allOf:
>   - if: 
>       properties:
>         compatible:
>           contains:
>             const: qcom,pmi632-lpg
>     then:
>       properties:
>         nvmem:
>           maxItems: 1
>         nvmem-names:
>           items:
>             - const: lpg_chan_sdam
>       required:
>         - nvmem
>         - qcom,pbs-client
>   - if: 
>       properties:
>         compatible:
>           contains:
>             const: qcom,pm8350c-pwm
>     then:
>       properties:
>         nvmem:
>           maxItems: 2
>         nvmem-names:
>           items:
>             - const: lpg_chan_sdam
>             - const: lut_sdam
>       required:
>        - nvmem

Yes.

> 
>>
>>> +
>>>    multi-led:
>>>      type: object
>>>      $ref: leds-class-multicolor.yaml#
>>> @@ -191,4 +216,64 @@ examples:
>>>        compatible = "qcom,pm8916-pwm";
>>>        #pwm-cells = <2>;
>>>      };
>>> +  - |
>>> +    #include <dt-bindings/leds/common.h>
>>> +
>>> +    led-controller {
>>> +      compatible = "qcom,pm8350c-pwm";
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +      #pwm-cells = <2>;
>>> +      nvmem-names = "lpg_chan_sdam" , "lut_sdam";
>>
>> Fix your whitespaces.
> Ack
>>
>>> +      nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
>>
>> Two entries, not one> 
>> Anyway, adding one property does not justify new example. Integrate it
>> into existing one.
> 
> So we actually cannot integrate these properties into existing examples.
> The current examples are for PMICs that use LUT peripherals (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/rgb/leds-qcom-lpg.c?h=v6.4#n1417).
> This patch series is adding support for PMICs that do not have a LUT peripheral
> and instead store LUT patterns and LPG configurations in either 1 or 2 NVMEM(s). 
>>
>>> +
>>> +      led@1 {
>>> +        reg = <1>;
>>> +        color = <LED_COLOR_ID_RED>;
>>> +        label = "red";
>>> +      };
>>> +
>>> +      led@2 {
>>> +        reg = <2>;
>>> +        color = <LED_COLOR_ID_GREEN>;
>>> +        label = "green";
>>> +      };
>>> +
>>> +      led@3 {
>>> +        reg = <3>;
>>> +        color = <LED_COLOR_ID_BLUE>;
>>> +        label = "blue";
>>> +      };
>>> +    };
>>> +  - |
>>> +    #include <dt-bindings/leds/common.h>
>>> +
>>> +    led-controller {
>>> +      compatible = "qcom,pmi632-lpg";
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +      #pwm-cells = <2>;
>>> +      nvmem-names = "lpg_chan_sdam";
>>> +      nvmem = <&pmi632_sdam7>;
>>> +      qcom,pbs-client = <&pmi632_pbs_client3>;
>>
>> One more example? Why?
>>
>> Why do you have here only one NVMEM cell? Aren't you missing constraints
>> in the binding?The use of the qcom,pbs-client is only used when we have a PMIC device that has a single PPG NVMEM, 
> which is why this was not included in the above 2 nvmem PPG example. I see how these two PPG examples
> are repetitive so I am ok with getting rid of one of them but I do think we should have at least one PPG example.


This example probably should replace one of the previous ones, because
it is bigger / more complete.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
index e6f1999cb22f..c9d53820bf83 100644
--- a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
@@ -63,6 +63,31 @@  properties:
         - description: dtest line to attach
         - description: flags for the attachment
 
+  nvmem:
+    description: >
+      Phandle(s) of the nvmem device(s) to access the LUT stored in the SDAM module(s).
+      This property is required only when LUT mode is supported and the LUT pattern is
+      stored in SDAM modules instead of a LUT module.
+    minItems: 1
+    maxItems: 2
+
+  nvmem-names:
+    description: >
+      The nvmem device name(s) for the SDAM module(s) where the LUT pattern data is stored.
+      This property is required only when LUT mode is supported with SDAM module instead of
+      LUT module.
+    minItems: 1
+    items:
+      - const: lpg_chan_sdam
+      - const: lut_sdam
+
+  qcom,pbs-client:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: >
+      Phandle of the PBS client used for sending the PBS trigger. This property is
+      required when LUT mode is supported and the LUT pattern is stored in a single
+      SDAM module instead of a LUT module.
+
   multi-led:
     type: object
     $ref: leds-class-multicolor.yaml#
@@ -191,4 +216,64 @@  examples:
       compatible = "qcom,pm8916-pwm";
       #pwm-cells = <2>;
     };
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    led-controller {
+      compatible = "qcom,pm8350c-pwm";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      #pwm-cells = <2>;
+      nvmem-names = "lpg_chan_sdam" , "lut_sdam";
+      nvmem = <&pmk8550_sdam_21 &pmk8550_sdam_22>;
+
+      led@1 {
+        reg = <1>;
+        color = <LED_COLOR_ID_RED>;
+        label = "red";
+      };
+
+      led@2 {
+        reg = <2>;
+        color = <LED_COLOR_ID_GREEN>;
+        label = "green";
+      };
+
+      led@3 {
+        reg = <3>;
+        color = <LED_COLOR_ID_BLUE>;
+        label = "blue";
+      };
+    };
+  - |
+    #include <dt-bindings/leds/common.h>
+
+    led-controller {
+      compatible = "qcom,pmi632-lpg";
+      #address-cells = <1>;
+      #size-cells = <0>;
+      #pwm-cells = <2>;
+      nvmem-names = "lpg_chan_sdam";
+      nvmem = <&pmi632_sdam7>;
+      qcom,pbs-client = <&pmi632_pbs_client3>;
+
+      led@1 {
+        reg = <1>;
+        color = <LED_COLOR_ID_RED>;
+        label = "red";
+      };
+
+      led@2 {
+        reg = <2>;
+        color = <LED_COLOR_ID_GREEN>;
+        label = "green";
+      };
+
+      led@3 {
+        reg = <3>;
+        color = <LED_COLOR_ID_BLUE>;
+        label = "blue";
+      };
+    };
+
 ...