Re: [PATCH v5 1/2] dt-bindings: uwb: Device tree information for Nxp SR1XX SOCs

[Rob Herring]

On Wed, Sep 14, 2022 at 07:59:43PM +0530, Manjunatha Venkatesh wrote:
> Ultra-wideband (UWB) is a short-range wireless communication protocol.
> 
> NXP has SR1XX family of UWB Subsystems (UWBS) devices. SR1XX SOCs
> are FiRa Compliant. SR1XX SOCs are flash less devices and they need
> Firmware Download on every device boot. More details on the SR1XX Family
> can be found at https://www.nxp.com/products/:UWB-TRIMENSION
> 
> The sr1xx driver work the SR1XX Family of UWBS, and uses UWB Controller
> Interface (UCI).  The corresponding details are available in the FiRa
> Consortium Website (https://www.firaconsortium.org/).
> 
> Link: https://lore.kernel.org/r/20220527184351.3829543-2-manjunatha.venkatesh@xxxxxxx
> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>
> ---
> Changes since v4:
>   - Devicetree documentation updated as per the review comments
>   - Text Aligment related issues are addressed
> 
>  .../bindings/uwb/nxp,uwb-sr1xx.yaml           | 63 +++++++++++++++++++

Use compatible string for filename.

>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> new file mode 100644
> index 000000000000..0f8c774b8306
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

Why 3 clause? Everywhere else for bindings is using BSD-2-Clause

> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/uwb/nxp,uwb-sr1xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ultra Wide Band(UWB)driver support for NXP SR1XX SOCs family

Bindings describe h/w devices, not drivers.

> +
> +description: The nxp-sr1xx driver works for the NXP SR1XX series of Ultra Wide
> +    Band devices namely, SR150 and SR100T devices, and uses UWB Controller Interface (UCI).
> +    The corresponding details are available in the FiRa Consortium Website.
> +    (https://www.firaconsortium.org/). More details on the SR1XX Family can be
> +    found at https://www.nxp.com/products/:UWB-TRIMENSION

Blank line.

> +maintainers:
> +  - Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,sr1xx
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 45000000
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-max-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /* for Raspberry Pi with pin control stuff for GPIO irq */
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    fragment@1 {
> +        target = <&spi0>;
> +        __overlay__ {

Remove overlay details from example. This should be just 'spi {'.

The schemas ignore '__' nodes so your example is not getting tested (and 
has errors).

> +            /* needed to avoid dtc warning */
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            status = "disabled";

Examples should be enabled. Drop.

> +
> +            sr1xx: sr1xx@0 {
> +                compatible = "nxp,sr1xx";
> +                reg = <0>;    /* CE0 */
> +                /* GPIO_24 (PIN 18) Host Irq*/
> +                nxp,sr1xx-irq-gpio = <&gpio 24 0>;

Use 'interrupts'. Also, not documented.

> +                /* GPIO_18(PIN 12) Chip Enable*/
> +                nxp,sr1xx-ce-gpio = <&gpio 18 0>;

-gpios is the preferred form.

> +                /* GPIO_23(PIN 16) Read Indication from Host to SR1xx*/
> +                nxp,sr1xx-ri-gpio = <&gpio 23 0>;
> +                /*max supported frequency */
> +                spi-max-frequency = <20000000>;
> +            };
> +        };
> +    };
> --
> 2.37.2
> 
>