Re: [PATCH 1/2] dt-bindings: add adpd188 schema

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Fri, 8 Oct 2021 14:27:46 +0300
<andrei.drimbarean@analog.com> wrote:

> From: Andrei Drimbarean <andrei.drimbarean@analog.com>
Hi Andrei

Welcome to IIO!

Anyhow, now for review.  Comments inline.

All patches need a patch description.  For a binding it's normal to put
a little bit about the device here.

> 
> Signed-off-by: Andrei Drimbarean <andrei.drimbarean@analog.com>
> ---
>  .../bindings/iio/light/adi,adpd188.yaml       | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/adi,adpd188.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/adi,adpd188.yaml b/Documentation/devicetree/bindings/iio/light/adi,adpd188.yaml
> new file mode 100644
> index 000000000000..3c08b0904803
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/adi,adpd188.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/adi,adpd188.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADPD188 device driver
> +
> +maintainers:
> +  - Andrei Drimbarean <andrei.drimbarean@analog.com>
> +
> +description: |
> +  Bindings for the Analog Devices ADPD188 device. 

What sort of device is it?  Give us a little bit of detail.

> The device support both SPI and I2C

Please keep the line length under 80 characters when it doesn't hurt readability to do so.

> +  interfaces. Datasheet can be found here:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/adpd188bi.pdf

Blank line after blocks such as here.

> +properties:
> +  compatible:
> +    enum:
> +      - adi,adpd188
> +
> +  reg:
> +    description: SPI chip select number or I2C slave address
> +    maxItems: 1

No real need to describe this as it's very standard.

    reg: true;

is probably enough info for this one.

> +
> +  interrupts:
> +    description: IRQ line for the device or device chain

Device chain? That's unusual enough that we should probably have
some more detail somewhere in this binding on what that means.

> +    maxItems: 1
> +
> +  spi-cpol: true
> +
> +  spi-cpha: true
> +
> +  spi-max-frequency:
> +    maximum: 10000000
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  status:
> +    const: 'okay'

That should not be in a binding document. It's a general property so
if we did list it, we would need it everywhere. It is also perfectly
acceptable to have a status that says the device isn't present.

> +
> +  adi,no-of-devices:
> +    description: Number of daisy-chained devices on an I2C bus
> +      string

More detail needed on this I think.
Also a default (0 or 1 based?)

> +    $ref: "http://devicetree.org/schemas/types.yaml#/definitions/uint8"

Are there any power supplies that should be described somewhere here?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      status = "okay";
> +
> +      adpd188@64 {
> +        compatible = "adi,adpd188";
> +        reg = <0x64>;
> +        interrupts = <9 1>;
> +        interrupt-parent = <&gpio>;
> +        adi,no-of-devices = <8>;
> +      };
> +    };
> +
> +additionalProperties: false
additionalProperties belongs above the 'examples' block.