Re: [RFC PATCH v3 03/15] dt-bindings: regulator: Document ROHM BD71282 regulator bindings

[Rob Herring]

On Fri, Nov 01, 2019 at 01:31:46PM +0200, Matti Vaittinen wrote:
> Document ROHM BD71828 PMIC regulator device tree bindings.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> ---
> 
> Changes from v2 - my first encounter with yaml :/
> 
>  .../regulator/rohm,bd71828-regulator.yaml     | 123 ++++++++++++++++++
>  1 file changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml b/Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
> new file mode 100644
> index 000000000000..60715d8b92df
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/rohm,bd71828-regulator.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/rohm,bd71828-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BD71828 Power Management Integrated Circuit regulators
> +
> +maintainers:
> +  - Liam Girdwood <lgirdwood@xxxxxxxxx>
> +  - Mark Brown <broonie@xxxxxxxxxx>
> +  - Rob Herring <robh+dt@xxxxxxxxxx>
> +  - Mark Rutland <mark.rutland@xxxxxxx>
> +
> +description: |
> +  This module is part of the ROHM BD71828 MFD device. For more details
> +  see Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml.
> +
> +  The regulator controller is represented as a sub-node of the PMIC node
> +  on the device tree.
> +
> +  Regulator nodes should be named to BUCK_<number> and LDO_<number>.
> +  The valid names for BD71828 regulator nodes are
> +  BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6, BUCK7
> +  LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7
> +
> +patternProperties:
> +  "^LDO[1-7]$":
> +    type: object
> +    allOf:
> +      - $ref: regulator.yaml#
> +    description:
> +      Properties for single LDO regulator.
> +
> +    properties:
> +      #Is there a nice way to check the name is same as node name but lower case

Well, lowercase nodenames are preferred... But still, no, there's not.

And I think you could just drop this and the nodename is used instead.

> +      regulator-name:
> +        description:
> +          should be "ldo1", ..., "ldo7"

You can at least do:

pattern: "^ldo[1-7]$"


> +
> +  "^BUCK[1-7]$":
> +    type: object
> +    allOf:
> +      - $ref: regulator.yaml#
> +    description:
> +      Properties for single BUCK regulator.
> +
> +    properties:
> +      #Is there a nice way to check the name is same as node name but lower case
> +      regulator-name:
> +        description:
> +          should be "buck1", ..., "buck7"
> +
> +      rohm,dvs-run-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          PMIC default "RUN" state voltage in uV. See below table for
> +          bucks which support this.

Use standard unit-suffixes on all these (-microvolt). And then drop the 
$ref.

Any constraints on the range?

> +
> +      rohm,dvs-idle-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          PMIC default "IDLE" state voltage in uV. See below table for
> +          bucks which support this.
> +
> +      rohm,dvs-suspend-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          PMIC default "SUSPEND" state voltage in uV. See below table for
> +          bucks which support this.
> +
> +      rohm,dvs-lpsr-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          PMIC default "LPSR" state voltage in uV. See below table for
> +          bucks which support this.
> +
> +#Supported default DVS states:
> +#buck		| run		| idle		| suspend	| lpsr
> +#----------------------------------------------------------------------------
> +#1, 2, 6, and 7	| supported	| supported	| 	supported (*)
> +#----------------------------------------------------------------------------
> +#3, 4, and 5	| 			supported (**)
> +#----------------------------------------------------------------------------
> +#(*)  LPSR and SUSPEND states use same voltage but both states have own enable /
> +#     disable settings. Voltage 0 can be specified for a state to make regulator
> +#     disabled on that state.
> +#(**) All states use same voltage but have own enable / disable settings.
> +#     Voltage 0 can be specified for a state to make regulator disabled on that
> +#     state.
> +
> +      rohm,dvs-runlvl-ctrl:
> +        description: |
> +          buck control is done based on run-level. Regulator is not
> +          individually controllable. See ../mfd/rohm,bd71828-pmic.yaml for
> +          how to specify run-level control mechanism. Only bucks 1, 2, 6
> +          and 7 support this.
> +        type: boolean
> +
> +      rohm,dvs-runlevel0-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          voltage for run-level 0. Microvolts.
> +
> +      rohm,dvs-runlevel1-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          voltage for run-level 1. Microvolts.
> +
> +      rohm,dvs-runlevel2-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          voltage for run-level 2. Microvolts.
> +
> +      rohm,dvs-runlevel3-voltage:
> +        $ref: "/schemas/types.yaml#/definitions/uint32"
> +        description:
> +          voltage for run-level 3. Microvolts.

Perhaps an array of 4 values for runlevel?

> +
> +    required:
> +      - regulator-name
> +  additionalProperties: false
> +additionalProperties: false
> -- 
> 2.21.0
> 
> 
> -- 
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]