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

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

[Manjunatha Venkatesh]

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

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

[Manjunatha Venkatesh]

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 +++++++++++++++++++
 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)
+
+%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
+
+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
+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__ {
+            /* needed to avoid dtc warning */
+            #address-cells = <1>;
+            #size-cells = <0>;
+            status = "disabled";
+
+            sr1xx: sr1xx@0 {
+                compatible = "nxp,sr1xx";
+                reg = <0>;    /* CE0 */
+                /* GPIO_24 (PIN 18) Host Irq*/
+                nxp,sr1xx-irq-gpio = <&gpio 24 0>;
+                /* GPIO_18(PIN 12) Chip Enable*/
+                nxp,sr1xx-ce-gpio = <&gpio 18 0>;
+                /* 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

[PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

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

Link: https://lore.kernel.org/r/20220527184351.3829543-4-manjunatha.venkatesh@nxp.com
Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
---
Changes since v4:
  - Commit Message Description updated
  - Funtion prototype related warnings fixed
  - Devicetree documentation updated as per the review comments
  - ACPI platform support added
  - Text Aligment related issues are addressed
  - Some Corner case scenarios error handling fixed as per review comments
  - Debug Logging improvement done as per review comments
  - Licensing part updated to GPL-2.0 OR BSD-3-Clause

 MAINTAINERS              |   6 +
 drivers/misc/Kconfig     |  11 +
 drivers/misc/Makefile    |   3 +-
 drivers/misc/nxp-sr1xx.c | 794 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 813 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/nxp-sr1xx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 936490dcc97b..17413d93f5e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14695,6 +14695,12 @@ S:	Orphan
 F:	Documentation/devicetree/bindings/net/nfc/nxp,nci.yaml
 F:	drivers/nfc/nxp-nci

+NXP-SR1XX UWB DRIVER
+M:	Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
+F:	drivers/misc/nxp-sr1xx.c
+
 NXP i.MX 8QXP/8QM JPEG V4L2 DRIVER
 M:	Mirela Rabulea <mirela.rabulea@nxp.com>
 R:	NXP Linux Team <linux-imx@nxp.com>
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 94e9fb4cdd76..31921850c71b 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -471,6 +471,17 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.

+config NXP_UWB
+    tristate "NXP UCI(Uwb Command Interface) protocol driver support"
+    depends on SPI
+    help
+      This option enables the UWB driver for NXP sr1xx device.
+      Such device supports UCI packet structure, FiRa compliant.
+
+      Say Y here to compile support for nxp-sr1xx into the kernel or
+      say M to compile it as a module. The module will be called
+      nxp-sr1xx.ko
+
 config OPEN_DICE
 	tristate "Open Profile for DICE driver"
 	depends on OF_RESERVED_MEM
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 2be8542616dd..ee8ca32c66f6 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -60,4 +60,5 @@ 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_VCPU_STALL_DETECTOR)	+= vcpu_stall_detector.o
+obj-$(CONFIG_NXP_UWB) 		+= nxp-sr1xx.o
diff --git a/drivers/misc/nxp-sr1xx.c b/drivers/misc/nxp-sr1xx.c
new file mode 100644
index 000000000000..6ca9a2b54b86
--- /dev/null
+++ b/drivers/misc/nxp-sr1xx.c
@@ -0,0 +1,794 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+/*
+ * Copyright 2018-2022 NXP.
+ *
+ * SPI driver for UWB SR1xx
+ * Author: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
+ */
+
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/spi/spi.h>
+#include <linux/uaccess.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
+
+/* Macro to define SPI clock frequency */
+#define SR1XX_SPI_CLOCK 16000000L
+#define WAKEUP_SRC_TIMEOUT (2000)
+
+/* Maximum UCI packet size supported from the driver */
+#define MAX_UCI_PKT_SIZE 4200
+
+struct sr1xx_spi_platform_data {
+	struct gpio_desc *gpiod_irq;	/* SR1XX will interrupt host for any ntf */
+	struct gpio_desc *gpiod_ce;	/* SW reset gpio */
+	struct gpio_desc *gpiod_spi_handshake;	/* Host ready to read data */
+};
+
+/* Device specific macro and structure */
+struct sr1xx_dev {
+	wait_queue_head_t read_wq;	/* Wait queue for read interrupt */
+	struct spi_device *spi;	/* Spi device structure */
+	struct miscdevice sr1xx_device;	/* Char device as misc driver */
+	unsigned int ce_gpio;	/* SW reset gpio */
+	unsigned int irq_gpio;	/* SR1XX will interrupt host for any ntf */
+	unsigned int spi_handshake_gpio;	/* Host ready to read data */
+	bool irq_enabled;	/* Flag to indicate disable/enable irq sequence */
+	bool irq_received;	/* Flag to indicate that irq is received */
+	spinlock_t irq_enabled_lock;	/* Spin lock for read irq */
+	unsigned char *tx_buffer;	/* Transmit buffer */
+	unsigned char *rx_buffer;	/* Receive buffer */
+	unsigned int write_count;	/* Holds nubers of byte written */
+	unsigned int read_count;	/* Hold nubers of byte read */
+	struct mutex
+	 sr1xx_access_lock;	/* Lock used to synchronize read and write */
+	size_t total_bytes_to_read;	/* Total bytes read from the device */
+	bool is_extended_len_bit_set;	/* Variable to check ext payload Len */
+	bool read_abort_requested;	/* Used to indicate read abort request */
+	bool is_fw_dwnld_enabled;	/* Used to indicate fw download mode */
+	int mode;		/* Indicate write or read mode */
+	long timeout_in_ms;	/* Wait event interrupt timeout in ms */
+};
+
+/* Power enable/disable and read abort ioctl arguments */
+enum { PWR_DISABLE = 0, PWR_ENABLE, ABORT_READ_PENDING };
+
+enum spi_status_codes {
+	TRANSCEIVE_SUCCESS,
+	TRANSCEIVE_FAIL,
+	IRQ_WAIT_REQUEST,
+	IRQ_WAIT_TIMEOUT
+};
+
+/* Spi write/read operation mode */
+enum spi_operation_modes { SR1XX_WRITE_MODE, SR1XX_READ_MODE };
+static int sr1xx_dev_open(struct inode *inode, struct file *filp)
+{
+	struct sr1xx_dev *sr1xx_dev =
+	    container_of(filp->private_data, struct sr1xx_dev, sr1xx_device);
+
+	filp->private_data = sr1xx_dev;
+	return 0;
+}
+
+static void sr1xx_disable_irq(struct sr1xx_dev *sr1xx_dev)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sr1xx_dev->irq_enabled_lock, flags);
+	if ((sr1xx_dev->irq_enabled)) {
+		disable_irq_nosync(sr1xx_dev->spi->irq);
+		sr1xx_dev->irq_received = true;
+		sr1xx_dev->irq_enabled = false;
+	}
+	spin_unlock_irqrestore(&sr1xx_dev->irq_enabled_lock, flags);
+}
+
+static void sr1xx_enable_irq(struct sr1xx_dev *sr1xx_dev)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sr1xx_dev->irq_enabled_lock, flags);
+	if (!sr1xx_dev->irq_enabled) {
+		enable_irq(sr1xx_dev->spi->irq);
+		sr1xx_dev->irq_enabled = true;
+		sr1xx_dev->irq_received = false;
+	}
+	spin_unlock_irqrestore(&sr1xx_dev->irq_enabled_lock, flags);
+}
+
+static irqreturn_t sr1xx_dev_irq_handler(int irq, void *dev_id)
+{
+	struct sr1xx_dev *sr1xx_dev = dev_id;
+
+	sr1xx_disable_irq(sr1xx_dev);
+	/* Wake up waiting readers */
+	wake_up(&sr1xx_dev->read_wq);
+	if (device_may_wakeup(&sr1xx_dev->spi->dev))
+		pm_wakeup_event(&sr1xx_dev->spi->dev, WAKEUP_SRC_TIMEOUT);
+	return IRQ_HANDLED;
+}
+
+static long sr1xx_dev_ioctl(struct file *filp, unsigned int cmd,
+			    unsigned long arg)
+{
+	int ret = 0;
+	struct sr1xx_dev *sr1xx_dev = NULL;
+
+	sr1xx_dev = filp->private_data;
+
+	switch (cmd) {
+	case SR1XX_SET_PWR:
+		if (arg == PWR_ENABLE) {
+			gpio_set_value(sr1xx_dev->ce_gpio, 1);
+			usleep_range(10000, 12000);
+		} else if (arg == PWR_DISABLE) {
+			gpio_set_value(sr1xx_dev->ce_gpio, 0);
+			sr1xx_disable_irq(sr1xx_dev);
+			usleep_range(10000, 12000);
+		} else if (arg == ABORT_READ_PENDING) {
+			sr1xx_dev->read_abort_requested = true;
+			sr1xx_disable_irq(sr1xx_dev);
+			/* Wake up waiting readers */
+			wake_up(&sr1xx_dev->read_wq);
+		}
+		break;
+	case SR1XX_SET_FWD:
+		if (arg == 1) {
+			sr1xx_dev->is_fw_dwnld_enabled = true;
+			sr1xx_dev->read_abort_requested = false;
+		} else if (arg == 0) {
+			sr1xx_dev->is_fw_dwnld_enabled = false;
+		}
+		break;
+	default:
+		dev_err(&sr1xx_dev->spi->dev, " Error case");
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+/**
+ * sr1xx_wait_for_irq_gpio_low
+ *
+ * Function to wait till irq gpio goes low state
+ *
+ */
+static void sr1xx_wait_for_irq_gpio_low(struct sr1xx_dev *sr1xx_dev)
+{
+	int retry_count = 0;
+
+	do {
+		udelay(10);
+		retry_count++;
+		if (retry_count == MAX_RETRY_COUNT_FOR_HANDSHAKE) {
+			dev_info(&sr1xx_dev->spi->dev,
+				 "Slave not released the IRQ even after 10ms");
+			break;
+		}
+	} while (gpio_get_value(sr1xx_dev->irq_gpio));
+}
+
+/**
+ * sr1xx_dev_transceive
+ * @op_mode indicates write/read operation
+ *
+ * Write and Read logic implemented under same api with
+ * mutex lock protection so write and read synchronized
+ *
+ * During Uwb ranging sequence(read) need to block write sequence
+ * in order to avoid some race condition scenarios.
+ *
+ * Returns     : Number of bytes write/read if read is success else (-1)
+ *               otherwise indicate each error code
+ */
+static int sr1xx_dev_transceive(struct sr1xx_dev *sr1xx_dev, int op_mode,
+				int count)
+{
+	int ret, retry_count;
+
+	mutex_lock(&sr1xx_dev->sr1xx_access_lock);
+	sr1xx_dev->mode = op_mode;
+	sr1xx_dev->total_bytes_to_read = 0;
+	sr1xx_dev->is_extended_len_bit_set = 0;
+	ret = -1;
+	retry_count = 0;
+
+	switch (sr1xx_dev->mode) {
+	case SR1XX_WRITE_MODE:{
+			sr1xx_dev->write_count = 0;
+			/* UCI Header write */
+			ret = spi_write(sr1xx_dev->spi, sr1xx_dev->tx_buffer,
+					UCI_HEADER_LEN);
+			if (ret < 0) {
+				ret = -EIO;
+				dev_err(&sr1xx_dev->spi->dev,
+					"spi_write header : Failed.\n");
+				goto transceive_end;
+			} else {
+				count -= UCI_HEADER_LEN;
+			}
+			if (count > 0) {
+				/* In between header write and payload write slave need some time */
+				usleep_range(30, 50);
+				/* UCI Payload write */
+				ret = spi_write(sr1xx_dev->spi,
+						sr1xx_dev->tx_buffer +
+						UCI_HEADER_LEN, count);
+				if (ret < 0) {
+					ret = -EIO;
+					dev_err(&sr1xx_dev->spi->dev,
+						"spi_write payload : Failed.\n");
+					goto transceive_end;
+				}
+			}
+			sr1xx_dev->write_count = count + UCI_HEADER_LEN;
+			ret = TRANSCEIVE_SUCCESS;
+		}
+		break;
+	case SR1XX_READ_MODE:{
+			if (!gpio_get_value(sr1xx_dev->irq_gpio)) {
+				dev_err(&sr1xx_dev->spi->dev,
+					"IRQ might have gone low due to write ");
+				ret = IRQ_WAIT_REQUEST;
+				goto transceive_end;
+			}
+			retry_count = 0;
+			gpio_set_value(sr1xx_dev->spi_handshake_gpio, 1);
+			while (gpio_get_value(sr1xx_dev->irq_gpio)) {
+				if (retry_count ==
+				    MAX_RETRY_COUNT_FOR_IRQ_CHECK)
+					break;
+				udelay(10);
+				retry_count++;
+			}
+			sr1xx_enable_irq(sr1xx_dev);
+			sr1xx_dev->read_count = 0;
+			retry_count = 0;
+			/* Wait for inetrrupt upto 500ms */
+			ret =
+			    wait_event_interruptible_timeout(sr1xx_dev->read_wq,
+							     sr1xx_dev->irq_received,
+							     sr1xx_dev->timeout_in_ms);
+			if (ret == 0) {
+				dev_err(&sr1xx_dev->spi->dev,
+					"wait_event_interruptible timeout() : Failed.\n");
+				ret = IRQ_WAIT_TIMEOUT;
+				goto transceive_end;
+			}
+			if (!gpio_get_value(sr1xx_dev->irq_gpio)) {
+				dev_err(&sr1xx_dev->spi->dev,
+					"Second IRQ is Low");
+				ret = -1;
+				goto transceive_end;
+			}
+			ret =
+			    spi_read(sr1xx_dev->spi,
+				     (void *)sr1xx_dev->rx_buffer,
+				     UCI_HEADER_LEN);
+			if (ret < 0) {
+				dev_err(&sr1xx_dev->spi->dev,
+					"sr1xx_dev_read: spi read error %d\n ",
+					ret);
+				goto transceive_end;
+			}
+			sr1xx_dev->is_extended_len_bit_set =
+			    (sr1xx_dev->rx_buffer[UCI_EXT_PAYLOAD_LEN_IND_OFFSET] &
+			     UCI_EXT_PAYLOAD_LEN_IND_OFFSET_MASK);
+			sr1xx_dev->total_bytes_to_read =
+			    sr1xx_dev->rx_buffer[UCI_PAYLOAD_LEN_OFFSET];
+			if (sr1xx_dev->is_extended_len_bit_set) {
+				sr1xx_dev->total_bytes_to_read =
+				    ((sr1xx_dev->total_bytes_to_read << 8) |
+				     sr1xx_dev->rx_buffer[UCI_EXT_PAYLOAD_LEN_OFFSET]);
+			}
+			if (sr1xx_dev->total_bytes_to_read >
+			    (MAX_UCI_PKT_SIZE - UCI_HEADER_LEN)) {
+				dev_err(&sr1xx_dev->spi->dev,
+					"Length %d  exceeds the max limit %d....",
+					(int)sr1xx_dev->total_bytes_to_read,
+					(int)MAX_UCI_PKT_SIZE);
+				ret = -1;
+				goto transceive_end;
+			}
+			if (sr1xx_dev->total_bytes_to_read > 0) {
+				ret = spi_read(sr1xx_dev->spi,
+					       (void *)(sr1xx_dev->rx_buffer +
+							UCI_HEADER_LEN),
+					       sr1xx_dev->total_bytes_to_read);
+				if (ret < 0) {
+					dev_err(&sr1xx_dev->spi->dev,
+						"sr1xx_dev_read: spi read error.. %d\n ",
+						ret);
+					goto transceive_end;
+				}
+			}
+			sr1xx_dev->read_count =
+			    (unsigned int)(sr1xx_dev->total_bytes_to_read +
+					   UCI_HEADER_LEN);
+			sr1xx_wait_for_irq_gpio_low(sr1xx_dev);
+			ret = TRANSCEIVE_SUCCESS;
+			gpio_set_value(sr1xx_dev->spi_handshake_gpio, 0);
+		} break;
+	default:
+		dev_err(&sr1xx_dev->spi->dev, "invalid operation .....");
+		break;
+	}
+transceive_end:
+	if (sr1xx_dev->mode == SR1XX_READ_MODE)
+		gpio_set_value(sr1xx_dev->spi_handshake_gpio, 0);
+
+	mutex_unlock(&sr1xx_dev->sr1xx_access_lock);
+	return ret;
+}
+
+/**
+ * sr1xx_hbci_write
+ *
+ * Used to write hbci(SR1xx BootROM Command Interface) packets
+ * during firmware download sequence.
+ *
+ * Returns: TRANSCEIVE_SUCCESS on success or -1 on fail
+ */
+static int sr1xx_hbci_write(struct sr1xx_dev *sr1xx_dev, int count)
+{
+	int ret;
+
+	sr1xx_dev->write_count = 0;
+	/* HBCI write */
+	ret = spi_write(sr1xx_dev->spi, sr1xx_dev->tx_buffer, count);
+	if (ret < 0) {
+		ret = -EIO;
+		dev_err(&sr1xx_dev->spi->dev,
+			"spi_write fw download : Failed.\n");
+		goto hbci_write_fail;
+	}
+	sr1xx_dev->write_count = count;
+	sr1xx_enable_irq(sr1xx_dev);
+	ret = TRANSCEIVE_SUCCESS;
+	return ret;
+hbci_write_fail:
+	dev_err(&sr1xx_dev->spi->dev, "%s failed...%d", __func__, ret);
+	return ret;
+}
+
+static ssize_t sr1xx_dev_write(struct file *filp, const char *buf, size_t count,
+			       loff_t *offset)
+{
+	int ret;
+	struct sr1xx_dev *sr1xx_dev;
+
+	sr1xx_dev = filp->private_data;
+	if (count > SR1XX_MAX_TX_BUF_SIZE || count > SR1XX_TXBUF_SIZE) {
+		dev_err(&sr1xx_dev->spi->dev, "%s : Write Size Exceeds\n",
+			__func__);
+		ret = -ENOBUFS;
+		goto write_end;
+	}
+	if (copy_from_user(sr1xx_dev->tx_buffer, buf, count)) {
+		dev_err(&sr1xx_dev->spi->dev,
+			"%s : failed to copy from user space\n", __func__);
+		return -EFAULT;
+	}
+	if (sr1xx_dev->is_fw_dwnld_enabled)
+		ret = sr1xx_hbci_write(sr1xx_dev, count);
+	else
+		ret = sr1xx_dev_transceive(sr1xx_dev, SR1XX_WRITE_MODE, count);
+	if (ret == TRANSCEIVE_SUCCESS)
+		ret = sr1xx_dev->write_count;
+	else
+		dev_err(&sr1xx_dev->spi->dev, "write failed......");
+write_end:
+	return ret;
+}
+
+/**
+ * sr1xx_hbci_read
+ *
+ * Function used to read data from sr1xx on SPI line
+ * as part of firmware download sequence.
+ *
+ * Returns: Number of bytes read if read is success else (-1)
+ *               otherwise indicate each error code
+ */
+static ssize_t sr1xx_hbci_read(struct sr1xx_dev *sr1xx_dev, char *buf,
+			       size_t count)
+{
+	int ret = -EIO;
+
+	if (count > SR1XX_RXBUF_SIZE) {
+		dev_err(&sr1xx_dev->spi->dev, "count(%zu) out of range(0-%d)\n",
+			count, SR1XX_RXBUF_SIZE);
+		ret = -EINVAL;
+		goto hbci_fail;
+	}
+	/* Wait for inetrrupt upto 500ms */
+	ret = wait_event_interruptible_timeout(sr1xx_dev->read_wq,
+					       sr1xx_dev->irq_received,
+					       sr1xx_dev->timeout_in_ms);
+	if (ret == 0) {
+		dev_err(&sr1xx_dev->spi->dev,
+			"hbci wait_event_interruptible timeout() : Failed.\n");
+		ret = -1;
+		goto hbci_fail;
+	}
+	if (sr1xx_dev->read_abort_requested) {
+		sr1xx_dev->read_abort_requested = false;
+		dev_err(&sr1xx_dev->spi->dev, "HBCI Abort Read pending......");
+		return ret;
+	}
+	if (!gpio_get_value(sr1xx_dev->irq_gpio)) {
+		dev_err(&sr1xx_dev->spi->dev,
+			"IRQ is low during firmware download");
+		goto hbci_fail;
+	}
+	ret = spi_read(sr1xx_dev->spi, (void *)sr1xx_dev->rx_buffer, count);
+	if (ret < 0) {
+		dev_err(&sr1xx_dev->spi->dev,
+			"sr1xx_dev_read: spi read error %d\n ", ret);
+		goto hbci_fail;
+	}
+	ret = count;
+	if (copy_to_user(buf, sr1xx_dev->rx_buffer, count)) {
+		dev_err(&sr1xx_dev->spi->dev,
+			"sr1xx_dev_read: copy to user failed\n");
+		ret = -EFAULT;
+	}
+	return ret;
+hbci_fail:
+	dev_err(&sr1xx_dev->spi->dev, "Error sr1xx_fw_download ret %d Exit\n",
+		ret);
+	return ret;
+}
+
+static ssize_t sr1xx_dev_read(struct file *filp, char *buf, size_t count,
+			      loff_t *offset)
+{
+	struct sr1xx_dev *sr1xx_dev = filp->private_data;
+	int ret = -EIO;
+
+	/* 500ms timeout in jiffies */
+	sr1xx_dev->timeout_in_ms = ((500 * HZ) / 1000);
+	memset(sr1xx_dev->rx_buffer, 0x00, SR1XX_RXBUF_SIZE);
+	if (!gpio_get_value(sr1xx_dev->irq_gpio)) {
+		if (filp->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
+			goto read_end;
+		}
+	}
+	/* HBCI packet read */
+	if (sr1xx_dev->is_fw_dwnld_enabled) {
+		ret = sr1xx_hbci_read(sr1xx_dev, buf, count);
+		goto read_end;
+	}
+	/* UCI packet read */
+	do {
+		sr1xx_enable_irq(sr1xx_dev);
+		if (!sr1xx_dev->read_abort_requested) {
+			ret = wait_event_interruptible(sr1xx_dev->read_wq,
+						       sr1xx_dev->irq_received);
+			if (ret) {
+				dev_err(&sr1xx_dev->spi->dev,
+					"wait_event_interruptible() : Failed.\n");
+				goto read_end;
+			}
+		}
+		if (sr1xx_dev->read_abort_requested) {
+			sr1xx_dev->read_abort_requested = false;
+			dev_err(&sr1xx_dev->spi->dev,
+				"Abort Read pending......");
+			goto read_end;
+		}
+		ret = sr1xx_dev_transceive(sr1xx_dev, SR1XX_READ_MODE, count);
+		if (ret == IRQ_WAIT_REQUEST) {
+			dev_err(&sr1xx_dev->spi->dev,
+				"Irg is low due to write hence irq is requested again...");
+		}
+	} while (ret == IRQ_WAIT_REQUEST);
+	if (ret == TRANSCEIVE_SUCCESS) {
+		if (copy_to_user(buf, sr1xx_dev->rx_buffer,
+				 sr1xx_dev->read_count)) {
+			dev_err(&sr1xx_dev->spi->dev,
+				"%s: copy to user failed\n", __func__);
+			ret = -EFAULT;
+			goto read_end;
+		}
+		ret = sr1xx_dev->read_count;
+	} else if (ret == IRQ_WAIT_TIMEOUT) {
+		dev_err(&sr1xx_dev->spi->dev,
+			"Second irq is not received..Time out...");
+		ret = -ETIME;
+	} else {
+		dev_err(&sr1xx_dev->spi->dev, "spi read failed...%d", ret);
+		ret = -EIO;
+	}
+read_end:
+	return ret;
+}
+
+static int sr1xx_hw_setup(struct device *dev,
+			  struct sr1xx_spi_platform_data *platform_data)
+{
+	int result = 0;
+
+	platform_data->gpiod_irq =
+	    devm_gpiod_get(dev, "nxp,sr1xx-irq", GPIOD_IN);
+	platform_data->gpiod_ce =
+	    devm_gpiod_get(dev, "nxp,sr1xx-ce", GPIOD_OUT_LOW);
+	platform_data->gpiod_spi_handshake =
+	    devm_gpiod_get(dev, "nxp,sr1xx-ri", GPIOD_OUT_LOW);
+	if (IS_ERR(platform_data->gpiod_irq)) {
+		dev_err(dev, "Failed fetching gpiod for irq\n");
+		return -EINVAL;
+	}
+	if (IS_ERR(platform_data->gpiod_ce)) {
+		dev_err(dev, "Failed fetching gpiod for ce\n");
+		return -EINVAL;
+	}
+	if (IS_ERR(platform_data->gpiod_spi_handshake)) {
+		dev_err(dev, "Failed fetching gpiod for spi handshake\n");
+		return -EINVAL;
+	}
+	dev_info(dev, "irq_gpio = %d, ce_gpio = %d, spi_handshake_gpio = %d\n",
+		 desc_to_gpio(platform_data->gpiod_irq),
+		 desc_to_gpio(platform_data->gpiod_ce),
+		 desc_to_gpio(platform_data->gpiod_spi_handshake));
+	return result;
+}
+
+static inline void sr1xx_set_data(struct spi_device *spi, void *data)
+{
+	dev_set_drvdata(&spi->dev, data);
+}
+
+static inline void *sr1xx_get_data(const struct spi_device *spi)
+{
+	return dev_get_drvdata(&spi->dev);
+}
+
+/* Possible fops on the sr1xx device */
+static const struct file_operations sr1xx_dev_fops = {
+	.owner = THIS_MODULE,
+	.read = sr1xx_dev_read,
+	.write = sr1xx_dev_write,
+	.open = sr1xx_dev_open,
+	.unlocked_ioctl = sr1xx_dev_ioctl,
+};
+
+/**
+ * sr1xx_gpio_cleanup
+ *
+ * Release requested gpios
+ *
+ */
+static void sr1xx_gpio_cleanup(struct device *dev,
+			       struct sr1xx_spi_platform_data *pdata)
+{
+	if (!IS_ERR(pdata->gpiod_ce))
+		devm_gpiod_put(dev, pdata->gpiod_ce);
+	if (!IS_ERR(pdata->gpiod_irq))
+		devm_gpiod_put(dev, pdata->gpiod_irq);
+	if (!IS_ERR(pdata->gpiod_spi_handshake))
+		devm_gpiod_put(dev, pdata->gpiod_spi_handshake);
+}
+
+static int sr1xx_probe(struct spi_device *spi)
+{
+	int ret;
+	struct sr1xx_spi_platform_data platform_data;
+	struct sr1xx_dev *sr1xx_dev = NULL;
+	unsigned int irq_flags;
+
+	dev_info(&spi->dev, "%s chip select : %d , bus number = %d\n", __func__,
+		 spi->chip_select, spi->master->bus_num);
+
+	sr1xx_dev = kzalloc(sizeof(*sr1xx_dev), GFP_KERNEL);
+	if (!sr1xx_dev) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+	ret = sr1xx_hw_setup(&spi->dev, &platform_data);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Failed hw_setup\n");
+		goto err_setup;
+	}
+
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_0;
+	spi->max_speed_hz = SR1XX_SPI_CLOCK;
+	ret = spi_setup(spi);
+	if (ret < 0) {
+		dev_err(&spi->dev, "failed to do spi_setup()\n");
+		goto err_setup;
+	}
+
+	sr1xx_dev->spi = spi;
+	sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR;
+	sr1xx_dev->sr1xx_device.name = "sr1xx";
+	sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops;
+	sr1xx_dev->sr1xx_device.parent = &spi->dev;
+	sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq);
+	sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce);
+	sr1xx_dev->spi_handshake_gpio =
+	    desc_to_gpio(platform_data.gpiod_spi_handshake);
+
+	dev_set_drvdata(&spi->dev, sr1xx_dev);
+
+	/* init mutex and queues */
+	init_waitqueue_head(&sr1xx_dev->read_wq);
+	mutex_init(&sr1xx_dev->sr1xx_access_lock);
+
+	spin_lock_init(&sr1xx_dev->irq_enabled_lock);
+
+	ret = misc_register(&sr1xx_dev->sr1xx_device);
+	if (ret < 0) {
+		dev_err(&spi->dev, "misc_register failed! %d\n", ret);
+		goto err_setup;
+	}
+
+	sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL);
+	sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL);
+	if (!sr1xx_dev->tx_buffer) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+	if (!sr1xx_dev->rx_buffer) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+
+	sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio);
+	if (sr1xx_dev->spi->irq < 0) {
+		dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n",
+			sr1xx_dev->irq_gpio);
+		goto err_exit;
+	}
+	/* request irq. The irq is set whenever the chip has data available
+	 * for reading. It is cleared when all data has been read.
+	 */
+	irq_flags = IRQ_TYPE_LEVEL_HIGH;
+	sr1xx_dev->irq_enabled = true;
+	sr1xx_dev->irq_received = false;
+
+	ret = request_irq(sr1xx_dev->spi->irq, sr1xx_dev_irq_handler, irq_flags,
+			  sr1xx_dev->sr1xx_device.name, sr1xx_dev);
+	if (ret) {
+		dev_err(&spi->dev, "request_irq failed\n");
+		goto err_exit;
+	}
+	sr1xx_disable_irq(sr1xx_dev);
+	return 0;
+err_exit:
+	if (sr1xx_dev) {
+		kfree(sr1xx_dev->tx_buffer);
+		kfree(sr1xx_dev->rx_buffer);
+		misc_deregister(&sr1xx_dev->sr1xx_device);
+	}
+err_setup:
+	if (sr1xx_dev)
+		mutex_destroy(&sr1xx_dev->sr1xx_access_lock);
+	sr1xx_gpio_cleanup(&spi->dev, &platform_data);
+	kfree(sr1xx_dev);
+	dev_err(&spi->dev, "ERROR: Exit : %s ret %d\n", __func__, ret);
+	return ret;
+}
+
+static void sr1xx_remove(struct spi_device *spi)
+{
+	struct sr1xx_dev *sr1xx_dev = sr1xx_get_data(spi);
+
+	if (!sr1xx_dev) {
+		dev_err(&spi->dev, "sr1xx_dev is NULL\n");
+		return;
+	}
+	gpio_free(sr1xx_dev->ce_gpio);
+	mutex_destroy(&sr1xx_dev->sr1xx_access_lock);
+	free_irq(sr1xx_dev->spi->irq, sr1xx_dev);
+	gpio_free(sr1xx_dev->irq_gpio);
+	gpio_free(sr1xx_dev->spi_handshake_gpio);
+	misc_deregister(&sr1xx_dev->sr1xx_device);
+	if (sr1xx_dev) {
+		kfree(sr1xx_dev->tx_buffer);
+		kfree(sr1xx_dev->rx_buffer);
+		kfree(sr1xx_dev);
+	}
+}
+
+/**
+ * sr1xx_dev_suspend
+ *
+ * Executed before putting the system into a sleep state
+ *
+ */
+int sr1xx_dev_suspend(struct device *dev)
+{
+	struct sr1xx_dev *sr1xx_dev = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(sr1xx_dev->spi->irq);
+	return 0;
+}
+
+/**
+ * sr1xx_dev_resume
+ *
+ * Executed after waking the system up from a sleep state
+ *
+ */
+int sr1xx_dev_resume(struct device *dev)
+{
+	struct sr1xx_dev *sr1xx_dev = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(sr1xx_dev->spi->irq);
+
+	return 0;
+}
+
+static const struct of_device_id sr1xx_dt_match[] = {
+	{
+	 .compatible = "nxp,sr1xx",
+	 },
+	{}
+};
+
+static const struct acpi_device_id sr1xx_acpi_match[] = {
+	{"PRP0001", 0},
+	{"", 0},
+};
+
+MODULE_DEVICE_TABLE(acpi, sr1xx_acpi_match);
+
+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,
+		   .of_match_table = sr1xx_dt_match,
+		   .acpi_match_table = sr1xx_acpi_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);
+}
+
+module_exit(sr1xx_dev_exit);
+
+MODULE_AUTHOR("Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>");
+MODULE_DESCRIPTION("NXP SR1XX SPI driver");
+MODULE_LICENSE("GPL");
--
2.37.2

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

