[PATCH 0/2] staging: iio: imu: Add CEVA BNO08x through the staging tree

[PATCH 0/2] staging: iio: imu: Add CEVA BNO08x through the staging tree

[Jacopo Mondi]

Hello,
   this series adds support for the CEVA BNO08x Sensor Hub through the staging
tree.

The driver currently only support reading a rotation vector in quaternion, which
is just one of the many features supported by the device.

What's relevant here is the protocol handling part, which is used to exposed a
triggered buffer and direct access IIO interfaces. The BNO08x family implements
a packed-based protocol which requires parsing on the driver side. This driver
implements such parsing routine by copying the full received packets and
synchronize the possible multiple consumers which might want to access the data.

Sent through the staging tree as the HW platform I used to test is not available
anymore and I cannot guarantee maintainership of the driver.

However I hope the driver can serve as a useful reference for future
developments.

Thanks
  j

Jacopo Mondi (2):
  dt-bindings: staging: iio: imu: Document CEVA BNO08x
  staging: iio: imu: Add CEVA BNO08x driver

 .../bindings/staging/iio/imu/ceva,bno08x.yaml |  63 ++
 MAINTAINERS                                   |   7 +
 drivers/staging/iio/Kconfig                   |   1 +
 drivers/staging/iio/Makefile                  |   1 +
 drivers/staging/iio/imu/bno08x/Kconfig        |  11 +
 drivers/staging/iio/imu/bno08x/Makefile       |   3 +
 drivers/staging/iio/imu/bno08x/bno08x.c       | 618 ++++++++++++++++++
 7 files changed, 704 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
 create mode 100644 drivers/staging/iio/imu/bno08x/Kconfig
 create mode 100644 drivers/staging/iio/imu/bno08x/Makefile
 create mode 100644 drivers/staging/iio/imu/bno08x/bno08x.c

--
2.35.1

[PATCH 1/2] dt-bindings: staging: iio: imu: Document CEVA BNO08x

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

Document CEVA BNO08x (080, 085 and 086) Sensor Hub.

The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
firmware.

Datasheet:
https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../bindings/staging/iio/imu/ceva,bno08x.yaml | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml

diff --git a/Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml b/Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
new file mode 100644
index 000000000000..f7d6ea6d9147
--- /dev/null
+++ b/Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/staging/iio/imu/ceva,bno08x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CEVA BNO080/85/86
+
+maintainers:
+  - Jacopo Mondi <jacopo@jmondi.org>
+
+description: |
+  CEVA BNO08x is a System in Package (SiP) that integrates a triaxial
+  accelerometer, triaxial gyroscope, magnetometer and a 32-bit ARM Cortex-M0+
+  microcontroller running CEVA's SH-2 firmware.
+
+  The BNO08x family is documented at
+  https://www.ceva-dsp.com/product/bno080-085/
+  https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
+
+  The SH-2 firmware reference is available at
+  https://cdn.sparkfun.com/assets/4/d/9/3/8/SH-2-Reference-Manual-v1.2.pdf
+  https://cdn.sparkfun.com/assets/7/6/9/3/c/Sensor-Hub-Transport-Protocol-v1.7.pdf
+
+properties:
+  compatible:
+    const: ceva,bno08x
+
+  reg:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    description:
+      Phandle to the GPIO connected to the chip H_INTN (Host Interrupt) pin.
+      The interrupt type shall be IRQ_TYPE_EDGE_FALLING.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bno080: imu@4b {
+            compatible = "ceva,bno08x";
+            interrupt-parent = <&gpio5>;
+            interrupts = <8 IRQ_TYPE_EDGE_FALLING>;
+            reg = <0x4b>;
+        };
+    };
+
--
2.35.1

Re: [PATCH 1/2] dt-bindings: staging: iio: imu: Document CEVA BNO08x

[Rob Herring]

On Thu, Jun 16, 2022 at 12:00:05PM +0200, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Document CEVA BNO08x (080, 085 and 086) Sensor Hub.
> 
> The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> firmware.
> 
> Datasheet:
> https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../bindings/staging/iio/imu/ceva,bno08x.yaml | 63 +++++++++++++++++++

You expect this binding to change? If so, then why define the ABI. If 
not, then why is it in staging? IOW, I don't really want additions to 
bindings/staging/.

If this isn't in a .dts file and the driver isn't stable yet, then just 
don't document it until it is.

>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml b/Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
> new file mode 100644
> index 000000000000..f7d6ea6d9147
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/staging/iio/imu/ceva,bno08x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CEVA BNO080/85/86
> +
> +maintainers:
> +  - Jacopo Mondi <jacopo@jmondi.org>
> +
> +description: |
> +  CEVA BNO08x is a System in Package (SiP) that integrates a triaxial
> +  accelerometer, triaxial gyroscope, magnetometer and a 32-bit ARM Cortex-M0+
> +  microcontroller running CEVA's SH-2 firmware.
> +
> +  The BNO08x family is documented at
> +  https://www.ceva-dsp.com/product/bno080-085/
> +  https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> +
> +  The SH-2 firmware reference is available at
> +  https://cdn.sparkfun.com/assets/4/d/9/3/8/SH-2-Reference-Manual-v1.2.pdf
> +  https://cdn.sparkfun.com/assets/7/6/9/3/c/Sensor-Hub-Transport-Protocol-v1.7.pdf
> +
> +properties:
> +  compatible:
> +    const: ceva,bno08x

Don't use wildcards in compatibles.

> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +    description:
> +      Phandle to the GPIO connected to the chip H_INTN (Host Interrupt) pin.
> +      The interrupt type shall be IRQ_TYPE_EDGE_FALLING.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        bno080: imu@4b {
> +            compatible = "ceva,bno08x";
> +            interrupt-parent = <&gpio5>;
> +            interrupts = <8 IRQ_TYPE_EDGE_FALLING>;
> +            reg = <0x4b>;
> +        };
> +    };
> +
> --
> 2.35.1
> 
> 

[PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

Add support for CEVA BNO08x Sensor Hub.

The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
firmware.

Datasheet:
https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 MAINTAINERS                             |   7 +
 drivers/staging/iio/Kconfig             |   1 +
 drivers/staging/iio/Makefile            |   1 +
 drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
 drivers/staging/iio/imu/bno08x/Makefile |   3 +
 drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++
 6 files changed, 641 insertions(+)
 create mode 100644 drivers/staging/iio/imu/bno08x/Kconfig
 create mode 100644 drivers/staging/iio/imu/bno08x/Makefile
 create mode 100644 drivers/staging/iio/imu/bno08x/bno08x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 96b8c3560117..4b709599c82b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4314,6 +4314,13 @@ F:	certs/
 F:	scripts/extract-cert.c
 F:	scripts/sign-file.c
 
+CEVA BNO08x SENSOR HUB
+ML	Jacopo Mondi <jacopo@jmondi.org>
+L:	linux-iio@vger.kernel.org
+S:	Odd Fixes
+F:	Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
+F	drivers/staging/iio/imu/bno08x/
+
 CFAG12864B LCD DRIVER
 M:	Miguel Ojeda <ojeda@kernel.org>
 S:	Maintained
diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
index a8e970db179d..bcecace954a5 100644
--- a/drivers/staging/iio/Kconfig
+++ b/drivers/staging/iio/Kconfig
@@ -11,6 +11,7 @@ source "drivers/staging/iio/addac/Kconfig"
 source "drivers/staging/iio/cdc/Kconfig"
 source "drivers/staging/iio/frequency/Kconfig"
 source "drivers/staging/iio/impedance-analyzer/Kconfig"
+source "drivers/staging/iio/imu/Kconfig"
 source "drivers/staging/iio/meter/Kconfig"
 source "drivers/staging/iio/resolver/Kconfig"
 
diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
index b15904b99581..eafcc25b64f9 100644
--- a/drivers/staging/iio/Makefile
+++ b/drivers/staging/iio/Makefile
@@ -9,5 +9,6 @@ obj-y += addac/
 obj-y += cdc/
 obj-y += frequency/
 obj-y += impedance-analyzer/
+obj-y += imu/
 obj-y += meter/
 obj-y += resolver/
diff --git a/drivers/staging/iio/imu/bno08x/Kconfig b/drivers/staging/iio/imu/bno08x/Kconfig
new file mode 100644
index 000000000000..109ac11ed4f9
--- /dev/null
+++ b/drivers/staging/iio/imu/bno08x/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config CEVA_BNO08x_IMU
+	tristate "CEVA BNO08x IMU driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for CEVA BNO08x IMU.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called bno08x.
diff --git a/drivers/staging/iio/imu/bno08x/Makefile b/drivers/staging/iio/imu/bno08x/Makefile
new file mode 100644
index 000000000000..48bbbf12a366
--- /dev/null
+++ b/drivers/staging/iio/imu/bno08x/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_CEVA_BNO08x_IMU) += bno08x.o
diff --git a/drivers/staging/iio/imu/bno08x/bno08x.c b/drivers/staging/iio/imu/bno08x/bno08x.c
new file mode 100644
index 000000000000..e07735a85b34
--- /dev/null
+++ b/drivers/staging/iio/imu/bno08x/bno08x.c
@@ -0,0 +1,618 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CEVA BNO08x IMU driver.
+ *
+ * Copyright (c) 2021 Jacopo Mondi <jacopo@jmondi.org>
+ */
+
+#include <linux/atomic.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+
+#define DRIVER_NAME "bno08x"
+
+#define BNO08x_SHTP_HDR_SIZE		0x4
+#define BNO08x_SHTP_HDR_LEN_LSB		0x0
+#define BNO08x_SHTP_HDR_LEN_MSB		0x1
+#define BNO08x_SHTP_HDR_CHAN		0x2
+#define BNO08x_SHTP_HDR_SEQ		0x3
+
+#define BNO08x_SHTP_MAX_SIZE		128
+#define BNO08x_SHTP_ADV_MAX_SIZE	512
+
+#define BNO08x_SHTP_SOFT_RESET		0x01
+#define BNO08x_SHTP_PROD_ID		0xf8
+#define BNO08x_SHTP_PROD_ID_REQ		0xf9
+
+#define BNO08x_SHTP_GET_FEATURE_RESP	0xfc
+#define BNO08x_SHTP_SET_FEATURE_CMD	0xfd
+#define BNO08x_SHTP_GET_FEATURE_REQ	0xfe
+
+#define BNO08x_ROT_SCAN_INDEX		0x01
+#define BNO08x_REPORTID_ROTATION_VEC	0x05
+#define BNO08x_REPORTID_TIMESTAMP_BASE	0xfb
+
+enum bno08x_shtp_channels {
+	BNO08x_SHTP_COMMAND_CHAN,
+	BNO08x_SHTP_EXECTUABLE_CHAN,
+	BNO08x_SHTP_CONTROL_CHAN,
+	BNO08x_SHTP_REPORTS_CHAN,
+	BNO08x_SHTP_WAKE_REPORTS_CHAN,
+	BNO08x_SHTP_GYRO_CHAN,
+	BNO08x_NUM_CHANNELS,
+};
+
+struct bno08x_dev {
+	struct i2c_client *client;
+
+	struct iio_trigger *trig;
+
+	/* Completions for cargo transfer. */
+	struct completion cargo_ready;
+	struct completion waiters_done;
+
+	/* cargo sequence number per channel. */
+	u32 seq_num[BNO08x_NUM_CHANNELS];
+
+	/* Mask of the enabled report ids. */
+	u64 enabled_reports_mask;
+
+#define BNO08x_CARGO_BUFFER_SIZE	1024lu
+	struct {
+		u8 buffer[BNO08x_CARGO_BUFFER_SIZE];
+		size_t len;
+		atomic_t waiters;
+
+		/* Protects access to the cargo buffer content. */
+		struct mutex mutex;
+	} cargo;
+};
+
+static irqreturn_t bno08x_interrupt(int irq, void *dev_id)
+{
+	struct iio_dev *iio_dev = dev_id;
+
+	if (iio_dev->trig)
+		iio_trigger_poll((void *)iio_dev->trig);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t bno08x_dump_cargo(int irq, void *dev_id)
+{
+	struct iio_dev *iio_dev = dev_id;
+	struct bno08x_dev *bno08x = iio_priv(iio_dev);
+	struct i2c_client *client = bno08x->client;
+	u8 *cargo = bno08x->cargo.buffer;
+	size_t to_read;
+	size_t len;
+	int ret;
+
+	mutex_lock(&bno08x->cargo.mutex);
+
+	/* Read only the header first to know how many bytes we expect to receive. */
+	ret = i2c_master_recv(client, bno08x->cargo.buffer, BNO08x_SHTP_HDR_SIZE);
+	if (ret != BNO08x_SHTP_HDR_SIZE)
+		goto out_unlock;
+
+	/*
+	 * Clear the top bit: it means a cargo is a continuation of the last one.
+	 * Ignore it for now.
+	 */
+	len = (cargo[BNO08x_SHTP_HDR_LEN_MSB] << 8 |
+	       cargo[BNO08x_SHTP_HDR_LEN_LSB]) & ~BIT(15);
+
+	if (len == 0)
+		goto out_complete;
+
+	if (len > BNO08x_CARGO_BUFFER_SIZE)
+		dev_warn(&client->dev,
+			 "Cargo size exceeds buffer: content will be unusable\n");
+
+	/*
+	 * Read the full cargo now that we know its length.
+	 *
+	 * If the reported length exceeds the max transfer size, read the cargo
+	 * in chunks. Its content will be unusable though.
+	 */
+	to_read = len;
+	len = min(len, BNO08x_CARGO_BUFFER_SIZE);
+	while (to_read) {
+		memset(bno08x->cargo.buffer, 0, len);
+
+		ret = i2c_master_recv(client, bno08x->cargo.buffer, len);
+		if (ret != len) {
+			dev_err(&client->dev,
+				"Failed to read cargo of size %lu: %d\n",
+				len, ret);
+			goto out_complete;
+		}
+
+		to_read -= len;
+	}
+
+	bno08x->cargo.len = len;
+
+out_complete:
+	if (atomic_read(&bno08x->cargo.waiters) == 0)
+		goto out_unlock;
+
+	/*
+	 * Wake up all waiters and let them read the cargo buffer.
+	 *
+	 * Unlock the cargo mutex only after all waiters are done, to avoid
+	 * this interrupt handler re-writing the buffer content with the next
+	 * cargo before waiters have read it.
+	 */
+	complete_all(&bno08x->cargo_ready);
+
+	wait_for_completion(&bno08x->waiters_done);
+	reinit_completion(&bno08x->waiters_done);
+
+out_unlock:
+	mutex_unlock(&bno08x->cargo.mutex);
+
+	return IRQ_HANDLED;
+}
+
+static int bno08x_write_cargo(struct bno08x_dev *bno08x,
+			      enum bno08x_shtp_channels channel,
+			      u8 *cargo, u16 length)
+{
+	u16 cargo_length = length + BNO08x_SHTP_HDR_SIZE;
+	struct i2c_client *client = bno08x->client;
+	int ret;
+
+	/* Assemble header */
+	cargo[BNO08x_SHTP_HDR_LEN_LSB] = cargo_length & 0xff;
+	cargo[BNO08x_SHTP_HDR_LEN_MSB] = cargo_length >> 8;
+	cargo[BNO08x_SHTP_HDR_CHAN] = channel;
+	cargo[BNO08x_SHTP_HDR_SEQ] = bno08x->seq_num[channel]++;
+
+	ret = i2c_master_send(client, cargo, cargo_length);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write cargo to channel %u: %d\n",
+			channel, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int bno08x_wait_for_cargo_timeout(struct bno08x_dev *bno08x, u8 *cargo,
+					 size_t len)
+{
+	static const unsigned long complete_timeout = 500; /* msecs */
+	struct i2c_client *client = bno08x->client;
+	int ret = 0;
+
+	atomic_inc(&bno08x->cargo.waiters);
+
+	if (!wait_for_completion_timeout(&bno08x->cargo_ready,
+					 msecs_to_jiffies(complete_timeout))) {
+		dev_dbg(&client->dev, "Wait for chip interrupt timeout.\n");
+		ret = -EIO;
+		goto out_unlock;
+	}
+
+	/* The caller is not interested in the data. */
+	if (!len)
+		goto out_unlock;
+
+	/*
+	 * Multiple readers might want to inspect the cargo content in response
+	 * to the complete_all(cargo_ready) signal.
+	 *
+	 * Copy data into the caller buffer to allow multiple threads to
+	 * access the cargo content independently.
+	 */
+	ret = min(len, bno08x->cargo.len);
+	memcpy(cargo, bno08x->cargo.buffer, ret);
+
+out_unlock:
+	/* The last waiter unlocks the cargo read routine. */
+	if (atomic_dec_return(&bno08x->cargo.waiters) == 0) {
+		reinit_completion(&bno08x->cargo_ready);
+		complete(&bno08x->waiters_done);
+	}
+
+	return ret;
+}
+
+static const struct iio_chan_spec bno08x_iio_channels[] = {
+	{
+		.type = IIO_ROT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = BNO08x_ROT_SCAN_INDEX,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.shift = 0,
+			.endianness = IIO_LE,
+		},
+	},
+};
+
+static int bno08x_enable_feature_report(struct bno08x_dev *bno08x, u8 report_id)
+{
+	struct i2c_client *client = bno08x->client;
+	/* Reporting interval hardcoded to 5 msec. */
+	unsigned int report_interval_usec = 5000;
+	u8 cargo[BNO08x_SHTP_MAX_SIZE] = {};
+	u8 *data = cargo + BNO08x_SHTP_HDR_SIZE;
+	unsigned int max_attempts = 50;
+	int ret;
+
+	if (bno08x->enabled_reports_mask & BIT(report_id))
+		return 0;
+
+	/*
+	 * Enable reporting the requested feature. The full "feature set
+	 * request" package size is 17 bytes.
+	 */
+	data[0] = BNO08x_SHTP_SET_FEATURE_CMD;
+	data[1] = report_id;
+
+	data[5] = (report_interval_usec >> 0);
+	data[6] = (report_interval_usec >> 8);
+	data[7] = (report_interval_usec >> 16);
+	data[8] = (report_interval_usec >> 24);
+
+	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 17);
+	if (ret)
+		return ret;
+
+	/*
+	 * Verify the requested feature is enabled inspecting the
+	 * 'feature request status" response cargo.
+	 */
+	memset(cargo, 0, BNO08x_SHTP_MAX_SIZE);
+	data[0] = BNO08x_SHTP_GET_FEATURE_REQ;
+	data[1] = report_id;
+
+	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 2);
+	if (ret)
+		return ret;
+
+	/*
+	 * Wait for a "get feature response". The datasheet says it will "arrive
+	 * some time in the future". Read up to 10 packets then give up.
+	 */
+	while (--max_attempts) {
+		memset(cargo, 0, 5);
+		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 5);
+		if (ret < 0)
+			continue;
+
+		if (ret < 5)
+			continue;
+
+		if (data[0] == BNO08x_SHTP_GET_FEATURE_RESP ||
+		    data[1] == report_id)
+			break;
+	}
+	if (!max_attempts) {
+		dev_err(&client->dev,
+			"Failed to parse GET_FEATURE_RESPONSE: bad header\n");
+		return -EINVAL;
+	}
+
+	bno08x->enabled_reports_mask |= BIT(report_id);
+	dev_dbg(&client->dev, "Reporting of feature %x enabled!\n", report_id);
+
+	return 0;
+}
+
+static int bno08x_read_raw_rotation(struct bno08x_dev *bno08x, int *vals,
+				    int *val_len, int max_len)
+{
+	struct i2c_client *client = bno08x->client;
+	unsigned int max_attempts = 50;
+	u8 cargo[24];
+	int ret;
+
+	if (max_len < 3)
+		return -EINVAL;
+
+	ret = bno08x_enable_feature_report(bno08x, BNO08x_REPORTID_ROTATION_VEC);
+	if (ret)
+		return ret;
+
+	/*
+	 * Read the rotation vector in I, J and K quaternions.
+	 *
+	 * We're going to receive several other cargos before an actual rotation
+	 * vector input report, so we parse the cargo fields and only proceed
+	 * with a cargo that refers to the rotation vector report id.
+	 *
+	 * Use a sentinel to avoid looping forever, unfortunately we can't
+	 * really know how many cargos we have to discard before receiving the
+	 * 'right' one.
+	 */
+	while (--max_attempts) {
+		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 24);
+		if (ret < 0)
+			continue;
+
+		if (ret != 24)
+			continue;
+
+		/*
+		 * Parse the cargo content to make sure it's a well-formed input
+		 * report containing rotation vector information.
+		 *
+		 * The layout of an input report cargo is reported by Figure 5.2
+		 * of "BNO080/85/86 Datasheet 1000-3927 v.1.11".
+		 *
+		 * The rotation vector input report is 24 bytes long.
+		 */
+		if (cargo[2] == BNO08x_SHTP_REPORTS_CHAN &&
+		    cargo[4] == BNO08x_REPORTID_TIMESTAMP_BASE &&
+		    cargo[9] == BNO08x_REPORTID_ROTATION_VEC)
+			break;
+	}
+	if (!max_attempts) {
+		dev_err(&client->dev, "Failed to receive the correct input report\n");
+		return -EINVAL;
+	}
+
+	/* Read the I, J, K quaternions. */
+	vals[0] = cargo[13] | (cargo[14] << 8);
+	vals[1] = cargo[15] | (cargo[16] << 8);
+	vals[2] = cargo[17] | (cargo[18] << 8);
+	*val_len = 3;
+
+	return IIO_VAL_INT_MULTIPLE;
+}
+
+static int bno08x_read_raw_multi(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan, int max_len,
+				 int *vals, int *val_len, long mask)
+{
+	struct bno08x_dev *bno08x = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return bno08x_read_raw_rotation(bno08x, vals, val_len, max_len);
+
+	case IIO_CHAN_INFO_SCALE:
+		vals[0] = 0;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct iio_info bno08x_iio_info = {
+	.read_raw_multi = bno08x_read_raw_multi,
+};
+
+static int bno08x_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool enable)
+{
+	struct iio_dev *iio_dev = iio_trigger_get_drvdata(trig);
+	struct bno08x_dev *bno08x = iio_priv(iio_dev);
+	int ret;
+
+	if (!enable)
+		return 0;
+
+	ret = bno08x_enable_feature_report(bno08x, BNO08x_REPORTID_ROTATION_VEC);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct iio_trigger_ops bno08x_trigger_ops = {
+	.set_trigger_state = &bno08x_data_rdy_trigger_set_state,
+};
+
+static irqreturn_t bno08x_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *iio_dev = pf->indio_dev;
+	struct bno08x_dev *bno08x = iio_priv(iio_dev);
+	u8 cargo[BNO08x_CARGO_BUFFER_SIZE];
+	int ret;
+
+	ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, BNO08x_CARGO_BUFFER_SIZE);
+	if (ret < 0)
+		goto done;
+
+	if (ret < 24)
+		goto done;
+
+	/* Make sure the cargo matches the active scan channel. */
+	if (*iio_dev->active_scan_mask & BIT(BNO08x_ROT_SCAN_INDEX) &&
+	    cargo[2] == BNO08x_SHTP_REPORTS_CHAN &&
+	    cargo[4] == BNO08x_REPORTID_TIMESTAMP_BASE &&
+	    cargo[9] == BNO08x_REPORTID_ROTATION_VEC) {
+		u16 data;
+
+		/* Unit quaternion I component. */
+		data = le16_to_cpu(cargo[13]);
+		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
+
+		/* Unit quaternion J component. */
+		data = le16_to_cpu(cargo[15]);
+		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
+
+		/* Unit quaternion K component. */
+		data = le16_to_cpu(cargo[17]);
+		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
+	}
+
+done:
+	iio_trigger_notify_done(iio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int bno08x_check_prod_id(struct bno08x_dev *bno08x)
+{
+	struct i2c_client *client = bno08x->client;
+	u8 cargo[BNO08x_SHTP_MAX_SIZE] = {};
+	u8 *data = cargo + BNO08x_SHTP_HDR_SIZE;
+	int ret;
+
+	data[0] = BNO08x_SHTP_PROD_ID_REQ;
+	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 2);
+	if (ret)
+		return ret;
+
+	while (true) {
+		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 20);
+		if (ret < 0) {
+			dev_dbg(&client->dev, "Failed to read PROD_ID: %d\n", ret);
+			return ret;
+		}
+
+		if (ret >= 20 && data[0] == BNO08x_SHTP_PROD_ID)
+			break;
+	}
+
+	dev_dbg(&client->dev, "FW version: 0x%x.%x\n", data[2], data[3]);
+
+	return 0;
+}
+
+static int bno08x_soft_reset(struct bno08x_dev *bno08x)
+{
+	struct i2c_client *client = bno08x->client;
+	u8 cargo[20] = {};
+	int ret;
+
+	/* Soft reset the device: write 0x01 to EXECUTABLE channel. */
+	cargo[BNO08x_SHTP_HDR_SIZE] = BNO08x_SHTP_SOFT_RESET;
+	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_EXECTUABLE_CHAN, cargo, 1);
+	if (ret) {
+		dev_err(&client->dev, "Failed to soft-reset BNO08x\n");
+		return ret;
+	}
+
+	/*
+	 * Now read the all the channel advertisments packets.
+	 * We don't care about the content.
+	 */
+	do {
+		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 0);
+		if (ret < 0) {
+			dev_dbg(&client->dev,
+				"Failed to read channel advertisments: %d\n", ret);
+			return ret;
+		}
+	} while (ret != 0);
+
+	/*
+	 * Give the chip some time to stabilize. There's no mention of any
+	 * delay required after startup in the manual, but this makes operation
+	 * stable through module load/unload.
+	 */
+	usleep_range(15000, 20000);
+
+	return 0;
+}
+
+static int bno08x_probe(struct i2c_client *client)
+{
+	struct bno08x_dev *bno08x;
+	struct iio_dev *iio_dev;
+	int ret;
+
+	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bno08x));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(&client->dev, iio_dev);
+
+	bno08x = iio_priv(iio_dev);
+	bno08x->client = client;
+
+	mutex_init(&bno08x->cargo.mutex);
+	init_completion(&bno08x->cargo_ready);
+	init_completion(&bno08x->waiters_done);
+
+	iio_dev->info = &bno08x_iio_info;
+	iio_dev->name = DRIVER_NAME;
+	iio_dev->channels = bno08x_iio_channels;
+	iio_dev->num_channels = ARRAY_SIZE(bno08x_iio_channels);
+	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
+
+	ret = devm_iio_triggered_buffer_setup(&client->dev, iio_dev,
+					      iio_pollfunc_store_time,
+					      bno08x_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	bno08x->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
+					      DRIVER_NAME,
+					      iio_device_id(iio_dev));
+	if (!bno08x->trig)
+		return -ENOMEM;
+
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						bno08x_interrupt,
+						bno08x_dump_cargo,
+						IRQF_TRIGGER_FALLING,
+						iio_dev->name, iio_dev);
+		if (ret)
+			return ret;
+	}
+
+	bno08x->trig->dev.parent = &client->dev;
+	bno08x->trig->ops = &bno08x_trigger_ops;
+	iio_trigger_set_drvdata(bno08x->trig, iio_dev);
+
+	ret = devm_iio_trigger_register(&client->dev, bno08x->trig);
+	if (ret)
+		return ret;
+
+	iio_dev->trig = iio_trigger_get(bno08x->trig);
+
+	ret = bno08x_soft_reset(bno08x);
+	if (ret)
+		return ret;
+
+	bno08x_check_prod_id(bno08x);
+
+	return devm_iio_device_register(&client->dev, iio_dev);
+}
+
+static const struct of_device_id bno08x_of_match[] = {
+	{ .compatible = "ceva,bno08x" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bno08x_of_match);
+
+static struct i2c_driver bno08x_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = of_match_ptr(bno08x_of_match),
+	},
+	.probe_new = bno08x_probe,
+};
+module_i2c_driver(bno08x_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("CEVA BNO08x IMU driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Greg Kroah-Hartman]

On Thu, Jun 16, 2022 at 12:00:06PM +0200, Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Add support for CEVA BNO08x Sensor Hub.
> 
> The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> firmware.
> 
> Datasheet:
> https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  MAINTAINERS                             |   7 +
>  drivers/staging/iio/Kconfig             |   1 +
>  drivers/staging/iio/Makefile            |   1 +
>  drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
>  drivers/staging/iio/imu/bno08x/Makefile |   3 +
>  drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++

Why are you adding a new driver to staging/iio/ and not to the real part
of the kernel?  What needs to be done to it to get it out of here?

I'd prefer that we not add any new iio drivers if at all possible,
please fix them up properly and merge them to the correct location.

thanks,

greg k-h

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Jacopo Mondi]

Hi Greg

On Thu, Jun 16, 2022 at 12:26:18PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 16, 2022 at 12:00:06PM +0200, Jacopo Mondi wrote:
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Add support for CEVA BNO08x Sensor Hub.
> >
> > The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> > integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> > and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> > firmware.
> >
> > Datasheet:
> > https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  MAINTAINERS                             |   7 +
> >  drivers/staging/iio/Kconfig             |   1 +
> >  drivers/staging/iio/Makefile            |   1 +
> >  drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
> >  drivers/staging/iio/imu/bno08x/Makefile |   3 +
> >  drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++
>
> Why are you adding a new driver to staging/iio/ and not to the real part
> of the kernel?  What needs to be done to it to get it out of here?

Mostly because I don't have access to the hw development platform
anymore, and the one I had was not reliable enough for any further
development anyway.

I went for staging as I cannot guarantee I can test any requested
change on the series.

I'm afraid this is a "take it or leave it" series, and as the driver
does anyway work (even if for a single feature) I thought it was worth
sending it upstream anyway.

>
> I'd prefer that we not add any new iio drivers if at all possible,
> please fix them up properly and merge them to the correct location.
>
> thanks,
>
> greg k-h

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Greg Kroah-Hartman]

