2022-04-26 20:17:08

by Andrea Merello

[permalink] [raw]
Subject: [v5 00/14] Add support for Bosch BNO055 IMU

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/14 to 6/14: 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/14: fix binary attributes didn't work with IIO

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

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

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

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

Differences wrt v4:
- be more tolerant wrt unrecognized chip IDs
- rename 'serial_number' attribute in 'serialnumber'
- fix missing NULL variable initialization
- use sign_extend32() instead of s16 casting where appropriate
- fix quaternion unit
- minor stuff (e.g. comments..)
- reduce (slightly) locking in serdev driver
- rework tracepoint support (e.g. remove dedicated config option)

Differences wrt other BNO055 drivers:

Previously at least another driver for the very same chip has been posted
to the Linux ML [0], but it has been never merged, and it seems no one
cared of it since quite a long time.

This driver differs from the above driver on the following aspects:

- This driver supports also serial access (to be reliable, reset pin is
required to be wired)

- The above driver tried to support all IMU HW modes by allowing to
choose one in the DT, and adapting IIO attributes accordingly. This
driver does not rely on DT for this, instead settings are done via
sysfs attributes. All IIO attributes are always exposed; more on this
later on. This driver however supports only a subset of the
HW-supported modes.

- This driver has some support for managing the IMU calibration

Supported operation modes:

- AMG (accelerometer, magnetometer and gyroscope) mode, which provides
raw (uncalibrated) measurements from the said sensors, and allows for
setting some parameters about them (e.g. filter cut-off frequency, max
sensor ranges, etc).

- Fusion mode, which still provides AMG measures, while it also provides
other data calculated by the IMU (e.g. rotation angles, linear
acceleration, etc). In this mode user has no freedom to set any sensor
parameter, since the HW locks them. Autocalibration and correction is
performed by the IMU.

IIO attributes exposing sensors parameters are always present, but in
fusion modes the available values are constrained to just the one used by
the HW. This is reflected in the '*_available' IIO attributes.

Trying to set a not-supported value always falls back to the closest
supported one, which in this case is just the one in use by the HW.

IIO attributes for unavailable measurements (e.g. Euler angles in AMG
mode) can't be read (return -EBUSY, or refuse to enable buffer).

IMU calibration:

The IMU supports for two sets of calibration parameters:

- SIC matrix. user-provided; this driver doesn't currently support it

- Offset and radius parameters. The IMU automatically finds out them when
it is running in fusion mode; supported by this driver.

The driver provides access to autocalibration flags (i.e. you can known
if the IMU has successfully autocalibrated) and to calibration data blob.
The user can save this blob in a "firmware" file (i.e. in /lib/firmware)
that the driver looks for at probe time. If found, then the IMU is
initialized with this calibration data. This saves the user from
performing the calibration procedure every time (which consist of moving
the IMU in various way).

The driver looks for calibration data file using two different names:
first a file whose name is suffixed with the IMU unique ID is searched
for; this is useful when there is more than one IMU instance. If this
file is not found, then a "generic" calibration file is searched for
(which can be used when only one IMU is present, without struggling with
fancy names, that changes on each device).

In AMG mode the IIO 'offset' attributes provide access to the offsets
from calibration data (if any), so that the user can apply them to the
accel, angvel and magn IIO attributes. In fusion mode they are not needed
and read as zero.


Access protocols and serdev module:

The serial protocol is quite simple, but there are tricks to make it
really works. Those tricks and workarounds are documented in the driver
source file.

The core BNO055 driver tries to group readings in burst when appropriate,
in order to optimize triggered buffer operation. The threshold for
splitting a burst (i.e. max number of unused bytes in the middle of a
burst that will be throw away) is provided to the core driver by the
lowlevel access driver (either serdev or I2C) at probe time.

[0] https://www.spinics.net/lists/linux-iio/msg25508.html

Andrea Merello (14):
iio: add modifiers for linear acceleration
iio: document linear acceleration modifiers
iio: event_monitor: add linear acceleration modifiers
iio: add modifers for pitch, yaw, roll
iio: document pitch, yaw, roll modifiers
iio: event_monitor: add pitch, yaw and roll modifiers
iio: add support for binary attributes
iio: imu: add Bosch Sensortec BNO055 core driver
iio: document bno055 private sysfs attributes
iio: document "serialnumber" sysfs attribute
dt-bindings: iio/imu: Add Bosch BNO055
iio: imu: add BNO055 serdev driver
iio: imu: add BNO055 I2C driver
docs: iio: add documentation for BNO055 driver

Documentation/ABI/testing/sysfs-bus-iio | 25 +
.../ABI/testing/sysfs-bus-iio-bno055 | 81 +
.../bindings/iio/imu/bosch,bno055.yaml | 59 +
Documentation/iio/bno055.rst | 50 +
Documentation/iio/index.rst | 2 +
drivers/iio/imu/Kconfig | 1 +
drivers/iio/imu/Makefile | 1 +
drivers/iio/imu/bno055/Kconfig | 25 +
drivers/iio/imu/bno055/Makefile | 10 +
drivers/iio/imu/bno055/bno055.c | 1710 +++++++++++++++++
drivers/iio/imu/bno055/bno055.h | 12 +
drivers/iio/imu/bno055/bno055_i2c.c | 57 +
drivers/iio/imu/bno055/bno055_ser_core.c | 560 ++++++
drivers/iio/imu/bno055/bno055_ser_trace.h | 104 +
drivers/iio/industrialio-core.c | 10 +-
include/uapi/linux/iio/types.h | 7 +-
tools/iio/iio_event_monitor.c | 6 +
17 files changed, 2718 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-bno055
create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bno055.yaml
create mode 100644 Documentation/iio/bno055.rst
create mode 100644 drivers/iio/imu/bno055/Kconfig
create mode 100644 drivers/iio/imu/bno055/Makefile
create mode 100644 drivers/iio/imu/bno055/bno055.c
create mode 100644 drivers/iio/imu/bno055/bno055.h
create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c
create mode 100644 drivers/iio/imu/bno055/bno055_ser_core.c
create mode 100644 drivers/iio/imu/bno055/bno055_ser_trace.h

--
2.17.1


2022-04-27 00:01:18

by Andrea Merello

[permalink] [raw]
Subject: [v5 07/14] iio: add support for binary attributes

From: Andrea Merello <[email protected]>

When a IIO device is registered, the IIO core creates an attribute group on
its own, where it puts the channel attributes, and where it copies the
attributes in indio_dev->info->attrs.

Unfortunately it doesn't take care of binary attributes (i.e. it only
consider indio_dev->info->attrs->attrs, and it ignores
indio_dev->info->attrs->bin_attrs).

Fix this by making the IIO layer take care also of the binary attributes.

Note that while it is necessary to copy the non-binary attributes because
the IIO layer needs more room to add the channels attribute, it should be
enough to assign the bin_attrs pointer to the binary attributes pointed by
indio_dev->info->attrs->bin_attrs.

Signed-off-by: Andrea Merello <[email protected]>
---
drivers/iio/industrialio-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index aa5f98d3b334..aef30f0b9465 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1567,7 +1567,7 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
ret = -ENOMEM;
goto error_clear_attrs;
}
- /* Copy across original attributes */
+ /* Copy across original attributes, and point to original binary attributes */
if (indio_dev->info->attrs) {
memcpy(iio_dev_opaque->chan_attr_group.attrs,
indio_dev->info->attrs->attrs,
@@ -1575,6 +1575,8 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
*attrcount_orig);
iio_dev_opaque->chan_attr_group.is_visible =
indio_dev->info->attrs->is_visible;
+ iio_dev_opaque->chan_attr_group.bin_attrs =
+ indio_dev->info->attrs->bin_attrs;
}
attrn = attrcount_orig;
/* Add all elements from the list. */
--
2.17.1

2022-04-27 03:59:25

by Andrea Merello

[permalink] [raw]
Subject: [v5 12/14] iio: imu: add BNO055 serdev driver

From: Andrea Merello <[email protected]>

Add a serdev driver for communicating to a BNO055 IMU via serial bus, and
enable the BNO055 core driver to work in this scenario.