[Arnd Bergmann]

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

Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg KH]

On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -471,6 +471,17 @@ config HISI_HIKEY_USB
>  	  switching between the dual-role USB-C port and the USB-A host ports
>  	  using only one USB controller.
> 
> +config NXP_UWB
> +    tristate "NXP UCI(Uwb Command Interface) protocol driver support"
> +    depends on SPI
> +    help
> +      This option enables the UWB driver for NXP sr1xx device.
> +      Such device supports UCI packet structure, FiRa compliant.
> +
> +      Say Y here to compile support for nxp-sr1xx into the kernel or
> +      say M to compile it as a module. The module will be called
> +      nxp-sr1xx.ko

No tabs?

> +
>  config OPEN_DICE
>  	tristate "Open Profile for DICE driver"
>  	depends on OF_RESERVED_MEM
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 2be8542616dd..ee8ca32c66f6 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -60,4 +60,5 @@ 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_VCPU_STALL_DETECTOR)	+= vcpu_stall_detector.o
> +obj-$(CONFIG_NXP_UWB) 		+= nxp-sr1xx.o
> diff --git a/drivers/misc/nxp-sr1xx.c b/drivers/misc/nxp-sr1xx.c
> new file mode 100644
> index 000000000000..6ca9a2b54b86
> --- /dev/null
> +++ b/drivers/misc/nxp-sr1xx.c
> @@ -0,0 +1,794 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

