Add support for Bosch BNO055 IMU

From: Andrea Merello <[email protected]>

Signed-off-by Andrea Merello <[email protected]>

From: Andrea Merello <[email protected]>

This patch adds modifiers for reporting rotations as euler angles (i.e. yaw, pitch and roll).

From: Andrea Merello <[email protected]>

This patch adds a core driver for the BNO055 IMU from Bosch. This IMU can be connected via both serial and I2C busses; separate patches will add support for them.

The driver supports "AMG" (Accelerometer, Magnetometer, Gyroscope) mode, that provides raw data from the said internal sensors, and a couple of "fusion" modes (i.e. the IMU also do calculations in order to provide euler angles, quaternions, linear acceleration and gravity measurements).

In fusion modes the AMG data is still available (with some calibration refinements done by the IMU), but certain settings such as low pass filters cut-off frequency and sensors ranges are fixed, while in AMG mode they can be customized; this is why AMG mode can still be interesting.

Signed-off-by Andrea Merello <[email protected]>

---

drivers/iio/imu/bno055/bno055.c new file +6 -0

+
+static int bno055_read_simple_chan(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   int *val, int *val2, long mask)
+{
+	struct bno055_priv *priv = iio_priv(indio_dev);

drivers/iio/imu/bno055/bno055.c new file +18 -0

+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		if (priv->operation_mode != BNO055_OPR_MODE_AMG) {
+			*val = 0;
+		} else {
+			ret = regmap_bulk_read(priv->regmap,
+					       chan->address +
+					       BNO055_REG_OFFSET_ADDR,
+					       &raw_val, sizeof(raw_val));
+			if (ret < 0)
+				return ret;
+			/*
+			 * IMU reports sensor offests; IIO wants correction
+			 * offset, thus we need the 'minus' here.
+			 */
+			*val = -(s16)le16_to_cpu(raw_val);
+		}
+		return IIO_VAL_INT;

+}

drivers/iio/imu/bno055/bno055.c new file +19 -0

+
+static int bno055_read_quaternion(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  int size, int *vals, int *val_len,
+				  long mask)
+{
+	struct bno055_priv *priv = iio_priv(indio_dev);
+	__le16 raw_vals[4];
+	int i, ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (size < 4)
+			return -EINVAL;
+		ret = regmap_bulk_read(priv->regmap,
+				       BNO055_QUAT_DATA_W_LSB_REG,
+				       raw_vals, sizeof(raw_vals));
+		if (ret < 0)
+			return ret;

drivers/iio/imu/bno055/bno055.c new file +6 -0

+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+

drivers/iio/imu/bno055/bno055.c new file +17 -0

+		dev_err(dev, "Unrecognized chip ID 0x%x", val);
+		return -ENODEV;
+	}
+
+	/*
+	 * In case we haven't a HW reset pin, we can still reset the chip via
+	 * register write. This is probably nonsense in case we can't even
+	 * communicate with the chip or the chip isn't the one we expect (i.e.
+	 * we don't write to unknown chips), so we perform SW reset only after
+	 * chip magic ID check
+	 */
+	if (!priv->reset_gpio) {
+		ret = bno055_system_reset(priv);
+		if (ret)
+			return ret;
+	}
+

drivers/iio/imu/bno055/bno055.c new file +4 -0

+	 */
+	fw_name_buf = kasprintf(GFP_KERNEL,
+				BNO055_FW_UID_NAME,
+				BNO055_UID_LEN, priv->uid);

drivers/iio/imu/bno055/bno055.c new file +11 -0

+
+	if (caldata) {
+		caldata_data = caldata->data;
+		caldata_size = caldata->size;
+	}
+	ret = bno055_init(priv, caldata_data, caldata_size);
+	if (caldata)
+		release_firmware(caldata);
+	if (ret)
+		return ret;
+

Quoted 11 lines · On Fri, 15 Apr 2022 14:59:59 +0200 Andrea Merello <[email protected]>…

From: Andrea Merello <[email protected]>

This patch adds a core driver for the BNO055 IMU from Bosch. This IMU can be connected via both serial and I2C busses; separate patches will add support for them.