Signed-off-by: Andrea Merello <[email protected]>
---
drivers/iio/imu/bno055/Kconfig | 10 +
drivers/iio/imu/bno055/Makefile | 5 +
drivers/iio/imu/bno055/bno055_ser_core.c | 560 ++++++++++++++++++++++
drivers/iio/imu/bno055/bno055_ser_trace.h | 104 ++++
4 files changed, 679 insertions(+)
create mode 100644 drivers/iio/imu/bno055/bno055_ser_core.c
create mode 100644 drivers/iio/imu/bno055/bno055_ser_trace.h

diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig
index d0ab3221fba5..d014b68cd43d 100644
--- a/drivers/iio/imu/bno055/Kconfig
+++ b/drivers/iio/imu/bno055/Kconfig
@@ -2,3 +2,13 @@

config BOSCH_BNO055_IIO
tristate
+
+config BOSCH_BNO055_SERIAL
+ tristate "Bosch BNO055 attached via UART"
+ depends on SERIAL_DEV_BUS
+ select BOSCH_BNO055_IIO
+ help
+ Enable this to support Bosch BNO055 IMUs attached via UART.
+
+ This driver can also be built as a module. If so, the module will be
+ called bno055_sl.
diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile
index 56cc4de60a7e..212307ce9c08 100644
--- a/drivers/iio/imu/bno055/Makefile
+++ b/drivers/iio/imu/bno055/Makefile
@@ -1,3 +1,8 @@
# SPDX-License-Identifier: GPL-2.0

