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

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

[Manjunatha Venkatesh]

Here's a sixth 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           |  65 ++
 MAINTAINERS                                   |   7 +
 drivers/misc/Kconfig                          |  11 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/nxp-sr1xx.c                      | 728 ++++++++++++++++++
 include/uapi/misc/sr1xx.h                     |  13 +
 6 files changed, 825 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
 create mode 100644 drivers/misc/nxp-sr1xx.c
 create mode 100644 include/uapi/misc/sr1xx.h

-- 
2.38.1

[PATCH v6 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/425858dc-59fe-2311-61ae-3b6dc77a2576@nxp.com
Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
Signed-off-by: Kwame Adwere <kwame.adwere@nxp.com>
---
Changes since v5:
  - Moved ioctl command definitions into header file.
  - Version 5 patch review comments addressed.
  - Corporate lawyer sign-off updated.

 .../bindings/uwb/nxp,uwb-sr1xx.yaml           | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml

diff --git a/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
new file mode 100644
index 000000000000..2a1caa661633
--- /dev/null
+++ b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/uwb/nxp,uwb-sr1xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title:  NXP SR1XX SOC family of Ultra Wide Band(UWB) device bindings
+
+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>;
+        spi {
+            /* needed to avoid dtc warning */
+            #address-cells = <1>;
+            #size-cells = <0>;
+            interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
+            status = "enabled";
+
+            sr1xx: sr1xx@0 {
+                compatible = "nxp,sr1xx";
+                reg = <0>;    /* CE0 */
+                /* GPIO_24 (PIN 18) Host Irq*/
+                nxp,sr1xx-irq-gpios = <&gpio 24 0>;
+                /* GPIO_18(PIN 12) Chip Enable*/
+                nxp,sr1xx-ce-gpios = <&gpio 18 0>;
+                /* GPIO_23(PIN 16) Read Indication from Host to SR1xx*/
+                nxp,sr1xx-ri-gpios = <&gpio 23 0>;
+                /*max supported frequency */
+                spi-max-frequency = <20000000>;
+            };
+        };
+    };
-- 
2.38.1

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

[Krzysztof Kozlowski]

