Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and mfd/regulator properties

From: Rob Herring
Date: Mon Aug 17 2020 - 16:12:19 EST

On Mon, Aug 17, 2020 at 09:11:02AM +0200, Mauro Carvalho Chehab wrote:
> Add documentation for the properties needed by the HiSilicon
> 6421v600 driver, and by the SPMI controller used to access
> the chipset.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> ---
> .../mfd/hisilicon,hi6421-spmi-pmic.yaml | 182 ++++++++++++++++++
> .../spmi/hisilicon,hisi-spmi-controller.yaml | 54 ++++++
> 2 files changed, 236 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> new file mode 100644
> index 000000000000..95494114554d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> @@ -0,0 +1,182 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/hisilicon,hi6421-spmi-pmic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon 6421v600 SPMI PMIC
> +
> +maintainers:
> + - Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> +
> +description: |
> + HiSilicon 6421v600 uses a MIPI System Power Management (SPMI) bus in order
> + to provide interrupts and power supply.
> +
> + The GPIO and interrupt settings are represented as part of the top-level PMIC
> + node.
> +
> + The SPMI controller part is provided by
> + Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml.
> +
> +properties:
> + $nodename:
> + pattern: "pmic@[0-9a-f]"
> +
> + compatible:
> + const: hisilicon,hi6421-spmi-pmic

-spmi-pmic is redundant. Can the hi6421 be anything else?

> +
> + reg:
> + maxItems: 1
> +
> + spmi-channel:
> + description: number of the SPMI channel where the PMIC is connected

This looks like a common (to SPMI), but it's not something defined in
spmi.txt (which should ideally be converted to schema first). Minimally,
it needs a better explanation and determination if it should be common
or is HiSilicon specific.

> +
> + '#interrupt-cells':
> + const: 2
> +
> + interrupt-controller:
> + description:
> + Identify that the PMIC is capable of behaving as an interrupt controller.

No need to redefine common properties if nothing specific to this device
to say. Just:

interrupt-controller: true

> +
> + gpios:
> + maxItems: 1
> +
> + irq-num:
> + description: Interrupt request number
> +
> + 'irq-array':
> + description: Interrupt request array
> +
> + 'irq-mask-addr':
> + description: Address for the interrupt request mask
> +
> + 'irq-addr':
> + description: Address for the interrupt request

What's all these non-standard interrupt properties?

> +
> + regulators:
> + type: object

additionalProperties: false

> +
> + properties:
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + patternProperties:
> + '^ldo@[0-9]+$':

Unit-addresses are hex.

Also, doesn't match the example.

> + type: object
> +
> + $ref: "/schemas/regulator/regulator.yaml#"
> +
> + properties:
> + reg:
> + description: Enable register.
> +

> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0

No child nodes, you don't need these.

> +
> + vsel-reg:
> + description: Voltage selector register.

'reg' can have multiple entries if you want.

> +
> + enable-mask:
> + description: Bitmask used to enable the regulator.

But if there's a shared enable reg, then you shouldn't have duplicate
addresses (same 'reg' value in multiple nodes).

These perhaps should be driver data rather than in DT as it's all fixed
for this chip. We don't try to parameterize everything in DT.

> +
> + voltage-table:
> + description: Table with the selector items for the voltage regulator.
> + minItems: 2
> + maxItems: 16

Needs a type $ref.

> +
> + off-on-delay-us:
> + description: Time required for changing state to enabled in microseconds.
> +
> + startup-delay-us:
> + description: Startup time in microseconds.
> +
> + idle-mode-mask:
> + description: Bitmask used to put the regulator on idle mode.
> +
> + eco-microamp:
> + description: Maximum current while on idle mode.
> +
> + required:
> + - reg
> + - vsel-reg
> + - enable-mask
> + - voltage-table
> + - off-on-delay-us
> + - startup-delay-us
> +
> +required:
> + - compatible
> + - reg
> + - regulators

Add:

additionalProperties: false

> +
> +examples:
> + - |
> + /* pmic properties */
> +
> + pmic: pmic@0 {
> + compatible = "hisilicon,hi6421-spmi-pmic";
> + slave_id = <0>;

Not documented. I believe this is part of 'reg'.

> + reg = <0 0>;
> +
> + #interrupt-cells = <2>;
> + interrupt-controller;
> + gpios = <&gpio28 0 0>;
> + irq-num = <16>;
> + irq-array = <2>;
> + irq-mask-addr = <0x202 2>;
> + irq-addr = <0x212 2>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + regulators {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ldo3: ldo3@16 {
> + reg = <0x16>;
> + vsel-reg = <0x51>;
> +
> + regulator-name = "ldo3";
> + regulator-min-microvolt = <1500000>;
> + regulator-max-microvolt = <2000000>;
> + regulator-boot-on;
> +
> + enable-mask = <0x01>;
> +
> + voltage-table = <1500000>, <1550000>, <1600000>, <1650000>,
> + <1700000>, <1725000>, <1750000>, <1775000>,
> + <1800000>, <1825000>, <1850000>, <1875000>,
> + <1900000>, <1925000>, <1950000>, <2000000>;
> + off-on-delay-us = <20000>;
> + startup-delay-us = <120>;
> + };
> +
> + ldo4: ldo4@17 { /* 40 PIN */
> + reg = <0x17>;
> + vsel-reg = <0x52>;
> +
> + regulator-name = "ldo4";
> + regulator-min-microvolt = <1725000>;
> + regulator-max-microvolt = <1900000>;
> + regulator-boot-on;
> +
> + enable-mask = <0x01>;
> + idle-mode-mask = <0x10>;
> + eco-microamp = <10000>;
> +
> + hi6421-vsel = <0x52 0x07>;

Not documented.

> + voltage-table = <1725000>, <1750000>, <1775000>, <1800000>,
> + <1825000>, <1850000>, <1875000>, <1900000>;
> + off-on-delay-us = <20000>;
> + startup-delay-us = <120>;
> + };
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
> new file mode 100644
> index 000000000000..5aeb2ae12024
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spmi/hisilicon,hisi-spmi-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon SPMI controller
> +
> +maintainers:
> + - Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> +
> +description: |
> + The HiSilicon SPMI controller is found on some Kirin-based designs.
> + It is a MIPI System Power Management (SPMI) controller.
> +
> + The PMIC part is provided by
> + Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml.
> +
> +properties:
> + $nodename:
> + pattern: "spmi@[0-9a-f]"
> +
> + compatible:
> + const: hisilicon,spmi-controller

Needs an SoC specific compatible.

> +
> + reg:
> + maxItems: 1
> +
> + "#address-cells":
> + const: 2
> +
> + "#size-cells":
> + const: 0
> +
> + spmi-channel:
> + description: number of the SPMI channel where the PMIC is connected
> +
> +patternProperties:
> + "^pmic@[0-9a-f]$":
> + $ref: "/schemas/mfd/hisilicon,hi6421-spmi-pmic.yaml#"
> +
> +examples:
> + - |
> + spmi: spmi@fff24000 {
> + compatible = "hisilicon,spmi-controller";
> + #address-cells = <2>;
> + #size-cells = <0>;
> + status = "ok";
> + reg = <0x0 0xfff24000 0x0 0x1000>;
> + spmi-channel = <2>;

Does this go in the SPMI controller or child (pmic)?

> +
> + /* pmic properties */
> +
> + };
> --
> 2.26.2
>