Re: [PATCH] dt-bindings: bd718x7: Yamlify and add BD71850

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

Thanks for taking a look on this Rob,

On Thu, 2019-12-26 at 15:16 -0700, Rob Herring wrote:
> On Tue, Dec 17, 2019 at 10:48:24AM +0200, Matti Vaittinen wrote:
> > Convert ROHM bd71837 and bd71847 PMIC binding text docs to yaml.
> > Split
> > the binding document to two separate documents (own for BD71837 and
> > BD71847)
> > as they have different amount of regulators. This way we can better
> > enforce
> > the node name check for regulators. ROHM is also providing BD71850
> > - which
> > is almost identical to BD71847 - main difference is some initial
> > regulator
> > states. The BD71850 can be driven by same driver and it has same
> > buck/LDO
> > setup as BD71847 - add it to BD71847 binding document and introduce
> > compatible for it.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > 
> > Oh dear how bad I am with yaml...
> 
> Looks pretty good overall.

Thanks. But it was not easy for me. It took bunch of trials to fix all
the errors...

> I hope 'yamlify' doesn't catch on. :)

I can almost see that in OED already xD

> > Lee, I think the support series for BD71828 included some changes
> > to drivers/mfd/rohm-bd718x7.c - I will add BD71850 compatible to
> > next
> > version of that series in order to avoid conflicts. Does that work
> > for you?
> > 
> >  .../bindings/mfd/rohm,bd71837-pmic.txt        |  90 --------
> >  .../bindings/mfd/rohm,bd71837-pmic.yaml       | 198
> > ++++++++++++++++++
> >  .../bindings/mfd/rohm,bd71847-pmic.yaml       | 181
> > ++++++++++++++++
> >  .../regulator/rohm,bd71837-regulator.txt      | 162 --------------
> >  .../regulator/rohm,bd71837-regulator.yaml     | 103 +++++++++
> >  .../regulator/rohm,bd71847-regulator.yaml     |  97 +++++++++
> >  6 files changed, 579 insertions(+), 252 deletions(-)
> >  delete mode 100644
> > Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
> >  create mode 100644
> > Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml
> >  delete mode 100644
> > Documentation/devicetree/bindings/regulator/rohm,bd71837-
> > regulator.txt
> >  create mode 100644
> > Documentation/devicetree/bindings/regulator/rohm,bd71837-
> > regulator.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/regulator/rohm,bd71847-
> > regulator.yaml
> > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-
> > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71837-
> > pmic.yaml
> > new file mode 100644
> > index 000000000000..3a6d408aebbd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.yaml
> > @@ -0,0 +1,198 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/rohm,bd71837-pmic.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ROHM BD71837 Power Management Integrated Circuit bindings
> > +
> > +maintainers:
> > +  - Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > +
> > +description: |
> > +  BD71837MWV is programmable Power Management ICs for powering
> > single-core,
> > +  dual-core, and quad-core SoCs such as NXP-i.MX 8M. It is
> > optimized for low
> > +  BOM cost and compact solution footprint. BD71837MWV  integrates
> > 8 Buck
> > +  regulators and 7 LDOs.
> > +  Datasheet for BD71837 is available at
> > +  
> > https://www.rohm.com/products/power-management/power-management-ic-for-system/industrial-consumer-applications/nxp-imx/bd71837amwv-product
> > +
> > +properties:
> > +  compatible:
> > +    const: rohm,bd71837
> > +
> > +  reg:
> > +    description:
> > +      I2C slave address.
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  "#clock-cells":
> > +    const: 0
> > +
> > +# The BD718x7 supports two different HW states as reset target
> > states. States
> > +# are called as SNVS and READY. At READY state all the PMIC power
> > outputs go
> > +# down and OTP is reload. At the SNVS state all other logic and
> > external
> > +# devices apart from the SNVS power domain are shut off. Please
> > refer to NXP
> > +# i.MX8 documentation for further information regarding SNVS
> > state. When a
> > +# reset is done via SNVS state the PMIC OTP data is not reload.
> > This causes
> > +# power outputs that have been under SW control to stay down when
> > reset has
> > +# switched power state to SNVS. If reset is done via READY state
> > the power
> > +# outputs will be returned to HW control by OTP loading. Thus the
> > reset
> > +# target state is set to READY by default. If SNVS state is used
> > the boot
> > +# crucial regulators must have the regulator-always-on and
> > regulator-boot-on
> > +# properties set in regulator node.
> > +
> > +  rohm,reset-snvs-powered:
> > +    description: |
> > +      Transfer PMIC to SNVS state at reset
> > +    type: boolean
> > +
> > +# Configure the "short press" and "long press" timers for the
> > power button.
> > +# Values are rounded to what hardware supports (500ms multiple for
> > short and
> > +# 1000ms multiple for long). If these properties are not present
> > the existing
> > +# configuration (from bootloader or OTP) is not touched.
> 
> You can use 'multipleOf' keyword below for some constraints.

Umm.. Actually, now that I checked the datasheet for proper
constraints... Minimum value for both short and long press is 10ms.
After that the values are multiples of 500ms or 1000ms - but first
value is 10ms. I'll add minimum and maximum and update the comment
accordingly. Thanks for catching this :)