Please no.  If you really want to dual-license your Linux kernel code,
that's fine, but I will insist that you get a signed-off-by from your
corporate lawyer so that I know that they agree with this and are
willing to handle all of the complex issues that this entails as it will
require work on their side over time.

If that's not worth bothering your lawyers over, please just stick with
GPL as the only license.


> +/*
> + * Copyright 2018-2022 NXP.
> + *
> + * SPI driver for UWB SR1xx
> + * Author: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> + */
> +
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_gpio.h>
> +#include <linux/spi/spi.h>
> +#include <linux/uaccess.h>
> +
> +#define SR1XX_MAGIC 0xEA
> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)

You can't stick ioctl command definitions in a .c file that userspace
never sees.  How are your userspace tools supposed to know what the
ioctl is and how it is defined?

How was this ever tested and where is your userspace code that interacts
with this code?

thanks,

greg k-h

Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg KH]

On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
> +/**
> + * sr1xx_dev_transceive
> + * @op_mode indicates write/read operation
> + *
> + * Write and Read logic implemented under same api with
> + * mutex lock protection so write and read synchronized
> + *
> + * During Uwb ranging sequence(read) need to block write sequence
> + * in order to avoid some race condition scenarios.
> + *
> + * Returns     : Number of bytes write/read if read is success else (-1)

I'm sure I mentioned this before, but NEVER use magic "-1" as an error
value.  Use the real in-kernel -ERROR numbers for error codes please.
This needs to be fixed in many places in this code.

thanks,

greg k-h

Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Arnd Bergmann]

On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:

> 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/).

I know nothing about UWB, so I have no idea if the user interface
you propose here makes sense. My guess is that there is a good chance
that there are other implementations of UWB that would not work
with this specific driver interface, so you probably need a
slightly higher-level abstraction.

We had an older subsystem that was called UWB and that got removed
a while ago:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/uwb?id=caa6772db4c1deb5d9add48e95d6eab50699ee5e

Is that the same UWB or something completely different?

> +#define SR1XX_MAGIC 0xEA
> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)

This should be in a uapi header.

> +static int sr1xx_dev_open(struct inode *inode, struct file *filp)
> +{
> +	struct sr1xx_dev *sr1xx_dev =
> +	    container_of(filp->private_data, struct sr1xx_dev, sr1xx_device);
> +
> +	filp->private_data = sr1xx_dev;

This looks dangerous if the file gets opened more than once
and filp->private_data can have two different values.

> +static long sr1xx_dev_ioctl(struct file *filp, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	int ret = 0;
> +	struct sr1xx_dev *sr1xx_dev = NULL;
> +
> +	sr1xx_dev = filp->private_data;
> +
> +	switch (cmd) {
> +	case SR1XX_SET_PWR:
> +		if (arg == PWR_ENABLE) {
> +			gpio_set_value(sr1xx_dev->ce_gpio, 1);
> +			usleep_range(10000, 12000);

The usage of 'arg' does not match the definition of the command
number, which expects a pointer to 'long'. If you want to keep
the behavior, I suggest changing the #define.

> +static void sr1xx_wait_for_irq_gpio_low(struct sr1xx_dev *sr1xx_dev)
> +{
> +	int retry_count = 0;
> +
> +	do {
> +		udelay(10);
> +		retry_count++;
> +		if (retry_count == MAX_RETRY_COUNT_FOR_HANDSHAKE) {
> +			dev_info(&sr1xx_dev->spi->dev,
> +				 "Slave not released the IRQ even after 10ms");
> +			break;
> +		}
> +	} while (gpio_get_value(sr1xx_dev->irq_gpio));
> +}

The way to wait for a timeout is to compare against the timestamp
before the loop, using e.g. "time_before(jiffies, timeout)"
or possibly using ktime_get() instead of jiffies if you want to
be more accurate.

10ms is really too long for a busy-loop anyway, so better use
usleep_range() from a non-atomic context.

> +/* Possible fops on the sr1xx device */
> +static const struct file_operations sr1xx_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.read = sr1xx_dev_read,
> +	.write = sr1xx_dev_write,
> +	.open = sr1xx_dev_open,
> +	.unlocked_ioctl = sr1xx_dev_ioctl,
> +};

