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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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