[v9,1/2] dt-bindings: leds: Add Qualcomm Light Pulse Generator binding

This adds the binding document describing the three hardware blocks
related to the Light Pulse Generator found in a wide range of Qualcomm
PMICs.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v8:
- None

Changes since v7:
- Added qcom,pmc8180c-lpg
- Defined constraints for qcom,power-source
- Changes qcom,dtest to matrix and added constraints
- Changed example from LED_COLOR_ID_MULTI to LED_COLOR_ID_RGB

 .../bindings/leds/leds-qcom-lpg.yaml          | 164 ++++++++++++++++++
 1 file changed, 164 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml

On Tue, 22 Jun 2021 20:50:38 -0700, Bjorn Andersson wrote:
> This adds the binding document describing the three hardware blocks
> related to the Light Pulse Generator found in a wide range of Qualcomm
> PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v8:
> - None
> 
> Changes since v7:
> - Added qcom,pmc8180c-lpg
> - Defined constraints for qcom,power-source
> - Changes qcom,dtest to matrix and added constraints
> - Changed example from LED_COLOR_ID_MULTI to LED_COLOR_ID_RGB
> 
>  .../bindings/leds/leds-qcom-lpg.yaml          | 164 ++++++++++++++++++
>  1 file changed, 164 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

On Tue, 22 Jun 2021 20:50:38 -0700
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:

> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml

The file name should be based on one of the compatible strings, for
example the first one:
  qcom,pm8150b-lpg.yaml

> +      led@1 {
> +        reg = <1>;
> +        label = "green:user1";
> +      };

`label` is deprecated, please don't use in new bindings in examples.
Instead use color, function and function-enumerator, i.e.

  color = <LED_COLOR_ID_GREEN>;
  function = LED_FUNCTION_xxx;
  function-enumerator = <N>;

On Thu 24 Jun 18:19 CDT 2021, Marek Behun wrote:

> On Tue, 22 Jun 2021 20:50:38 -0700
> Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> 
> > +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> 
> The file name should be based on one of the compatible strings, for
> example the first one:
>   qcom,pm8150b-lpg.yaml
> 

The majority of the files in leds/ are named leds-*.yaml, is this a new
scheme for LED bindings?

> > +      led@1 {
> > +        reg = <1>;
> > +        label = "green:user1";
> > +      };
> 
> `label` is deprecated,

Sorry, I missed the comment in the middle of the description about this.
Is there any particular reason why this isn't marked deprecated: true?

> please don't use in new bindings in examples.
> Instead use color, function and function-enumerator, i.e.
> 
>   color = <LED_COLOR_ID_GREEN>;
>   function = LED_FUNCTION_xxx;
>   function-enumerator = <N>;
> 

Can you point me to something helping me regarding what "function" to
use?

For this particular devboard that the example comes from I have 4 LEDs
that are named "user1", "user2", "user3" and "user4" in the board
documentation. I can make up whatever for the example, but I would like
to get the following dts additions follow the expected guidelines.

Regards,
Bjorn

Hello Bjorn,

Am Thu, Jun 24, 2021 at 06:44:54PM -0500 schrieb Bjorn Andersson:
> On Thu 24 Jun 18:19 CDT 2021, Marek Behun wrote:
> > please don't use in new bindings in examples.
> > Instead use color, function and function-enumerator, i.e.
> > 
> >   color = <LED_COLOR_ID_GREEN>;
> >   function = LED_FUNCTION_xxx;
> >   function-enumerator = <N>;
> > 
> 
> Can you point me to something helping me regarding what "function" to
> use?
> 
> For this particular devboard that the example comes from I have 4 LEDs
> that are named "user1", "user2", "user3" and "user4" in the board
> documentation. I can make up whatever for the example, but I would like
> to get the following dts additions follow the expected guidelines.

I asked myself the same question in the past.  The wohle list is in
'include/dt-bindings/leds/common.h' and I in my personal project I
opted for LED_FUNCTION_INDICATOR, but yes, the confusion is real.

Greets
Alex