There should be a .compat_ioctl callback, either using the
same sr1xx_dev_ioctl function if you keep using the 'arg'
value directly, or 'compat_ptr_ioctl()' if you move to
pointers to arguments, or a custom function if you have
a mix of the two.

> +static int sr1xx_probe(struct spi_device *spi)
...
> +	ret = sr1xx_hw_setup(&spi->dev, &platform_data);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed hw_setup\n");
> +		goto err_setup;
> +	}
....
> +
> +	sr1xx_dev->spi = spi;
> +	sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR;
> +	sr1xx_dev->sr1xx_device.name = "sr1xx";
> +	sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops;
> +	sr1xx_dev->sr1xx_device.parent = &spi->dev;
> +	sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq);
> +	sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce);
> +	sr1xx_dev->spi_handshake_gpio =
> +	    desc_to_gpio(platform_data.gpiod_spi_handshake);

The temporary 'platform_data' structure seems useless here,
just fold its members into the sr1xx_dev structure itself.
No need to store both a gpio descriptor and a number, you
can simplify this to always use the descriptor.

> +	sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL);
> +	sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL);
> +	if (!sr1xx_dev->tx_buffer) {
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +	if (!sr1xx_dev->rx_buffer) {
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +
> +	sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio);
> +	if (sr1xx_dev->spi->irq < 0) {
> +		dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n",
> +			sr1xx_dev->irq_gpio);
> +		goto err_exit;
> +	}

