Re: [PATCH v3 2/3] Documentation: DT: binding documentation for regulator-poweroff

Rob Herring
2020-12-08T15:40:35Z

On Mon, Dec 07, 2020 at 03:27:55PM +0100, Michael Klein wrote:
> Add devicetree binding documentation for regulator-poweroff driver.
>
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
> .../power/reset/regulator-poweroff.yaml | 53 +++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> new file mode 100644
> index 000000000000..8c8ce6bb031a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/regulator-poweroff.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/regulator-poweroff.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Force-disable power regulators to turn the power off.
> +
> +maintainers:
> + - Michael Klein <michael@fossekall.de>
> +
> +description: |
> + When the power-off handler is called, one more regulators are disabled
> + by calling regulator_force_disable(). If the power is still on and the
> + CPU still running after a 3000ms delay, a WARN_ON(1) is emitted.

WARN_ON is a Linux thing. Bindings are independent from Linux.

> +
> +properties:
> + compatible:
> + const: "regulator-poweroff"
> +
> + regulator-names:

We already have 'regulator-name' which is something different, and
*-names already has a defined usage as a companion to other properties
('foo-names' goes with 'foos'). More on this below...

> + description:
> + Array of regulator names
> + $ref: /schemas/types.yaml#/definitions/string-array
> +
> + REGULATOR-supply:
> + description:
> + For any REGULATOR listed in regulator-names, a phandle
> + to the corresponding regulator node
> + $ref: /schemas/types.yaml#/definitions/phandle

*-supply already has a type.

> +
> + timeout-ms:
> + description:
> + Time to wait before asserting a WARN_ON(1). If nothing is
> + specified, 3000 ms is used.
> + $ref: /schemas/types.yaml#/definitions/uint32

Do we really need to tune the timeout just for an error message?

> +
> +required:
> + - compatible
> + - regulator-names
> + - REGULATOR-supply
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + regulator-poweroff {
> + compatible = "regulator-poweroff";
> + regulator-names = "vcc1v2", "vcc-dram";
> + vcc1v2-supply = <&reg_vcc1v2>;
> + vcc-dram-supply = <&reg_vcc_dram>;

-supply names are supposed to be named based on the consumer names (e.g.
LDO1 regulator supplies vcc-supply). To avoid 'regulator-names' and
simplifier the driver, I'd just define fixed, known names. Something
like:

power1-supply
power2-supply
...

Rob