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