Instead of gpio_to_irq(), the DT binding should probably
list the interrupt directly using the "interrupts" property
pointing to the gpio controller.

> +
> +static const struct acpi_device_id sr1xx_acpi_match[] = {
> +	{"PRP0001", 0},
> +	{"", 0},
> +};

As far as I understand, you are not supposed to list
compatiblity with PRP0001 when using this on a PC, the
ACPI subsystem will instead look at the of_device_id
table.

> +static const struct dev_pm_ops sr1xx_dev_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(sr1xx_dev_suspend, sr1xx_dev_resume)
> +};

Use SYSTEM_SLEEP_PM_OPS() instead of SET_SYSTEM_SLEEP_PM_OPS()
to avoid a warning about unused functions.

> +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);
> +}
> +
> +module_exit(sr1xx_dev_exit);

module_spi_driver()

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

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

[Manjunatha Venkatesh]

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)?

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

[Krzysztof Kozlowski]

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

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Arnd Bergmann]

On Fri, Oct 7, 2022, at 4:04 PM, Manjunatha Venkatesh wrote:
>sr1xx_dev_open(struct inode *inode, struct file *filp)
>>> +{
>>> +     struct sr1xx_dev *sr1xx_dev =
>>> +         container_of(filp->private_data, struct sr1xx_dev, sr1xx_device);
>>> +
>>> +     filp->private_data = sr1xx_dev;
>> This looks dangerous if the file gets opened more than once
>> and filp->private_data can have two different values.
> Do you suggest us to use mutex lock inside open api?


>>> +
>>> +     sr1xx_dev->spi = spi;
>>> +     sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR;
>>> +     sr1xx_dev->sr1xx_device.name = "sr1xx";
>>> +     sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops;
>>> +     sr1xx_dev->sr1xx_device.parent = &spi->dev;
>>> +     sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq);
>>> +     sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce);
>>> +     sr1xx_dev->spi_handshake_gpio =
>>> +         desc_to_gpio(platform_data.gpiod_spi_handshake);
>> The temporary 'platform_data' structure seems useless here,
>> just fold its members into the sr1xx_dev structure itself.
>> No need to store both a gpio descriptor and a number, you
>> can simplify this to always use the descriptor.
> Just to keep separate function(sr1xx_hw_setup) for better readability
> we have added intermediate platform_data structure.

I'm fairly sure it adds nothing to readability if every reader has
to wonder about why you have a platform_data structure here when
the device was never used without devicetree.

>>> +     sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL);
>>> +     sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL);
>>> +     if (!sr1xx_dev->tx_buffer) {
>>> +             ret = -ENOMEM;
>>> +             goto err_exit;
>>> +     }
>>> +     if (!sr1xx_dev->rx_buffer) {
>>> +             ret = -ENOMEM;
>>> +             goto err_exit;
>>> +     }
>>> +
>>> +     sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio);
>>> +     if (sr1xx_dev->spi->irq < 0) {
>>> +             dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n",
>>> +                     sr1xx_dev->irq_gpio);
>>> +             goto err_exit;
>>> +     }
>> Instead of gpio_to_irq(), the DT binding should probably
>> list the interrupt directly using the "interrupts" property
>> pointing to the gpio controller. Since, we are configured this as generic GPIO in DT binding. So, we
> are using gpio_to_irq() to use as IRQ pin.    

