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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 27/05/2022 20:43, 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/).
> 
> Message-ID: <20220504171337.3416983-1-manjunatha.venkatesh@nxp.com>

This is a confusing tag... Why do you need it and what does it mean in
Linux kernel process?

> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> ---
>  .../bindings/uwb/nxp,uwb-sr1xx.yaml           | 67 +++++++++++++++++++
>  1 file changed, 67 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..226fec908968
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR 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

No references to driver. Please describe here the title of the hardware.

> +
> +maintainers:
> +  - Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> +
> +description: The sr1xx driver work for the NXP SR1XX Family of Ultra Wide
> +    Band Subsystem(UWBS), 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

The same. Additionally use proper white-spaces. Before every '('. After
every '.'.

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - nxp,sr1xx
> +
> +      - const: nxp,sr1xx

This is wrong and does not make any sense. You also did not test the
bindings.

Please test them before submitting. No need to waste reviewers time for
basic automation tasks.

Also do not include any wildcards (1xx) but specific model name.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-names
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi2: spi@ffd68000 {

No need for alias.

> +		compatible = "arm,pl022", "arm,primecell";
> +		reg = <0x0 0xffd68000 0x0 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&crg_ctrl HI3660_CLK_GATE_SPI2>;
> +		clock-names = "apb_pclk";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&spi2_pmx_func &spi2_cfg_func>;
> +		num-cs = <1>;
> +		cs-gpios = <&gpio27 2 0>;
> +		status = "ok";

No point of enabling this.

> +	sr1xx@0 {

The indentation and look of it is unacceptable...

> +	 compatible = "nxp,sr1xx";
> +	 reg = <0>;
> +	 nxp,sr1xx-irq = <&gpio26 1 0>;
> +	 nxp,sr1xx-ce = <&gpio2 5 0>;
> +	 nxp,sr1xx-ri = <&gpio24 1 0>;
> +	 spi-max-frequency = <20000000>;
> +	};
> +    };
> +
> +...


Best regards,
Krzysztof