linux-kernel - Re: [PATCH v4 1/2] dt-bindings: regulator: Add DT binding for cros-ec-regulator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <13c64346-b80b-9903-37fa-76b8fa47c01d@collabora.com>
Date:   Thu, 11 Jun 2020 10:14:45 +0200
From:   Enric Balletbo i Serra <enric.balletbo@...labora.com>
To:     Pi-Hsun Shih <pihsun@...omium.org>
Cc:     Nicolas Boichat <drinkcat@...omium.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        "open list:VOLTAGE AND CURRENT REGULATOR FRAMEWORK" 
        <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] dt-bindings: regulator: Add DT binding for
 cros-ec-regulator

Hi Pi-Hsun,

Thank you for the patch, one minor nitpick that in my opinion would be good to
address.

On 11/6/20 10:04, Pi-Hsun Shih wrote:
> Add DT binding documentation for cros-ec-regulator, a voltage regulator
> controlled by ChromeOS EC.
> 
> Signed-off-by: Pi-Hsun Shih <pihsun@...omium.org>
> ---
> Changes from v3:
> * Fix dt bindings file name.
> * Add full example.
> 
> Changes from v2:
> * No change
> 
> Changes from v1:
> * Change compatible string to google,regulator-cros-ec.
> * Use reg property in device tree.
> * Change license for dt binding according to checkpatch.pl.
> ---
>  .../regulator/google,cros-ec-regulator.yaml   | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml b/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
> new file mode 100644
> index 000000000000..7e341b65b2dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/google,cros-ec-regulator.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/google,cros-ec-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS EC controlled voltage regulators
> +
> +maintainers:
> +  - Pi-Hsun Shih <pihsun@...omium.org>
> +
> +description:
> +  Any property defined as part of the core regulator binding, defined in
> +  regulator.yaml, can also be used.
> +
> +allOf:
> +  - $ref: "regulator.yaml#"
> +
> +properties:
> +  compatible:
> +    const: google,regulator-cros-ec


Could we use "google,cros-ec-regulator" as compatible, so we are aligned with
the driver name and other cros-ec drivers? If you're agree you need to do
changes here, in the example and in the of table id.

In any case the file binding name should match the compatible name and that's
not the case now, as the file is google,cros-ec-regulator and the compatible is
google,regulator-cros-ec. So either fix the file binding name or the compatible
(I'd prefer the second).

Thanks,
  Enric

> +
> +  reg:
> +    maxItems: 1
> +    description: Identifier for the voltage regulator to ChromeOS EC.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cros_ec: ec@0 {
> +            compatible = "google,cros-ec-spi";
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            regulator@0 {
> +                compatible = "google,regulator-cros-ec";
> +                regulator-min-microvolt = <1800000>;
> +                regulator-max-microvolt = <3300000>;
> +                reg = <0>;
> +            };
> +        };
> +    };
> +...
>