I meant you should change the binding first, and then adapt the
code to match. Remove all references to gpio numbers from 
the code.

    Arnd

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 9/14/2022 8:25 PM, Greg KH wrote:
> Caution: EXT Email
>
> On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
>> +/**
>> + * sr1xx_dev_transceive
>> + * @op_mode indicates write/read operation
>> + *
>> + * Write and Read logic implemented under same api with
>> + * mutex lock protection so write and read synchronized
>> + *
>> + * During Uwb ranging sequence(read) need to block write sequence
>> + * in order to avoid some race condition scenarios.
>> + *
>> + * Returns     : Number of bytes write/read if read is success else (-1)
> I'm sure I mentioned this before, but NEVER use magic "-1" as an error
> value.  Use the real in-kernel -ERROR numbers for error codes please.
> This needs to be fixed in many places in this code.
Will fix this in the next patch submission.
> thanks,
>
> greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg Kroah-Hartman]

On Fri, Oct 07, 2022 at 07:34:25PM +0530, Manjunatha Venkatesh wrote:
> 
> On 9/14/2022 8:39 PM, Arnd Bergmann wrote:
> > Caution: EXT Email
> > 
> > On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
> > 
> > > 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 athttps://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/).
> > I know nothing about UWB, so I have no idea if the user interface
> > you propose here makes sense. My guess is that there is a good chance
> > that there are other implementations of UWB that would not work
> > with this specific driver interface, so you probably need a
> > slightly higher-level abstraction.
> > 
> > We had an older subsystem that was called UWB and that got removed
> > a while ago:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/uwb?id=caa6772db4c1deb5d9add48e95d6eab50699ee5e
> > 
> > Is that the same UWB or something completely different?
> Basically, it is SPI device driver which supports UCI(Ultra-wide band
> Command Interface) packet structure. It is not same as in mentioned link.

Why isn't this just a normal SPI driver and you do the "UCI" commands
from userspace through the device node there?

I know I asked this before, but I can't remember the answer, sorry, so
please include that in the changelog information when you resubmit.

thanks,

greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 9/14/2022 8:23 PM, Greg KH wrote:
> Caution: EXT Email
>
> On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -471,6 +471,17 @@ config HISI_HIKEY_USB
>>          switching between the dual-role USB-C port and the USB-A host ports
>>          using only one USB controller.
>>
>> +config NXP_UWB
>> +    tristate "NXP UCI(Uwb Command Interface) protocol driver support"
>> +    depends on SPI
>> +    help
>> +      This option enables the UWB driver for NXP sr1xx device.
>> +      Such device supports UCI packet structure, FiRa compliant.
>> +
>> +      Say Y here to compile support for nxp-sr1xx into the kernel or
>> +      say M to compile it as a module. The module will be called
>> +      nxp-sr1xx.ko
> No tabs?
Will fix this in the next patch submission.
>> +
>>   config OPEN_DICE
>>        tristate "Open Profile for DICE driver"
>>        depends on OF_RESERVED_MEM
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 2be8542616dd..ee8ca32c66f6 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -60,4 +60,5 @@ 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_VCPU_STALL_DETECTOR)    += vcpu_stall_detector.o
>> +obj-$(CONFIG_NXP_UWB)                += nxp-sr1xx.o
>> diff --git a/drivers/misc/nxp-sr1xx.c b/drivers/misc/nxp-sr1xx.c
>> new file mode 100644
>> index 000000000000..6ca9a2b54b86
>> --- /dev/null
>> +++ b/drivers/misc/nxp-sr1xx.c
>> @@ -0,0 +1,794 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> Please no.  If you really want to dual-license your Linux kernel code,
> that's fine, but I will insist that you get a signed-off-by from your
> corporate lawyer so that I know that they agree with this and are
> willing to handle all of the complex issues that this entails as it will
> require work on their side over time.
>
> If that's not worth bothering your lawyers over, please just stick with
> GPL as the only license.
Dual-license is signed-off by NXP corporate lawyer. Though, we would 
like to understand what complex issues which require work over the time?
>
>> +/*
>> + * Copyright 2018-2022 NXP.
>> + *
>> + * SPI driver for UWB SR1xx
>> + * Author: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
>> + */
>> +
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/uaccess.h>
>> +
>> +#define SR1XX_MAGIC 0xEA
>> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
>> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
> You can't stick ioctl command definitions in a .c file that userspace
> never sees.  How are your userspace tools supposed to know what the
> ioctl is and how it is defined?
We will move ioctl command definitions into user space header file as 
part of our next patch submission.
> How was this ever tested and where is your userspace code that interacts
> with this code?
We will share the corresponding user space code soon,meanwhile can you 
please suggest how to share this user space code?
> thanks,
>
> greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 10/7/2022 7:41 PM, Arnd Bergmann wrote:
> Caution: EXT Email
>
> On Fri, Oct 7, 2022, at 4:04 PM, Manjunatha Venkatesh wrote:
>> sr1xx_dev_open(struct inode *inode, struct file *filp)
>>>> +{
>>>> +     struct sr1xx_dev *sr1xx_dev =
>>>> +         container_of(filp->private_data, struct sr1xx_dev, sr1xx_device);
>>>> +
>>>> +     filp->private_data = sr1xx_dev;
>>> This looks dangerous if the file gets opened more than once
>>> and filp->private_data can have two different values.
>> Do you suggest us to use mutex lock inside open api?
>
>>>> +
>>>> +     sr1xx_dev->spi = spi;
>>>> +     sr1xx_dev->sr1xx_device.minor = MISC_DYNAMIC_MINOR;
>>>> +     sr1xx_dev->sr1xx_device.name = "sr1xx";
>>>> +     sr1xx_dev->sr1xx_device.fops = &sr1xx_dev_fops;
>>>> +     sr1xx_dev->sr1xx_device.parent = &spi->dev;
>>>> +     sr1xx_dev->irq_gpio = desc_to_gpio(platform_data.gpiod_irq);
>>>> +     sr1xx_dev->ce_gpio = desc_to_gpio(platform_data.gpiod_ce);
>>>> +     sr1xx_dev->spi_handshake_gpio =
>>>> +         desc_to_gpio(platform_data.gpiod_spi_handshake);
>>> The temporary 'platform_data' structure seems useless here,
>>> just fold its members into the sr1xx_dev structure itself.
>>> No need to store both a gpio descriptor and a number, you
>>> can simplify this to always use the descriptor.
>> Just to keep separate function(sr1xx_hw_setup) for better readability
>> we have added intermediate platform_data structure.
> I'm fairly sure it adds nothing to readability if every reader has
> to wonder about why you have a platform_data structure here when
> the device was never used without devicetree.
Will fix this in the next patch submission.
>>>> +     sr1xx_dev->tx_buffer = kzalloc(SR1XX_TXBUF_SIZE, GFP_KERNEL);
>>>> +     sr1xx_dev->rx_buffer = kzalloc(SR1XX_RXBUF_SIZE, GFP_KERNEL);
>>>> +     if (!sr1xx_dev->tx_buffer) {
>>>> +             ret = -ENOMEM;
>>>> +             goto err_exit;
>>>> +     }
>>>> +     if (!sr1xx_dev->rx_buffer) {
>>>> +             ret = -ENOMEM;
>>>> +             goto err_exit;
>>>> +     }
>>>> +
>>>> +     sr1xx_dev->spi->irq = gpio_to_irq(sr1xx_dev->irq_gpio);
>>>> +     if (sr1xx_dev->spi->irq < 0) {
>>>> +             dev_err(&spi->dev, "gpio_to_irq request failed gpio = 0x%x\n",
>>>> +                     sr1xx_dev->irq_gpio);
>>>> +             goto err_exit;
>>>> +     }
>>> Instead of gpio_to_irq(), the DT binding should probably
>>> list the interrupt directly using the "interrupts" property
>>> pointing to the gpio controller. Since, we are configured this as generic GPIO in DT binding. So, we
>> are using gpio_to_irq() to use as IRQ pin.
> I meant you should change the binding first, and then adapt the
> code to match. Remove all references to gpio numbers from
> the code.
Will work on this suggestion and update in the next patch submission.
>      Arnd

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 10/7/2022 8:27 PM, Greg Kroah-Hartman wrote:
> Caution: EXT Email
>
> On Fri, Oct 07, 2022 at 07:34:25PM +0530, Manjunatha Venkatesh wrote:
>> On 9/14/2022 8:39 PM, Arnd Bergmann wrote:
>>> Caution: EXT Email
>>>
>>> On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
>>>
>>>> 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 athttps://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fproducts%2F%3AUWB-TRIMENSION&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8478b7c0aa694618aae608daa87430fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638007514231447184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=y3t8eT%2BIX1OP%2B1wu%2B8hWp2HI%2FhnZj32L%2BDCcIA7m9hs%3D&reserved=0
>>>>
>>>> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.firaconsortium.org%2F&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8478b7c0aa694618aae608daa87430fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638007514231447184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xhFUUcJ7a3oU6pefXHTunBCI73%2Fy2PnnwsTn1KZbeFk%3D&reserved=0).
>>> I know nothing about UWB, so I have no idea if the user interface
>>> you propose here makes sense. My guess is that there is a good chance
>>> that there are other implementations of UWB that would not work
>>> with this specific driver interface, so you probably need a
>>> slightly higher-level abstraction.
>>>
>>> We had an older subsystem that was called UWB and that got removed
>>> a while ago:
>>>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2Fdrivers%2Fstaging%2Fuwb%3Fid%3Dcaa6772db4c1deb5d9add48e95d6eab50699ee5e&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8478b7c0aa694618aae608daa87430fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638007514231447184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gcF%2B%2FzD%2F0TWJ5AEJvXCGv5n%2FrPg2qXJigedOq4IeVPI%3D&reserved=0
>>>
>>> Is that the same UWB or something completely different?
>> Basically, it is SPI device driver which supports UCI(Ultra-wide band
>> Command Interface) packet structure. It is not same as in mentioned link.
> Why isn't this just a normal SPI driver and you do the "UCI" commands
> from userspace through the device node there?
>
> I know I asked this before, but I can't remember the answer, sorry, so
> please include that in the changelog information when you resubmit.
>
> thanks,
>
> greg k-h
The IO Handshake needed with SR1XX Family of SOCs cannot use the RAW SPI
Module's APIs and hence custom APIs are added for communication with the
UWBS,
With this will get required throughput for UWBS use cases to avoid multiple
round trip between user  and kernel mode.

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg Kroah-Hartman]

On Wed, Nov 30, 2022 at 09:39:59AM +0530, Manjunatha Venkatesh wrote:
> 
> On 10/7/2022 8:27 PM, Greg Kroah-Hartman wrote:
> > Caution: EXT Email
> > 
> > On Fri, Oct 07, 2022 at 07:34:25PM +0530, Manjunatha Venkatesh wrote:
> > > On 9/14/2022 8:39 PM, Arnd Bergmann wrote:
> > > > Caution: EXT Email
> > > > 
> > > > On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
> > > > 
> > > > > 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 athttps://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fproducts%2F%3AUWB-TRIMENSION&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8478b7c0aa694618aae608daa87430fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638007514231447184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=y3t8eT%2BIX1OP%2B1wu%2B8hWp2HI%2FhnZj32L%2BDCcIA7m9hs%3D&reserved=0
> > > > > 
> > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.firaconsortium.org%2F&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8478b7c0aa694618aae608daa87430fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638007514231447184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=xhFUUcJ7a3oU6pefXHTunBCI73%2Fy2PnnwsTn1KZbeFk%3D&reserved=0).
> > > > I know nothing about UWB, so I have no idea if the user interface
> > > > you propose here makes sense. My guess is that there is a good chance
> > > > that there are other implementations of UWB that would not work
> > > > with this specific driver interface, so you probably need a
> > > > slightly higher-level abstraction.
> > > > 
> > > > We had an older subsystem that was called UWB and that got removed
> > > > a while ago:
> > > > 
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2Fdrivers%2Fstaging%2Fuwb%3Fid%3Dcaa6772db4c1deb5d9add48e95d6eab50699ee5e&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C8478b7c0aa694618aae608daa87430fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638007514231447184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=gcF%2B%2FzD%2F0TWJ5AEJvXCGv5n%2FrPg2qXJigedOq4IeVPI%3D&reserved=0
> > > > 
> > > > Is that the same UWB or something completely different?
> > > Basically, it is SPI device driver which supports UCI(Ultra-wide band
> > > Command Interface) packet structure. It is not same as in mentioned link.
> > Why isn't this just a normal SPI driver and you do the "UCI" commands
> > from userspace through the device node there?
> > 
> > I know I asked this before, but I can't remember the answer, sorry, so
> > please include that in the changelog information when you resubmit.
> > 
> > thanks,
> > 
> > greg k-h
> The IO Handshake needed with SR1XX Family of SOCs cannot use the RAW SPI
> Module's APIs and hence custom APIs are added for communication with the
> UWBS,

I do not understand, what "IO handshake"?  What is missing from the
userspace spi api that is needed here?

> With this will get required throughput for UWBS use cases to avoid multiple
> round trip between user  and kernel mode.

Based on the speed of the SPI bus, this should not be an issue at all.
If it is, please provide us real performance numbers showing the
problem, as there are ways of speeding that up.