On Thu, Jun 16, 2022 at 12:36:34PM +0200, Jacopo Mondi wrote:
> Hi Greg
> 
> On Thu, Jun 16, 2022 at 12:26:18PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 16, 2022 at 12:00:06PM +0200, Jacopo Mondi wrote:
> > > From: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > Add support for CEVA BNO08x Sensor Hub.
> > >
> > > The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> > > integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> > > and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> > > firmware.
> > >
> > > Datasheet:
> > > https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  MAINTAINERS                             |   7 +
> > >  drivers/staging/iio/Kconfig             |   1 +
> > >  drivers/staging/iio/Makefile            |   1 +
> > >  drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
> > >  drivers/staging/iio/imu/bno08x/Makefile |   3 +
> > >  drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++
> >
> > Why are you adding a new driver to staging/iio/ and not to the real part
> > of the kernel?  What needs to be done to it to get it out of here?
> 
> Mostly because I don't have access to the hw development platform
> anymore, and the one I had was not reliable enough for any further
> development anyway.
> 
> I went for staging as I cannot guarantee I can test any requested
> change on the series.
> 
> I'm afraid this is a "take it or leave it" series, and as the driver
> does anyway work (even if for a single feature) I thought it was worth
> sending it upstream anyway.

Sure, but staging is not a dumping ground, if you want to submit it and
run away, I can't take it here, sorry.