The driver supports "AMG" (Accelerometer, Magnetometer, Gyroscope) mode, that provides raw data from the said internal sensors, and a couple of "fusion" modes (i.e. the IMU also do calculations in order to provide euler angles, quaternions, linear acceleration and gravity measurements).

Quoted 6 lines

In fusion modes the AMG data is still available (with some calibration refinements done by the IMU), but certain settings such as low pass filters cut-off frequency and sensors ranges are fixed, while in AMG mode they can be customized; this is why AMG mode can still be interesting.

Signed-off-by Andrea Merello <[email protected]>

Hi Andrea,

A few trivial things from me on this read through.

I haven't commented on a lot of the patches because I was happy with them.

Other than the small changes requested from me, we mostly need to get device tree review of the binding and allow time for others to take another look.

Thanks,

Jonathan

drivers/iio/imu/bno055/bno055.c line 1563

Quoted 4 lines

drivers/iio/imu/bno055/bno055.c new file +4 -0

+int bno055_probe(struct device *dev, struct regmap *regmap,
+              int xfer_burst_break_thr, bool sw_reset)
+{
+     const struct firmware *caldata;

See comment below. I think you need to set this to NULL here

drivers/iio/imu/bno055/bno055.c line 1614

Quoted 7 lines

drivers/iio/imu/bno055/bno055.c new file +6 -0

+
+     ret = regmap_read(priv->regmap, BNO055_CHIP_ID_REG, &val);
+     if (ret)
+             return ret;
+
+     if (val != BNO055_CHIP_ID_MAGIC) {

We've run into this a few times recently. Traditionally IIO has been very restrictive on allowing drivers to probe if the Who Am I type values don't match. That causes problems for backwards compatibility in device tree - e.g. (with made up compatible part number 055b :) compatible = "bosch,bno055b", "bosch,bno055"

The viewpoint of the dt maintainers is that we should assume the dt is correct and at most warn about missmatched IDs before trying to carry on. So to avoid hitting that again please relax this to a warning and cross your fingers after this point if it doesn't match. I'm fine on the firmware question because we know we are dealing with buggy firmware. Ideally we'll get some working firmware additions at somepoint then we can just label the bad firmwares and assume one less bug in the ones that don't match :)

Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron <[email protected]> ha scritto:

Quoted 27 lines

Quoted 11 lines · On Fri, 15 Apr 2022 14:59:59 +0200 Andrea Merello <[email protected]>…

From: Andrea Merello <[email protected]>

This patch adds a core driver for the BNO055 IMU from Bosch. This IMU can be connected via both serial and I2C busses; separate patches will add support for them.

The driver supports "AMG" (Accelerometer, Magnetometer, Gyroscope) mode, that provides raw data from the said internal sensors, and a couple of "fusion" modes (i.e. the IMU also do calculations in order to provide euler angles, quaternions, linear acceleration and gravity measurements).

Quoted 6 lines

In fusion modes the AMG data is still available (with some calibration refinements done by the IMU), but certain settings such as low pass filters cut-off frequency and sensors ranges are fixed, while in AMG mode they can be customized; this is why AMG mode can still be interesting.

Signed-off-by Andrea Merello <[email protected]>

Hi Andrea,

A few trivial things from me on this read through.

I haven't commented on a lot of the patches because I was happy with them.

Quoted 7 lines

Other than the small changes requested from me, we mostly need to get device tree review of the binding and allow time for others to take another look.

Thanks,

Jonathan

Ok, good! As usual, just a few inline comments, ok for the rest.

Quoted 5 lines

drivers/iio/imu/bno055/bno055.c line 1563

Quoted 4 lines

drivers/iio/imu/bno055/bno055.c new file +4 -0

+int bno055_probe(struct device *dev, struct regmap *regmap,
+              int xfer_burst_break_thr, bool sw_reset)
+{
+     const struct firmware *caldata;  

See comment below. I think you need to set this to NULL here

Hum. I'm confused here: I think I did set it to NULL (is some later LOC) in V2, but you argued against it, because hopefully request_firmware() does it by itself.
https://www.spinics.net/lists/linux-iio/msg64896.html

What has changed or what I've missed? Was your original point just to move the NULL assignment back at declaration time?

Quoted 15 lines

drivers/iio/imu/bno055/bno055.c line 1614

Quoted 8 lines

drivers/iio/imu/bno055/bno055.c new file +6 -0

+
+     ret = regmap_read(priv->regmap, BNO055_CHIP_ID_REG, &val);
+     if (ret)
+             return ret;
+
+     if (val != BNO055_CHIP_ID_MAGIC) {  

We've run into this a few times recently. Traditionally IIO has been very restrictive on allowing drivers to probe if the Who Am I type values don't match. That causes problems for backwards compatibility in device tree - e.g. (with made up compatible part number 055b :) compatible = "bosch,bno055b", "bosch,bno055"

Quoted 8 lines

The viewpoint of the dt maintainers is that we should assume the dt is correct and at most warn about missmatched IDs before trying to carry on. So to avoid hitting that again please relax this to a warning and cross your fingers after this point if it doesn't match. I'm fine on the firmware question because we know we are dealing with buggy firmware. Ideally we'll get some working firmware additions at somepoint then we can just label the bad firmwares and assume one less bug in the ones that don't match :)

To be honest my point wasn't about the correctness of the DT at all..

I've hit this several times when I was switching my test board from serial to i2c and vice-versa, because I made wrong connections or I forgot to switch FPGA image (which contains the serial IP here). I got my test script failing because the IIO device didn't pop up at all, which is better than getting e.g. random data. In the real world people may have less chance to have to worry about this, but they may when e.g. they have an RPi and a hand-wired IMU.

.. IOW I'm seeing this as a hardware self-test rather than a SW check.. But if the DT thing makes this a no-go, then I can live with the warning, and e.g. by making my script to check the kernel log..

Quoted 23 lines · On Tue, 19 Apr 2022 09:10:54 +0200 Andrea Merello <[email protected]>…

Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron <[email protected]> ha scritto:

Quoted 21 lines

Quoted 17 lines · On Fri, 15 Apr 2022 14:59:59 +0200 Andrea Merello <[email protected]>…

From: Andrea Merello <[email protected]>

This patch adds a core driver for the BNO055 IMU from Bosch. This IMU can be connected via both serial and I2C busses; separate patches will add support for them.

The driver supports "AMG" (Accelerometer, Magnetometer, Gyroscope) mode, that provides raw data from the said internal sensors, and a couple of "fusion" modes (i.e. the IMU also do calculations in order to provide euler angles, quaternions, linear acceleration and gravity measurements).

In fusion modes the AMG data is still available (with some calibration refinements done by the IMU), but certain settings such as low pass filters cut-off frequency and sensors ranges are fixed, while in AMG mode they can be customized; this is why AMG mode can still be interesting.

Signed-off-by Andrea Merello <[email protected]>

Quoted 33 lines

Quoted 14 lines

drivers/iio/imu/bno055/bno055.c line 1614

Quoted 7 lines

drivers/iio/imu/bno055/bno055.c new file +6 -0

+
+     ret = regmap_read(priv->regmap, BNO055_CHIP_ID_REG, &val);
+     if (ret)
+             return ret;
+
+     if (val != BNO055_CHIP_ID_MAGIC) {

We've run into this a few times recently. Traditionally IIO has been very restrictive on allowing drivers to probe if the Who Am I type values don't match. That causes problems for backwards compatibility in device tree - e.g. (with made up compatible part number 055b :) compatible = "bosch,bno055b", "bosch,bno055"

Quoted 8 lines

The viewpoint of the dt maintainers is that we should assume the dt is correct and at most warn about missmatched IDs before trying to carry on. So to avoid hitting that again please relax this to a warning and cross your fingers after this point if it doesn't match. I'm fine on the firmware question because we know we are dealing with buggy firmware. Ideally we'll get some working firmware additions at somepoint then we can just label the bad firmwares and assume one less bug in the ones that don't match :)

To be honest my point wasn't about the correctness of the DT at all..

I've hit this several times when I was switching my test board from serial to i2c and vice-versa, because I made wrong connections or I forgot to switch FPGA image (which contains the serial IP here). I got my test script failing because the IIO device didn't pop up at all, which is better than getting e.g. random data. In the real world people may have less chance to have to worry about this, but they may when e.g. they have an RPi and a hand-wired IMU.

Quoted 3 lines

.. IOW I'm seeing this as a hardware self-test rather than a SW check.. But if the DT thing makes this a no-go, then I can live with the warning, and e.g. by making my script to check the kernel log..

Hmm. I wonder if we can get the best of both worlds. Given there is a WHOAMI and these very rarely / never take the value of all 0's or all 1's (what you'd see with a wiring error) maybe we can sanity check against those to provide the hardware self-test element. Then accept any 'sane' value of WHOAMI, but with a warning?

Jonathan

Il giorno dom 24 apr 2022 alle ore 19:37 Jonathan Cameron <[email protected]> ha scritto:

Quoted 27 lines

Quoted 23 lines · On Tue, 19 Apr 2022 09:10:54 +0200 Andrea Merello <[email protected]>…

Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron <[email protected]> ha scritto:

Quoted 21 lines

Quoted 17 lines · On Fri, 15 Apr 2022 14:59:59 +0200 Andrea Merello <[email protected]>…

From: Andrea Merello <[email protected]>

This patch adds a core driver for the BNO055 IMU from Bosch. This IMU can be connected via both serial and I2C busses; separate patches will add support for them.

The driver supports "AMG" (Accelerometer, Magnetometer, Gyroscope) mode, that provides raw data from the said internal sensors, and a couple of "fusion" modes (i.e. the IMU also do calculations in order to provide euler angles, quaternions, linear acceleration and gravity measurements).

In fusion modes the AMG data is still available (with some calibration refinements done by the IMU), but certain settings such as low pass filters cut-off frequency and sensors ranges are fixed, while in AMG mode they can be customized; this is why AMG mode can still be interesting.

Signed-off-by Andrea Merello <[email protected]>

[...]

Quoted 37 lines

Quoted 33 lines

Quoted 14 lines

drivers/iio/imu/bno055/bno055.c line 1614

Quoted 7 lines

drivers/iio/imu/bno055/bno055.c new file +6 -0

+
+     ret = regmap_read(priv->regmap, BNO055_CHIP_ID_REG, &val);
+     if (ret)
+             return ret;
+
+     if (val != BNO055_CHIP_ID_MAGIC) {  

We've run into this a few times recently. Traditionally IIO has been very restrictive on allowing drivers to probe if the Who Am I type values don't match. That causes problems for backwards compatibility in device tree - e.g. (with made up compatible part number 055b :) compatible = "bosch,bno055b", "bosch,bno055"

Quoted 8 lines

The viewpoint of the dt maintainers is that we should assume the dt is correct and at most warn about missmatched IDs before trying to carry on. So to avoid hitting that again please relax this to a warning and cross your fingers after this point if it doesn't match. I'm fine on the firmware question because we know we are dealing with buggy firmware. Ideally we'll get some working firmware additions at somepoint then we can just label the bad firmwares and assume one less bug in the ones that don't match :)

To be honest my point wasn't about the correctness of the DT at all..

I've hit this several times when I was switching my test board from serial to i2c and vice-versa, because I made wrong connections or I forgot to switch FPGA image (which contains the serial IP here). I got my test script failing because the IIO device didn't pop up at all, which is better than getting e.g. random data. In the real world people may have less chance to have to worry about this, but they may when e.g. they have an RPi and a hand-wired IMU.

Quoted 3 lines

.. IOW I'm seeing this as a hardware self-test rather than a SW check.. But if the DT thing makes this a no-go, then I can live with the warning, and e.g. by making my script to check the kernel log..

Quoted 5 lines

Hmm. I wonder if we can get the best of both worlds. Given there is a WHOAMI and these very rarely / never take the value of all 0's or all 1's (what you'd see with a wiring error) maybe we can sanity check against those to provide the hardware self-test element. Then accept any 'sane' value of WHOAMI, but with a warning?

While trying to do this and testing it, I've realized that indeed when the BUS is broken (e.g. incorrect wiring) the probe() fails even earlier. When we are unable to communicate with the device, this is caught by the lower layer protocols (e.g. I2C sees no ACK, I suppose), so there is no need to fail here; the IIO device doesn't eventually pop up anyway.

So, I now revert my previous request to keep a check to bail out for crazy IDs here :) ; I'd say we can just relax the check to just a warning as you said before, without the need for checking for 0x00 and 0xff..

Quoted 3 lines

Jonathan

From: Andrea Merello <[email protected]>

This patch adds ABI documentation for the new "serial_number" sysfs attribute. The first user is the bno055 IIO driver.

Signed-off-by Andrea Merello <[email protected]>

---
  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/ABI/testing/sysfs-bus-iio +0 -0

index 2a6954ea1c71..3be613f64843 100644

Documentation/ABI/testing/sysfs-bus-iio +5 -0

+KernelVersion:	5.19
+Contact:	[email protected]
+Description:
+		An example format is 16-bytes, 2-digits-per-byte, HEX-string
+		representing the sensor unique ID number.

Quoted 10 lines · Andrea Merello

From: Andrea Merello <[email protected]>

This patch adds ABI documentation for the new "serial_number" sysfs attribute. The first user is the bno055 IIO driver.

Signed-off-by Andrea Merello <[email protected]>

---
  Documentation/ABI/testing/sysfs-bus-iio | 7 +++++++
  1 file changed, 7 insertions(+)

Documentation/ABI/testing/sysfs-bus-iio Contact: [email protected] · line 2052

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio +2 -0

index 2a6954ea1c71..3be613f64843 100644
@@ -2048,3 +2048,10 @@ Contact:	[email protected]
  Description:
  		Raw (unscaled) euler angles readings. Units after
  		application of scale are deg.
+
+What:		/sys/bus/iio/devices/iio:deviceX/serial_number  

Can we make this serialnumber? IIO uses underscores to separate different parts of the name and to a machine (e.g. libiio) serial_number looks like a channel called serial with an attribute called number.

Quoted 7 lines

Documentation/ABI/testing/sysfs-bus-iio +5 -0

+KernelVersion:	5.19
+Contact:	[email protected]
+Description:
+		An example format is 16-bytes, 2-digits-per-byte, HEX-string
+		representing the sensor unique ID number.  

From: Andrea Merello <[email protected]>

Introduce new documentation file for the Bosch BNO055 IMU

Signed-off-by Andrea Merello <[email protected]>

---
 .../bindings/iio/imu/bosch,bno055.yaml        | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bno055.yaml

From: Andrea Merello <[email protected]>

This path adds a serdev driver for communicating to a BNO055 IMU via serial bus, and it enables the BNO055 core driver to work in this scenario.

Signed-off-by Andrea Merello <[email protected]>

drivers/iio/imu/bno055/bno055_ser.c new file +0 -0

new file mode 100644
index 000000000000..360dfb0ee20a
@@ -0,0 +1,556 @@

drivers/iio/imu/bno055/bno055_ser.c new file +13 -0

+
+struct bno055_ser_priv {
+	struct serdev_device *serdev;
+	struct completion cmd_complete;
+	enum {
+		CMD_NONE,
+		CMD_READ,
+		CMD_WRITE,
+	} expect_response;
+	int expected_data_len;
+	u8 *response_buf;
+
+	/**

drivers/iio/imu/bno055/bno055_ser.c new file +17 -0

+	struct mutex lock;
+
+	/* Only accessed in RX callback context. No lock needed. */
+	struct {
+		enum {
+			RX_IDLE,
+			RX_START,
+			RX_DATA,
+		} state;
+		int databuf_count;
+		int expected_len;
+		int type;
+	} rx;
+
+	/* Never accessed in behalf of RX callback context. No lock needed */
+	bool cmd_stale;
+};

drivers/iio/imu/bno055/bno055_ser.c new file +18 -0

+
+/*
+ * Handler for received data; this is called from the reicever callback whenever
+ * it got some packet from the serial bus. The status tell us whether the
+ * packet is valid (i.e. header ok && received payload len consistent wrt the
+ * header). It's now our responsability to check whether this is what we
+ * expected, of whether we got some unexpected, yet valid, packet.
+ */
+static void bno055_ser_handle_rx(struct bno055_ser_priv *priv, int status)
+{
+	mutex_lock(&priv->lock);
+	switch (priv->expect_response) {
+	case CMD_NONE:
+		dev_warn(&priv->serdev->dev, "received unexpected, yet valid, data from sensor");
+		mutex_unlock(&priv->lock);
+		return;
+
+	case CMD_READ:

drivers/iio/imu/bno055/bno055_ser.c new file +46 -0

+	mutex_unlock(&priv->lock);
+}
+
+/*
+ * Serdev receiver FSM. This tracks the serial communication and parse the
+ * header. It pushes packets to bno055_ser_handle_rx(), eventually communicating
+ * failures (i.e. malformed packets).
+ * Ideally it doesn't know anything about upper layer (i.e. if this is the
+ * packet we were really expecting), but since we copies the payload into the
+ * receiver buffer (that is not valid when i.e. we don't expect data), we
+ * snoop a bit in the upper layer..
+ * Also, we assume to RX one pkt per time (i.e. the HW doesn't send anything
+ * unless we require to AND we don't queue more than one request per time).
+ */
+static int bno055_ser_receive_buf(struct serdev_device *serdev,
+				  const unsigned char *buf, size_t size)
+{
+	int status;
+	struct bno055_ser_priv *priv = serdev_device_get_drvdata(serdev);
+	int remaining = size;
+
+	if (size == 0)
+		return 0;
+
+	trace_recv(size, buf);
+	switch (priv->rx.state) {
+	case RX_IDLE:
+		/*
+		 * New packet.
+		 * Check for its 1st byte, that identifies the pkt type.
+		 */
+		if (buf[0] != 0xEE && buf[0] != 0xBB) {
+			dev_err(&priv->serdev->dev,
+				"Invalid packet start %x", buf[0]);
+			bno055_ser_handle_rx(priv, STATUS_CRIT);
+			break;
+		}
+		priv->rx.type = buf[0];
+		priv->rx.state = RX_START;
+		remaining--;
+		buf++;
+		priv->rx.databuf_count = 0;
+		fallthrough;
+
+	case RX_START:
+		/*

drivers/iio/imu/bno055/bno055_ser.c new file +15 -0

+
+			} else {
+				status = (buf[0] == 1) ? STATUS_OK : STATUS_FAIL;
+			}
+			bno055_ser_handle_rx(priv, status);
+			priv->rx.state = RX_IDLE;
+			break;
+
+		} else {
+			/*priv->rx.type == 0xBB */
+			priv->rx.state = RX_DATA;
+			priv->rx.expected_len = buf[0];
+			remaining--;
+			buf++;
+		}

drivers/iio/imu/bno055/bno055_ser.c new file +42 -0

+			 * sync and we don't know how to handle further data
+			 */
+			dev_err(&priv->serdev->dev, "BB pkt. Extra data received");
+			bno055_ser_handle_rx(priv, STATUS_CRIT);
+			priv->rx.state = RX_IDLE;
+			break;
+		}
+
+		mutex_lock(&priv->lock);
+		/*
+		 * NULL e.g. when read cmd is stale or when no read cmd is
+		 * actually pending.
+		 */
+		if (priv->response_buf &&
+		    /*
+		     * Snoop on the upper layer protocol stuff to make sure not
+		     * to write to an invalid memory. Apart for this, let's the
+		     * upper layer manage any inconsistency wrt expected data
+		     * len (as long as the serial protocol is consistent wrt
+		     * itself (i.e. response header is consistent with received
+		     * response len.
+		     */
+		    (priv->rx.databuf_count + remaining <= priv->expected_data_len))
+			memcpy(priv->response_buf + priv->rx.databuf_count,
+			       buf, remaining);
+		mutex_unlock(&priv->lock);
+
+		priv->rx.databuf_count += remaining;
+
+		/*
+		 * Reached expected len advertised by the IMU for the current
+		 * packet. Pass it to the upper layer (for us it is just valid).
+		 */
+		if (priv->rx.databuf_count == priv->rx.expected_len) {
+			bno055_ser_handle_rx(priv, STATUS_OK);
+			priv->rx.state = RX_IDLE;
+		}
+		break;
+	}
+
+	return size;
+}

On Fri, 15 Apr 2022 15:00:03 +0200
Andrea Merello <[email protected]> wrote:

Quoted 1 line

+CFLAGS_bno055_ser.o := -I$(src)

Via a bit of grepping I can see other instances of this pattern which point out that it's to do with allowing the tracing framework to see trace.h. Perhaps a similar comment here would be good (if nothing else I doubt I'll remember why this magic is here in a few years time!)

Quoted 2 lines

On Fri, 15 Apr 2022 15:00:03 +0200
Andrea Merello <[email protected]> wrote:

...

Quoted 6 lines

Quoted 1 line

+CFLAGS_bno055_ser.o := -I$(src)

Via a bit of grepping I can see other instances of this pattern which point out that it's to do with allowing the tracing framework to see trace.h. Perhaps a similar comment here would be good (if nothing else I doubt I'll remember why this magic is here in a few years time!)