Quoting Bjorn Andersson (2021-06-22 20:50:38)
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> new file mode 100644
> index 000000000000..10aee61a7ffc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> @@ -0,0 +1,164 @@
[....]
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    lpg {

Should the node name be led or leds?

> +      compatible = "qcom,pmi8994-lpg";

Shouldn't there be a reg property? I see the driver has them hardcoded
but if this is a child of the spmi node then it should have a reg
property (or many reg properties).

On Tue, Jun 22, 2021 at 08:50:38PM -0700, Bjorn Andersson wrote:
> This adds the binding document describing the three hardware blocks
> related to the Light Pulse Generator found in a wide range of Qualcomm
> PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v8:
> - None
> 
> Changes since v7:
> - Added qcom,pmc8180c-lpg
> - Defined constraints for qcom,power-source
> - Changes qcom,dtest to matrix and added constraints
> - Changed example from LED_COLOR_ID_MULTI to LED_COLOR_ID_RGB
> 
>  .../bindings/leds/leds-qcom-lpg.yaml          | 164 ++++++++++++++++++
>  1 file changed, 164 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> new file mode 100644
> index 000000000000..10aee61a7ffc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> @@ -0,0 +1,164 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-qcom-lpg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Light Pulse Generator
> +
> +maintainers:
> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> +
> +description: >
> +  The Qualcomm Light Pulse Generator consists of three different hardware blocks;
> +  a ramp generator with lookup table, the light pulse generator and a three
> +  channel current sink. These blocks are found in a wide range of Qualcomm PMICs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,pm8150b-lpg
> +      - qcom,pm8150l-lpg
> +      - qcom,pm8916-pwm
> +      - qcom,pm8941-lpg
> +      - qcom,pm8994-lpg
> +      - qcom,pmc8180c-lpg
> +      - qcom,pmi8994-lpg
> +      - qcom,pmi8998-lpg
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  qcom,power-source:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      power-source used to drive the output, as defined in the datasheet.
> +      Should be specified if the TRILED block is present
> +    enum: [0, 1, 3]
> +
> +  qcom,dtest:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    description: >
> +      A list of integer pairs, where each pair represent the dtest line the
> +      particular channel should be connected to and the flags denoting how the
> +      value should be outputed, as defined in the datasheet. The number of
> +      pairs should be the same as the number of channels.
> +    items:
> +      items:
> +        - description: dtest line to attach
> +        - description: flags for the attachment
> +
> +  multi-led:
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      "^led@[0-9a-f]$":
> +        type: object
> +        $ref: common.yaml#
> +
> +patternProperties:
> +  "^led@[0-9a-f]$":
> +    type: object
> +    $ref: common.yaml#
> +
> +    properties:
> +      reg: true
> +
> +    required:
> +      - reg
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    lpg {
> +      compatible = "qcom,pmi8994-lpg";
> +
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      qcom,power-source = <1>;
> +
> +      qcom,dtest = <0 0>,
> +                   <0 0>,
> +                   <0 0>,
> +                   <4 1>;
> +
> +      led@1 {
> +        reg = <1>;
> +        label = "green:user1";
> +      };
> +
> +      led@2 {
> +        reg = <2>;
> +        label = "green:user0";
> +        default-state = "on";
> +      };
> +
> +      led@3 {
> +        reg = <3>;
> +        label = "green:user2";
> +      };
> +
> +      led@4 {
> +        reg = <4>;
> +        label = "green:user3";
> +      };
> +    };
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +
> +    lpg {
> +      compatible = "qcom,pmi8994-lpg";
> +
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      qcom,power-source = <1>;
> +
> +      multi-led {
> +        color = <LED_COLOR_ID_RGB>;
> +        function = LED_FUNCTION_STATUS;
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led@1 {
> +          reg = <1>;
> +          color = <LED_COLOR_ID_RED>;
> +        };
> +
> +        led@2 {
> +          reg = <2>;
> +          color = <LED_COLOR_ID_GREEN>;
> +        };
> +
> +        led@3 {
> +          reg = <3>;
> +          color = <LED_COLOR_ID_BLUE>;
> +        };
> +      };
> +    };
> +  - |
> +    lpg {

nit: should the node be named 'lpg-pwm'?

IIUC a PMIC .dtsi could have both a 'lpg' and a 'lpg-pwm' node, even though
only one of them can be enabled at any time.

> +      compatible = "qcom,pm8916-pwm";
> +      #pwm-cells = <2>;
> +    };

On Thu 09 Sep 10:18 CDT 2021, Matthias Kaehlcke wrote:

> On Tue, Jun 22, 2021 at 08:50:38PM -0700, Bjorn Andersson wrote:
[..]
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    lpg {
> > +      compatible = "qcom,pmi8994-lpg";
> > +
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      qcom,power-source = <1>;
> > +
> > +      multi-led {
> > +        color = <LED_COLOR_ID_RGB>;
> > +        function = LED_FUNCTION_STATUS;
> > +
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        led@1 {
> > +          reg = <1>;
> > +          color = <LED_COLOR_ID_RED>;
> > +        };
> > +
> > +        led@2 {
> > +          reg = <2>;
> > +          color = <LED_COLOR_ID_GREEN>;
> > +        };
> > +
> > +        led@3 {
> > +          reg = <3>;
> > +          color = <LED_COLOR_ID_BLUE>;
> > +        };
> > +      };
> > +    };
> > +  - |
> > +    lpg {
> 
> nit: should the node be named 'lpg-pwm'?
> 
> IIUC a PMIC .dtsi could have both a 'lpg' and a 'lpg-pwm' node, even though
> only one of them can be enabled at any time.
> 

No, there's only the one "LPG", with N channels. The lpg exposes a pwm
chip and the child nodes may describe LEDs connected to the channels.
So this example is the configuration where there's no LEDs attached.

The compatible is "pwm", because the PM8916 lacks the pattern and RGB
blocks that makes up the LPG - and is hence named "PWM" in the datasheet
instead. So perhaps the example should be generically named "pwm"
instead.

In all other PMICs I know of the hardware block is named "lpg".

Regards,
Bjorn

> > +      compatible = "qcom,pm8916-pwm";
> > +      #pwm-cells = <2>;
> > +    };

Hei hei,

Am Wed, Sep 08, 2021 at 03:39:51AM +0000 schrieb Stephen Boyd:
> Quoting Bjorn Andersson (2021-06-22 20:50:38)
> > diff --git a/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> > new file mode 100644
> > index 000000000000..10aee61a7ffc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-qcom-lpg.yaml
> > @@ -0,0 +1,164 @@
> [....]
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/common.h>
> > +
> > +    lpg {
> 
> Should the node name be led or leds?

If I scan through Documentation/devicetree/bindings/leds and look at
other devices, it should probably be named "led-controller", right?

At least that's what Rob suggested when I converted the leds-pwm
binding last year, using generic node names:

https://lore.kernel.org/linux-leds/20200922155747.GA2734659@bogus/

Greets
Alex

> 
> > +      compatible = "qcom,pmi8994-lpg";
> 
> Shouldn't there be a reg property? I see the driver has them hardcoded
> but if this is a child of the spmi node then it should have a reg
> property (or many reg properties).