| Message ID | 20230621185949.2068-3-quic_amelende@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for LUT PPG | expand |
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
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"; + }; + }; + ...
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(+)