obj-$(CONFIG_BOSCH_BNO055_IIO) += bno055.o
+obj-$(CONFIG_BOSCH_BNO055_SERIAL) += bno055_ser.o
+bno055_ser-y := bno055_ser_core.o
+# define_trace.h needs to know how to find our header
+CFLAGS_bno055_ser_trace.o := -I$(src)
+bno055_ser-$(CONFIG_TRACING) += bno055_ser_trace.o
diff --git a/drivers/iio/imu/bno055/bno055_ser_core.c b/drivers/iio/imu/bno055/bno055_ser_core.c
new file mode 100644
index 000000000000..235131900260
--- /dev/null
+++ b/drivers/iio/imu/bno055/bno055_ser_core.c
@@ -0,0 +1,560 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Serial line interface for Bosh BNO055 IMU (via serdev).
+ * This file implements serial communication up to the register read/write
+ * level.
+ *
+ * Copyright (C) 2021-2022 Istituto Italiano di Tecnologia
+ * Electronic Design Laboratory
+ * Written by Andrea Merello <[email protected]>
+ *
+ * This driver is besed on
+ * Plantower PMS7003 particulate matter sensor driver
+ * Which is
+ * Copyright (c) Tomasz Duszynski <[email protected]>
+ */
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/serdev.h>
+
+#include "bno055_ser_trace.h"
+#include "bno055.h"
+
+/*
+ * Register writes cmd have the following format
+ * +------+------+-----+-----+----- ... ----+
+ * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
+ * +------+------+-----+-----+----- ... ----+
+ *
+ * Register write responses have the following format
+ * +------+----------+
+ * | 0xEE | ERROCODE |
+ * +------+----------+
+ *
+ * .. except when writing the SYS_RST bit (i.e. triggering a system reset); in
+ * case the IMU accepts the command, then it resets without responding. We don't
+ * handle this (yet) here (so we inform the common bno055 code not to perform
+ * sw resets - bno055 on serial bus basically requires the hw reset pin).
+ *
+ * Register read have the following format
+ * +------+------+-----+-----+
+ * | 0xAA | 0xO1 | REG | LEN |
+ * +------+------+-----+-----+
+ *
+ * Successful register read response have the following format
+ * +------+-----+----- ... ----+
+ * | 0xBB | LEN | payload[LEN] |
+ * +------+-----+----- ... ----+
+ *
+ * Failed register read response have the following format
+ * +------+--------+
+ * | 0xEE | ERRCODE| (ERRCODE always > 1)
+ * +------+--------+
+ *
+ * Error codes are
+ * 01: OK
+ * 02: read/write FAIL
+ * 04: invalid address
+ * 05: write on RO
+ * 06: wrong start byte
+ * 07: bus overrun
+ * 08: len too high
+ * 09: len too low
+ * 10: bus RX byte timeout (timeout is 30mS)
+ *
+ *
+ * **WORKAROUND ALERT**
+ *
+ * Serial communication seems very fragile: the BNO055 buffer seems to overflow
+ * very easy; BNO055 seems able to sink few bytes, then it needs a brief pause.
+ * On the other hand, it is also picky on timeout: if there is a pause > 30mS in
+ * between two bytes then the transaction fails (IMU internal RX FSM resets).
+ *
+ * BNO055 has been seen also failing to process commands in case we send them
+ * too close each other (or if it is somehow busy?)
+ *
+ * In particular I saw these scenarios:
+ * 1) If we send 2 bytes per time, then the IMU never(?) overflows.
+ * 2) If we send 4 bytes per time (i.e. the full header), then the IMU could
+ * overflow, but it seem to sink all 4 bytes, then it returns error.
+ * 3) If we send more than 4 bytes, the IMU could overflow, and I saw it sending
+ * error after 4 bytes are sent; we have troubles in synchronizing again,
+ * because we are still sending data, and the IMU interprets it as the 1st
+ * byte of a new command.
+ *
+ * While we must avoid case 3, we could send 4 bytes per time and eventually
+ * retry in case of failure; this seemed convenient for reads (which requires
+ * TXing exactly 4 bytes), however it has been seen that, depending by the IMU
+ * settings (e.g. LPF), failures became less or more frequent; in certain IMU
+ * configurations they are very rare, but in certain others we keeps failing
+ * even after like 30 retries.
+ *
+ * So, we just split TXes in [2-bytes + delay] steps, and still keep an eye on
+ * the IMU response; in case it overflows (which is now unlikely), we retry.
+ */
+
+/*
+ * Read operation overhead:
+ * 4 bytes req + 2byte resp hdr.
+ * 6 bytes = 60 bit (considering 1start + 1stop bits).
+ * 60/115200 = ~520uS + about 2500mS dealay -> ~3mS
+ * In 3mS we could read back about 34 bytes that means 17 samples, this means
+ * that in case of scattered read in which the gap is 17 samples or less it is
+ * still convenient to go for a burst.
+ * We have to take into account also IMU response time - IMU seems to be often
+ * reasonably quick to respond, but sometimes it seems to be in some "critical
+ * section" in which it delays handling of serial protocol. Because of this we
+ * round-up to 22, which is the max number of samples, always bursting indeed.
+ */
+#define BNO055_SER_XFER_BURST_BREAK_THRESHOLD 22
+
+struct bno055_ser_priv {
+ 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.
+ */
+ enum {
+ STATUS_CRIT = -1,
+ STATUS_OK = 0,
+ STATUS_FAIL = 1,
+ } cmd_status;
+
+ /*
+ * Protects all the above fields, which are accessed in behalf of both
+ * the serdev RX callback and the regmap side
+ */
+ struct mutex lock;
+
+ /* Only accessed in serdev RX callback context*/
+ struct {
+ enum {
+ RX_IDLE,
+ RX_START,
+ RX_DATA,
+ } state;
+ int databuf_count;
+ int expected_len;
+ int type;
+ } rx;
+
+ /* Never accessed in behalf of serdev RX callback context */
+ bool cmd_stale;
+
+ struct completion cmd_complete;
+ struct serdev_device *serdev;
+};
+
+static int bno055_ser_send_chunk(struct bno055_ser_priv *priv, const u8 *data, int len)
+{
+ int ret;
+
+ trace_send_chunk(len, data);
+ ret = serdev_device_write(priv->serdev, data, len, msecs_to_jiffies(25));
+ if (ret < 0)
+ return ret;
+
+ if (ret < len)
+ return -EIO;
+
+ return 0;
+}
+
+/*
+ * Sends a read or write command.
+ * 'data' can be NULL (used in read case). 'len' parameter is always valid; in
+ * case 'data' is non-NULL then it must match 'data' size.
+ */
+static int bno055_ser_do_send_cmd(struct bno055_ser_priv *priv,
+ bool read, int addr, int len, const u8 *data)
+{
+ u8 hdr[] = {0xAA, read, addr, len};
+ int chunk_len;
+ int ret;
+
+ ret = bno055_ser_send_chunk(priv, hdr, 2);
+ if (ret)
+ goto fail;
+ usleep_range(2000, 3000);
+ ret = bno055_ser_send_chunk(priv, hdr + 2, 2);
+ if (ret)
+ goto fail;
+
+ if (read)
+ return 0;
+
+ while (len) {
+ chunk_len = min(len, 2);
+ usleep_range(2000, 3000);
+ ret = bno055_ser_send_chunk(priv, data, chunk_len);
+ if (ret)
+ goto fail;
+ data += chunk_len;
+ len -= chunk_len;
+ }
+
+ return 0;
+fail:
+ /* waiting more than 30mS should clear the BNO055 internal state */
+ usleep_range(40000, 50000);
+ return ret;
+}
+
+static int bno055_ser_send_cmd(struct bno055_ser_priv *priv,
+ bool read, int addr, int len, const u8 *data)
+{
+ const int retry_max = 5;
+ int retry = retry_max;
+ int ret = 0;
+
+ /*
+ * In case previous command was interrupted we still need to wait it to
+ * complete before we can issue new commands
+ */
+ if (priv->cmd_stale) {
+ ret = wait_for_completion_interruptible_timeout(&priv->cmd_complete,
+ msecs_to_jiffies(100));
+ if (ret == -ERESTARTSYS)
+ return -ERESTARTSYS;
+
+ priv->cmd_stale = false;
+ /* if serial protocol broke, bail out */
+ if (priv->cmd_status == STATUS_CRIT)
+ return -EIO;
+ }
+
+ /*
+ * Try to convince the IMU to cooperate.. as explained in the comments
+ * at the top of this file, the IMU could also refuse the command (i.e.
+ * it is not ready yet); retry in this case.
+ */
+ do {
+ mutex_lock(&priv->lock);
+ priv->expect_response = read ? CMD_READ : CMD_WRITE;
+ reinit_completion(&priv->cmd_complete);
+ mutex_unlock(&priv->lock);
+
+ if (retry != retry_max)
+ trace_cmd_retry(read, addr, retry_max - retry);
+ ret = bno055_ser_do_send_cmd(priv, read, addr, len, data);
+ if (ret)
+ continue;
+
+ ret = wait_for_completion_interruptible_timeout(&priv->cmd_complete,
+ msecs_to_jiffies(100));
+ if (ret == -ERESTARTSYS) {
+ priv->cmd_stale = true;
+ return -ERESTARTSYS;
+ }
+
+ if (!ret)
+ return -ETIMEDOUT;
+
+ if (priv->cmd_status == STATUS_OK)
+ return 0;
+ if (priv->cmd_status == STATUS_CRIT)
+ return -EIO;
+
+ /* loop in case priv->cmd_status == STATUS_FAIL */
+ } while (--retry);
+
+ if (ret < 0)
+ return ret;
+ if (priv->cmd_status == STATUS_FAIL)
+ return -EINVAL;
+ return 0;
+}
+
+static int bno055_ser_write_reg(void *context, const void *_data, size_t count)
+{
+ const u8 *data = _data;
+ struct bno055_ser_priv *priv = context;
+
+ if (count < 2) {
+ dev_err(&priv->serdev->dev, "Invalid write count %zu", count);
+ return -EINVAL;
+ }
+
+ trace_write_reg(data[0], data[1]);
+ return bno055_ser_send_cmd(priv, 0, data[0], count - 1, data + 1);
+}
+
+static int bno055_ser_read_reg(void *context,
+ const void *_reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ int ret;
+ int reg_addr;
+ const u8 *reg = _reg;
+ struct bno055_ser_priv *priv = context;
+
+ if (val_size > 128) {
+ dev_err(&priv->serdev->dev, "Invalid read valsize %zu", val_size);
+ return -EINVAL;
+ }
+
+ reg_addr = *reg;
+ trace_read_reg(reg_addr, val_size);
+ mutex_lock(&priv->lock);
+ priv->expected_data_len = val_size;
+ priv->response_buf = val;
+ mutex_unlock(&priv->lock);
+
+ ret = bno055_ser_send_cmd(priv, 1, reg_addr, val_size, NULL);
+
+ mutex_lock(&priv->lock);
+ priv->response_buf = NULL;
+ mutex_unlock(&priv->lock);
+
+ return ret;
+}
+
+/*
+ * 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:
+ priv->cmd_status = status;
+ if (status == STATUS_OK &&
+ priv->rx.databuf_count != priv->expected_data_len) {
+ /*
+ * If we got here, then the lower layer serial protocol
+ * seems consistent with itself; if we got an unexpected
+ * amount of data then signal it as a non critical error
+ */
+ priv->cmd_status = STATUS_FAIL;
+ dev_warn(&priv->serdev->dev,
+ "received an unexpected amount of, yet valid, data from sensor");
+ }
+ break;
+
+ case CMD_WRITE:
+ priv->cmd_status = status;
+ break;
+ }
+
+ priv->expect_response = CMD_NONE;
+ mutex_unlock(&priv->lock);
+ complete(&priv->cmd_complete);
+}
+
+/*
+ * 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:
+ /*
+ * Packet RX in progress, we expect either 1-byte len or 1-byte
+ * status depending by the packet type.
+ */
+ if (remaining == 0)
+ break;
+
+ if (priv->rx.type == 0xEE) {
+ if (remaining > 1) {
+ dev_err(&priv->serdev->dev, "EE pkt. Extra data received");
+ status = STATUS_CRIT;
+ } 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++;
+ }
+ fallthrough;
+
+ case RX_DATA:
+ /* Header parsed; now receiving packet data payload */
+ if (remaining == 0)
+ break;
+
+ if (priv->rx.databuf_count + remaining > priv->rx.expected_len) {
+ /*
+ * This is an inconsistency in serial protocol, we lost
+ * 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;
+}
+
+static const struct serdev_device_ops bno055_ser_serdev_ops = {
+ .receive_buf = bno055_ser_receive_buf,
+ .write_wakeup = serdev_device_write_wakeup,
+};
+
+static struct regmap_bus bno055_ser_regmap_bus = {
+ .write = bno055_ser_write_reg,
+ .read = bno055_ser_read_reg,
+};
+
+static int bno055_ser_probe(struct serdev_device *serdev)
+{
+ struct bno055_ser_priv *priv;
+ struct regmap *regmap;
+ int ret;
+
+ priv = devm_kzalloc(&serdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ serdev_device_set_drvdata(serdev, priv);
+ priv->serdev = serdev;
+ mutex_init(&priv->lock);
+ init_completion(&priv->cmd_complete);
+
+ serdev_device_set_client_ops(serdev, &bno055_ser_serdev_ops);
+ ret = devm_serdev_device_open(&serdev->dev, serdev);
+ if (ret)
+ return ret;
+
+ if (serdev_device_set_baudrate(serdev, 115200) != 115200) {
+ dev_err(&serdev->dev, "Cannot set required baud rate");
+ return -EIO;
+ }
+
+ ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+ if (ret) {
+ dev_err(&serdev->dev, "Cannot set required parity setting");
+ return ret;
+ }
+ serdev_device_set_flow_control(serdev, false);
+
+ regmap = devm_regmap_init(&serdev->dev, &bno055_ser_regmap_bus,
+ priv, &bno055_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(&serdev->dev, PTR_ERR(regmap),
+ "Unable to init register map");
+
+ return bno055_probe(&serdev->dev, regmap,
+ BNO055_SER_XFER_BURST_BREAK_THRESHOLD, false);
+}
+
+static const struct of_device_id bno055_ser_of_match[] = {
+ { .compatible = "bosch,bno055" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bno055_ser_of_match);
+
+static struct serdev_device_driver bno055_ser_driver = {
+ .driver = {
+ .name = "bno055-ser",
+ .of_match_table = bno055_ser_of_match,
+ },
+ .probe = bno055_ser_probe,
+};
+module_serdev_device_driver(bno055_ser_driver);
+
+MODULE_AUTHOR("Andrea Merello <[email protected]>");
+MODULE_DESCRIPTION("Bosch BNO055 serdev interface");
+MODULE_IMPORT_NS(IIO_BNO055);
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/imu/bno055/bno055_ser_trace.h b/drivers/iio/imu/bno055/bno055_ser_trace.h
new file mode 100644
index 000000000000..51d079b91f63
--- /dev/null
+++ b/drivers/iio/imu/bno055/bno055_ser_trace.h
@@ -0,0 +1,104 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#if !defined(__BNO055_SERDEV_TRACE_H__) || defined(TRACE_HEADER_MULTI_READ)
+#define __BNO055_SERDEV_TRACE_H__
+
+#include <linux/tracepoint.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM bno055_ser
+
+TRACE_EVENT(send_chunk,
+ TP_PROTO(int len, const u8 *data),
+ TP_ARGS(len, data),
+ TP_STRUCT__entry(
+ __field(int, len)
+ __dynamic_array(u8, chunk, len)
+ ),
+ TP_fast_assign(
+ __entry->len = len;
+ memcpy(__get_dynamic_array(chunk),
+ data, __entry->len);
+ ),
+ TP_printk("len: %d, data: = %*ph",
+ __entry->len, __entry->len, __get_dynamic_array(chunk)
+ )
+);
+
+TRACE_EVENT(cmd_retry,
+ TP_PROTO(bool read, int addr, int retry),
+ TP_ARGS(read, addr, retry),
+ TP_STRUCT__entry(
+ __field(bool, read)
+ __field(int, addr)
+ __field(int, retry)
+ ),
+ TP_fast_assign(
+ __entry->read = read;
+ __entry->addr = addr;
+ __entry->retry = retry;
+ ),
+ TP_printk("%s addr 0x%x retry #%d",
+ __entry->read ? "read" : "write",
+ __entry->addr, __entry->retry
+ )
+);
+
+TRACE_EVENT(write_reg,
+ TP_PROTO(u8 addr, u8 value),
+ TP_ARGS(addr, value),
+ TP_STRUCT__entry(
+ __field(u8, addr)
+ __field(u8, value)
+ ),
+ TP_fast_assign(
+ __entry->addr = addr;
+ __entry->value = value;
+ ),
+ TP_printk("reg 0x%x = 0x%x",
+ __entry->addr, __entry->value
+ )
+);
+
+TRACE_EVENT(read_reg,
+ TP_PROTO(int addr, size_t len),
+ TP_ARGS(addr, len),
+ TP_STRUCT__entry(
+ __field(int, addr)
+ __field(size_t, len)
+ ),
+ TP_fast_assign(
+ __entry->addr = addr;
+ __entry->len = len;
+ ),
+ TP_printk("reg 0x%x (len %zu)",
+ __entry->addr, __entry->len
+ )
+);
+
+TRACE_EVENT(recv,
+ TP_PROTO(size_t len, const unsigned char *buf),
+ TP_ARGS(len, buf),
+ TP_STRUCT__entry(
+ __field(size_t, len)
+ __dynamic_array(unsigned char, buf, len)
+ ),
+ TP_fast_assign(
+ __entry->len = len;
+ memcpy(__get_dynamic_array(buf),
+ buf, __entry->len);
+ ),
+ TP_printk("len: %d, data: = %*ph",
+ __entry->len, __entry->len, __get_dynamic_array(buf)
+ )
+);
+
+#endif /* __BNO055_SERDEV_TRACE_H__ || TRACE_HEADER_MULTI_READ */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE bno055_ser_trace
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
2.17.1

2022-04-27 09:02:40

by kernel test robot

[permalink] [raw]
Subject: Re: [v5 12/14] iio: imu: add BNO055 serdev driver

Hi Andrea,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master linux/master v5.18-rc4 next-20220426]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/675ca9cd13af45cc5943dd15caad5e866fd7c971
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
git checkout 675ca9cd13af45cc5943dd15caad5e866fd7c971
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