thanks,

greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg KH]

On Wed, Nov 30, 2022 at 09:10:08AM +0530, Manjunatha Venkatesh wrote:
> 
> On 9/14/2022 8:23 PM, Greg KH wrote:

Note, originally you all were "rushed" to get this accepted, and now
this took 2 1/2 months to respond back to a code review?  Something is
wrong here, when responding so late, almost all context is lost :(

> > Caution: EXT Email
> > 
> > On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
> > > +++ b/drivers/misc/nxp-sr1xx.c
> > > @@ -0,0 +1,794 @@
> > > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> > Please no.  If you really want to dual-license your Linux kernel code,
> > that's fine, but I will insist that you get a signed-off-by from your
> > corporate lawyer so that I know that they agree with this and are
> > willing to handle all of the complex issues that this entails as it will
> > require work on their side over time.
> > 
> > If that's not worth bothering your lawyers over, please just stick with
> > GPL as the only license.
> Dual-license is signed-off by NXP corporate lawyer.

We need a signed-off-by on the patch itself.

> Though, we would like to understand what complex issues which require
> work over the time?

I am not a lawyer and can not advise you of this, please work with yours
to set into place the requirements you will have to keep this working
properly.  Note, it is not trivial, and will require work on your end.

I will push back again, and ask "Why?"  Why do you want this dual
licensed?  What is driving that requirement and what will having it
licensed like this enable you to do that having it just under GPL-2.0
will not?

> > > +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> > > +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
> > You can't stick ioctl command definitions in a .c file that userspace
> > never sees.  How are your userspace tools supposed to know what the
> > ioctl is and how it is defined?
> We will move ioctl command definitions into user space header file as part
> of our next patch submission.
> > How was this ever tested and where is your userspace code that interacts
> > with this code?
> We will share the corresponding user space code soon,meanwhile can you
> please suggest how to share this user space code?

You all have ways of posting code publicly :)

thanks,

greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 11/30/2022 12:57 PM, Greg KH wrote:
> Caution: EXT Email
>
> On Wed, Nov 30, 2022 at 09:10:08AM +0530, Manjunatha Venkatesh wrote:
>> On 9/14/2022 8:23 PM, Greg KH wrote:
> Note, originally you all were "rushed" to get this accepted, and now
> this took 2 1/2 months to respond back to a code review?  Something is
> wrong here, when responding so late, almost all context is lost :(

Sorry for the delayed response,further we will make sure address the 
review comments ASAP.

>
>>> Caution: EXT Email
>>>
>>> On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
>>>> +++ b/drivers/misc/nxp-sr1xx.c
>>>> @@ -0,0 +1,794 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>>> Please no.  If you really want to dual-license your Linux kernel code,
>>> that's fine, but I will insist that you get a signed-off-by from your
>>> corporate lawyer so that I know that they agree with this and are
>>> willing to handle all of the complex issues that this entails as it will
>>> require work on their side over time.
>>>
>>> If that's not worth bothering your lawyers over, please just stick with
>>> GPL as the only license.
>> Dual-license is signed-off by NXP corporate lawyer.
> We need a signed-off-by on the patch itself.
As part of Version6 patch submission signed-off by NXP corporate lawyer 
updated
>> Though, we would like to understand what complex issues which require
>> work over the time?
> I am not a lawyer and can not advise you of this, please work with yours
> to set into place the requirements you will have to keep this working
> properly.  Note, it is not trivial, and will require work on your end.
>
> I will push back again, and ask "Why?"  Why do you want this dual
> licensed?  What is driving that requirement and what will having it
> licensed like this enable you to do that having it just under GPL-2.0
> will not?

Our corporate lawyer suggested to use this dual license for NXP UWB product.

>>>> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
>>>> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
>>> You can't stick ioctl command definitions in a .c file that userspace
>>> never sees.  How are your userspace tools supposed to know what the
>>> ioctl is and how it is defined?
>> We will move ioctl command definitions into user space header file as part
>> of our next patch submission.
>>> How was this ever tested and where is your userspace code that interacts
>>> with this code?
>> We will share the corresponding user space code soon,meanwhile can you
>> please suggest how to share this user space code?
> You all have ways of posting code publicly :)

NXP UWB user space code available at below shared path.

https://github.com/NXP/uwb-driver-testapp

>
> thanks,
>
> greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg KH]

On Tue, Dec 20, 2022 at 07:39:52PM +0530, Manjunatha Venkatesh wrote:
> On 11/30/2022 12:57 PM, Greg KH wrote:
> > Caution: EXT Email
> > 
> > On Wed, Nov 30, 2022 at 09:10:08AM +0530, Manjunatha Venkatesh wrote:
> > > On 9/14/2022 8:23 PM, Greg KH wrote:
> > Note, originally you all were "rushed" to get this accepted, and now
> > this took 2 1/2 months to respond back to a code review?  Something is
> > wrong here, when responding so late, almost all context is lost :(
> 
> Sorry for the delayed response,further we will make sure address the review
> comments ASAP.
> 
> > 
> > > > Caution: EXT Email
> > > > 
> > > > On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
> > > > > +++ b/drivers/misc/nxp-sr1xx.c
> > > > > @@ -0,0 +1,794 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> > > > Please no.  If you really want to dual-license your Linux kernel code,
> > > > that's fine, but I will insist that you get a signed-off-by from your
> > > > corporate lawyer so that I know that they agree with this and are
> > > > willing to handle all of the complex issues that this entails as it will
> > > > require work on their side over time.
> > > > 
> > > > If that's not worth bothering your lawyers over, please just stick with
> > > > GPL as the only license.
> > > Dual-license is signed-off by NXP corporate lawyer.
> > We need a signed-off-by on the patch itself.
> As part of Version6 patch submission signed-off by NXP corporate lawyer
> updated
> > > Though, we would like to understand what complex issues which require
> > > work over the time?
> > I am not a lawyer and can not advise you of this, please work with yours
> > to set into place the requirements you will have to keep this working
> > properly.  Note, it is not trivial, and will require work on your end.
> > 
> > I will push back again, and ask "Why?"  Why do you want this dual
> > licensed?  What is driving that requirement and what will having it
> > licensed like this enable you to do that having it just under GPL-2.0
> > will not?
> 
> Our corporate lawyer suggested to use this dual license for NXP UWB product.

And I need a tangable _reason_ why a dual license is needed here.
Remember, dual licensing takes from the community and does not give
back, so justifications for why this is required is essencial if you
wish for people to at the very least, review your code...

> 
> > > > > +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
> > > > > +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
> > > > You can't stick ioctl command definitions in a .c file that userspace
> > > > never sees.  How are your userspace tools supposed to know what the
> > > > ioctl is and how it is defined?
> > > We will move ioctl command definitions into user space header file as part
> > > of our next patch submission.
> > > > How was this ever tested and where is your userspace code that interacts
> > > > with this code?
> > > We will share the corresponding user space code soon,meanwhile can you
> > > please suggest how to share this user space code?
> > You all have ways of posting code publicly :)
> 
> NXP UWB user space code available at below shared path.
> 
> https://github.com/NXP/uwb-driver-testapp

And the code there shows that you did not write the kernel side
correctly :(

Please fix it all up for your next submission.

thanks,

greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 11/30/2022 12:53 PM, Greg Kroah-Hartman wrote:
> Caution: EXT Email
>
> On Wed, Nov 30, 2022 at 09:39:59AM +0530, Manjunatha Venkatesh wrote:
>> On 10/7/2022 8:27 PM, Greg Kroah-Hartman wrote:
>>> Caution: EXT Email
>>>
>>> On Fri, Oct 07, 2022 at 07:34:25PM +0530, Manjunatha Venkatesh wrote:
>>>> On 9/14/2022 8:39 PM, Arnd Bergmann wrote:
>>>>> Caution: EXT Email
>>>>>
>>>>> On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
>>>>>
>>>>>> 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 athttps://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fproducts%2F%3AUWB-TRIMENSION&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C46c5718c03ee429cf57208dad2a3cad7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638053898170779252%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=S2BswHaF22edAfiZXEKUwGfUTNi1nuQzQSdGDb26peI%3D&reserved=0
>>>>>>
>>>>>> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.firaconsortium.org%2F&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C46c5718c03ee429cf57208dad2a3cad7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638053898170779252%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=0fFcimUd6gOxTV0EKS%2BfxRZfrMDg0fytq1eSDmkMZ9E%3D&reserved=0).
>>>>> I know nothing about UWB, so I have no idea if the user interface
>>>>> you propose here makes sense. My guess is that there is a good chance
>>>>> that there are other implementations of UWB that would not work
>>>>> with this specific driver interface, so you probably need a
>>>>> slightly higher-level abstraction.
>>>>>
>>>>> We had an older subsystem that was called UWB and that got removed
>>>>> a while ago:
>>>>>
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2Fdrivers%2Fstaging%2Fuwb%3Fid%3Dcaa6772db4c1deb5d9add48e95d6eab50699ee5e&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C46c5718c03ee429cf57208dad2a3cad7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638053898170779252%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XXYgofE9LlBCPGW1aKxKUOBEIGF0aQv%2Bh6x6iNATkLQ%3D&reserved=0
>>>>>
>>>>> Is that the same UWB or something completely different?
>>>> Basically, it is SPI device driver which supports UCI(Ultra-wide band
>>>> Command Interface) packet structure. It is not same as in mentioned link.
>>> Why isn't this just a normal SPI driver and you do the "UCI" commands
>>> from userspace through the device node there?
>>>
>>> I know I asked this before, but I can't remember the answer, sorry, so
>>> please include that in the changelog information when you resubmit.
>>>
>>> thanks,
>>>
>>> greg k-h
>> The IO Handshake needed with SR1XX Family of SOCs cannot use the RAW SPI
>> Module's APIs and hence custom APIs are added for communication with the
>> UWBS,
> I do not understand, what "IO handshake"?  What is missing from the
> userspace spi api that is needed here?
>
>> With this will get required throughput for UWBS use cases to avoid multiple
>> round trip between user  and kernel mode.
> Based on the speed of the SPI bus, this should not be an issue at all.
> If it is, please provide us real performance numbers showing the
> problem, as there are ways of speeding that up.