Perhaps just let this live as a patch on the mailing list and if anyone
else wants to pick it up and maintain it, they can do so.

thanks,

greg k-h

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Jacopo Mondi]

Hi Greg

On Thu, Jun 16, 2022 at 12:57:30PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 16, 2022 at 12:36:34PM +0200, Jacopo Mondi wrote:
> > Hi Greg
> >
> > On Thu, Jun 16, 2022 at 12:26:18PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 16, 2022 at 12:00:06PM +0200, Jacopo Mondi wrote:
> > > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > Add support for CEVA BNO08x Sensor Hub.
> > > >
> > > > The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> > > > integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> > > > and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> > > > firmware.
> > > >
> > > > Datasheet:
> > > > https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  MAINTAINERS                             |   7 +
> > > >  drivers/staging/iio/Kconfig             |   1 +
> > > >  drivers/staging/iio/Makefile            |   1 +
> > > >  drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
> > > >  drivers/staging/iio/imu/bno08x/Makefile |   3 +
> > > >  drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++
> > >
> > > Why are you adding a new driver to staging/iio/ and not to the real part
> > > of the kernel?  What needs to be done to it to get it out of here?
> >
> > Mostly because I don't have access to the hw development platform
> > anymore, and the one I had was not reliable enough for any further
> > development anyway.
> >
> > I went for staging as I cannot guarantee I can test any requested
> > change on the series.
> >
> > I'm afraid this is a "take it or leave it" series, and as the driver
> > does anyway work (even if for a single feature) I thought it was worth
> > sending it upstream anyway.
>
> Sure, but staging is not a dumping ground, if you want to submit it and
> run away, I can't take it here, sorry.
>
> Perhaps just let this live as a patch on the mailing list and if anyone
> else wants to pick it up and maintain it, they can do so.

That would be fine with me. Dan already had a comment, let's see if
Jonathan or others from the IIO subsystem have more and see what to
do.

Thanks
   j
>
> thanks,
>
> greg k-h

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Jonathan Cameron]

On Thu, 16 Jun 2022 14:30:14 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Greg
> 
> On Thu, Jun 16, 2022 at 12:57:30PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 16, 2022 at 12:36:34PM +0200, Jacopo Mondi wrote:  
> > > Hi Greg
> > >
> > > On Thu, Jun 16, 2022 at 12:26:18PM +0200, Greg Kroah-Hartman wrote:  
> > > > On Thu, Jun 16, 2022 at 12:00:06PM +0200, Jacopo Mondi wrote:  
> > > > > From: Jacopo Mondi <jacopo@jmondi.org>
> > > > >
> > > > > Add support for CEVA BNO08x Sensor Hub.
> > > > >
> > > > > The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> > > > > integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> > > > > and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> > > > > firmware.
> > > > >
> > > > > Datasheet:
> > > > > https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  MAINTAINERS                             |   7 +
> > > > >  drivers/staging/iio/Kconfig             |   1 +
> > > > >  drivers/staging/iio/Makefile            |   1 +
> > > > >  drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
> > > > >  drivers/staging/iio/imu/bno08x/Makefile |   3 +
> > > > >  drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++  
> > > >
> > > > Why are you adding a new driver to staging/iio/ and not to the real part
> > > > of the kernel?  What needs to be done to it to get it out of here?  
> > >
> > > Mostly because I don't have access to the hw development platform
> > > anymore, and the one I had was not reliable enough for any further
> > > development anyway.
> > >
> > > I went for staging as I cannot guarantee I can test any requested
> > > change on the series.
> > >
> > > I'm afraid this is a "take it or leave it" series, and as the driver
> > > does anyway work (even if for a single feature) I thought it was worth
> > > sending it upstream anyway.  
> >
> > Sure, but staging is not a dumping ground, if you want to submit it and
> > run away, I can't take it here, sorry.
> >
> > Perhaps just let this live as a patch on the mailing list and if anyone
> > else wants to pick it up and maintain it, they can do so.  
> 
> That would be fine with me. Dan already had a comment, let's see if
> Jonathan or others from the IIO subsystem have more and see what to
> do.
>

We can do a superficial review, but I'm thinking the same as Greg - maybe
just living on the mailing list until someone has hardware and can test is
the best plan long term.  Note, one option would be to emulate it. We've done
that for a few bits of hard to setup or hard to acquire hardware where we
want to be able to refactor the code.  Also because it's nice to have CI :)
Vincent's (+CC) Roadtest is great for that though this device may be a bit complex
to easily emulate.

https://lore.kernel.org/all/20220311162445.346685-1-vincent.whitchurch@axis.com/

Jonathan

> Thanks
>    j
> >
> > thanks,
> >
> > greg k-h  

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Dan Carpenter]

TLDR: One bug and some nits.