>> make[5]: *** No rule to make target 'drivers/iio/imu/bno055/bno055_ser_trace.o', needed by 'drivers/iio/imu/bno055/built-in.a'.
make[5]: Target '__build' not remade because of errors.

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-04-27 09:04:07

by Andrea Merello

[permalink] [raw]
Subject: [v5 02/14] iio: document linear acceleration modifiers

From: Andrea Merello <[email protected]>

Introduce ABI documentation for new IIO modifiers used for reporting
"linear acceleration" measures.

Signed-off-by: Andrea Merello <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index d4ccc68fdcf0..aadddd43bf22 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -233,6 +233,15 @@ Description:
Has all of the equivalent parameters as per voltageY. Units
after application of scale and offset are m/s^2.

+What: /sys/bus/iio/devices/iio:deviceX/in_accel_linear_x_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_accel_linear_y_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_accel_linear_z_raw
+KernelVersion: 5.19
+Contact: [email protected]
+Description:
+ As per in_accel_X_raw attributes, but minus the
+ acceleration due to gravity.
+
What: /sys/bus/iio/devices/iio:deviceX/in_gravity_x_raw
What: /sys/bus/iio/devices/iio:deviceX/in_gravity_y_raw
What: /sys/bus/iio/devices/iio:deviceX/in_gravity_z_raw
--
2.17.1

2022-04-27 09:34:14

by Andrea Merello

[permalink] [raw]
Subject: [v5 05/14] iio: document pitch, yaw, roll modifiers

From: Andrea Merello <[email protected]>

Introduce ABI documentation for new modifiers used for reporting rotations
expressed as euler angles (i.e. yaw, pitch, roll).

It looks like we have some unit inconsistency along various IIO modifiers:
it seems that incli is in deg, angl is in radians and rot isn't documented,
but at least the adis16209 driver has rot in deg.

Here we use deg (so angl is the only one using radians).

Signed-off-by: Andrea Merello <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-iio | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index aadddd43bf22..2a6954ea1c71 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2039,3 +2039,12 @@ Description:
Available range for the forced calibration value, expressed as:

- a range specified as "[min step max]"
+
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_yaw_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_pitch_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_roll_raw
+KernelVersion: 5.19
+Contact: [email protected]
+Description:
+ Raw (unscaled) euler angles readings. Units after
+ application of scale are deg.
--
2.17.1

2022-04-27 09:42:27

by Andrea Merello

[permalink] [raw]
Subject: [v5 04/14] iio: add modifers for pitch, yaw, roll

From: Andrea Merello <[email protected]>

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

Signed-off-by: Andrea Merello <[email protected]>
---
drivers/iio/industrialio-core.c | 3 +++
include/uapi/linux/iio/types.h | 3 +++
2 files changed, 6 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index d087b2607cc9..aa5f98d3b334 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -137,6 +137,9 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_LINEAR_X] = "linear_x",
[IIO_MOD_LINEAR_Y] = "linear_y",
[IIO_MOD_LINEAR_Z] = "linear_z",
+ [IIO_MOD_PITCH] = "pitch",
+ [IIO_MOD_YAW] = "yaw",
+ [IIO_MOD_ROLL] = "roll",
};

/* relies on pairs of these shared then separate */
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 0993f6b697fc..4853701ba70d 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -98,6 +98,9 @@ enum iio_modifier {
IIO_MOD_LINEAR_X,
IIO_MOD_LINEAR_Y,
IIO_MOD_LINEAR_Z,
+ IIO_MOD_PITCH,
+ IIO_MOD_YAW,
+ IIO_MOD_ROLL,
};