Can be done better way, see dwc3 or drivers/base/ trace point implementations.

--
With Best Regards,
Andy Shevchenko

Il giorno sab 16 apr 2022 alle ore 10:45 Andy Shevchenko <[email protected]> ha scritto:

Quoted 3 lines · Jonathan Cameron

Quoted 2 lines

On Fri, 15 Apr 2022 15:00:03 +0200
Andrea Merello <[email protected]> wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 1 line

+CFLAGS_bno055_ser.o := -I$(src)

Via a bit of grepping I can see other instances of this pattern which point out that it's to do with allowing the tracing framework to see trace.h. Perhaps a similar comment here would be good (if nothing else I doubt I'll remember why this magic is here in a few years time!)

Quoted 1 line

Can be done better way, see dwc3 or drivers/base/ trace point implementations.

May you elaborate, please? It appears that both dwc3 and driver/base use this same trick of tweaking the CFLAGS in the Makefile in order to fix the header file thing. What I see is different is that they both use an (almost empty) trace.c file. Is this what you are suggesting?

Quoted 2 lines · On Fri, 15 Apr 2022 15:00:03 +0200 Andrea Merello <[email protected]>…

From: Andrea Merello <[email protected]>

Quoted 5 lines

This path adds a serdev driver for communicating to a BNO055 IMU via serial bus, and it enables the BNO055 core driver to work in this scenario.