On Thu, Jun 16, 2022 at 12:00:06PM +0200, Jacopo Mondi wrote:
> +static irqreturn_t bno08x_dump_cargo(int irq, void *dev_id)
> +{
> +	struct iio_dev *iio_dev = dev_id;
> +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> +	struct i2c_client *client = bno08x->client;
> +	u8 *cargo = bno08x->cargo.buffer;
> +	size_t to_read;
> +	size_t len;
> +	int ret;
> +
> +	mutex_lock(&bno08x->cargo.mutex);
> +
> +	/* Read only the header first to know how many bytes we expect to receive. */
> +	ret = i2c_master_recv(client, bno08x->cargo.buffer, BNO08x_SHTP_HDR_SIZE);
> +	if (ret != BNO08x_SHTP_HDR_SIZE)
> +		goto out_unlock;
> +
> +	/*
> +	 * Clear the top bit: it means a cargo is a continuation of the last one.
> +	 * Ignore it for now.
> +	 */
> +	len = (cargo[BNO08x_SHTP_HDR_LEN_MSB] << 8 |
> +	       cargo[BNO08x_SHTP_HDR_LEN_LSB]) & ~BIT(15);
> +
> +	if (len == 0)
> +		goto out_complete;
> +
> +	if (len > BNO08x_CARGO_BUFFER_SIZE)
> +		dev_warn(&client->dev,
> +			 "Cargo size exceeds buffer: content will be unusable\n");
> +
> +	/*
> +	 * Read the full cargo now that we know its length.
> +	 *
> +	 * If the reported length exceeds the max transfer size, read the cargo
> +	 * in chunks. Its content will be unusable though.
> +	 */
> +	to_read = len;
> +	len = min(len, BNO08x_CARGO_BUFFER_SIZE);
> +	while (to_read) {

This exit condition doesn't work.  The min() needs to be done inside the
loop.  Otherise if len is set to BNO08x_CARGO_BUFFER_SIZE + 1 then the
second iteration should receive 1 byte but it instead recieves
BNO08x_CARGO_BUFFER_SIZE bytes.  The to_read will go into negative
numbers and keep looping until something breaks.

> +		memset(bno08x->cargo.buffer, 0, len);
> +
> +		ret = i2c_master_recv(client, bno08x->cargo.buffer, len);
> +		if (ret != len) {
> +			dev_err(&client->dev,
> +				"Failed to read cargo of size %lu: %d\n",
> +				len, ret);
> +			goto out_complete;
> +		}
> +
> +		to_read -= len;
> +	}
> +
> +	bno08x->cargo.len = len;

This should be been the original len instead of min() value.

> +
> +out_complete:
> +	if (atomic_read(&bno08x->cargo.waiters) == 0)
> +		goto out_unlock;
> +
> +	/*
> +	 * Wake up all waiters and let them read the cargo buffer.
> +	 *
> +	 * Unlock the cargo mutex only after all waiters are done, to avoid
> +	 * this interrupt handler re-writing the buffer content with the next
> +	 * cargo before waiters have read it.
> +	 */
> +	complete_all(&bno08x->cargo_ready);
> +
> +	wait_for_completion(&bno08x->waiters_done);
> +	reinit_completion(&bno08x->waiters_done);
> +
> +out_unlock:
> +	mutex_unlock(&bno08x->cargo.mutex);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bno08x_write_cargo(struct bno08x_dev *bno08x,
> +			      enum bno08x_shtp_channels channel,
> +			      u8 *cargo, u16 length)
> +{
> +	u16 cargo_length = length + BNO08x_SHTP_HDR_SIZE;
> +	struct i2c_client *client = bno08x->client;
> +	int ret;
> +
> +	/* Assemble header */
> +	cargo[BNO08x_SHTP_HDR_LEN_LSB] = cargo_length & 0xff;
> +	cargo[BNO08x_SHTP_HDR_LEN_MSB] = cargo_length >> 8;
> +	cargo[BNO08x_SHTP_HDR_CHAN] = channel;
> +	cargo[BNO08x_SHTP_HDR_SEQ] = bno08x->seq_num[channel]++;
> +
> +	ret = i2c_master_send(client, cargo, cargo_length);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to write cargo to channel %u: %d\n",
> +			channel, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bno08x_wait_for_cargo_timeout(struct bno08x_dev *bno08x, u8 *cargo,
> +					 size_t len)
> +{
> +	static const unsigned long complete_timeout = 500; /* msecs */
> +	struct i2c_client *client = bno08x->client;
> +	int ret = 0;
> +
> +	atomic_inc(&bno08x->cargo.waiters);
> +
> +	if (!wait_for_completion_timeout(&bno08x->cargo_ready,
> +					 msecs_to_jiffies(complete_timeout))) {
> +		dev_dbg(&client->dev, "Wait for chip interrupt timeout.\n");
> +		ret = -EIO;
> +		goto out_unlock;
> +	}
> +
> +	/* The caller is not interested in the data. */
> +	if (!len)
> +		goto out_unlock;
> +
> +	/*
> +	 * Multiple readers might want to inspect the cargo content in response
> +	 * to the complete_all(cargo_ready) signal.
> +	 *
> +	 * Copy data into the caller buffer to allow multiple threads to
> +	 * access the cargo content independently.
> +	 */
> +	ret = min(len, bno08x->cargo.len);
> +	memcpy(cargo, bno08x->cargo.buffer, ret);
> +
> +out_unlock:
> +	/* The last waiter unlocks the cargo read routine. */
> +	if (atomic_dec_return(&bno08x->cargo.waiters) == 0) {
> +		reinit_completion(&bno08x->cargo_ready);
> +		complete(&bno08x->waiters_done);
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct iio_chan_spec bno08x_iio_channels[] = {
> +	{
> +		.type = IIO_ROT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = BNO08x_ROT_SCAN_INDEX,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.shift = 0,
> +			.endianness = IIO_LE,
> +		},
> +	},
> +};
> +
> +static int bno08x_enable_feature_report(struct bno08x_dev *bno08x, u8 report_id)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	/* Reporting interval hardcoded to 5 msec. */
> +	unsigned int report_interval_usec = 5000;
> +	u8 cargo[BNO08x_SHTP_MAX_SIZE] = {};
> +	u8 *data = cargo + BNO08x_SHTP_HDR_SIZE;
> +	unsigned int max_attempts = 50;
> +	int ret;
> +
> +	if (bno08x->enabled_reports_mask & BIT(report_id))
> +		return 0;
> +
> +	/*
> +	 * Enable reporting the requested feature. The full "feature set
> +	 * request" package size is 17 bytes.
> +	 */
> +	data[0] = BNO08x_SHTP_SET_FEATURE_CMD;
> +	data[1] = report_id;
> +
> +	data[5] = (report_interval_usec >> 0);
> +	data[6] = (report_interval_usec >> 8);
> +	data[7] = (report_interval_usec >> 16);
> +	data[8] = (report_interval_usec >> 24);
> +
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 17);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Verify the requested feature is enabled inspecting the
> +	 * 'feature request status" response cargo.
> +	 */
> +	memset(cargo, 0, BNO08x_SHTP_MAX_SIZE);
> +	data[0] = BNO08x_SHTP_GET_FEATURE_REQ;
> +	data[1] = report_id;
> +
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 2);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Wait for a "get feature response". The datasheet says it will "arrive
> +	 * some time in the future". Read up to 10 packets then give up.
> +	 */
> +	while (--max_attempts) {
> +		memset(cargo, 0, 5);
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 5);
> +		if (ret < 0)
> +			continue;
> +
> +		if (ret < 5)
> +			continue;
> +
> +		if (data[0] == BNO08x_SHTP_GET_FEATURE_RESP ||
> +		    data[1] == report_id)
> +			break;
> +	}
> +	if (!max_attempts) {
> +		dev_err(&client->dev,
> +			"Failed to parse GET_FEATURE_RESPONSE: bad header\n");
> +		return -EINVAL;
> +	}
> +
> +	bno08x->enabled_reports_mask |= BIT(report_id);
> +	dev_dbg(&client->dev, "Reporting of feature %x enabled!\n", report_id);
> +
> +	return 0;
> +}
> +
> +static int bno08x_read_raw_rotation(struct bno08x_dev *bno08x, int *vals,
> +				    int *val_len, int max_len)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	unsigned int max_attempts = 50;
> +	u8 cargo[24];
> +	int ret;
> +
> +	if (max_len < 3)
> +		return -EINVAL;
> +
> +	ret = bno08x_enable_feature_report(bno08x, BNO08x_REPORTID_ROTATION_VEC);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Read the rotation vector in I, J and K quaternions.
> +	 *
> +	 * We're going to receive several other cargos before an actual rotation
> +	 * vector input report, so we parse the cargo fields and only proceed
> +	 * with a cargo that refers to the rotation vector report id.
> +	 *
> +	 * Use a sentinel to avoid looping forever, unfortunately we can't
> +	 * really know how many cargos we have to discard before receiving the
> +	 * 'right' one.
> +	 */
> +	while (--max_attempts) {
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 24);
> +		if (ret < 0)
> +			continue;
> +
> +		if (ret != 24)
> +			continue;
> +
> +		/*
> +		 * Parse the cargo content to make sure it's a well-formed input
> +		 * report containing rotation vector information.
> +		 *
> +		 * The layout of an input report cargo is reported by Figure 5.2
> +		 * of "BNO080/85/86 Datasheet 1000-3927 v.1.11".
> +		 *
> +		 * The rotation vector input report is 24 bytes long.
> +		 */
> +		if (cargo[2] == BNO08x_SHTP_REPORTS_CHAN &&
> +		    cargo[4] == BNO08x_REPORTID_TIMESTAMP_BASE &&
> +		    cargo[9] == BNO08x_REPORTID_ROTATION_VEC)
> +			break;
> +	}
> +	if (!max_attempts) {
> +		dev_err(&client->dev, "Failed to receive the correct input report\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Read the I, J, K quaternions. */
> +	vals[0] = cargo[13] | (cargo[14] << 8);
> +	vals[1] = cargo[15] | (cargo[16] << 8);
> +	vals[2] = cargo[17] | (cargo[18] << 8);
> +	*val_len = 3;
> +
> +	return IIO_VAL_INT_MULTIPLE;
> +}
> +
> +static int bno08x_read_raw_multi(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan, int max_len,
> +				 int *vals, int *val_len, long mask)
> +{
> +	struct bno08x_dev *bno08x = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return bno08x_read_raw_rotation(bno08x, vals, val_len, max_len);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		vals[0] = 0;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info bno08x_iio_info = {
> +	.read_raw_multi = bno08x_read_raw_multi,
> +};
> +
> +static int bno08x_data_rdy_trigger_set_state(struct iio_trigger *trig,
> +					     bool enable)
> +{
> +	struct iio_dev *iio_dev = iio_trigger_get_drvdata(trig);
> +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> +	int ret;
> +
> +	if (!enable)
> +		return 0;
> +
> +	ret = bno08x_enable_feature_report(bno08x, BNO08x_REPORTID_ROTATION_VEC);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops bno08x_trigger_ops = {
> +	.set_trigger_state = &bno08x_data_rdy_trigger_set_state,
> +};
> +
> +static irqreturn_t bno08x_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio_dev = pf->indio_dev;
> +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> +	u8 cargo[BNO08x_CARGO_BUFFER_SIZE];
> +	int ret;
> +
> +	ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, BNO08x_CARGO_BUFFER_SIZE);
> +	if (ret < 0)
> +		goto done;
> +
> +	if (ret < 24)
> +		goto done;
> +
> +	/* Make sure the cargo matches the active scan channel. */
> +	if (*iio_dev->active_scan_mask & BIT(BNO08x_ROT_SCAN_INDEX) &&
> +	    cargo[2] == BNO08x_SHTP_REPORTS_CHAN &&
> +	    cargo[4] == BNO08x_REPORTID_TIMESTAMP_BASE &&
> +	    cargo[9] == BNO08x_REPORTID_ROTATION_VEC) {
> +		u16 data;
> +
> +		/* Unit quaternion I component. */
> +		data = le16_to_cpu(cargo[13]);
> +		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
> +
> +		/* Unit quaternion J component. */
> +		data = le16_to_cpu(cargo[15]);
> +		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
> +
> +		/* Unit quaternion K component. */
> +		data = le16_to_cpu(cargo[17]);
> +		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
> +	}
> +
> +done:
> +	iio_trigger_notify_done(iio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bno08x_check_prod_id(struct bno08x_dev *bno08x)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	u8 cargo[BNO08x_SHTP_MAX_SIZE] = {};
> +	u8 *data = cargo + BNO08x_SHTP_HDR_SIZE;
> +	int ret;
> +
> +	data[0] = BNO08x_SHTP_PROD_ID_REQ;
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 2);
> +	if (ret)
> +		return ret;
> +
> +	while (true) {
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 20);
> +		if (ret < 0) {
> +			dev_dbg(&client->dev, "Failed to read PROD_ID: %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (ret >= 20 && data[0] == BNO08x_SHTP_PROD_ID)

Instead of >= 20 it's clearer to say == 20.


> +			break;
> +	}
> +
> +	dev_dbg(&client->dev, "FW version: 0x%x.%x\n", data[2], data[3]);
> +
> +	return 0;
> +}
> +
> +static int bno08x_soft_reset(struct bno08x_dev *bno08x)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	u8 cargo[20] = {};
> +	int ret;
> +
> +	/* Soft reset the device: write 0x01 to EXECUTABLE channel. */
> +	cargo[BNO08x_SHTP_HDR_SIZE] = BNO08x_SHTP_SOFT_RESET;
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_EXECTUABLE_CHAN, cargo, 1);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to soft-reset BNO08x\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Now read the all the channel advertisments packets.
> +	 * We don't care about the content.
> +	 */
> +	do {
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 0);
> +		if (ret < 0) {
> +			dev_dbg(&client->dev,
> +				"Failed to read channel advertisments: %d\n", ret);
> +			return ret;
> +		}
> +	} while (ret != 0);