enum iio_event_type {
--
2.17.1

2022-04-27 09:46:03

by Andrea Merello

[permalink] [raw]
Subject: [v5 10/14] iio: document "serialnumber" sysfs attribute

From: Andrea Merello <[email protected]>

Add ABI documentation for the new "serialnumber" 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(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 2a6954ea1c71..eadf0326a56e 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -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/serialnumber
+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.
--
2.17.1

2022-04-27 09:55:54

by Andrea Merello

[permalink] [raw]
Subject: [v5 14/14] docs: iio: add documentation for BNO055 driver

From: Andrea Merello <[email protected]>

The bno055 driver is rather complex and have some oddities and not-obvious
things that worth to document (e.g. calibration files).

Signed-off-by: Andrea Merello <[email protected]>
---
Documentation/iio/bno055.rst | 50 ++++++++++++++++++++++++++++++++++++
Documentation/iio/index.rst | 2 ++
2 files changed, 52 insertions(+)
create mode 100644 Documentation/iio/bno055.rst

diff --git a/Documentation/iio/bno055.rst b/Documentation/iio/bno055.rst
new file mode 100644
index 000000000000..af21376d7a25
--- /dev/null
+++ b/Documentation/iio/bno055.rst
@@ -0,0 +1,50 @@
+.. SPDX-License-Identifier: GPL-2.0
+==============================
+BNO055 driver
+==============================
+
+1. Overview
+===========
+
+This driver supports Bosch BNO055 IMUs (on both serial and I2C busses).
+
+Accelerometer, magnetometer and gyroscope measures are always provided.
+When "fusion_enable" sysfs attribute is set to 1, orientation (both Euler
+angles and quaternion), linear velocity and gravity vector are also
+provided, but some sensor settings (e.g. low pass filtering and range)
+became locked (the IMU firmware controls them).
+
+This driver supports also IIO buffers.
+
+2. Calibration
+==============
+
+The IMU continuously performs an autocalibration procedure if (and only if)
+operating in fusion mode. The magnetometer autocalibration can however be
+disabled writing 0 in the sysfs in_magn_calibration_fast_enable attribute.
+
+The driver provides access to autocalibration flags (i.e. you can known if
+the IMU has successfully autocalibrated) and to the calibration data blob.
+
+The user can save this blob in a firmware file (i.e. in /lib/firmware) that
+the driver looks for at probe time. If found, then the IMU is initialized
+with this calibration data. This saves the user from performing the
+calibration procedure every time (which consist of moving the IMU in
+various way).
+
+The driver looks for calibration data file using two different names: first
+a file whose name is suffixed with the IMU unique ID (exposed in sysfs as
+serial_number) is searched for; this is useful when there is more than one
+IMU instance. If this file is not found, then a "generic" calibration file
+is searched for (which can be used when only one IMU is present, without
+struggling with fancy names, that change on each device).
+
+Valid calibration file names would be e.g.
+ bno055-caldata-0e7c26a33541515120204a35342b04ff.dat
+ bno055-caldata.dat
+
+In non-fusion mode the IIO 'offset' attributes provide access to the
+offsets from calibration data (if any), so that the user can apply them to
+the accel, angvel and magn IIO attributes. In fusion mode they are not
+needed (the IMU firmware internally applies those corrections) and they
+read as zero.
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index 58b7a4ebac51..1b7292c58cd0 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -10,3 +10,5 @@ Industrial I/O
iio_configfs

ep93xx_adc
+
+ bno055
--
2.17.1

2022-04-27 10:30:09

by Andrea Merello

[permalink] [raw]
Subject: [v5 03/14] iio: event_monitor: add linear acceleration modifiers

From: Andrea Merello <[email protected]>

Following the introduction of IIO linear acceleration modifiers, update the
event_monitor tool accordingly.

Signed-off-by: Andrea Merello <[email protected]>
---
tools/iio/iio_event_monitor.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index 2f4581658859..1fee44abb836 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -122,6 +122,9 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_PM4] = "pm4",
[IIO_MOD_PM10] = "pm10",
[IIO_MOD_O2] = "o2",
+ [IIO_MOD_LINEAR_X] = "linear_x",
+ [IIO_MOD_LINEAR_Y] = "linear_y",
+ [IIO_MOD_LINEAR_Z] = "linear_z",
};

static bool event_is_known(struct iio_event_data *event)
--
2.17.1

2022-04-27 11:38:19

by Andrea Merello

[permalink] [raw]
Subject: [v5 01/14] iio: add modifiers for linear acceleration

From: Andrea Merello <[email protected]>

Add IIO_MOD_LINEAR_X, IIO_MOD_LINEAR_Y and IIO_MOD_LINEAR_Z modifiers to te
IIO core, which is preparatory for adding the Bosch BNO055 IMU driver.

Bosch BNO055 IMU can report raw accelerations (among x, y and z axis) as
well as the so called "linear accelerations" (again, among x, y and z axis)
which is basically the acceleration after subtracting gravity and for which
those new modifiers are for.

Signed-off-by: Andrea Merello <[email protected]>
---
drivers/iio/industrialio-core.c | 3 +++
include/uapi/linux/iio/types.h | 4 +++-
2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2f48e9a97274..d087b2607cc9 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -134,6 +134,9 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_ETHANOL] = "ethanol",
[IIO_MOD_H2] = "h2",
[IIO_MOD_O2] = "o2",
+ [IIO_MOD_LINEAR_X] = "linear_x",
+ [IIO_MOD_LINEAR_Y] = "linear_y",
+ [IIO_MOD_LINEAR_Z] = "linear_z",
};

/* relies on pairs of these shared then separate */
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 472cead10d8d..0993f6b697fc 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -95,6 +95,9 @@ enum iio_modifier {
IIO_MOD_ETHANOL,
IIO_MOD_H2,
IIO_MOD_O2,
+ IIO_MOD_LINEAR_X,
+ IIO_MOD_LINEAR_Y,
+ IIO_MOD_LINEAR_Z,
};

enum iio_event_type {
@@ -115,4 +118,3 @@ enum iio_event_direction {
};

#endif /* _UAPI_IIO_TYPES_H_ */
-
--
2.17.1

2022-04-27 14:12:16

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [v5 12/14] iio: imu: add BNO055 serdev driver

On Wed, Apr 27, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
>
> Hi Andrea,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on jic23-iio/togreg]
> [also build test ERROR on linus/master linux/master v5.18-rc4 next-20220426]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
> base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
> compiler: arceb-elf-gcc (GCC) 11.3.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/675ca9cd13af45cc5943dd15caad5e866fd7c971
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
> git checkout 675ca9cd13af45cc5943dd15caad5e866fd7c971
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All errors (new ones prefixed by >>):
>
> >> make[5]: *** No rule to make target 'drivers/iio/imu/bno055/bno055_ser_trace.o', needed by 'drivers/iio/imu/bno055/built-in.a'.
> make[5]: Target '__build' not remade because of errors.

You need to add a C-file with the only line

#include <..._trace.h>

And drop that include from the ..._core.c.


--
With Best Regards,
Andy Shevchenko

2022-04-27 14:13:56

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [v5 00/14] Add support for Bosch BNO055 IMU

On Tue, Apr 26, 2022 at 3:11 PM Andrea Merello <[email protected]> wrote:
>
> 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/14 to 6/14: 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/14: fix binary attributes didn't work with IIO
>
> 8/14 to 11/14: add the core IIO BNO055 driver and documentation for sysfs
> attributes and DT bindings
>
> 12/14: adds serdev BNO055 driver to actually use the IMU via serial line
>
> 13/14: adds I2C BNO055 driver to actually use the IMU via I2C wiring
>
> 14/14: add a documentation file that describe the bno055 driver and
> specifically the calibration

FWIW,
Reviewed-by: Andy Shevchenko <[email protected]>
for non-commented patches (12 out of 14 AFAICS).

