Re: [PATCH 2/4] dt-bindings: regulator: add document bindings for mpq7920

[Maxime Ripard]

Hi,

On Thu, Dec 19, 2019 at 11:37:19AM +0100, Saravanan Sekar wrote:
> Add device tree binding information for mpq7920 regulator driver.
> Example bindings for mpq7920 are added.
>
> Signed-off-by: Saravanan Sekar <sravanhome@xxxxxxxxx>
> ---
>  .../bindings/regulator/mpq7920.yaml           | 149 ++++++++++++++++++
>  1 file changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mpq7920.yaml
>
> diff --git a/Documentation/devicetree/bindings/regulator/mpq7920.yaml b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
> new file mode 100644
> index 000000000000..79000b745cfd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/mpq7920.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Monolithic Power System MPQ7920 PMIC
> +
> +maintainers:
> +  - Saravanan Sekar <sravanhome@xxxxxxxxx>
> +
> +properties:
> +  $nodename:
> +    pattern: "mpq@[0-9a-f]{1,2}"

The node name is supposed to be the class of the device, so pmic or
regulator seems more suited here.

> +  compatible:
> +    enum:
> +	- mps,mpq7920
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: string
> +    description: |
> +      list of regulators provided by this controller, must be named
> +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
> +      The valid names for regulators are
> +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5

This should be an enum with the valid values

> +
> +    properties:
> +       mps,time-slot:

I'm not sure what this is supposed to be doing?

Is that another property in the regulator node, or regulators is
supposed to be a node?

> +         - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +         power on/off sequence time slot/interval must be one of following values
> +         With:
> +          * 0: 0.5ms
> +          * 1: 2ms
> +          * 2: 8ms
> +          * 3: 16ms
> +          Defaults to 0.5ms if not specified.

So it's not an array, but just a single value?

Either wai, the valid values should be an enum, and the default
specified using the default keyword.

> +
> +    properties:
> +       mps,fixed-on-time:

You don't need to set properties all the time.

> +          - $ref: "/schemas/types.yaml#/definitions/boolean"
> +       description: |
> +           select power on sequence with fixed time interval mentioned in
> +           time-slot reg for all the regulators.

Can't you just derive that from the fact that time-slot is present?

> +    properties:
> +       mps,fixed-off-time:
> +          - $ref: "/schemas/types.yaml#/definitions/boolean"
> +       description: |
> +          select power off sequence with fixed time interval mentioned in
> +          time-slot reg for all the regulators.

Same thing here

> +    properties:
> +       mps,inc-off-time:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +          An array of 8, linearly increase power off sequence time
> +          slot/interval for each regulator must be one of following values
> +         * 0 to 15

This should be an enum, or a combination of minimum/maximum. And the
size of the array should be fixed too.

> +    properties:
> +       mps,inc-on-time:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +          An array of 8, linearly increase power on sequence time
> +          slot/interval for each regulator must be one of following values
> +          * 0 to 15

Ditto,

> +    properties:
> +       mps,switch-freq:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8"
> +       description: |
> +          switching frequency must be one of following values
> +          * 0 : 1.1MHz
> +          * 1 : 1.65MHz
> +          * 2 : 2.2MHz
> +          * 3 : 2.75MHz
> +          Defaults to 2.2 MHz if not specified.

enum and default

> +    properties:
> +       mps,buck-softstart:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +          An array of 4 contains soft start time of each buck, must be one of
> +          following values
> +          * 0 : 150us
> +          * 1 : 300us
> +          * 2 : 610us
> +          * 3 : 920us
> +          Defaults to 300us if not specified.

Same story than mps,inc-off-time

> +    properties:
> +       mps,buck-ovp:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +           An array of 4 contains over voltage protection of each buck, must be
> +           one of following values
> +           * 0 : disabled
> +           * 1 : enabled
> +           Defaults is enabled if not specified.

Ditto

> +    properties:
> +        mps,buck-phase-delay:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +           An array of 4 contains each buck phase delay must be one of following
> +           values
> +           * 0: 0deg
> +           * 1: 90deg
> +           * 2: 180deg
> +           * 3: 270deg
> +           Defaults to 0deg for buck1 & buck2, 90deg for buck3 & buck4 if not
> +           specified.

ditto

> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mpq7920@69 {
> +	  compatible = "mps,mpq7920";
> +          reg = <0x69>;
> +
> +          mps,switch-freq = <1>;
> +          mps,buck-softstart = /bits/ 8 <1 2 1 3>;
> +          mps,buck-ovp = /bits/ 8 <1 0 1 1>;
> +
> +          regulators {
> +            buck1 {

So regulators isn't a string after all?

If it's supposed to be a node, it should be type: object

and you can check that the node has a valid value using propertyNames

Maxime

Attachment: signature.asc
Description: PGP signature