> 
> Same for the other file.
> 
> > +
> > +  rohm,short-press-ms:
> > +    description:
> > +      Short press duration in milliseconds
> > +
> > +  rohm,long-press-ms:
> > +    description:
> > +      Long press duration in milliseconds
> > +
> > +  regulators:
> > +    $ref: ../regulator/rohm,bd71837-regulator.yaml
> > +    description:
> > +      List of child nodes that specify the regulators.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - "#clock-cells"
> > +  - regulators
> 
> additionalProperties: false

Ok.

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/leds/common.h>
> > +    #
> > +
> > +    i2c {
> > +      pmic: pmic@4b {
> > +            compatible = "rohm,bd71837";
> > +            reg = <0x4b>;
> > +            interrupt-parent = <&gpio1>;
> > +            interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
> > +            #clock-cells = <0>;
> > +            clocks = <&osc 0>;
> > +            clock-output-names = "bd71837-32k-out";
> 
> Not documented.

I'll just drop this from the example for now. We can add ref to common
clock bindings later - if they are needed at some point.

> > +            rohm,reset-snvs-powered;
> > +
> > +            regulators {
> > +                buck1: BUCK1 {
> > +                    regulator-name = "buck1";
> > +                    regulator-min-microvolt = <700000>;
> > +                    regulator-max-microvolt = <1300000>;
> > +                    regulator-boot-on;
> > +                    regulator-always-on;
> > +                    regulator-ramp-delay = <1250>;
> > +                    rohm,dvs-run-voltage = <900000>;
> > +                    rohm,dvs-idle-voltage = <850000>;
> > +                    rohm,dvs-suspend-voltage = <800000>;
> > +                };
> > +                buck2: BUCK2 {
> > +                    regulator-name = "buck2";
> > +                    regulator-min-microvolt = <700000>;
> > +                    regulator-max-microvolt = <1300000>;
> > +                    regulator-boot-on;
> > +                    regulator-always-on;
> > +                    regulator-ramp-delay = <1250>;
> > +                    rohm,dvs-run-voltage = <1000000>;
> > +                    rohm,dvs-idle-voltage = <900000>;
> > +                };
> > +                buck3: BUCK3 {
> > +                    regulator-name = "buck3";
> > +                    regulator-min-microvolt = <700000>;
> > +                    regulator-max-microvolt = <1300000>;
> > +                    regulator-boot-on;
> > +                    rohm,dvs-run-voltage = <1000000>;
> > +                };
> > +                buck4: BUCK4 {
> > +                    regulator-name = "buck4";
> > +                    regulator-min-microvolt = <700000>;
> > +                    regulator-max-microvolt = <1300000>;
> > +                    regulator-boot-on;
> > +                    rohm,dvs-run-voltage = <1000000>;
> > +                };
> > +                buck5: BUCK5 {
> > +                    regulator-name = "buck5";
> > +                    regulator-min-microvolt = <700000>;
> > +                    regulator-max-microvolt = <1350000>;
> > +                    regulator-boot-on;
> > +                };
> > +                buck6: BUCK6 {
> > +                    regulator-name = "buck6";
> > +                    regulator-min-microvolt = <3000000>;
> > +                    regulator-max-microvolt = <3300000>;
> > +                    regulator-boot-on;
> > +                };
> > +                buck7: BUCK7 {
> > +                    regulator-name = "buck7";
> > +                    regulator-min-microvolt = <1605000>;
> > +                    regulator-max-microvolt = <1995000>;
> > +                    regulator-boot-on;
> > +                };
> > +                buck8: BUCK8 {
> > +                    regulator-name = "buck8";
> > +                    regulator-min-microvolt = <800000>;
> > +                    regulator-max-microvolt = <1400000>;
> > +                };
> > +
> > +                ldo1: LDO1 {
> > +                    regulator-name = "ldo1";
> > +                    regulator-min-microvolt = <3000000>;
> > +                    regulator-max-microvolt = <3300000>;
> > +                    regulator-boot-on;
> > +                };
> > +                ldo2: LDO2 {
> > +                    regulator-name = "ldo2";
> > +                    regulator-min-microvolt = <900000>;
> > +                    regulator-max-microvolt = <900000>;
> > +                    regulator-boot-on;
> > +                };
> > +                ldo3: LDO3 {
> > +                    regulator-name = "ldo3";
> > +                    regulator-min-microvolt = <1800000>;
> > +                    regulator-max-microvolt = <3300000>;
> > +                };
> > +                ldo4: LDO4 {
> > +                    regulator-name = "ldo4";
> > +                    regulator-min-microvolt = <900000>;
> > +                    regulator-max-microvolt = <1800000>;
> > +                };
> > +                ldo5: LDO5 {
> > +                    regulator-name = "ldo5";
> > +                    regulator-min-microvolt = <1800000>;
> > +                    regulator-max-microvolt = <3300000>;
> > +                };
> > +                ldo6: LDO6 {
> > +                    regulator-name = "ldo6";
> > +                    regulator-min-microvolt = <900000>;
> > +                    regulator-max-microvolt = <1800000>;
> > +                };
> > +                ldo7_reg: LDO7 {
> > +                    regulator-name = "ldo7";
> > +                    regulator-min-microvolt = <1800000>;
> > +                    regulator-max-microvolt = <3300000>;
> > +                };
> > +            };
> > +        };
> > +    };