> Differences wrt v4:
> - be more tolerant wrt unrecognized chip IDs
> - rename 'serial_number' attribute in 'serialnumber'
> - fix missing NULL variable initialization
> - use sign_extend32() instead of s16 casting where appropriate
> - fix quaternion unit
> - minor stuff (e.g. comments..)
> - reduce (slightly) locking in serdev driver
> - rework tracepoint support (e.g. remove dedicated config option)
>
> Differences wrt other BNO055 drivers:
>
> Previously at least another driver for the very same chip has been posted
> to the Linux ML [0], but it has been never merged, and it seems no one
> cared of it since quite a long time.
>
> This driver differs from the above driver on the following aspects:
>
> - This driver supports also serial access (to be reliable, reset pin is
> required to be wired)
>
> - The above driver tried to support all IMU HW modes by allowing to
> choose one in the DT, and adapting IIO attributes accordingly. This
> driver does not rely on DT for this, instead settings are done via
> sysfs attributes. All IIO attributes are always exposed; more on this
> later on. This driver however supports only a subset of the
> HW-supported modes.
>
> - This driver has some support for managing the IMU calibration
>
> Supported operation modes:
>
> - AMG (accelerometer, magnetometer and gyroscope) mode, which provides
> raw (uncalibrated) measurements from the said sensors, and allows for
> setting some parameters about them (e.g. filter cut-off frequency, max
> sensor ranges, etc).
>
> - Fusion mode, which still provides AMG measures, while it also provides
> other data calculated by the IMU (e.g. rotation angles, linear
> acceleration, etc). In this mode user has no freedom to set any sensor
> parameter, since the HW locks them. Autocalibration and correction is
> performed by the IMU.
>
> IIO attributes exposing sensors parameters are always present, but in
> fusion modes the available values are constrained to just the one used by
> the HW. This is reflected in the '*_available' IIO attributes.
>
> Trying to set a not-supported value always falls back to the closest
> supported one, which in this case is just the one in use by the HW.
>
> IIO attributes for unavailable measurements (e.g. Euler angles in AMG
> mode) can't be read (return -EBUSY, or refuse to enable buffer).
>
> IMU calibration:
>
> The IMU supports for two sets of calibration parameters:
>
> - SIC matrix. user-provided; this driver doesn't currently support it
>
> - Offset and radius parameters. The IMU automatically finds out them when
> it is running in fusion mode; supported by this driver.
>
> The driver provides access to autocalibration flags (i.e. you can known
> if the IMU has successfully autocalibrated) and to calibration data blob.
> The user can save this blob in a "firmware" file (i.e. in /lib/firmware)
> that the driver looks for at probe time. If found, then the IMU is
> initialized with this calibration data. This saves the user from
> performing the calibration procedure every time (which consist of moving
> the IMU in various way).
>
> The driver looks for calibration data file using two different names:
> first a file whose name is suffixed with the IMU unique ID is searched
> for; this is useful when there is more than one IMU instance. If this
> file is not found, then a "generic" calibration file is searched for
> (which can be used when only one IMU is present, without struggling with
> fancy names, that changes on each device).
>
> In AMG mode the IIO 'offset' attributes provide access to the offsets
> from calibration data (if any), so that the user can apply them to the
> accel, angvel and magn IIO attributes. In fusion mode they are not needed
> and read as zero.
>
>
> Access protocols and serdev module:
>
> The serial protocol is quite simple, but there are tricks to make it
> really works. Those tricks and workarounds are documented in the driver
> source file.
>
> The core BNO055 driver tries to group readings in burst when appropriate,
> in order to optimize triggered buffer operation. The threshold for
> splitting a burst (i.e. max number of unused bytes in the middle of a
> burst that will be throw away) is provided to the core driver by the
> lowlevel access driver (either serdev or I2C) at probe time.
>
> [0] https://www.spinics.net/lists/linux-iio/msg25508.html
>
> Andrea Merello (14):
> iio: add modifiers for linear acceleration
> iio: document linear acceleration modifiers
> iio: event_monitor: add linear acceleration modifiers
> iio: add modifers for pitch, yaw, roll
> iio: document pitch, yaw, roll modifiers
> iio: event_monitor: add pitch, yaw and roll modifiers
> iio: add support for binary attributes
> iio: imu: add Bosch Sensortec BNO055 core driver
> iio: document bno055 private sysfs attributes
> iio: document "serialnumber" sysfs attribute
> dt-bindings: iio/imu: Add Bosch BNO055
> iio: imu: add BNO055 serdev driver
> iio: imu: add BNO055 I2C driver
> docs: iio: add documentation for BNO055 driver
>
> Documentation/ABI/testing/sysfs-bus-iio | 25 +
> .../ABI/testing/sysfs-bus-iio-bno055 | 81 +
> .../bindings/iio/imu/bosch,bno055.yaml | 59 +
> Documentation/iio/bno055.rst | 50 +
> Documentation/iio/index.rst | 2 +
> drivers/iio/imu/Kconfig | 1 +
> drivers/iio/imu/Makefile | 1 +
> drivers/iio/imu/bno055/Kconfig | 25 +
> drivers/iio/imu/bno055/Makefile | 10 +
> drivers/iio/imu/bno055/bno055.c | 1710 +++++++++++++++++
> drivers/iio/imu/bno055/bno055.h | 12 +
> drivers/iio/imu/bno055/bno055_i2c.c | 57 +
> drivers/iio/imu/bno055/bno055_ser_core.c | 560 ++++++
> drivers/iio/imu/bno055/bno055_ser_trace.h | 104 +
> drivers/iio/industrialio-core.c | 10 +-
> include/uapi/linux/iio/types.h | 7 +-
> tools/iio/iio_event_monitor.c | 6 +
> 17 files changed, 2718 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-bno055
> create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bno055.yaml
> create mode 100644 Documentation/iio/bno055.rst
> create mode 100644 drivers/iio/imu/bno055/Kconfig
> create mode 100644 drivers/iio/imu/bno055/Makefile
> create mode 100644 drivers/iio/imu/bno055/bno055.c
> create mode 100644 drivers/iio/imu/bno055/bno055.h
> create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c
> create mode 100644 drivers/iio/imu/bno055/bno055_ser_core.c
> create mode 100644 drivers/iio/imu/bno055/bno055_ser_trace.h
>
> --
> 2.17.1



--
With Best Regards,
Andy Shevchenko

2022-04-27 14:53:32

by kernel test robot

[permalink] [raw]
Subject: Re: [v5 12/14] iio: imu: add BNO055 serdev driver

Hi Andrea,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master linux/master v5.18-rc4 next-20220427]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: alpha-randconfig-r036-20220427 (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/675ca9cd13af45cc5943dd15caad5e866fd7c971
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
git checkout 675ca9cd13af45cc5943dd15caad5e866fd7c971
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/iio/imu/bno055/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/iio/imu/bno055/bno055.c:1317:5: warning: no previous prototype for 'bno055_debugfs_reg_access' [-Wmissing-prototypes]
1317 | int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
| ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/bno055_debugfs_reg_access +1317 drivers/iio/imu/bno055/bno055.c

2f40e22369ed70 Andrea Merello 2022-04-26 1316
2f40e22369ed70 Andrea Merello 2022-04-26 @1317 int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
2f40e22369ed70 Andrea Merello 2022-04-26 1318 unsigned int writeval, unsigned int *readval)
2f40e22369ed70 Andrea Merello 2022-04-26 1319 {
2f40e22369ed70 Andrea Merello 2022-04-26 1320 return 0;
2f40e22369ed70 Andrea Merello 2022-04-26 1321 }
2f40e22369ed70 Andrea Merello 2022-04-26 1322 #endif
2f40e22369ed70 Andrea Merello 2022-04-26 1323

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-05-02 14:02:02

by Andrea Merello

[permalink] [raw]
Subject: Re: [v5 00/14] Add support for Bosch BNO055 IMU

Il giorno dom 1 mag 2022 alle ore 18:54 Jonathan Cameron
<[email protected]> ha scritto:
>
> On Wed, 27 Apr 2022 15:42:49 +0200
> Andy Shevchenko <[email protected]> wrote:
>
> > On Tue, Apr 26, 2022 at 3:11 PM Andrea Merello <[email protected]> wrote:
> > >
> > > 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/14 to 6/14: 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/14: fix binary attributes didn't work with IIO
> > >
> > > 8/14 to 11/14: add the core IIO BNO055 driver and documentation for sysfs
> > > attributes and DT bindings
> > >
> > > 12/14: adds serdev BNO055 driver to actually use the IMU via serial line
> > >
> > > 13/14: adds I2C BNO055 driver to actually use the IMU via I2C wiring
> > >
> > > 14/14: add a documentation file that describe the bno055 driver and
> > > specifically the calibration
> >
> > FWIW,
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > for non-commented patches (12 out of 14 AFAICS).
> >
> FWIW I'm fine with the series once you've tidied up the stuff Andy picked up
> on.
>
> Thanks Andy for the detailed reviewing btw.
>
> Jonathan

I'm very grateful to both of you and to everyone who commented on
those patches. Thanks :). Beside the "Reviewed-by" tags where
appropriate, is it usual/appropriate to put some tag like "Thanks-to
.. [for comments]" ?

BTW I have also gone through some kernel-robot reports; they also
state "If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>". I'd say that it would
be OK to add this tag to a patch that just fixes what is reported, but
I'm unsure whether it is appropriate to add this tag to the patches in
my series, because they add the code and the fix at once. Any advice
here?

Andrea

2022-05-02 15:07:13

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [v5 00/14] Add support for Bosch BNO055 IMU

On Wed, 27 Apr 2022 15:42:49 +0200
Andy Shevchenko <[email protected]> wrote:

> On Tue, Apr 26, 2022 at 3:11 PM Andrea Merello <[email protected]> wrote:
> >
> > 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/14 to 6/14: 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/14: fix binary attributes didn't work with IIO
> >
> > 8/14 to 11/14: add the core IIO BNO055 driver and documentation for sysfs
> > attributes and DT bindings
> >
> > 12/14: adds serdev BNO055 driver to actually use the IMU via serial line
> >
> > 13/14: adds I2C BNO055 driver to actually use the IMU via I2C wiring
> >
> > 14/14: add a documentation file that describe the bno055 driver and
> > specifically the calibration
>
> FWIW,
> Reviewed-by: Andy Shevchenko <[email protected]>
> for non-commented patches (12 out of 14 AFAICS).
>
FWIW I'm fine with the series once you've tidied up the stuff Andy picked up
on.

