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

  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(+)
  

On Wed, 21 Jun 2023 11:59:46 -0700, 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(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/leds/leds-qcom-lpg.example.dtb: /example-4/led-controller: failed to match any schema with compatible: ['qcom,pmi632-lpg']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230621185949.2068-3-quic_amelende@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
  

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
  

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
  

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