Signed-off-by Andrea Merello <[email protected]>

Hi Andrea

A few really trivial things in here from me.

Quoted 17 lines

drivers/iio/imu/bno055/bno055_ser.c new file +17 -0

+struct bno055_ser_priv {
+     struct serdev_device *serdev;
+     struct completion cmd_complete;
+     enum {
+             CMD_NONE,
+             CMD_READ,
+             CMD_WRITE,
+     } expect_response;
+     int expected_data_len;
+     u8 *response_buf;
+
+     /**
+      * enum cmd_status - represent the status of a command sent to the HW.
+      * @STATUS_CRIT: The command failed: the serial communication failed.
+      * @STATUS_OK:   The command executed successfully.
+      * @STATUS_FAIL: The command failed: HW responded with an error.
+      */

drivers/iio/imu/bno055/bno055_ser.c line 142

Quoted 5 lines

drivers/iio/imu/bno055/bno055_ser.c new file +5 -0

+     enum {
+             STATUS_CRIT = -1,
+             STATUS_OK = 0,
+             STATUS_FAIL = 1,
+     } cmd_status;

Locks need documentation to say what scope they cover. In this case I think it is most but not quite all of this structure.

See comment on completion below.

Quoted 17 lines

drivers/iio/imu/bno055/bno055_ser.c new file +17 -0

+     struct mutex lock;
+
+     /* Only accessed in RX callback context. No lock needed. */
+     struct {
+             enum {
+                     RX_IDLE,
+                     RX_START,
+                     RX_DATA,
+             } state;
+             int databuf_count;
+             int expected_len;
+             int type;
+     } rx;
+
+     /* Never accessed in behalf of RX callback context. No lock needed */
+     bool cmd_stale;
+};