Thanks Andy for the detailed reviewing btw.

Jonathan

> > Differences wrt v4:
> > - be more tolerant wrt unrecognized chip IDs
> > - rename 'serial_number' attribute in 'serialnumber'
> > - fix missing NULL variable initialization
> > - use sign_extend32() instead of s16 casting where appropriate
> > - fix quaternion unit
> > - minor stuff (e.g. comments..)
> > - reduce (slightly) locking in serdev driver
> > - rework tracepoint support (e.g. remove dedicated config option)
> >
> > Differences wrt other BNO055 drivers:
> >
> > Previously at least another driver for the very same chip has been posted
> > to the Linux ML [0], but it has been never merged, and it seems no one
> > cared of it since quite a long time.
> >
> > This driver differs from the above driver on the following aspects:
> >
> > - This driver supports also serial access (to be reliable, reset pin is
> > required to be wired)
> >
> > - The above driver tried to support all IMU HW modes by allowing to
> > choose one in the DT, and adapting IIO attributes accordingly. This
> > driver does not rely on DT for this, instead settings are done via
> > sysfs attributes. All IIO attributes are always exposed; more on this
> > later on. This driver however supports only a subset of the
> > HW-supported modes.
> >
> > - This driver has some support for managing the IMU calibration
> >
> > Supported operation modes:
> >
> > - AMG (accelerometer, magnetometer and gyroscope) mode, which provides
> > raw (uncalibrated) measurements from the said sensors, and allows for
> > setting some parameters about them (e.g. filter cut-off frequency, max
> > sensor ranges, etc).
> >
> > - Fusion mode, which still provides AMG measures, while it also provides
> > other data calculated by the IMU (e.g. rotation angles, linear
> > acceleration, etc). In this mode user has no freedom to set any sensor
> > parameter, since the HW locks them. Autocalibration and correction is
> > performed by the IMU.
> >
> > IIO attributes exposing sensors parameters are always present, but in
> > fusion modes the available values are constrained to just the one used by
> > the HW. This is reflected in the '*_available' IIO attributes.
> >
> > Trying to set a not-supported value always falls back to the closest
> > supported one, which in this case is just the one in use by the HW.
> >
> > IIO attributes for unavailable measurements (e.g. Euler angles in AMG
> > mode) can't be read (return -EBUSY, or refuse to enable buffer).
> >
> > IMU calibration:
> >
> > The IMU supports for two sets of calibration parameters:
> >
> > - SIC matrix. user-provided; this driver doesn't currently support it
> >
> > - Offset and radius parameters. The IMU automatically finds out them when
> > it is running in fusion mode; supported by this driver.
> >
> > The driver provides access to autocalibration flags (i.e. you can known
> > if the IMU has successfully autocalibrated) and to calibration data blob.
> > The user can save this blob in a "firmware" file (i.e. in /lib/firmware)
> > that the driver looks for at probe time. If found, then the IMU is
> > initialized with this calibration data. This saves the user from
> > performing the calibration procedure every time (which consist of moving
> > the IMU in various way).
> >
> > The driver looks for calibration data file using two different names:
> > first a file whose name is suffixed with the IMU unique ID is searched
> > for; this is useful when there is more than one IMU instance. If this
> > file is not found, then a "generic" calibration file is searched for
> > (which can be used when only one IMU is present, without struggling with
> > fancy names, that changes on each device).
> >
> > In AMG mode the IIO 'offset' attributes provide access to the offsets
> > from calibration data (if any), so that the user can apply them to the
> > accel, angvel and magn IIO attributes. In fusion mode they are not needed
> > and read as zero.
> >
> >
> > Access protocols and serdev module:
> >
> > The serial protocol is quite simple, but there are tricks to make it
> > really works. Those tricks and workarounds are documented in the driver
> > source file.
> >
> > The core BNO055 driver tries to group readings in burst when appropriate,
> > in order to optimize triggered buffer operation. The threshold for
> > splitting a burst (i.e. max number of unused bytes in the middle of a
> > burst that will be throw away) is provided to the core driver by the
> > lowlevel access driver (either serdev or I2C) at probe time.
> >
> > [0] https://www.spinics.net/lists/linux-iio/msg25508.html
> >
> > Andrea Merello (14):
> > iio: add modifiers for linear acceleration
> > iio: document linear acceleration modifiers
> > iio: event_monitor: add linear acceleration modifiers
> > iio: add modifers for pitch, yaw, roll
> > iio: document pitch, yaw, roll modifiers
> > iio: event_monitor: add pitch, yaw and roll modifiers
> > iio: add support for binary attributes
> > iio: imu: add Bosch Sensortec BNO055 core driver
> > iio: document bno055 private sysfs attributes
> > iio: document "serialnumber" sysfs attribute
> > dt-bindings: iio/imu: Add Bosch BNO055
> > iio: imu: add BNO055 serdev driver
> > iio: imu: add BNO055 I2C driver
> > docs: iio: add documentation for BNO055 driver
> >
> > Documentation/ABI/testing/sysfs-bus-iio | 25 +
> > .../ABI/testing/sysfs-bus-iio-bno055 | 81 +
> > .../bindings/iio/imu/bosch,bno055.yaml | 59 +
> > Documentation/iio/bno055.rst | 50 +
> > Documentation/iio/index.rst | 2 +
> > drivers/iio/imu/Kconfig | 1 +
> > drivers/iio/imu/Makefile | 1 +
> > drivers/iio/imu/bno055/Kconfig | 25 +
> > drivers/iio/imu/bno055/Makefile | 10 +
> > drivers/iio/imu/bno055/bno055.c | 1710 +++++++++++++++++
> > drivers/iio/imu/bno055/bno055.h | 12 +
> > drivers/iio/imu/bno055/bno055_i2c.c | 57 +
> > drivers/iio/imu/bno055/bno055_ser_core.c | 560 ++++++
> > drivers/iio/imu/bno055/bno055_ser_trace.h | 104 +
> > drivers/iio/industrialio-core.c | 10 +-
> > include/uapi/linux/iio/types.h | 7 +-
> > tools/iio/iio_event_monitor.c | 6 +
> > 17 files changed, 2718 insertions(+), 2 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-bno055
> > create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bno055.yaml
> > create mode 100644 Documentation/iio/bno055.rst
> > create mode 100644 drivers/iio/imu/bno055/Kconfig
> > create mode 100644 drivers/iio/imu/bno055/Makefile
> > create mode 100644 drivers/iio/imu/bno055/bno055.c
> > create mode 100644 drivers/iio/imu/bno055/bno055.h
> > create mode 100644 drivers/iio/imu/bno055/bno055_i2c.c
> > create mode 100644 drivers/iio/imu/bno055/bno055_ser_core.c
> > create mode 100644 drivers/iio/imu/bno055/bno055_ser_trace.h
> >
> > --
> > 2.17.1
>
>
>

2022-05-02 17:09:32

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [v5 00/14] Add support for Bosch BNO055 IMU

On Mon, May 2, 2022 at 8:33 AM Andrea Merello <[email protected]> wrote:
> Il giorno dom 1 mag 2022 alle ore 18:54 Jonathan Cameron
> <[email protected]> ha scritto:
> > On Wed, 27 Apr 2022 15:42:49 +0200
> > Andy Shevchenko <[email protected]> wrote:
> > > On Tue, Apr 26, 2022 at 3:11 PM Andrea Merello <[email protected]> wrote:

...

> > > FWIW,
> > > Reviewed-by: Andy Shevchenko <[email protected]>
> > > for non-commented patches (12 out of 14 AFAICS).
> > >
> > FWIW I'm fine with the series once you've tidied up the stuff Andy picked up
> > on.
> >
> > Thanks Andy for the detailed reviewing btw.

You/re welcome!

> I'm very grateful to both of you and to everyone who commented on
> those patches. Thanks :). Beside the "Reviewed-by" tags where
> appropriate, is it usual/appropriate to put some tag like "Thanks-to
> .. [for comments]" ?

Nope, just mention that in the cover letter.

> BTW I have also gone through some kernel-robot reports; they also
> state "If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>". I'd say that it would
> be OK to add this tag to a patch that just fixes what is reported, but
> I'm unsure whether it is appropriate to add this tag to the patches in
> my series, because they add the code and the fix at once. Any advice
> here?

