Re: [PATCH v2] iio: accel: yamaha-yas: Add DT bindings

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 17 Nov 2020 13:18:10 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds device tree bindings for the Yamaha YAS5xx
> magnetometers/compass sensors.
> 
> Cc: devicetree@vger.kernel.org
> Cc: phone-devel@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Hi Linus,

A few minor things inline.  Looking forward to the driver :)

Jonathan

> ---
> ChangeLog v1->v2:
> - Add Yamaha to the vendor list, I was surprised to find
>   they were not yet listed.
> 
> I am still working on the actual driver for the magnetometer
> but why not send out the DT bindings for review, the
> hardware variants are easy to describe. This makes it possibe
> for people to include these magnetometers in device
> trees.
> ---
>  .../bindings/iio/magnetometer/yamaha,yas.yaml | 80 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/yamaha,yas.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/yamaha,yas.yaml b/Documentation/devicetree/bindings/iio/magnetometer/yamaha,yas.yaml
> new file mode 100644
> index 000000000000..6fc4cfe4a417
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/yamaha,yas.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/yamaha,yas.yaml#

Hmm. Is the generic name safe?   Might we get some future devices
that this binding doesn't cover?

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Yamaha YAS5xx magnetometer sensors
> +
> +maintainers:
> +  - Linus Walleij <linus.walleij@linaro.org>
> +
> +description:
> +  The Yamaha YAS5xx magnetometers is a line of 3-axis magnetometers
> +  first introduced by Yamaha in 2006 with the YAS529. Over the years
> +  this magnetometer has been minatyrized and appeared in a number of
> +  different variants.

Run a spell checker over this.  miniaturised

> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - yamaha,yas529
> +          - yamaha,yas530
> +          - yamaha,yas532
> +          - yamaha,yas533
> +          - yamaha,yas535
> +          - yamaha,yas536
> +          - yamaha,yas537
> +          - yamaha,yas539
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: The YAS5xx sensors has a RSTN pin used to reset
> +      the logic inside the sensor. This GPIO line should connect
> +      to that pin and be marked as GPIO_ACTIVE_LOW.
> +
> +  interrupts:
> +    maxItems: 1
> +    description: Interrupt for INT pin for variants that support
> +      interrupt generation. This polarity, whether the interrupt
> +      is active on the rising or the falling edge, is configurable
> +      in the hardware.

Worth adding the if magic to check this isn't provide for variants
that don't have an interrupt pin?
I guess it's fairly unlikely anyone would have a dt file that claims
there is an interrupt that doesn't exist, but nice to keep the
bindings as restrictive as can be easily done.

> +
> +  vdd-supply:
> +    description: An optional regulator providing core power supply
> +      on the VDD pin, typically 1.8 V or 3.0 V.
> +
> +  iovdd-supply:
> +    description: An optional regulator providing I/O power supply
> +      for the I2C interface on the IOVDD pin, typically 1.8 V.
> +      This is not present on all variants of the component, some
> +      have only the VDD voltage.

Likewise, can we restrict this to the right variants.

> +
> +  mount-matrix:
> +    description: An optional 3x3 mounting rotation matrix.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        magnetometer@2e {
> +          compatible = "yamaha,yas530";
> +          reg = <0x2e>;
> +          vdd-supply = <&ldo1_reg>;
> +          iovdd-supply = <&ldo2_reg>;
> +          reset-gpios = <&gpio6 12 GPIO_ACTIVE_LOW>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 2735be1a8470..0340674c72bd 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1210,6 +1210,8 @@ patternProperties:
>      description: Shenzhen Xunlong Software CO.,Limited
>    "^xylon,.*":
>      description: Xylon
> +  "^yamaha,.*":
> +    description: Yamaha Corporation
>    "^ylm,.*":
>      description: Shenzhen Yangliming Electronic Technology Co., Ltd.
>    "^yna,.*":