mbox series

[v5,0/2] Uwb: Nxp: Driver for SR1XX SOCs Patch Series

Message ID 20220914142944.576482-1-manjunatha.venkatesh@nxp.com
Headers show
Series Uwb: Nxp: Driver for SR1XX SOCs Patch Series | expand

Message

Manjunatha Venkatesh Sept. 14, 2022, 2:29 p.m. UTC
Here's a fifth version of a Nxp Uwb SR1xx driver.
Following changes are done with respect to patch series.
  - Device Tree Document for SR1XX SOCs patch added
  - sr1xx driver updated with previous review comment

Manjunatha Venkatesh (2):
  dt-bindings: uwb: Device tree information for Nxp SR1XX SOCs
  misc: nxp-sr1xx: UWB driver support for sr1xx series chip

 .../bindings/uwb/nxp,uwb-sr1xx.yaml           |  63 ++
 MAINTAINERS                                   |   6 +
 drivers/misc/Kconfig                          |  11 +
 drivers/misc/Makefile                         |   3 +-
 drivers/misc/nxp-sr1xx.c                      | 794 ++++++++++++++++++
 5 files changed, 876 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
 create mode 100644 drivers/misc/nxp-sr1xx.c

--
2.37.2

Comments

Arnd Bergmann Sept. 14, 2022, 2:36 p.m. UTC | #1
On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,sr1xx
> +

You should not have wildcard names in the compatible string.
Make this a specific model number without 'xx', and
have the devices list their own name along with the oldest
one they are compatible with.

     Arnd
Rob Herring (Arm) Sept. 16, 2022, 7:26 p.m. UTC | #2
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@nxp.com
> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> ---
> 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@nxp.com>
> +
> +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
> 
>
Manjunatha Venkatesh Oct. 7, 2022, 11:39 a.m. UTC | #3
On 9/14/2022 8:06 PM, Arnd Bergmann wrote:
> Caution: EXT Email
>
> On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - nxp,sr1xx
>> +
> You should not have wildcard names in the compatible string.
> Make this a specific model number without 'xx', and
> have the devices list their own name along with the oldest
> one they are compatible with.
>
>       Arnd
This driver is common for both sr100 and sr150,so we have used sr1xx
naming convention or can we use name with highest version(sr150)?
Krzysztof Kozlowski Oct. 7, 2022, 12:30 p.m. UTC | #4
On 07/10/2022 13:39, Manjunatha Venkatesh wrote:
> 
> On 9/14/2022 8:06 PM, Arnd Bergmann wrote:
>> Caution: EXT Email
>>
>> On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - nxp,sr1xx
>>> +
>> You should not have wildcard names in the compatible string.
>> Make this a specific model number without 'xx', and
>> have the devices list their own name along with the oldest
>> one they are compatible with.
>>
>>       Arnd
> This driver is common for both sr100 and sr150,so we have used sr1xx
> naming convention or can we use name with highest version(sr150)?

In general each device needs its compatible, so you would need two of
them. However if one is compatible with the other, then express it as
well. IOW, driver binds to one compatible, binding describes both (one
as fallback). There are many, many of such examples in the kernel.

Best regards,
Krzysztof