For this we specifically amended the kernel documentation recently.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

"The tag is intended for bugs; please do not use it to credit feature requests."

--
With Best Regards,
Andy Shevchenko

2022-05-02 23:40:55

by Andrea Merello

[permalink] [raw]
Subject: Re: [v5 00/14] Add support for Bosch BNO055 IMU

Il giorno lun 2 mag 2022 alle ore 09:48 Andy Shevchenko
<[email protected]> ha scritto:
>

[ .. ]

> > BTW I have also gone through some kernel-robot reports; they also
> > state "If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>". I'd say that it would
> > be OK to add this tag to a patch that just fixes what is reported, but
> > I'm unsure whether it is appropriate to add this tag to the patches in
> > my series, because they add the code and the fix at once. Any advice
> > here?
>
> For this we specifically amended the kernel documentation recently.
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
>
> "The tag is intended for bugs; please do not use it to credit feature requests."

Well, no any feature request to credit here; a bug and its fix are
involved. Sounds more like a "yes" so far.. But it wouldn't be clear
what the robot did report indeed (squashed bugs and fixes).. Maybe a
"thank" in the cover letter also to it would suffice?

> --
> With Best Regards,
> Andy Shevchenko

2022-05-03 00:45:34

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [v5 00/14] Add support for Bosch BNO055 IMU

On Mon, May 2, 2022 at 10:31 AM Andrea Merello <[email protected]> wrote:
> Il giorno lun 2 mag 2022 alle ore 09:48 Andy Shevchenko
> <[email protected]> ha scritto:

...

> > > BTW I have also gone through some kernel-robot reports; they also
> > > state "If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <[email protected]>". I'd say that it would
> > > be OK to add this tag to a patch that just fixes what is reported, but
> > > I'm unsure whether it is appropriate to add this tag to the patches in
> > > my series, because they add the code and the fix at once. Any advice
> > > here?
> >
> > For this we specifically amended the kernel documentation recently.
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> >
> > "The tag is intended for bugs; please do not use it to credit feature requests."
>
> Well, no any feature request to credit here; a bug and its fix are
> involved. Sounds more like a "yes" so far.. But it wouldn't be clear
> what the robot did report indeed (squashed bugs and fixes).. Maybe a
> "thank" in the cover letter also to it would suffice?

The reported from bots should be used when your patches.changes
induced by those reports, it seems otherwise in your case.,


--
With Best Regards,
Andy Shevchenko

2022-05-03 08:59:11

by Andrea Merello

[permalink] [raw]
Subject: Re: [v5 12/14] iio: imu: add BNO055 serdev driver

Il giorno mer 27 apr 2022 alle ore 15:41 Andy Shevchenko
<[email protected]> ha scritto:
>
> On Wed, Apr 27, 2022 at 10:11 AM kernel test robot <[email protected]> wrote:
> >
> > Hi Andrea,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on jic23-iio/togreg]
> > [also build test ERROR on linus/master linux/master v5.18-rc4 next-20220426]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> > config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
> > compiler: arceb-elf-gcc (GCC) 11.3.0
> > reproduce (this is a W=1 build):
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # https://github.com/intel-lab-lkp/linux/commit/675ca9cd13af45cc5943dd15caad5e866fd7c971
> > git remote add linux-review https://github.com/intel-lab-lkp/linux
> > git fetch --no-tags linux-review Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20220426-212132
> > git checkout 675ca9cd13af45cc5943dd15caad5e866fd7c971
> > # save the config file
> > mkdir build_dir && cp config build_dir/.config
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <[email protected]>
> >
> > All errors (new ones prefixed by >>):
> >
> > >> make[5]: *** No rule to make target 'drivers/iio/imu/bno055/bno055_ser_trace.o', needed by 'drivers/iio/imu/bno055/built-in.a'.
> > make[5]: Target '__build' not remade because of errors.
>
> You need to add a C-file with the only line
>
> #include <..._trace.h>
>
> And drop that include from the _core.c.

Hum, I'm a bit confused here: the bno055_ser_core.c file explicitly
looks for that tracepoints (e.g. it calls trace_send_chunks() and
friends); dropping the include prevents build here because there would
be no definition for those tracepoints.

There is already a C file bno055_ser_trace.c that just contains the
said include and it defines CREATE_TRACE_POINTS; I see other drivers
like dwc3 do the same..

But my problem is that I cannot reproduce the issue found by the bot:
the compiler that is downloaded by the script doesn't run on my build
box because it wants a newer libc (I was hoping that those compilers
were statically linked, but they aren't), while any other attempt I
did with other older compilers resulted in either successful build or
failed with other weird, apparently unrelated, errors about relocation
issues (of course I tried with the arch and config used by the build
bot).

Is there any build farm publicly available or something like that?

> --
> With Best Regards,
> Andy Shevchenko

2022-05-03 20:09:09

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [v5 12/14] iio: imu: add BNO055 serdev driver

On Tue, May 3, 2022 at 3:30 PM Andrea Merello <[email protected]> wrote:
>
> Il giorno mar 3 mag 2022 alle ore 09:48 Andrea Merello
> <[email protected]> ha scritto:
>
> [...]
>
> > > You need to add a C-file with the only line
> > >
> > > #include <..._trace.h>
> > >
> > > And drop that include from the _core.c.
> >
> > Hum, I'm a bit confused here: the bno055_ser_core.c file explicitly
> > looks for that tracepoints (e.g. it calls trace_send_chunks() and
> > friends); dropping the include prevents build here because there would
> > be no definition for those tracepoints.
> >
> > There is already a C file bno055_ser_trace.c that just contains the
> > said include and it defines CREATE_TRACE_POINTS; I see other drivers
> > like dwc3 do the same..
>
> Oops.. it turned out that I just had this almost-empty C file as
> untracked in my git tree, and it ended up not being included in
> patches also. Being it laying in my src tree caused the build to
> succeed.
>
> I have been misled by the other problem I (still) have (below); I was
> focused on the wrong thing, sorry.

So, there are two reports:
1) missed C file;
2) possible missed prototype.

To solve 1) you need to add the C file to the patch.
To solve 2) you need either declare it static or put it into the
header file (I haven't checked deeply which one is your case).

> > But my problem is that I cannot reproduce the issue found by the bot:
> > the compiler that is downloaded by the script doesn't run on my build
> > box because it wants a newer libc (I was hoping that those compilers
> > were statically linked, but they aren't), while any other attempt I
> > did with other older compilers resulted in either successful build or
> > failed with other weird, apparently unrelated, errors about relocation
> > issues (of course I tried with the arch and config used by the build
> > bot).

You may use compilers from kernel.org that don't require any libc at
all (only good for kernel compilation).

> > Is there any build farm publicly available or something like that?

Not of my knowledge.


--
With Best Regards,
Andy Shevchenko

2022-05-04 06:36:48

by Andrea Merello

[permalink] [raw]
Subject: Re: [v5 12/14] iio: imu: add BNO055 serdev driver

Il giorno mar 3 mag 2022 alle ore 09:48 Andrea Merello
<[email protected]> ha scritto:

[...]

> > You need to add a C-file with the only line
> >
> > #include <..._trace.h>
> >
> > And drop that include from the _core.c.
>
> Hum, I'm a bit confused here: the bno055_ser_core.c file explicitly
> looks for that tracepoints (e.g. it calls trace_send_chunks() and
> friends); dropping the include prevents build here because there would
> be no definition for those tracepoints.
>
> There is already a C file bno055_ser_trace.c that just contains the
> said include and it defines CREATE_TRACE_POINTS; I see other drivers
> like dwc3 do the same..

Oops.. it turned out that I just had this almost-empty C file as
untracked in my git tree, and it ended up not being included in
patches also. Being it laying in my src tree caused the build to
succeed.

I have been misled by the other problem I (still) have (below); I was
focused on the wrong thing, sorry.

> But my problem is that I cannot reproduce the issue found by the bot:
> the compiler that is downloaded by the script doesn't run on my build
> box because it wants a newer libc (I was hoping that those compilers
> were statically linked, but they aren't), while any other attempt I
> did with other older compilers resulted in either successful build or
> failed with other weird, apparently unrelated, errors about relocation
> issues (of course I tried with the arch and config used by the build
> bot).
>
> Is there any build farm publicly available or something like that?
>
> > --
> > With Best Regards,
> > Andy Shevchenko