Re: [PATCH v4 3/3] misc: uwb: nxp: sr1xx: UWB driver support for sr1xx series chip

From: Krzysztof Kozlowski
Date: Sun May 29 2022 - 07:42:34 EST

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/).
>
> Internally driver will handle two modes of operation.
> 1.HBCI mode (sr1xx BootROM Code Interface)
> Firmware download uses HBCI ptotocol packet structure which is
> Nxp proprietary,Firmware File(.bin) stored in user space context
> and during device init sequence pick the firmware packet in chunk
> and send it to the driver with write() api call.
>
> After the firmware download sequence at the end UWBS will
> send device status notification and its indication of device entered
> UCI mode.
> Here after any command/response/notification will follow
> UCI packet structure.
>
> 2.UCI mode (UWB Command interface)
> Once Firmware download finishes sr1xx will switch to UCI mode.
> Then driver exchange command/response/notification as per the FIRA UCI
> standard format between user space and sr1xx device.
> Any response or notification received from sr1xx through SPI line
> will convey to user space.
>
> Its Interrupt based driver and IO Handshake needed with SR1XX Family of
> SOCs.
> This driver needs dts config update as per the sr1xx data sheet.
> Corresponding document available in Documentation/devicetree/bindings/uwb
>
> Message-ID: <20220504171337.3416983-1-manjunatha.venkatesh@xxxxxxx>

Same comment.


> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>
> ---
> Changes since v3:
> - Commit Message Description updated
> - Renamed file from sr1xx.c to nxp-sr1xx.c
> - Removed unwanted header files
> - Removed sr1xx.h file since its not required for single source file
>

Thank you for your patch. There is something to discuss/improve.

> drivers/misc/Kconfig | 12 +
> drivers/misc/Makefile | 1 +
> drivers/misc/nxp-sr1xx.c | 834 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 847 insertions(+)
> create mode 100644 drivers/misc/nxp-sr1xx.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 41d2bb0ae23a..4f81d5758940 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -483,6 +483,18 @@ config OPEN_DICE
>
> If unsure, say N.
>
> +config NXP_UWB

It is not a misc driver. It is communication driver, so closer to
network. Please find appropriate subsystem.

> + tristate "Nxp UCI(Uwb Command Interface) protocol driver support"

Isn't the name of company NXP?

> + depends on SPI
> + help
> + This option enables the UWB driver for Nxp sr1xx

The same...

> + device. Such device supports UCI packet structure,
> + FiRa compliant.
> +
> + Say Y here to compile support for sr1xx into the kernel or
> + say M to compile it as a module. The module will be called
> + sr1xx.ko
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 70e800e9127f..d4e6e4f1ec29 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
> obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
> obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
> obj-$(CONFIG_OPEN_DICE) += open-dice.o
> +obj-$(CONFIG_NXP_UWB) += nxp-sr1xx.o

Looks like wrong order.

> diff --git a/drivers/misc/nxp-sr1xx.c b/drivers/misc/nxp-sr1xx.c
> new file mode 100644
> index 000000000000..25648712a61b
> --- /dev/null
> +++ b/drivers/misc/nxp-sr1xx.c
> @@ -0,0 +1,834 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SPI driver for UWB SR1xx
> + * Copyright (C) 2018-2022 NXP.
> + *
> + * Author: Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>
> + */
> +#include <linux/miscdevice.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +
> +#define SR1XX_MAGIC 0xEA
> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
> +
> +#define UCI_HEADER_LEN 4
> +#define HBCI_HEADER_LEN 4
> +#define UCI_PAYLOAD_LEN_OFFSET 3
> +
> +#define UCI_EXT_PAYLOAD_LEN_IND_OFFSET 1
> +#define UCI_EXT_PAYLOAD_LEN_IND_OFFSET_MASK 0x80
> +#define UCI_EXT_PAYLOAD_LEN_OFFSET 2
> +
> +#define SR1XX_TXBUF_SIZE 4200
> +#define SR1XX_RXBUF_SIZE 4200
> +#define SR1XX_MAX_TX_BUF_SIZE 4200
> +
> +#define MAX_RETRY_COUNT_FOR_IRQ_CHECK 100
> +#define MAX_RETRY_COUNT_FOR_HANDSHAKE 1000
> +

(...)

> +
> +static const struct dev_pm_ops sr1xx_dev_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(
> + sr1xx_dev_suspend, sr1xx_dev_resume) };
> +
> +static struct spi_driver sr1xx_driver = {
> + .driver = {
> + .name = "sr1xx",
> + .pm = &sr1xx_dev_pm_ops,
> + .bus = &spi_bus_type,
> + .owner = THIS_MODULE,

No, this was removed some years ago... Please run all regular static
tools on your code sparse, smatch and coccinelle.

There is no need for us to do the review of things pointed out by tools.


> + .of_match_table = sr1xx_dt_match,
> + },
> + .probe = sr1xx_probe,
> + .remove = sr1xx_remove,
> +};
> +
> +static int __init sr1xx_dev_init(void)
> +{
> + return spi_register_driver(&sr1xx_driver);
> +}
> +
> +module_init(sr1xx_dev_init);
> +
> +static void __exit sr1xx_dev_exit(void)
> +{
> + spi_unregister_driver(&sr1xx_driver);
> +}

Why this isn't a module_spi_driver?

> +
> +module_exit(sr1xx_dev_exit);
> +
> +MODULE_AUTHOR("Manjunatha Venkatesh <manjunatha.venkatesh@xxxxxxx>");
> +MODULE_DESCRIPTION("NXP SR1XX SPI driver");
> +MODULE_LICENSE("GPL");


Best regards,
Krzysztof