Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 19 Dec 2019 12:18:11 +0100
From:   Maxime Ripard <mripard@kernel.org>
To:     Saravanan Sekar <sravanhome@gmail.com>
Cc:     lgirdwood@gmail.com, broonie@kernel.org, robh+dt@kernel.org,
        mark.rutland@arm.com, heiko@sntech.de, shawnguo@kernel.org,
        laurent.pinchart@ideasonboard.com, icenowy@aosc.io,
        mchehab+samsung@kernel.org, davem@davemloft.net,
        gregkh@linuxfoundation.org, Jonathan.Cameron@huawei.com,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/4] dt-bindings: regulator: add document bindings for
 mpq7920
Message-ID: <20191219111811.6e3m3qvuijfi6ew6@gilmour.lan>
References: <20191219103721.10935-1-sravanhome@gmail.com>
 <20191219103721.10935-3-sravanhome@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="yqt63ee6xqsyxvu7"
Content-Disposition: inline
In-Reply-To: <20191219103721.10935-3-sravanhome@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--yqt63ee6xqsyxvu7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi,

On Thu, Dec 19, 2019 at 11:37:19AM +0100, Saravanan Sekar wrote:
> Add device tree binding information for mpq7920 regulator driver.
> Example bindings for mpq7920 are added.
>
> Signed-off-by: Saravanan Sekar <sravanhome@gmail.com>
> ---
>  .../bindings/regulator/mpq7920.yaml           | 149 ++++++++++++++++++
>  1 file changed, 149 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mpq7920.yaml
>
> diff --git a/Documentation/devicetree/bindings/regulator/mpq7920.yaml b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
> new file mode 100644
> index 000000000000..79000b745cfd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mpq7920.yaml
> @@ -0,0 +1,149 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/mpq7920.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Monolithic Power System MPQ7920 PMIC
> +
> +maintainers:
> +  - Saravanan Sekar <sravanhome@gmail.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "mpq@[0-9a-f]{1,2}"

The node name is supposed to be the class of the device, so pmic or
regulator seems more suited here.

> +  compatible:
> +    enum:
> +	- mps,mpq7920
> +
> +  reg:
> +    maxItems: 1
> +
> +  regulators:
> +    type: string
> +    description: |
> +      list of regulators provided by this controller, must be named
> +      after their hardware counterparts BUCK[1-4], one LDORTC, and LDO[2-5]
> +      The valid names for regulators are
> +      buck1, buck2, buck3, buck4, ldortc, ldo2, ldo3, ldo4, ldo5

This should be an enum with the valid values

> +
> +    properties:
> +       mps,time-slot:

I'm not sure what this is supposed to be doing?

Is that another property in the regulator node, or regulators is
supposed to be a node?

> +         - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +         power on/off sequence time slot/interval must be one of following values
> +         With:
> +          * 0: 0.5ms
> +          * 1: 2ms
> +          * 2: 8ms
> +          * 3: 16ms
> +          Defaults to 0.5ms if not specified.

So it's not an array, but just a single value?

Either wai, the valid values should be an enum, and the default
specified using the default keyword.

> +
> +    properties:
> +       mps,fixed-on-time:

You don't need to set properties all the time.

> +          - $ref: "/schemas/types.yaml#/definitions/boolean"
> +       description: |
> +           select power on sequence with fixed time interval mentioned in
> +           time-slot reg for all the regulators.

Can't you just derive that from the fact that time-slot is present?

> +    properties:
> +       mps,fixed-off-time:
> +          - $ref: "/schemas/types.yaml#/definitions/boolean"
> +       description: |
> +          select power off sequence with fixed time interval mentioned in
> +          time-slot reg for all the regulators.

Same thing here

> +    properties:
> +       mps,inc-off-time:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +          An array of 8, linearly increase power off sequence time
> +          slot/interval for each regulator must be one of following values
> +         * 0 to 15

This should be an enum, or a combination of minimum/maximum. And the
size of the array should be fixed too.

> +    properties:
> +       mps,inc-on-time:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +          An array of 8, linearly increase power on sequence time
> +          slot/interval for each regulator must be one of following values
> +          * 0 to 15

Ditto,

> +    properties:
> +       mps,switch-freq:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8"
> +       description: |
> +          switching frequency must be one of following values
> +          * 0 : 1.1MHz
> +          * 1 : 1.65MHz
> +          * 2 : 2.2MHz
> +          * 3 : 2.75MHz
> +          Defaults to 2.2 MHz if not specified.

enum and default

> +    properties:
> +       mps,buck-softstart:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +          An array of 4 contains soft start time of each buck, must be one of
> +          following values
> +          * 0 : 150us
> +          * 1 : 300us
> +          * 2 : 610us
> +          * 3 : 920us
> +          Defaults to 300us if not specified.

Same story than mps,inc-off-time

> +    properties:
> +       mps,buck-ovp:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +           An array of 4 contains over voltage protection of each buck, must be
> +           one of following values
> +           * 0 : disabled
> +           * 1 : enabled
> +           Defaults is enabled if not specified.

Ditto

> +    properties:
> +        mps,buck-phase-delay:
> +          - $ref: "/schemas/types.yaml#/definitions/uint8-array"
> +       description: |
> +           An array of 4 contains each buck phase delay must be one of following
> +           values
> +           * 0: 0deg
> +           * 1: 90deg
> +           * 2: 180deg
> +           * 3: 270deg
> +           Defaults to 0deg for buck1 & buck2, 90deg for buck3 & buck4 if not
> +           specified.

ditto

> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mpq7920@69 {
> +	  compatible = "mps,mpq7920";
> +          reg = <0x69>;
> +
> +          mps,switch-freq = <1>;
> +          mps,buck-softstart = /bits/ 8 <1 2 1 3>;
> +          mps,buck-ovp = /bits/ 8 <1 0 1 1>;
> +
> +          regulators {
> +            buck1 {

So regulators isn't a string after all?

If it's supposed to be a node, it should be type: object

and you can check that the node has a valid value using propertyNames

Maxime

--yqt63ee6xqsyxvu7
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCXftccwAKCRDj7w1vZxhR
xU4wAP9CjzOBCIts5MjFAsKirU/odMTpDcippKeKU9HZ77MYzgD/WHz9CiRGB65G
Rv6j2CQuzQV1YsVR9XBHZnZJro+J/Ag=
=awE3
-----END PGP SIGNATURE-----

--yqt63ee6xqsyxvu7--