Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1585341214-25285-1-git-send-email-tharvey@gateworks.com>
 <1585341214-25285-2-git-send-email-tharvey@gateworks.com> <20200417095831.GI2167633@dell>
In-Reply-To: <20200417095831.GI2167633@dell>
From:   Tim Harvey <tharvey@gateworks.com>
Date:   Mon, 20 Apr 2020 08:28:17 -0700
Message-ID: <CAJ+vNU32JcADb-p4+rpX5fzA8PkfknMr0Qd-UCMs-6AxLRGc5w@mail.gmail.com>
Subject: Re: [PATCH v8 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings
To:     Lee Jones <lee.jones@linaro.org>
Cc:     Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Linux HWMON List <linux-hwmon@vger.kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Device Tree Mailing List <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Robert Jones <rjones@gateworks.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Apr 17, 2020 at 2:57 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 27 Mar 2020, Tim Harvey wrote:
>
> > This patch adds documentation of device-tree bindings for the
> > Gateworks System Controller (GSC).
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>

<snip>

> > ---
> >  .../devicetree/bindings/mfd/gateworks-gsc.yaml     | 194 +++++++++++++++++++++
> >  1 file changed, 194 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > new file mode 100644
> > index 00000000..a96751c9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml
> > @@ -0,0 +1,194 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/gateworks-gsc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Gateworks System Controller multi-function device
>
> I'd prefer if you didn't use Linuxisums in DT docs.
>
> A 'multi-function device' isn't a thing - we made it up.
>
> Nowhere in the documentation [0] is the Gateworks System Controller
> described as a multi-function device.
>
> [0] http://trac.gateworks.com/wiki/gsc
>
> > +description: |
> > +  The GSC is a Multifunction I2C slave device with the following submodules:
>
> No it isn't.  It's a:
>
>   "The Gateworks System Controller (GSC) is a device present across
>    various Gateworks product families that provides a set of system
>    related feature such as the following (refer to the board hardware
>    user manuals to see what features are present)"
>
> > +   - Watchdog Timer
> > +   - GPIO
> > +   - Pushbutton controller
> > +   - Hardware Monitor with ADC's for temperature and voltage rails and
> > +     fan controller
>
> Why is "Monitor" capitalised, but "controller" is not?
>
> I would s/Monitor/monitor/ here.
>
> > +maintainers:
> > +  - Tim Harvey <tharvey@gateworks.com>
> > +  - Robert Jones <rjones@gateworks.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "gsc@[0-9a-f]{1,2}"
> > +  compatible:
> > +    const: gw,gsc
> > +
> > +  reg:
> > +    description: I2C device address
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  adc:
> > +    type: object
> > +    description: Optional Hardware Monitoring module
>
> Again, an odd thing to capitalise.
>
> > +    properties:
> > +      compatible:
> > +        const: gw,gsc-adc
> > +
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^channel@[0-9]+$":
> > +        type: object
> > +        description: |
> > +          Properties for a single ADC which can report cooked values
> > +          (ie temperature sensor based on thermister), raw values
> > +          (ie voltage rail with a pre-scaling resistor divider).
>
> /ie/i.e./
>
> > +        properties:
> > +          reg:
> > +            description: Register of the ADC
> > +            maxItems: 1
> > +
> > +          label:
> > +            description: Name of the ADC input
> > +
> > +          gw,mode:
> > +            description: |
> > +              conversion mode:
> > +                0 - temperature, in C*10
> > +                1 - pre-scaled voltage value
> > +                2 - scaled voltage based on an optional resistor divider
> > +                    and optional offset
> > +            allOf:
> > +              - $ref: /schemas/types.yaml#/definitions/uint32
>
> Rob just submitted a patch-set to remove 'allOf's from '$ref'
> properties.
>
> > +            enum: [0, 1, 2]
> > +
> > +          gw,voltage-divider-ohms:
> > +            description: values of resistors for divider on raw ADC input
>
> s/values/Values/
>
> > +            maxItems: 2
> > +            items:
> > +             minimum: 1000
> > +             maximum: 1000000
> > +
> > +          gw,voltage-offset-microvolt:
> > +            description: |
> > +              A positive voltage offset to apply to a raw ADC
> > +              (ie to compensate for a diode drop).
>
> s/ie/i.e/
>
> > +            minimum: 0
> > +            maximum: 1000000
> > +
> > +        required:
> > +          - gw,mode
> > +          - reg
> > +          - label
> > +
> > +    required:
> > +      - compatible
> > +      - "#address-cells"
> > +      - "#size-cells"
> > +
> > +patternProperties:
> > +  "^fan-controller@[0-9a-f]+$":
> > +    type: object
> > +    description: Optional FAN controller
>
> "Fan"
>
> > +    properties:
> > +      compatible:
> > +        const: gw,gsc-fan
> > +
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +      reg:
> > +        description: The fan controller base address
> > +        maxItems: 1
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - "#address-cells"
> > +      - "#size-cells"
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - "#interrupt-cells"
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        gsc@20 {
> > +            compatible = "gw,gsc";
> > +            reg = <0x20>;
> > +            interrupt-parent = <&gpio1>;
> > +            interrupts = <4 GPIO_ACTIVE_LOW>;
> > +            interrupt-controller;
> > +            #interrupt-cells = <1>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            adc {
> > +                compatible = "gw,gsc-adc";
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                channel@0 { /* A0: Board Temperature */
> > +                    reg = <0x00>;
> > +                    label = "temp";
> > +                    gw,mode = <0>;
> > +                };
> > +
> > +                channel@2 { /* A1: Input Voltage (raw ADC) */
> > +                    reg = <0x02>;
> > +                    label = "vdd_vin";
> > +                    gw,mode = <1>;
> > +                    gw,voltage-divider-ohms = <22100 1000>;
> > +                    gw,voltage-offset-microvolt = <800000>;
> > +                };
> > +
> > +                channel@b { /* A2: Battery voltage */
> > +                    reg = <0x0b>;
> > +                    label = "vdd_bat";
> > +                    gw,mode = <1>;
> > +                };
> > +            };
> > +
> > +            fan-controller@2c {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +                compatible = "gw,gsc-fan";
> > +                reg = <0x2c>;
> > +            };
> > +        };
> > +    };
>

Lee,

Thanks for the review. I will send a v9 once you have had time to
review the mfd driver patch in this series.

Best Regards,

Tim