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

[Michael Klein]

Thanks for reviewing!

On Tue, Dec 08, 2020 at 11:13:58AM +0100, Maxime Ripard wrote:
> 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@xxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxx>
> > +
> > +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.
> > +
> > +properties:
> > +  compatible:
> > +    const: "regulator-poweroff"
> > +
> > +  regulator-names:
> > +    description:
> > +      Array of regulator names
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +
> > +  REGULATOR-supply:
>
> This should be a patternProperties
>
> > +    description:
> > +      For any REGULATOR listed in regulator-names, a phandle
> > +      to the corresponding regulator node
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +  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
> > +
> > +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>;
> > +    };
>
> I'm not entirely sure how multiple regulators would work here. I guess
> the ordering is board/purpose sensitive. In this particular case, I
> assume that vcc1v2 would be shut down before vcc-dram?

yes, the regulators are shut down from left to right.

> If so, I would expect that one regulator_force_disable is run, the CPU
> is disabled and you never get the chance to cut vcc-dram.

I assume that any relevant regulator here has enough capacitance on the 
output that provides enough charge to disable any remaining regulators 
(my board has 3*10µF for vcc1v2 and 1*10µF for vcc-dram). But there is 
of course no guarantee, so I'm shutting down the most relevant (in terms 
of current consumption) regulator first.

In any case, if it's deemed unnecessary to allow more than one regulator 
in the driver I could remove the regulator-names property altogether and 
reduce the DT node to:

  regulator-poweroff {
      compatible = "regulator-poweroff";
      poweroff-supply = <&reg_vcc1v2>;
  };

--
Michael