This condition can never be true so the loop can be deleted.

> +
> +	/*
> +	 * Give the chip some time to stabilize. There's no mention of any
> +	 * delay required after startup in the manual, but this makes operation
> +	 * stable through module load/unload.
> +	 */
> +	usleep_range(15000, 20000);
> +
> +	return 0;
> +}
> +
> +static int bno08x_probe(struct i2c_client *client)
> +{
> +	struct bno08x_dev *bno08x;
> +	struct iio_dev *iio_dev;
> +	int ret;
> +
> +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bno08x));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&client->dev, iio_dev);
> +
> +	bno08x = iio_priv(iio_dev);
> +	bno08x->client = client;
> +
> +	mutex_init(&bno08x->cargo.mutex);
> +	init_completion(&bno08x->cargo_ready);
> +	init_completion(&bno08x->waiters_done);
> +
> +	iio_dev->info = &bno08x_iio_info;
> +	iio_dev->name = DRIVER_NAME;
> +	iio_dev->channels = bno08x_iio_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(bno08x_iio_channels);
> +	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, iio_dev,
> +					      iio_pollfunc_store_time,
> +					      bno08x_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	bno08x->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> +					      DRIVER_NAME,
> +					      iio_device_id(iio_dev));
> +	if (!bno08x->trig)
> +		return -ENOMEM;
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						bno08x_interrupt,
> +						bno08x_dump_cargo,
> +						IRQF_TRIGGER_FALLING,
> +						iio_dev->name, iio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	bno08x->trig->dev.parent = &client->dev;
> +	bno08x->trig->ops = &bno08x_trigger_ops;
> +	iio_trigger_set_drvdata(bno08x->trig, iio_dev);
> +
> +	ret = devm_iio_trigger_register(&client->dev, bno08x->trig);
> +	if (ret)
> +		return ret;
> +
> +	iio_dev->trig = iio_trigger_get(bno08x->trig);
> +
> +	ret = bno08x_soft_reset(bno08x);
> +	if (ret)
> +		return ret;
> +
> +	bno08x_check_prod_id(bno08x);

Check the return value from this?

> +
> +	return devm_iio_device_register(&client->dev, iio_dev);
> +}

regards,
dan carpenter

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Jonathan Cameron]

On Thu, 16 Jun 2022 12:00:06 +0200
Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:

> From: Jacopo Mondi <jacopo@jmondi.org>
> 
> Add support for CEVA BNO08x Sensor Hub.
> 
> The BNO08X family (BNO080/85/86) is a System in Package (SiP) that
> integrates a triaxial accelerometer, triaxial gyroscope, magnetometer
> and a 32-bit ARM Cortex-M0+ microcontroller running CEVA's SH-2
> firmware.
> 
> Datasheet:
> https://www.ceva-dsp.com/wp-content/uploads/2019/10/BNO080_085-Datasheet.pdf
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Hi Jacopo

Review for whoever takes this forwards in the future :)

It's proving hard enough to get drivers that have been in staging
for years out, don't want to add new ones!

Jonathan


