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

[Krzysztof Kozlowski]

On 20/12/2022 16:47, 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/425858dc-59fe-2311-61ae-3b6dc77a2576@xxxxxxx
> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>
> Signed-off-by: Kwame Adwere <kwame.adwere@xxxxxxx>
> ---
> Changes since v5:
>   - Moved ioctl command definitions into header file.

How is this related to the binding?

>   - Version 5 patch review comments addressed.
>   - Corporate lawyer sign-off updated.
> 
>  .../bindings/uwb/nxp,uwb-sr1xx.yaml           | 65 +++++++++++++++++++
>  1 file changed, 65 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..2a1caa661633
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +

Drop blank line.

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

Drop double space and "device bindings"

> +
> +description: The nxp-sr1xx driver works for the NXP SR1XX series of Ultra Wide

Drop references to driver. Missing new line after description.

> +    Band devices namely, SR150 and SR100T devices, and uses UWB Controller Interface (UCI).

This does not look like wrapped according to Linux coding style.

> +    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

You need to describe the hardware here.

> +
> +maintainers:
> +  - Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,sr1xx

Really?

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

I stopped reviewing. Rest is not necessarily good, but we get there later.

Best regards,
Krzysztof