On 20/12/2022 16:47, Manjunatha Venkatesh wrote:
> Ultra-wideband (UWB) is a short-range wireless communication protocol.
> 
> NXP has SR1XX family of UWB Subsystems (UWBS) devices. SR1XX SOCs
> are FiRa Compliant. SR1XX SOCs are flash less devices and they need
> Firmware Download on every device boot. More details on the SR1XX Family
> can be found at https://www.nxp.com/products/:UWB-TRIMENSION
> 
> The sr1xx driver work the SR1XX Family of UWBS, and uses UWB Controller
> Interface (UCI).  The corresponding details are available in the FiRa
> Consortium Website (https://www.firaconsortium.org/).
> 
> Link: https://lore.kernel.org/r/425858dc-59fe-2311-61ae-3b6dc77a2576@nxp.com
> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> Signed-off-by: Kwame Adwere <kwame.adwere@nxp.com>
> ---
> Changes since v5:
>   - Moved ioctl command definitions into header file.

How is this related to the binding?

>   - Version 5 patch review comments addressed.
>   - Corporate lawyer sign-off updated.
> 
>  .../bindings/uwb/nxp,uwb-sr1xx.yaml           | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> new file mode 100644
> index 000000000000..2a1caa661633
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +

Drop blank line.

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

Drop double space and "device bindings"

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

Drop references to driver. Missing new line after description.

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

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

> +    The corresponding details are available in the FiRa Consortium Website.
> +    (https://www.firaconsortium.org/). More details on the SR1XX Family can be
> +    found at https://www.nxp.com/products/:UWB-TRIMENSION

You need to describe the hardware here.

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

Really?

This is a friendly reminder during the review process.

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

Thank you.

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

Best regards,
Krzysztof

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

[Rob Herring]

On Tue, 20 Dec 2022 21:17:46 +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/425858dc-59fe-2311-61ae-3b6dc77a2576@nxp.com
> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> Signed-off-by: Kwame Adwere <kwame.adwere@nxp.com>
> ---
> Changes since v5:
>   - Moved ioctl command definitions into header file.
>   - Version 5 patch review comments addressed.
>   - Corporate lawyer sign-off updated.
> 
>  .../bindings/uwb/nxp,uwb-sr1xx.yaml           | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.yaml:8:8: [warning] too many spaces after colon (colons)

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.example.dts:31.31-32 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:406: Documentation/devicetree/bindings/uwb/nxp,uwb-sr1xx.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1492: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221220154747.2372597-2-manjunatha.venkatesh@nxp.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH v6 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/dfe167cf-5d4e-6fc7-c954-25f719b1e843@nxp.com
Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
Signed-off-by: Kwame Adwere <kwame.adwere@nxp.com>
---
Changes since v5:
  - Moved ioctl command definitions into header file.
  - Version 5 patch review comments addressed.
  - Corporate lawyer sign-off updated.

 MAINTAINERS               |   7 +
 drivers/misc/Kconfig      |  11 +
 drivers/misc/Makefile     |   1 +
 drivers/misc/nxp-sr1xx.c  | 728 ++++++++++++++++++++++++++++++++++++++
 include/uapi/misc/sr1xx.h |  13 +
 5 files changed, 760 insertions(+)
 create mode 100644 drivers/misc/nxp-sr1xx.c
 create mode 100644 include/uapi/misc/sr1xx.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 5490b1f94803..39e5a92a5486 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15083,6 +15083,13 @@ 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
+F:	include/uapi/misc/sr1xx.h
+
 NXP i.MX 8MP DW100 V4L2 DRIVER
 M:	Xavier Roumegue <xavier.roumegue@oss.nxp.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 9947b7892bd5..5a8fe0fe28b8 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -493,6 +493,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 87b54a4a4422..b62c77b22ad7 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -64,3 +64,4 @@ obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
 obj-$(CONFIG_OPEN_DICE)		+= open-dice.o
 obj-$(CONFIG_GP_PCI1XXXX)	+= mchp_pci1xxxx/
 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..397b38220364
--- /dev/null
+++ b/drivers/misc/nxp-sr1xx.c
@@ -0,0 +1,728 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+/*
+ * SPI driver for UWB SR1xx
+ * Copyright 2018-2022 NXP.
+ *
+ * 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>
+#include <uapi/misc/sr1xx.h>
+
+#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 UCI_MT_MASK 0xE0
+
+#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
+
+/* 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 */
+};
+
+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)
+{
+	long ret = 0;
+	struct sr1xx_dev *sr1xx_dev = NULL;
+
+	sr1xx_dev = filp->private_data;
+	if (sr1xx_dev == NULL) {
+		ret = -EINVAL;
+		pr_err("%s sr1xx_dev is NULL\n", __func__);
+		return ret;
+	}
+	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)
+{
+	unsigned long timeout;
+
+	timeout = jiffies + msecs_to_jiffies(10);
+
+	do {
+		usleep_range(10, 15);
+		if (time_after(jiffies, timeout)) {
+			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
+ *               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 = -EIO;
+	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 = -EIO;
+			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;
+		}
+		if ((sr1xx_dev->rx_buffer[0] & UCI_MT_MASK) == 0) {
+			sr1xx_dev->total_bytes_to_read =
+				sr1xx_dev->rx_buffer[UCI_PAYLOAD_LEN_OFFSET];
+			sr1xx_dev->total_bytes_to_read =
+				((sr1xx_dev->total_bytes_to_read << 8) |
+				 sr1xx_dev
+					 ->rx_buffer[UCI_EXT_PAYLOAD_LEN_OFFSET]);
+		} else {
+			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 = -ENOBUFS;
+			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 error code 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
+ *              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 = -EIO;
+		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 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,
+	.compat_ioctl = sr1xx_dev_ioctl,
+};
+
+static int sr1xx_probe(struct spi_device *spi)
+{
+	int ret;
+	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;
+	}
+
+	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(
+		devm_gpiod_get(&spi->dev, "nxp,sr1xx-irq", GPIOD_IN));
+	sr1xx_dev->ce_gpio = desc_to_gpio(
+		devm_gpiod_get(&spi->dev, "nxp,sr1xx-ce", GPIOD_OUT_LOW));
+	sr1xx_dev->spi_handshake_gpio = desc_to_gpio(
+		devm_gpiod_get(&spi->dev, "nxp,sr1xx-ri", GPIOD_OUT_LOW));
+
+	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);
+	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 = { 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,
+};
+
+module_spi_driver(sr1xx_driver);
+
+MODULE_AUTHOR("Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>");
+MODULE_DESCRIPTION("NXP SR1XX SPI driver");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/misc/sr1xx.h b/include/uapi/misc/sr1xx.h
new file mode 100644
index 000000000000..e774108e6297
--- /dev/null
+++ b/include/uapi/misc/sr1xx.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * SPI driver for UWB SR1xx
+ * Copyright (C) 2018-2022 NXP.
+ *
+ * Author: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
+ */
+#define SR1XX_MAGIC 0xEA
+#define SR1XX_SET_PWR _IOW(SR1XX_MAGIC, 0x01, uint32_t)
+#define SR1XX_SET_FWD _IOW(SR1XX_MAGIC, 0x02, uint32_t)
+
+/* Power enable/disable and read abort ioctl arguments */
+enum { PWR_DISABLE = 0, PWR_ENABLE, ABORT_READ_PENDING };
-- 
2.38.1

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

[Krzysztof Kozlowski]

On 20/12/2022 16:47, Manjunatha Venkatesh wrote:
> Ultra-wideband (UWB) is a short-range wireless communication protocol.
> 
> NXP has SR1XX family of UWB Subsystems (UWBS) devices. SR1XX SOCs
> are FiRa Compliant. SR1XX SOCs are flash less devices and they need
> Firmware Download on every device boot. More details on the SR1XX Family
> can be found at https://www.nxp.com/products/:UWB-TRIMENSION
> 

(...)

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

Lines of comments or code should not be part of anyone's Key Performance
Indicators (KPI). Don't bring your KPI to Linux. Drop all such useless
comments and adjust your style to Linux coding style.

Best regards,
Krzysztof

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

[Greg KH]

On Tue, Dec 20, 2022 at 09:17:47PM +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/).
> 
> 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/dfe167cf-5d4e-6fc7-c954-25f719b1e843@nxp.com