> ---
>  MAINTAINERS                             |   7 +
>  drivers/staging/iio/Kconfig             |   1 +
>  drivers/staging/iio/Makefile            |   1 +
>  drivers/staging/iio/imu/bno08x/Kconfig  |  11 +
>  drivers/staging/iio/imu/bno08x/Makefile |   3 +
>  drivers/staging/iio/imu/bno08x/bno08x.c | 618 ++++++++++++++++++++++++
>  6 files changed, 641 insertions(+)
>  create mode 100644 drivers/staging/iio/imu/bno08x/Kconfig
>  create mode 100644 drivers/staging/iio/imu/bno08x/Makefile
>  create mode 100644 drivers/staging/iio/imu/bno08x/bno08x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 96b8c3560117..4b709599c82b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4314,6 +4314,13 @@ F:	certs/
>  F:	scripts/extract-cert.c
>  F:	scripts/sign-file.c
>  
> +CEVA BNO08x SENSOR HUB
> +ML	Jacopo Mondi <jacopo@jmondi.org>
> +L:	linux-iio@vger.kernel.org
> +S:	Odd Fixes
> +F:	Documentation/devicetree/bindings/staging/iio/imu/ceva,bno08x.yaml
> +F	drivers/staging/iio/imu/bno08x/
> +
>  CFAG12864B LCD DRIVER
>  M:	Miguel Ojeda <ojeda@kernel.org>
>  S:	Maintained
> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
> index a8e970db179d..bcecace954a5 100644
> --- a/drivers/staging/iio/Kconfig
> +++ b/drivers/staging/iio/Kconfig
> @@ -11,6 +11,7 @@ source "drivers/staging/iio/addac/Kconfig"
>  source "drivers/staging/iio/cdc/Kconfig"
>  source "drivers/staging/iio/frequency/Kconfig"
>  source "drivers/staging/iio/impedance-analyzer/Kconfig"
> +source "drivers/staging/iio/imu/Kconfig"
>  source "drivers/staging/iio/meter/Kconfig"
>  source "drivers/staging/iio/resolver/Kconfig"
>  
> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
> index b15904b99581..eafcc25b64f9 100644
> --- a/drivers/staging/iio/Makefile
> +++ b/drivers/staging/iio/Makefile
> @@ -9,5 +9,6 @@ obj-y += addac/
>  obj-y += cdc/
>  obj-y += frequency/
>  obj-y += impedance-analyzer/
> +obj-y += imu/
>  obj-y += meter/
>  obj-y += resolver/
> diff --git a/drivers/staging/iio/imu/bno08x/Kconfig b/drivers/staging/iio/imu/bno08x/Kconfig
> new file mode 100644
> index 000000000000..109ac11ed4f9
> --- /dev/null
> +++ b/drivers/staging/iio/imu/bno08x/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config CEVA_BNO08x_IMU
> +	tristate "CEVA BNO08x IMU driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for CEVA BNO08x IMU.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called bno08x.
> diff --git a/drivers/staging/iio/imu/bno08x/Makefile b/drivers/staging/iio/imu/bno08x/Makefile
> new file mode 100644
> index 000000000000..48bbbf12a366
> --- /dev/null
> +++ b/drivers/staging/iio/imu/bno08x/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_CEVA_BNO08x_IMU) += bno08x.o
> diff --git a/drivers/staging/iio/imu/bno08x/bno08x.c b/drivers/staging/iio/imu/bno08x/bno08x.c
> new file mode 100644
> index 000000000000..e07735a85b34
> --- /dev/null
> +++ b/drivers/staging/iio/imu/bno08x/bno08x.c
> @@ -0,0 +1,618 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CEVA BNO08x IMU driver.
> + *
> + * Copyright (c) 2021 Jacopo Mondi <jacopo@jmondi.org>
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +
> +#define DRIVER_NAME "bno08x"
> +
> +#define BNO08x_SHTP_HDR_SIZE		0x4
> +#define BNO08x_SHTP_HDR_LEN_LSB		0x0
> +#define BNO08x_SHTP_HDR_LEN_MSB		0x1
> +#define BNO08x_SHTP_HDR_CHAN		0x2
> +#define BNO08x_SHTP_HDR_SEQ		0x3
> +
> +#define BNO08x_SHTP_MAX_SIZE		128
> +#define BNO08x_SHTP_ADV_MAX_SIZE	512
> +
> +#define BNO08x_SHTP_SOFT_RESET		0x01
> +#define BNO08x_SHTP_PROD_ID		0xf8
> +#define BNO08x_SHTP_PROD_ID_REQ		0xf9
> +
> +#define BNO08x_SHTP_GET_FEATURE_RESP	0xfc
> +#define BNO08x_SHTP_SET_FEATURE_CMD	0xfd
> +#define BNO08x_SHTP_GET_FEATURE_REQ	0xfe
> +
> +#define BNO08x_ROT_SCAN_INDEX		0x01
> +#define BNO08x_REPORTID_ROTATION_VEC	0x05
> +#define BNO08x_REPORTID_TIMESTAMP_BASE	0xfb
> +
> +enum bno08x_shtp_channels {
> +	BNO08x_SHTP_COMMAND_CHAN,
> +	BNO08x_SHTP_EXECTUABLE_CHAN,
> +	BNO08x_SHTP_CONTROL_CHAN,
> +	BNO08x_SHTP_REPORTS_CHAN,
> +	BNO08x_SHTP_WAKE_REPORTS_CHAN,
> +	BNO08x_SHTP_GYRO_CHAN,
> +	BNO08x_NUM_CHANNELS,
> +};
> +
> +struct bno08x_dev {
> +	struct i2c_client *client;
> +
> +	struct iio_trigger *trig;
> +
> +	/* Completions for cargo transfer. */
> +	struct completion cargo_ready;
> +	struct completion waiters_done;
> +
> +	/* cargo sequence number per channel. */
> +	u32 seq_num[BNO08x_NUM_CHANNELS];
> +
> +	/* Mask of the enabled report ids. */
> +	u64 enabled_reports_mask;
> +
> +#define BNO08x_CARGO_BUFFER_SIZE	1024lu
> +	struct {
> +		u8 buffer[BNO08x_CARGO_BUFFER_SIZE];
> +		size_t len;
> +		atomic_t waiters;
> +
> +		/* Protects access to the cargo buffer content. */
> +		struct mutex mutex;
> +	} cargo;
> +};
> +
> +static irqreturn_t bno08x_interrupt(int irq, void *dev_id)
> +{
> +	struct iio_dev *iio_dev = dev_id;
> +
> +	if (iio_dev->trig)
> +		iio_trigger_poll((void *)iio_dev->trig);

Trigger isn't guaranteed to be the one you think it is :)
also with an interrupt doing other things, seems unlikely
you can always assume trigger fired...

> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t bno08x_dump_cargo(int irq, void *dev_id)
> +{
> +	struct iio_dev *iio_dev = dev_id;
> +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> +	struct i2c_client *client = bno08x->client;
> +	u8 *cargo = bno08x->cargo.buffer;
> +	size_t to_read;
> +	size_t len;
> +	int ret;
> +
> +	mutex_lock(&bno08x->cargo.mutex);
> +
> +	/* Read only the header first to know how many bytes we expect to receive. */
> +	ret = i2c_master_recv(client, bno08x->cargo.buffer, BNO08x_SHTP_HDR_SIZE);
> +	if (ret != BNO08x_SHTP_HDR_SIZE)
> +		goto out_unlock;
> +
> +	/*
> +	 * Clear the top bit: it means a cargo is a continuation of the last one.
> +	 * Ignore it for now.
> +	 */
> +	len = (cargo[BNO08x_SHTP_HDR_LEN_MSB] << 8 |
> +	       cargo[BNO08x_SHTP_HDR_LEN_LSB]) & ~BIT(15);

FIELD_GET() with appropriate mask and an unaligned_get_le16 probably (assuming
the LSB is next to the MSB)

> +
> +	if (len == 0)
> +		goto out_complete;
> +
> +	if (len > BNO08x_CARGO_BUFFER_SIZE)
> +		dev_warn(&client->dev,
> +			 "Cargo size exceeds buffer: content will be unusable\n");
> +
> +	/*
> +	 * Read the full cargo now that we know its length.
> +	 *
> +	 * If the reported length exceeds the max transfer size, read the cargo
> +	 * in chunks. Its content will be unusable though.
> +	 */
> +	to_read = len;
> +	len = min(len, BNO08x_CARGO_BUFFER_SIZE);
> +	while (to_read) {
> +		memset(bno08x->cargo.buffer, 0, len);
> +
> +		ret = i2c_master_recv(client, bno08x->cargo.buffer, len);
> +		if (ret != len) {
> +			dev_err(&client->dev,
> +				"Failed to read cargo of size %lu: %d\n",
> +				len, ret);
> +			goto out_complete;
> +		}
> +
> +		to_read -= len;
> +	}
> +
> +	bno08x->cargo.len = len;
> +
> +out_complete:
> +	if (atomic_read(&bno08x->cargo.waiters) == 0)
> +		goto out_unlock;
> +
> +	/*
> +	 * Wake up all waiters and let them read the cargo buffer.
> +	 *
> +	 * Unlock the cargo mutex only after all waiters are done, to avoid
> +	 * this interrupt handler re-writing the buffer content with the next
> +	 * cargo before waiters have read it.
> +	 */
> +	complete_all(&bno08x->cargo_ready);
> +
> +	wait_for_completion(&bno08x->waiters_done);
> +	reinit_completion(&bno08x->waiters_done);
> +
> +out_unlock:
> +	mutex_unlock(&bno08x->cargo.mutex);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bno08x_write_cargo(struct bno08x_dev *bno08x,
> +			      enum bno08x_shtp_channels channel,
> +			      u8 *cargo, u16 length)
> +{
> +	u16 cargo_length = length + BNO08x_SHTP_HDR_SIZE;
> +	struct i2c_client *client = bno08x->client;
> +	int ret;
> +
> +	/* Assemble header */
> +	cargo[BNO08x_SHTP_HDR_LEN_LSB] = cargo_length & 0xff;
> +	cargo[BNO08x_SHTP_HDR_LEN_MSB] = cargo_length >> 8;
unaligned_put_le16()

> +	cargo[BNO08x_SHTP_HDR_CHAN] = channel;
> +	cargo[BNO08x_SHTP_HDR_SEQ] = bno08x->seq_num[channel]++;
> +
> +	ret = i2c_master_send(client, cargo, cargo_length);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to write cargo to channel %u: %d\n",
> +			channel, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bno08x_wait_for_cargo_timeout(struct bno08x_dev *bno08x, u8 *cargo,
> +					 size_t len)
Wait for seems a bit confusing to me as I read it as waiting for a timeout
which is a little odd :)

bn08x_read_cargo_timeout() maybe?  That's not great either thogu.

> +{
> +	static const unsigned long complete_timeout = 500; /* msecs */
> +	struct i2c_client *client = bno08x->client;
> +	int ret = 0;
> +
> +	atomic_inc(&bno08x->cargo.waiters);
> +
> +	if (!wait_for_completion_timeout(&bno08x->cargo_ready,
> +					 msecs_to_jiffies(complete_timeout))) {
> +		dev_dbg(&client->dev, "Wait for chip interrupt timeout.\n");
> +		ret = -EIO;
> +		goto out_unlock;
> +	}
> +
> +	/* The caller is not interested in the data. */
> +	if (!len)
> +		goto out_unlock;
> +
> +	/*
> +	 * Multiple readers might want to inspect the cargo content in response
> +	 * to the complete_all(cargo_ready) signal.
> +	 *
> +	 * Copy data into the caller buffer to allow multiple threads to
> +	 * access the cargo content independently.
> +	 */
> +	ret = min(len, bno08x->cargo.len);
> +	memcpy(cargo, bno08x->cargo.buffer, ret);
> +
> +out_unlock:
> +	/* The last waiter unlocks the cargo read routine. */
> +	if (atomic_dec_return(&bno08x->cargo.waiters) == 0) {
> +		reinit_completion(&bno08x->cargo_ready);
> +		complete(&bno08x->waiters_done);
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct iio_chan_spec bno08x_iio_channels[] = {
> +	{
> +		.type = IIO_ROT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = BNO08x_ROT_SCAN_INDEX,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.shift = 0,
> +			.endianness = IIO_LE,
> +		},
> +	},
> +};
> +
> +static int bno08x_enable_feature_report(struct bno08x_dev *bno08x, u8 report_id)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	/* Reporting interval hardcoded to 5 msec. */
> +	unsigned int report_interval_usec = 5000;
> +	u8 cargo[BNO08x_SHTP_MAX_SIZE] = {};
> +	u8 *data = cargo + BNO08x_SHTP_HDR_SIZE;
> +	unsigned int max_attempts = 50;
> +	int ret;
> +
> +	if (bno08x->enabled_reports_mask & BIT(report_id))
> +		return 0;
> +
> +	/*
> +	 * Enable reporting the requested feature. The full "feature set
> +	 * request" package size is 17 bytes.
> +	 */
> +	data[0] = BNO08x_SHTP_SET_FEATURE_CMD;
> +	data[1] = report_id;
> +
> +	data[5] = (report_interval_usec >> 0);
> +	data[6] = (report_interval_usec >> 8);
> +	data[7] = (report_interval_usec >> 16);
> +	data[8] = (report_interval_usec >> 24);

put_unaligned_le32()

> +
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 17);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Verify the requested feature is enabled inspecting the
> +	 * 'feature request status" response cargo.
> +	 */
> +	memset(cargo, 0, BNO08x_SHTP_MAX_SIZE);
> +	data[0] = BNO08x_SHTP_GET_FEATURE_REQ;
> +	data[1] = report_id;
> +
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 2);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Wait for a "get feature response". The datasheet says it will "arrive
> +	 * some time in the future". Read up to 10 packets then give up.
> +	 */
> +	while (--max_attempts) {
> +		memset(cargo, 0, 5);
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 5);
> +		if (ret < 0)
> +			continue;
> +
> +		if (ret < 5)
> +			continue;
> +
> +		if (data[0] == BNO08x_SHTP_GET_FEATURE_RESP ||
> +		    data[1] == report_id)
> +			break;
> +	}
> +	if (!max_attempts) {
> +		dev_err(&client->dev,
> +			"Failed to parse GET_FEATURE_RESPONSE: bad header\n");
> +		return -EINVAL;
> +	}
> +
> +	bno08x->enabled_reports_mask |= BIT(report_id);
> +	dev_dbg(&client->dev, "Reporting of feature %x enabled!\n", report_id);
> +
> +	return 0;
> +}
> +
> +static int bno08x_read_raw_rotation(struct bno08x_dev *bno08x, int *vals,
> +				    int *val_len, int max_len)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	unsigned int max_attempts = 50;
> +	u8 cargo[24];
> +	int ret;
> +
> +	if (max_len < 3)
> +		return -EINVAL;
> +
> +	ret = bno08x_enable_feature_report(bno08x, BNO08x_REPORTID_ROTATION_VEC);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Read the rotation vector in I, J and K quaternions.
> +	 *
> +	 * We're going to receive several other cargos before an actual rotation
> +	 * vector input report, so we parse the cargo fields and only proceed
> +	 * with a cargo that refers to the rotation vector report id.
> +	 *
> +	 * Use a sentinel to avoid looping forever, unfortunately we can't
> +	 * really know how many cargos we have to discard before receiving the
> +	 * 'right' one.
> +	 */
> +	while (--max_attempts) {
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 24);
> +		if (ret < 0)
> +			continue;
> +
> +		if (ret != 24)
> +			continue;
> +
> +		/*
> +		 * Parse the cargo content to make sure it's a well-formed input
> +		 * report containing rotation vector information.
> +		 *
> +		 * The layout of an input report cargo is reported by Figure 5.2
> +		 * of "BNO080/85/86 Datasheet 1000-3927 v.1.11".
> +		 *
> +		 * The rotation vector input report is 24 bytes long.
> +		 */
> +		if (cargo[2] == BNO08x_SHTP_REPORTS_CHAN &&
> +		    cargo[4] == BNO08x_REPORTID_TIMESTAMP_BASE &&
> +		    cargo[9] == BNO08x_REPORTID_ROTATION_VEC)
> +			break;
> +	}
> +	if (!max_attempts) {
> +		dev_err(&client->dev, "Failed to receive the correct input report\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Read the I, J, K quaternions. */
> +	vals[0] = cargo[13] | (cargo[14] << 8);

Use get_unaligned_le16()

> +	vals[1] = cargo[15] | (cargo[16] << 8);
> +	vals[2] = cargo[17] | (cargo[18] << 8);
> +	*val_len = 3;
> +
> +	return IIO_VAL_INT_MULTIPLE;
> +}
> +
> +static int bno08x_read_raw_multi(struct iio_dev *indio_dev,
> +				 struct iio_chan_spec const *chan, int max_len,
> +				 int *vals, int *val_len, long mask)
> +{
> +	struct bno08x_dev *bno08x = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return bno08x_read_raw_rotation(bno08x, vals, val_len, max_len);
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		vals[0] = 0;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info bno08x_iio_info = {
> +	.read_raw_multi = bno08x_read_raw_multi,
> +};
> +
> +static int bno08x_data_rdy_trigger_set_state(struct iio_trigger *trig,
> +					     bool enable)
> +{
> +	struct iio_dev *iio_dev = iio_trigger_get_drvdata(trig);
> +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> +	int ret;
> +
> +	if (!enable)
> +		return 0;
> +
> +	ret = bno08x_enable_feature_report(bno08x, BNO08x_REPORTID_ROTATION_VEC);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return bn08x_enable...

> +}
> +
> +static const struct iio_trigger_ops bno08x_trigger_ops = {
> +	.set_trigger_state = &bno08x_data_rdy_trigger_set_state,
> +};
> +
> +static irqreturn_t bno08x_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio_dev = pf->indio_dev;
> +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> +	u8 cargo[BNO08x_CARGO_BUFFER_SIZE];
> +	int ret;
> +
> +	ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, BNO08x_CARGO_BUFFER_SIZE);
> +	if (ret < 0)
> +		goto done;
> +
I'm lazy - if it's less than 0 it's definitely less than 24 so no need for separate
paths unless you add some dev_dbg/err or similar which might make sense here.

> +	if (ret < 24)
> +		goto done;
> +
> +	/* Make sure the cargo matches the active scan channel. */
> +	if (*iio_dev->active_scan_mask & BIT(BNO08x_ROT_SCAN_INDEX) &&
> +	    cargo[2] == BNO08x_SHTP_REPORTS_CHAN &&
> +	    cargo[4] == BNO08x_REPORTID_TIMESTAMP_BASE &&
> +	    cargo[9] == BNO08x_REPORTID_ROTATION_VEC) {
> +		u16 data;
> +
> +		/* Unit quaternion I component. */
> +		data = le16_to_cpu(cargo[13]);
> +		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);

If it's a single scan with multiple components, you need to
build the entire scan up here, then push it to the buffer in one go.

> +
> +		/* Unit quaternion J component. */
> +		data = le16_to_cpu(cargo[15]);
> +		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
> +
> +		/* Unit quaternion K component. */
> +		data = le16_to_cpu(cargo[17]);
> +		iio_push_to_buffers_with_timestamp(iio_dev, &data, pf->timestamp);
> +	}
> +
> +done:
> +	iio_trigger_notify_done(iio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int bno08x_check_prod_id(struct bno08x_dev *bno08x)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	u8 cargo[BNO08x_SHTP_MAX_SIZE] = {};
> +	u8 *data = cargo + BNO08x_SHTP_HDR_SIZE;
> +	int ret;
> +
> +	data[0] = BNO08x_SHTP_PROD_ID_REQ;
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_CONTROL_CHAN, cargo, 2);
> +	if (ret)
> +		return ret;
> +
> +	while (true) {
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 20);