Quoted 7 lines

drivers/iio/imu/bno055/bno055_ser.c new file +7 -0

+             }
+             break;
+
+     case CMD_WRITE:
+             priv->cmd_status = status;
+             break;
+     }

drivers/iio/imu/bno055/bno055_ser.c line 361

Quoted 3 lines

drivers/iio/imu/bno055/bno055_ser.c new file +3 -0

+
+     priv->expect_response = CMD_NONE;
+     complete(&priv->cmd_complete);

I argued with myself a bit on whether the complete() should be inside the lock or not - but then concluded it doesn't really matter and moving it out is probably premature optimisation... Maybe it's worth moving it out simply so that it's clear the lock isn't needed to protect it, or am I missing something?

Quoted 4 lines

drivers/iio/imu/bno055/bno055_ser.c new file +4 -0

+     mutex_unlock(&priv->lock);
+}
+
+/*

From: Andrea Merello <[email protected]>

This path adds an I2C driver for communicating to a BNO055 IMU via I2C bus and it enables the BNO055 core driver to work in this scenario.

Signed-off-by Andrea Merello <[email protected]>

drivers/iio/imu/bno055/Makefile +1 -0

index 20128b1a1afc..20f911aad94b 100644
@@ -2,5 +2,6 @@
 
 obj-$(CONFIG_BOSCH_BNO055_IIO) += bno055.o
 obj-$(CONFIG_BOSCH_BNO055_SERIAL) += bno055_ser.o
+obj-$(CONFIG_BOSCH_BNO055_I2C) += bno055_i2c.o
 
 CFLAGS_bno055_ser.o := -I$(src)

drivers/iio/imu/bno055/bno055_i2c.c new file +1 -0

new file mode 100644
index 000000000000..d59bb4e661be
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0

drivers/iio/imu/bno055/bno055_i2c.c new file +45 -0

+#include <linux/regmap.h>
+
+#include "bno055.h"
+
+#define BNO055_I2C_XFER_BURST_BREAK_THRESHOLD 3 /* FIXME */
+
+static int bno055_i2c_probe(struct i2c_client *client)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i2c(client, &bno055_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(regmap),
+				     "Unable to init register map");
+
+	return bno055_probe(&client->dev, regmap,
+			    BNO055_I2C_XFER_BURST_BREAK_THRESHOLD, true);
+}
+
+static const struct i2c_device_id bno055_i2c_id[] = {
+	{"bno055", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bno055_i2c_id);
+
+static const struct of_device_id __maybe_unused bno055_i2c_of_match[] = {
+	{ .compatible = "bosch,bno055" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, bno055_i2c_of_match);
+
+static struct i2c_driver bno055_driver = {
+	.driver = {
+		.name = "bno055-i2c",
+		.of_match_table = of_match_ptr(bno055_i2c_of_match),
+	},
+	.probe_new = bno055_i2c_probe,
+	.id_table = bno055_i2c_id,
+};
+module_i2c_driver(bno055_driver);
+
+MODULE_AUTHOR("Andrea Merello");
+MODULE_DESCRIPTION("Bosch BNO055 I2C interface");
+MODULE_IMPORT_NS(IIO_BNO055);
+MODULE_LICENSE("GPL");

From: Andrea Merello <[email protected]>

This series (tries to) add support for Bosch BNO055 IMU to Linux IIO subsystem. It is made up several patches:

1/13 to 7/12: add some IIO modifiers, and their documentation, to the IIO
core layer, in order to being able to expose the linear acceleration and Euler angles among standard attributes. Also update the IIO event monitor tool

7/13: fix binary attributes didn't work with IIO

8/13 to 11/13: add the core IIO BNO055 driver and documentation for sysfs
attributes and DT bindings

12/13: adds serdev BNO055 driver to actually use the IMU via serial line

13/13: adds I2C BNO055 driver to actually use the IMU via I2C wiring

14/13: add a documentation file that describe the bno055 driver and
specifically the calibration