Why is this here pointing to an old version of the patch?

> Signed-off-by: Manjunatha Venkatesh <manjunatha.venkatesh@nxp.com>
> Signed-off-by: Kwame Adwere <kwame.adwere@nxp.com>
> ---
> Changes since v5:
>   - Moved ioctl command definitions into header file.
>   - Version 5 patch review comments addressed.
>   - Corporate lawyer sign-off updated.

Again, you are not documenting _WHY_ you want a dual license here, which
is what I asked for yesterday, why ignore that and ask us to review this
again?

> +#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 UCI_MT_MASK 0xE0
> +
> +#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

Please use tabs.

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

If you want to document a structure (which is a good thing), please do
so in a way we can understand it (i.e. with kernel doc format like we do
everywhere else in the kernel), not like this which is almost impossible
to follow.

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

Why is this a global symbol?


> +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)
> +{
> +	long ret = 0;
> +	struct sr1xx_dev *sr1xx_dev = NULL;
> +
> +	sr1xx_dev = filp->private_data;
> +	if (sr1xx_dev == NULL) {
> +		ret = -EINVAL;
> +		pr_err("%s sr1xx_dev is NULL\n", __func__);
> +		return ret;
> +	}
> +	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");

So userspace can spam the kernel log by sending your driver invalid
ioctls?  Please never do that.

> +		ret = -EINVAL;

Isn't this the wrong error value for an incorrect ioctl command?

> +	}
> +	return ret;
> +}
> +
> +/**
> + * sr1xx_wait_for_irq_gpio_low
> + *
> + * Function to wait till irq gpio goes low state
> + *
> + */

Odd commenting style, please either use the correct kerneldoc format, or
a simpler one, or nothing at all.  Please do not make up your own format
style just for a single driver as that is unmaintainable, right?

Also, I see no justification here for why this has to be a kernel
driver, OR where the userspace code is that controls this.

I've stopped here :(

greg k-h

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

[Guenter Roeck]

On Wed, Dec 21, 2022 at 07:12:49AM +0100, Greg KH wrote:
> On Tue, Dec 20, 2022 at 09:17:47PM +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/).
> > 
> > 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
> > 
[ ... ]
> 
> Also, I see no justification here for why this has to be a kernel
> driver, OR where the userspace code is that controls this.
> 

After seeing all the warnings generated by 0-day after this driver was
applied to chromeos-6.1, my basic question is similar but boils down to:
Why does this need a userspace component to start with, and why wasn't it
implemented as firmware download driver ?

Guenter