Why use MAX_SIZE for sizing then 20 here?

> +		if (ret < 0) {
> +			dev_dbg(&client->dev, "Failed to read PROD_ID: %d\n", ret);
> +			return ret;
> +		}
> +
> +		if (ret >= 20 && data[0] == BNO08x_SHTP_PROD_ID)
> +			break;
> +	}
> +
> +	dev_dbg(&client->dev, "FW version: 0x%x.%x\n", data[2], data[3]);

This is sometimes useful info. I'd put it out with dev_info() but up to you.

> +
> +	return 0;
> +}
> +
> +static int bno08x_soft_reset(struct bno08x_dev *bno08x)
> +{
> +	struct i2c_client *client = bno08x->client;
> +	u8 cargo[20] = {};
> +	int ret;
> +
> +	/* Soft reset the device: write 0x01 to EXECUTABLE channel. */
> +	cargo[BNO08x_SHTP_HDR_SIZE] = BNO08x_SHTP_SOFT_RESET;
> +	ret = bno08x_write_cargo(bno08x, BNO08x_SHTP_EXECTUABLE_CHAN, cargo, 1);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to soft-reset BNO08x\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Now read the all the channel advertisments packets.
> +	 * We don't care about the content.
> +	 */
> +	do {
> +		ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, 0);
> +		if (ret < 0) {
> +			dev_dbg(&client->dev,
> +				"Failed to read channel advertisments: %d\n", ret);
Error path, I would go with dev_err() as it's fatal.
> +			return ret;
> +		}
> +	} while (ret != 0);
> +
> +	/*
> +	 * Give the chip some time to stabilize. There's no mention of any
> +	 * delay required after startup in the manual, but this makes operation
> +	 * stable through module load/unload.
> +	 */
> +	usleep_range(15000, 20000);
> +
> +	return 0;
> +}
> +
> +static int bno08x_probe(struct i2c_client *client)
> +{
> +	struct bno08x_dev *bno08x;
> +	struct iio_dev *iio_dev;
> +	int ret;
> +
> +	iio_dev = devm_iio_device_alloc(&client->dev, sizeof(*bno08x));
> +	if (!iio_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&client->dev, iio_dev);

I'm not immediately seeing a user - perhaps there isn't one and this
should go away.

> +
> +	bno08x = iio_priv(iio_dev);
> +	bno08x->client = client;
> +
> +	mutex_init(&bno08x->cargo.mutex);
> +	init_completion(&bno08x->cargo_ready);
> +	init_completion(&bno08x->waiters_done);
> +
> +	iio_dev->info = &bno08x_iio_info;
> +	iio_dev->name = DRIVER_NAME;
> +	iio_dev->channels = bno08x_iio_channels;
> +	iio_dev->num_channels = ARRAY_SIZE(bno08x_iio_channels);
> +	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;

If triggered buffer is registered, this will be set by the core.
Only DIRECT_MODE should be set in drivers.

> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, iio_dev,
> +					      iio_pollfunc_store_time,
> +					      bno08x_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	bno08x->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> +					      DRIVER_NAME,
> +					      iio_device_id(iio_dev));
> +	if (!bno08x->trig)
> +		return -ENOMEM;
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						bno08x_interrupt,
> +						bno08x_dump_cargo,
> +						IRQF_TRIGGER_FALLING,

Not in the driver, that's for firmware to specify.  We have lots of old
drivers doing this which we can't cleanly fix, but don't do it in a new
one.

> +						iio_dev->name, iio_dev);

Unusual to create/register trigger if no interrupts provided...
Need some explanation.
> +		if (ret)
> +			return ret;
> +	}
> +
> +	bno08x->trig->dev.parent = &client->dev;
> +	bno08x->trig->ops = &bno08x_trigger_ops;
> +	iio_trigger_set_drvdata(bno08x->trig, iio_dev);
> +
> +	ret = devm_iio_trigger_register(&client->dev, bno08x->trig);
> +	if (ret)
> +		return ret;
> +
> +	iio_dev->trig = iio_trigger_get(bno08x->trig);
No validate callbacks, so I assume the device can use other triggers.
If that's the case then don't set a default. Which one to use is
a userspace policy decision.  If it can only use this one then enforce it.

> +
> +	ret = bno08x_soft_reset(bno08x);
> +	if (ret)
> +		return ret;
> +
> +	bno08x_check_prod_id(bno08x);

handle any errors returned.

> +
> +	return devm_iio_device_register(&client->dev, iio_dev);
> +}
> +
> +static const struct of_device_id bno08x_of_match[] = {
> +	{ .compatible = "ceva,bno08x" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, bno08x_of_match);
> +
> +static struct i2c_driver bno08x_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = of_match_ptr(bno08x_of_match),
> +	},
> +	.probe_new = bno08x_probe,
> +};
> +module_i2c_driver(bno08x_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("CEVA BNO08x IMU driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] staging: iio: imu: Add CEVA BNO08x driver

[Dan Carpenter]

On Fri, Jun 17, 2022 at 06:49:41PM +0100, Jonathan Cameron wrote:
> > +static irqreturn_t bno08x_trigger_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *iio_dev = pf->indio_dev;
> > +	struct bno08x_dev *bno08x = iio_priv(iio_dev);
> > +	u8 cargo[BNO08x_CARGO_BUFFER_SIZE];
> > +	int ret;
> > +
> > +	ret = bno08x_wait_for_cargo_timeout(bno08x, cargo, BNO08x_CARGO_BUFFER_SIZE);
> > +	if (ret < 0)
> > +		goto done;
> > +
> I'm lazy - if it's less than 0 it's definitely less than 24 so no need for separate
> paths unless you add some dev_dbg/err or similar which might make sense here.
> 

Also 24 is a magic number.  But you have to be a bit careful with
combining conditions.  The two common bugs are forgot to set the error
code and type promotion to unsigned long:

	if (ret < sizeof(buf))
		goto done;

Negative ret values are > sizeof(buf) and for 0 to sizeof - 1 then the
error code isn't set so this made up example would be totally broken.
None of that applies here, of course, but I like breaking the conditions
apart just as a habit.

> > +	if (ret < 24)
> > +		goto done;
> > +

regards,
dan carpenter