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;
MIME-Version: 1.0
References: <20190416084552.1538-1-masneyb@onstation.org> <20190422135238.27b9892d@archlinux>
In-Reply-To: <20190422135238.27b9892d@archlinux>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Tue, 23 Apr 2019 20:34:13 -0500
Message-ID: <CAL_JsqJo4jA=_dLJ7QJ-LOhSYrXDAcVN01119do0a+y4odpFng@mail.gmail.com>
Subject: Re: [PATCH 1/2] dt-bindings: iio: tsl2772: convert bindings to YAML format
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Brian Masney <masneyb@onstation.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald <pmeerw@pmeerw.net>,
        Mark Rutland <mark.rutland@arm.com>,
        "open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@vger.kernel.org>,
        devicetree@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Apr 22, 2019 at 7:52 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 16 Apr 2019 04:45:51 -0400
> Brian Masney <masneyb@onstation.org> wrote:
>
> > Convert the tsl2772 device tree bindings to the new YAML format.
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> Hi Brian,
>
> Good to see this.  I'm afraid it's all a bit new to me so what
> I haven't yet understood is how prescriptive we should be.
> For example, are the phandle references below needed or not?
>
> So for a while yet I'm going to be relying on Rob and others
> to review these and put me on the right track.
>
> Jonathan
>
> > ---
> >  .../devicetree/bindings/iio/light/tsl2772.txt | 42 ---------
> >  .../bindings/iio/light/tsl2772.yaml           | 85 +++++++++++++++++++
> >  2 files changed, 85 insertions(+), 42 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/iio/light/tsl2772.txt
> >  create mode 100644 Documentation/devicetree/bindings/iio/light/tsl2772.yaml
> >

> > diff --git a/Documentation/devicetree/bindings/iio/light/tsl2772.yaml b/Documentation/devicetree/bindings/iio/light/tsl2772.yaml
> > new file mode 100644
> > index 000000000000..b3ac182288d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/light/tsl2772.yaml
> > @@ -0,0 +1,85 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

Do you have the rights on the original file to add BSD license?

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/light/tsl2772.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMS/TAOS Ambient Light Sensor (ALS) and Proximity Detector
> > +
> > +maintainers:
> > +  - Brian Masney <masneyb@onstation.org>
> > +
> > +description: |
> > +  Ambient light sensing and proximity detection with an i2c interface.
> > +  https://ams.com/documents/20143/36005/TSL2772_DS000181_2-00.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - amstaos,tsl2571
> > +      - amstaos,tsl2671
> > +      - amstaos,tmd2671
> > +      - amstaos,tsl2771
> > +      - amstaos,tmd2771
> > +      - amstaos,tsl2572
> > +      - amstaos,tsl2672
> > +      - amstaos,tmd2672
> > +      - amstaos,tsl2772
> > +      - amstaos,tmd2772
> > +      - avago,apds9930
> > +
> > +  reg:
> > +    description: The I2C address of the device

No need for description on common properties unless you have something
unique to add.

> > +    maxItems: 1
> > +
> > +  amstaos,proximity-diodes:
> > +    description: Proximity diodes to enable
> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32-array
> > +      - minItems: 1
> > +        maxItems: 2
> > +        items:
> > +          minimum: 0
> > +          maximum: 1
>
> Do we need to represent that these can't be <1 0> ?
> (specified in old docs)
> We also have a tighter spec than the uint32-array format in types.yaml
> as don't allow <0>, <1> under the current binding where only <0, 1> is
> allowed.

The uint32-array definition is loose to avoid lots of warnings on
common properties where we can't define the exact size. Over time we
need to tighten up the dts syntax to be consistent (I have a dtc patch
which can spew out lots of warnings to fix these).

So this case is correct. '<0, 1>' means a single array with 2
elements. '<0>, <1>' means 2 arrays each with a single element. There
shouldn't be lots of occurrences.

> > +
> > +  interrupts:
> > +    description: Interrupt generated by the device

No need for description.

> > +    maxItems: 1
> > +
> > +  led-max-microamp:
> > +    description: Current for the proximity LED

This device only has proximity LEDs, right?

> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint32
> > +      - enum: [13000, 25000, 50000, 100000]

We should assume a common schema for 'led-max-microamp', so we just
need the enum.

> > +
> > +  vdd-supply:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Regulator that provides power to the sensor

Same here for '*-supply'. So just the description is enough.

> > +
> > +  vddio-supply:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Regulator that provides power to the bus
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        tsl2772@39 {

proximity-sensor@39

> > +                compatible = "amstaos,tsl2772";
> > +                reg = <0x39>;
> > +                interrupts-extended = <&msmgpio 61 IRQ_TYPE_EDGE_FALLING>;
> > +                vdd-supply = <&pm8941_l17>;
> > +                vddio-supply = <&pm8941_lvs1>;
> > +                amstaos,proximity-diodes = <0>;
> > +                led-max-microamp = <100000>;
> > +        };
> > +    };
> > +...
>