Not only throughput and also this driver customized ioctls to be controlled

from the user space for different scenarios.

Current driver have UCI (UWB Command Interface) specific header parsing 
logic.

There is a specific GPIOs hand shake mechanism required between Host 
Driver and UWBS

at driver level which is tightly coupled with our UWBS chip.

Basically UWBS expecting acknowledgement from Host driver after first 
interrupt request

triggered then Host driver acknowledge to UWBS through dedicated GPIOs.

After this one more interrupt request will be triggered from UWBS for 
read operation.

> thanks,
>
> greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Manjunatha Venkatesh]

On 12/20/2022 8:00 PM, Greg KH wrote:
> Caution: EXT Email
>
> On Tue, Dec 20, 2022 at 07:39:52PM +0530, Manjunatha Venkatesh wrote:
>> On 11/30/2022 12:57 PM, Greg KH wrote:
>>> Caution: EXT Email
>>>
>>> On Wed, Nov 30, 2022 at 09:10:08AM +0530, Manjunatha Venkatesh wrote:
>>>> On 9/14/2022 8:23 PM, Greg KH wrote:
>>> Note, originally you all were "rushed" to get this accepted, and now
>>> this took 2 1/2 months to respond back to a code review?  Something is
>>> wrong here, when responding so late, almost all context is lost :(
>> Sorry for the delayed response,further we will make sure address the review
>> comments ASAP.
>>
>>>>> Caution: EXT Email
>>>>>
>>>>> On Wed, Sep 14, 2022 at 07:59:44PM +0530, Manjunatha Venkatesh wrote:
>>>>>> +++ b/drivers/misc/nxp-sr1xx.c
>>>>>> @@ -0,0 +1,794 @@
>>>>>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>>>>> Please no.  If you really want to dual-license your Linux kernel code,
>>>>> that's fine, but I will insist that you get a signed-off-by from your
>>>>> corporate lawyer so that I know that they agree with this and are
>>>>> willing to handle all of the complex issues that this entails as it will
>>>>> require work on their side over time.
>>>>>
>>>>> If that's not worth bothering your lawyers over, please just stick with
>>>>> GPL as the only license.
>>>> Dual-license is signed-off by NXP corporate lawyer.
>>> We need a signed-off-by on the patch itself.
>> As part of Version6 patch submission signed-off by NXP corporate lawyer
>> updated
>>>> Though, we would like to understand what complex issues which require
>>>> work over the time?
>>> I am not a lawyer and can not advise you of this, please work with yours
>>> to set into place the requirements you will have to keep this working
>>> properly.  Note, it is not trivial, and will require work on your end.
>>>
>>> I will push back again, and ask "Why?"  Why do you want this dual
>>> licensed?  What is driving that requirement and what will having it
>>> licensed like this enable you to do that having it just under GPL-2.0
>>> will not?
>> Our corporate lawyer suggested to use this dual license for NXP UWB product.
> And I need a tangable _reason_ why a dual license is needed here.
> Remember, dual licensing takes from the community and does not give
> back, so justifications for why this is required is essencial if you
> wish for people to at the very least, review your code...

Sure, will get the actual reason from our corporate lawyer,meanwhile can 
we submit

next driver patch version where we have addressed previous code review 
comments.

>>>>>> +#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, long)
>>>>>> +#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, long)
>>>>> You can't stick ioctl command definitions in a .c file that userspace
>>>>> never sees.  How are your userspace tools supposed to know what the
>>>>> ioctl is and how it is defined?
>>>> We will move ioctl command definitions into user space header file as part
>>>> of our next patch submission.
>>>>> How was this ever tested and where is your userspace code that interacts
>>>>> with this code?
>>>> We will share the corresponding user space code soon,meanwhile can you
>>>> please suggest how to share this user space code?
>>> You all have ways of posting code publicly :)
>> NXP UWB user space code available at below shared path.
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FNXP%2Fuwb-driver-testapp&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C999fcbfc998c42d4ae5708dae296be78%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638071434311303825%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1Z4h8OUd%2BM%2Fy5dnVoiBioS1%2FbR4Rqul8tW7OB%2FPCRqI%3D&reserved=0
> And the code there shows that you did not write the kernel side
> correctly :(
>
> Please fix it all up for your next submission.
>
> thanks,
>
> greg k-h

Re: [EXT] Re: [PATCH v5 2/2] misc: nxp-sr1xx: UWB driver support for sr1xx series chip

[Greg Kroah-Hartman]

On Tue, Dec 20, 2022 at 08:03:08PM +0530, Manjunatha Venkatesh wrote:
> 
> On 11/30/2022 12:53 PM, Greg Kroah-Hartman wrote:
> > Caution: EXT Email
> > 
> > On Wed, Nov 30, 2022 at 09:39:59AM +0530, Manjunatha Venkatesh wrote:
> > > On 10/7/2022 8:27 PM, Greg Kroah-Hartman wrote:
> > > > Caution: EXT Email
> > > > 
> > > > On Fri, Oct 07, 2022 at 07:34:25PM +0530, Manjunatha Venkatesh wrote:
> > > > > On 9/14/2022 8:39 PM, Arnd Bergmann wrote:
> > > > > > Caution: EXT Email
> > > > > > 
> > > > > > On Wed, Sep 14, 2022, at 4:29 PM, Manjunatha Venkatesh wrote:
> > > > > > 
> > > > > > > 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 athttps://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fproducts%2F%3AUWB-TRIMENSION&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C46c5718c03ee429cf57208dad2a3cad7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638053898170779252%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=S2BswHaF22edAfiZXEKUwGfUTNi1nuQzQSdGDb26peI%3D&reserved=0
> > > > > > > 
> > > > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.firaconsortium.org%2F&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C46c5718c03ee429cf57208dad2a3cad7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638053898170779252%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=0fFcimUd6gOxTV0EKS%2BfxRZfrMDg0fytq1eSDmkMZ9E%3D&reserved=0).
> > > > > > I know nothing about UWB, so I have no idea if the user interface
> > > > > > you propose here makes sense. My guess is that there is a good chance
> > > > > > that there are other implementations of UWB that would not work
> > > > > > with this specific driver interface, so you probably need a
> > > > > > slightly higher-level abstraction.
> > > > > > 
> > > > > > We had an older subsystem that was called UWB and that got removed
> > > > > > a while ago:
> > > > > > 
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Fcommit%2Fdrivers%2Fstaging%2Fuwb%3Fid%3Dcaa6772db4c1deb5d9add48e95d6eab50699ee5e&data=05%7C01%7Cmanjunatha.venkatesh%40nxp.com%7C46c5718c03ee429cf57208dad2a3cad7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638053898170779252%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=XXYgofE9LlBCPGW1aKxKUOBEIGF0aQv%2Bh6x6iNATkLQ%3D&reserved=0
> > > > > > 
> > > > > > Is that the same UWB or something completely different?
> > > > > Basically, it is SPI device driver which supports UCI(Ultra-wide band
> > > > > Command Interface) packet structure. It is not same as in mentioned link.
> > > > Why isn't this just a normal SPI driver and you do the "UCI" commands
> > > > from userspace through the device node there?
> > > > 
> > > > I know I asked this before, but I can't remember the answer, sorry, so
> > > > please include that in the changelog information when you resubmit.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > The IO Handshake needed with SR1XX Family of SOCs cannot use the RAW SPI
> > > Module's APIs and hence custom APIs are added for communication with the
> > > UWBS,
> > I do not understand, what "IO handshake"?  What is missing from the
> > userspace spi api that is needed here?
> > 
> > > With this will get required throughput for UWBS use cases to avoid multiple
> > > round trip between user  and kernel mode.
> > Based on the speed of the SPI bus, this should not be an issue at all.
> > If it is, please provide us real performance numbers showing the
> > problem, as there are ways of speeding that up.
> 
> Not only throughput and also this driver customized ioctls to be controlled
> 
> from the user space for different scenarios.

Then you need to strongly document this.

> Current driver have UCI (UWB Command Interface) specific header parsing
> logic.

What does this mean?

> There is a specific GPIOs hand shake mechanism required between Host Driver
> and UWBS
> 
> at driver level which is tightly coupled with our UWBS chip.

Why can't you do this in userspace?

> Basically UWBS expecting acknowledgement from Host driver after first
> interrupt request
> 
> triggered then Host driver acknowledge to UWBS through dedicated GPIOs.

Again, why can't you do this in userspace?

> After this one more interrupt request will be triggered from UWBS for read
> operation.

Again, userspace?

thanks,

greg k-h