[4/4] iio: imu: add BNO055 serdev driver

Commit Message

Andrea Merello July 15, 2021, 2:17 p.m. UTC

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

Signed-off-by: Andrea Merello <[e-mail]>
Cc: Andrea Merello <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Matt Ranostay <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Vlad Dogaru <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/iio/imu/bno055/Kconfig     |   5 +
 drivers/iio/imu/bno055/Makefile    |   1 +
 drivers/iio/imu/bno055/bno055_sl.c | 576 +++++++++++++++++++++++++++++
 3 files changed, 582 insertions(+)
 create mode 100644 drivers/iio/imu/bno055/bno055_sl.c

Comments

kernel test robot July 15, 2021, 5:08 p.m. UTC | #1

Hi Andrea,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on robh/for-next linus/master v5.14-rc1 next-20210715]
[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/0day-ci/linux/commits/Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20210715-222018
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 10.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/0day-ci/linux/commit/616d1b9a99ec2045cdf6cc827751660a48ccc5d2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andrea-Merello/Add-support-for-Bosch-BNO055-IMU/20210715-222018
        git checkout 616d1b9a99ec2045cdf6cc827751660a48ccc5d2
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=arc 

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

All warnings (new ones prefixed by >>):

>> drivers/iio/imu/bno055/bno055.c:250:5: warning: no previous prototype for 'bno055_calibration_load' [-Wmissing-prototypes]
     250 | int bno055_calibration_load(struct bno055_priv *priv, const struct firmware *fw)
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/imu/bno055/bno055.c: In function '_bno055_write_raw':
>> drivers/iio/imu/bno055/bno055.c:770:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
     770 |   switch (mask) {
         |   ^~~~~~
   drivers/iio/imu/bno055/bno055.c:774:2: note: here
     774 |  default:
         |  ^~~~~~~


vim +/bno055_calibration_load +250 drivers/iio/imu/bno055/bno055.c

d13cfdff130569 Andrea Merello 2021-07-15  248  
d13cfdff130569 Andrea Merello 2021-07-15  249  /* must be called in configuration mode */
d13cfdff130569 Andrea Merello 2021-07-15 @250  int bno055_calibration_load(struct bno055_priv *priv, const struct firmware *fw)
d13cfdff130569 Andrea Merello 2021-07-15  251  {
d13cfdff130569 Andrea Merello 2021-07-15  252  	int i;
d13cfdff130569 Andrea Merello 2021-07-15  253  	unsigned int tmp;
d13cfdff130569 Andrea Merello 2021-07-15  254  	u8 cal[BNO055_CALDATA_LEN];
d13cfdff130569 Andrea Merello 2021-07-15  255  	int read, tot_read = 0;
d13cfdff130569 Andrea Merello 2021-07-15  256  	int ret = 0;
d13cfdff130569 Andrea Merello 2021-07-15  257  	char *buf = kmalloc(fw->size + 1, GFP_KERNEL);
d13cfdff130569 Andrea Merello 2021-07-15  258  
d13cfdff130569 Andrea Merello 2021-07-15  259  	if (!buf)
d13cfdff130569 Andrea Merello 2021-07-15  260  		return -ENOMEM;
d13cfdff130569 Andrea Merello 2021-07-15  261  
d13cfdff130569 Andrea Merello 2021-07-15  262  	memcpy(buf, fw->data, fw->size);
d13cfdff130569 Andrea Merello 2021-07-15  263  	buf[fw->size] = '\0';
d13cfdff130569 Andrea Merello 2021-07-15  264  	for (i = 0; i < BNO055_CALDATA_LEN; i++) {
d13cfdff130569 Andrea Merello 2021-07-15  265  		ret = sscanf(buf + tot_read, "%x%n",
d13cfdff130569 Andrea Merello 2021-07-15  266  			     &tmp, &read);
d13cfdff130569 Andrea Merello 2021-07-15  267  		if (ret != 1 || tmp > 0xff) {
d13cfdff130569 Andrea Merello 2021-07-15  268  			ret = -EINVAL;
d13cfdff130569 Andrea Merello 2021-07-15  269  			goto exit;
d13cfdff130569 Andrea Merello 2021-07-15  270  		}
d13cfdff130569 Andrea Merello 2021-07-15  271  		cal[i] = tmp;
d13cfdff130569 Andrea Merello 2021-07-15  272  		tot_read += read;
d13cfdff130569 Andrea Merello 2021-07-15  273  	}
d13cfdff130569 Andrea Merello 2021-07-15  274  	dev_dbg(priv->dev, "loading cal data: %*ph", BNO055_CALDATA_LEN, cal);
d13cfdff130569 Andrea Merello 2021-07-15  275  	ret = regmap_bulk_write(priv->regmap, BNO055_CALDATA_START,
d13cfdff130569 Andrea Merello 2021-07-15  276  				cal, BNO055_CALDATA_LEN);
d13cfdff130569 Andrea Merello 2021-07-15  277  exit:
d13cfdff130569 Andrea Merello 2021-07-15  278  	kfree(buf);
d13cfdff130569 Andrea Merello 2021-07-15  279  	return ret;
d13cfdff130569 Andrea Merello 2021-07-15  280  }
d13cfdff130569 Andrea Merello 2021-07-15  281  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Jonathan Cameron July 17, 2021, 3:50 p.m. UTC | #2

On Thu, 15 Jul 2021 16:17:42 +0200
Andrea Merello <[e-mail]> wrote:

> This path adds a serdev driver for communicating to a BNO055 IMU
> via serial bus, and enables the BNO055 core driver to work in this
> scenario.
> 
> Signed-off-by: Andrea Merello <[e-mail]>
> Cc: Andrea Merello <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Matt Ranostay <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Vlad Dogaru <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]

Hi Andrea,

A few comments inline.

Jonathan

> ---
>  drivers/iio/imu/bno055/Kconfig     |   5 +
>  drivers/iio/imu/bno055/Makefile    |   1 +
>  drivers/iio/imu/bno055/bno055_sl.c | 576 +++++++++++++++++++++++++++++
>  3 files changed, 582 insertions(+)
>  create mode 100644 drivers/iio/imu/bno055/bno055_sl.c
> 
> diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig
> index 2bfed8df4554..6d2e8c9f85b7 100644
> --- a/drivers/iio/imu/bno055/Kconfig
> +++ b/drivers/iio/imu/bno055/Kconfig
> @@ -5,3 +5,8 @@
>  
>  config BOSH_BNO055_IIO
>  	tristate
> +
> +config BOSH_BNO055_SERIAL
> +	tristate "Bosh BNO055 attached via serial bus"
> +	depends on SERIAL_DEV_BUS
> +	select BOSH_BNO055_IIO
> diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile
> index 15c5ddf8d648..b704b10b6bd1 100644
> --- a/drivers/iio/imu/bno055/Makefile
> +++ b/drivers/iio/imu/bno055/Makefile
> @@ -4,3 +4,4 @@
>  #
>  
>  obj-$(CONFIG_BOSH_BNO055_IIO) += bno055.o
> +obj-$(CONFIG_BOSH_BNO055_SERIAL) += bno055_sl.o
> diff --git a/drivers/iio/imu/bno055/bno055_sl.c b/drivers/iio/imu/bno055/bno055_sl.c
> new file mode 100644
> index 000000000000..9604d73d126c
> --- /dev/null
> +++ b/drivers/iio/imu/bno055/bno055_sl.c
> @@ -0,0 +1,576 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Serial line interface for Bosh BNO055 IMU (via serdev).
> + * This file implements serial communication up to the register read/write
> + * level.
> + *
> + * Copyright (C) 2021 Istituto Italiano di Tecnologia
> + * Electronic Design Laboratory
> + * Written by Andrea Merello <[e-mail]>
> + *
> + * This driver is besed on
> + *	Plantower PMS7003 particulate matter sensor driver
> + *	Which is
> + *	Copyright (c) Tomasz Duszynski <[e-mail]>
> + */
> +
> +#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/of_irq.h>
> +#include <linux/regmap.h>
> +#include <linux/serdev.h>
> +
> +#include "bno055.h"
> +
> +#define BNO055_SL_DRIVER_NAME "bno055-sl"
> +
> +/*
> + * Register writes cmd have the following format
> + * +------+------+-----+-----+----- ... ----+
> + * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
> + * +------+------+-----+-----+----- ... ----+
> + *
> + * Register write responses have the following format
> + * +------+----------+
> + * | 0xEE | ERROCODE |
> + * +------+----------+
> + *
> + * 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).
> + *
> + * BMU055 has been seen also failing to process commands in case we send them
> + * too close each other (or if it is somehow busy?)
> + *
> + * One idea would be to split data in chunks, and then wait 1-2mS between
> + * chunks (we hope not to exceed 30mS delay for any reason - which should
> + * be pretty a lot of time for us), and eventually retry in case the BNO055
> + * gets upset for any reason. This seems to work in avoiding the overflow
> + * errors, but indeed it seems slower than just perform a retry when an overflow
> + * error occur.
> + * 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.
> + *
> + * So, we workaround all this in the following way:
> + * In case of read we don't split the header but we rely on retries; This seems
> + * convenient for data read (where we TX only the hdr).
> + * For TX we split the transmission in 2-bytes chunks so that, we should not
> + * only avoid case 2 (which is still manageable), but we also hopefully avoid
> + * case 3, that would be by far worse.

Nice docs and this sounds terrible!

> + */
> +
> +/* Read operation overhead:
> + * 4 bytes req + 2byte resp hdr
> + * 6 bytes = 60 bit (considering 1start + 1stop bits).
> + * 60/115200 = ~520uS
> + * In 520uS we could read back about 34 bytes that means 3 samples, this means
> + * that in case of scattered read in which the gap is 3 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.
> + * By experiment, it seems convenient to burst up to about 5/6-samples-long gap
> + */
> +
> +#define BNO055_SL_XFER_BURST_BREAK_THRESHOLD 6
> +
> +struct bno055_sl_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 {
> +		STATUS_OK = 0,  /* command OK */
> +		STATUS_FAIL = 1,/* IMU communicated an error */
> +		STATUS_CRIT = -1/* serial communication with IMU failed */
> +	} cmd_status;
> +	struct mutex lock;
> +
> +	/* Only accessed in behalf of 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;
> +};
> +
> +static int bno055_sl_send_chunk(struct bno055_sl_priv *priv, u8 *data, int len)
> +{
> +	int ret;
> +
> +	dev_dbg(&priv->serdev->dev, "send (len: %d): %*ph", len, len, data);
> +	ret = serdev_device_write(priv->serdev, data, len,
> +				  msecs_to_jiffies(25));
> +	if (ret < len)
> +		return ret < 0 ? ret : -EIO;

Break this up perhaps as will be easier to read.

	if (ret < 0)
		return ret;

	if (ret < len)
		return -EIO;

	return 0;

> +	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_sl_do_send_cmd(struct bno055_sl_priv *priv,
> +				 int read, int addr, int len, u8 *data)
> +{
> +	int ret;
> +	int chunk_len;
> +	u8 hdr[] = {0xAA, !!read, addr, len};
> +
> +	if (read) {
> +		ret = bno055_sl_send_chunk(priv, hdr, 4);
> +	} else {
> +		ret = bno055_sl_send_chunk(priv, hdr, 2);
> +		if (ret)
> +			goto fail;
> +
> +		usleep_range(2000, 3000);
> +		ret = bno055_sl_send_chunk(priv, hdr + 2, 2);
> +	}
> +	if (ret)
> +		goto fail;
> +
> +	if (data) {
> +		while (len) {
> +			chunk_len = min(len, 2);
> +			usleep_range(2000, 3000);
> +			ret = bno055_sl_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 bno_sl_send_cmd(struct bno055_sl_priv *priv,
> +			   int read, int addr, int len, u8 *data)
> +{
> +	const int retry_max = 5;
> +	int retry = retry_max;
> +	int ret = 0;
> +
> +	/*
> +	 * In case previous command was interrupted we still neet 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)
> +			goto exit;
> +	}
> +
> +	/*
> +	 * 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.
> +	 */
> +	while (retry--) {
> +		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 - 1))
> +			dev_dbg(&priv->serdev->dev, "cmd retry: %d",
> +				retry_max - retry);
> +		ret = bno055_sl_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;
> +		} else if (!ret) {
> +			ret = -ETIMEDOUT;
> +			break;
> +		}
> +		ret = 0;
> +
> +		/*
> +		 * Poll if the IMU returns error (i.e busy), break if the IMU
> +		 * returns OK or if the serial communication broke
> +		 */
> +		if (priv->cmd_status <= 0)
> +			break;
> +	}
> +
> +exit:
> +	if (ret)
> +		return ret;
> +	if (priv->cmd_status == STATUS_CRIT)
> +		return -EIO;
> +	if (priv->cmd_status == STATUS_FAIL)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int bno055_sl_write_reg(void *context, const void *data, size_t count)
> +{
> +	int ret;
> +	int reg;
> +	u8 *write_data = (u8 *)data + 1;
> +	struct bno055_sl_priv *priv = context;
> +
> +	if (count < 2) {
> +		dev_err(&priv->serdev->dev, "Invalid write count %d", count);
> +		return -EINVAL;
> +	}
> +
> +	reg = ((u8 *)data)[0];
> +	dev_dbg(&priv->serdev->dev, "wr reg 0x%x = 0x%x", reg, ((u8 *)data)[1]);
> +	ret = bno_sl_send_cmd(priv, 0, reg, count - 1, write_data);
> +
> +	return ret;
> +}
> +
> +static int bno055_sl_read_reg(void *context,
> +			      const void *reg, size_t reg_size,
> +			      void *val, size_t val_size)
> +{
> +	int ret;
> +	int reg_addr;
> +	struct bno055_sl_priv *priv = context;
> +
> +	if (reg_size != 1) {

Can we plausibly hit this?  I would have though the regmap controls it
and is set appropriately.  Hence safe to drop this check.

> +		dev_err(&priv->serdev->dev, "Invalid read regsize %d",
> +			reg_size);
> +		return -EINVAL;
> +	}
> +
> +	if (val_size > 128) {
> +		dev_err(&priv->serdev->dev, "Invalid read valsize %d",
> +			val_size);
> +		return -EINVAL;
> +	}
> +
> +	reg_addr = ((u8 *)reg)[0];
> +	dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
> +	mutex_lock(&priv->lock);
> +	priv->expected_data_len = val_size;
> +	priv->response_buf = val;
> +	mutex_unlock(&priv->lock);
> +
> +	ret = bno_sl_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_sl_handle_rx(struct bno055_sl_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;
> +	complete(&priv->cmd_complete);
> +	mutex_unlock(&priv->lock);
> +}
> +
> +/*
> + * Serdev receiver FSM. This tracks the serial communication and parse the
> + * header. It pushes packets to bno055_sl_handle_rx(), eventually communicating
> + * failures (i.e. malformed packets).
> + * Idellay it doesn't know anything about upper layer (i.e. if this is the

Ideally

> + * 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_sl_receive_buf(struct serdev_device *serdev,
> +				 const unsigned char *buf, size_t size)
> +{
> +	int status;
> +	struct bno055_sl_priv *priv = serdev_device_get_drvdata(serdev);
> +	int _size = size;
> +
> +	if (size == 0)
> +		return 0;
> +
> +	dev_dbg(&priv->serdev->dev, "recv (len %d): %*ph ", size, 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_sl_handle_rx(priv, STATUS_CRIT);
> +			break;
> +		}
> +		priv->rx.type = buf[0];
> +		priv->rx.state = RX_START;
> +		size--;
> +		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 (size == 0)
> +			break;
> +
> +		if (priv->rx.type == 0xEE) {
> +			if (size > 1) {
> +				dev_err(&priv->serdev->dev, "EE pkt. Extra data received");
> +				status = STATUS_CRIT;
> +
> +			} else {
> +				status = (buf[0] == 1) ? STATUS_OK : STATUS_FAIL;
> +			}
> +			bno055_sl_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];
> +			size--;
> +			buf++;
> +		}
> +		fallthrough;
> +
> +	case RX_DATA:
> +		/* Header parsed; now receiving packet data payload */
> +		if (size == 0)
> +			break;
> +
> +		if (priv->rx.databuf_count + size > priv->rx.expected_len) {
> +			/*
> +			 * This is a 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_sl_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 + size <= priv->expected_data_len))
> +			memcpy(priv->response_buf + priv->rx.databuf_count,
> +			       buf, size);
> +		mutex_unlock(&priv->lock);
> +
> +		priv->rx.databuf_count += size;
> +
> +		/*
> +		 * 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_sl_handle_rx(priv, STATUS_OK);
> +			priv->rx.state = RX_IDLE;
> +		}
> +		break;
> +	}
> +
> +	return _size;
> +}
> +
> +static const struct serdev_device_ops bno055_sl_serdev_ops = {
> +	.receive_buf = bno055_sl_receive_buf,
> +	.write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static struct regmap_bus bno055_sl_regmap_bus = {
> +	.write = bno055_sl_write_reg,
> +	.read = bno055_sl_read_reg,
> +};
> +
> +static int bno055_sl_probe(struct serdev_device *serdev)
> +{
> +	struct bno055_sl_priv *priv;
> +	struct regmap *regmap;
> +	int ret;
> +	int irq = 0;
> +
> +	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_sl_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_sl_regmap_bus,
> +				  priv, &bno055_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&serdev->dev, "Unable to init register map");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	if (serdev->dev.of_node) {
If possible, use generic fw node functions from
linux/property.h rather than of specific ones. It 'might' be possible
to instantiate this from ACPI using the magic of PRP0001
(which uses the dt bindings from an entry in the DSDT table in ACPI
firmware).

> +		irq = of_irq_get(serdev->dev.of_node, 0);
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +		if (irq <= 0) {
> +			dev_info(&serdev->dev,
> +				 "Can't get IRQ resource (err %d)", irq);
Isn't there an explicit errno for when it fails to get it because it
isn't specified?  We want to catch that and error out on anything else.

Afterall if someone specified an IRQ that doesn't work, then they don't
want us to hid that fact.

> +			irq = 0;
> +		}
> +	}
> +
> +	return bno055_probe(&serdev->dev, regmap, irq,
> +			    BNO055_SL_XFER_BURST_BREAK_THRESHOLD);
> +}
> +
> +static const struct of_device_id bno055_sl_of_match[] = {
> +	{ .compatible = "bosch,bno055-serial" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, bno055_sl_of_match);
> +
> +static struct serdev_device_driver bno055_sl_driver = {
> +	.driver = {
> +		.name = BNO055_SL_DRIVER_NAME,
> +		.of_match_table = bno055_sl_of_match,
> +	},
> +	.probe = bno055_sl_probe,
> +};
> +module_serdev_device_driver(bno055_sl_driver);
> +
> +MODULE_AUTHOR("Andrea Merello <[e-mail]>");
> +MODULE_DESCRIPTION("Bosch BNO055 serdev interface");
> +MODULE_LICENSE("GPL v2");

Andrea Merello July 19, 2021, 8:49 a.m. UTC | #3

Il giorno sab 17 lug 2021 alle ore 17:48 Jonathan Cameron
<[e-mail]> ha scritto:
>
> On Thu, 15 Jul 2021 16:17:42 +0200
> Andrea Merello <[e-mail]> wrote:
>
> > This path adds a serdev driver for communicating to a BNO055 IMU
> > via serial bus, and enables the BNO055 core driver to work in this
> > scenario.
> >
> > Signed-off-by: Andrea Merello <[e-mail]>
> > Cc: Andrea Merello <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Matt Ranostay <[e-mail]>
> > Cc: Andy Shevchenko <[e-mail]>
> > Cc: Vlad Dogaru <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
>
> Hi Andrea,
>
> A few comments inline.
>
> Jonathan
>
> > ---
> >  drivers/iio/imu/bno055/Kconfig     |   5 +
> >  drivers/iio/imu/bno055/Makefile    |   1 +
> >  drivers/iio/imu/bno055/bno055_sl.c | 576 +++++++++++++++++++++++++++++
> >  3 files changed, 582 insertions(+)
> >  create mode 100644 drivers/iio/imu/bno055/bno055_sl.c
> >
> > diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig
> > index 2bfed8df4554..6d2e8c9f85b7 100644
> > --- a/drivers/iio/imu/bno055/Kconfig
> > +++ b/drivers/iio/imu/bno055/Kconfig
> > @@ -5,3 +5,8 @@
> >
> >  config BOSH_BNO055_IIO
> >       tristate
> > +
> > +config BOSH_BNO055_SERIAL
> > +     tristate "Bosh BNO055 attached via serial bus"
> > +     depends on SERIAL_DEV_BUS
> > +     select BOSH_BNO055_IIO
> > diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile
> > index 15c5ddf8d648..b704b10b6bd1 100644
> > --- a/drivers/iio/imu/bno055/Makefile
> > +++ b/drivers/iio/imu/bno055/Makefile
> > @@ -4,3 +4,4 @@
> >  #
> >
> >  obj-$(CONFIG_BOSH_BNO055_IIO) += bno055.o
> > +obj-$(CONFIG_BOSH_BNO055_SERIAL) += bno055_sl.o
> > diff --git a/drivers/iio/imu/bno055/bno055_sl.c b/drivers/iio/imu/bno055/bno055_sl.c
> > new file mode 100644
> > index 000000000000..9604d73d126c
> > --- /dev/null
> > +++ b/drivers/iio/imu/bno055/bno055_sl.c
> > @@ -0,0 +1,576 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Serial line interface for Bosh BNO055 IMU (via serdev).
> > + * This file implements serial communication up to the register read/write
> > + * level.
> > + *
> > + * Copyright (C) 2021 Istituto Italiano di Tecnologia
> > + * Electronic Design Laboratory
> > + * Written by Andrea Merello <[e-mail]>
> > + *
> > + * This driver is besed on
> > + *   Plantower PMS7003 particulate matter sensor driver
> > + *   Which is
> > + *   Copyright (c) Tomasz Duszynski <[e-mail]>
> > + */
> > +
> > +#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/of_irq.h>
> > +#include <linux/regmap.h>
> > +#include <linux/serdev.h>
> > +
> > +#include "bno055.h"
> > +
> > +#define BNO055_SL_DRIVER_NAME "bno055-sl"
> > +
> > +/*
> > + * Register writes cmd have the following format
> > + * +------+------+-----+-----+----- ... ----+
> > + * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
> > + * +------+------+-----+-----+----- ... ----+
> > + *
> > + * Register write responses have the following format
> > + * +------+----------+
> > + * | 0xEE | ERROCODE |
> > + * +------+----------+
> > + *
> > + * 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).
> > + *
> > + * BMU055 has been seen also failing to process commands in case we send them
> > + * too close each other (or if it is somehow busy?)
> > + *
> > + * One idea would be to split data in chunks, and then wait 1-2mS between
> > + * chunks (we hope not to exceed 30mS delay for any reason - which should
> > + * be pretty a lot of time for us), and eventually retry in case the BNO055
> > + * gets upset for any reason. This seems to work in avoiding the overflow
> > + * errors, but indeed it seems slower than just perform a retry when an overflow
> > + * error occur.
> > + * 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.
> > + *
> > + * So, we workaround all this in the following way:
> > + * In case of read we don't split the header but we rely on retries; This seems
> > + * convenient for data read (where we TX only the hdr).
> > + * For TX we split the transmission in 2-bytes chunks so that, we should not
> > + * only avoid case 2 (which is still manageable), but we also hopefully avoid
> > + * case 3, that would be by far worse.
>
> Nice docs and this sounds terrible!

Indeed.. If anyone has nicer ideas, or is aware about better
workaround, I would really love to know...

> > + */
> > +
> > +/* Read operation overhead:
> > + * 4 bytes req + 2byte resp hdr
> > + * 6 bytes = 60 bit (considering 1start + 1stop bits).
> > + * 60/115200 = ~520uS
> > + * In 520uS we could read back about 34 bytes that means 3 samples, this means
> > + * that in case of scattered read in which the gap is 3 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.
> > + * By experiment, it seems convenient to burst up to about 5/6-samples-long gap
> > + */
> > +
> > +#define BNO055_SL_XFER_BURST_BREAK_THRESHOLD 6
> > +
> > +struct bno055_sl_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 {
> > +             STATUS_OK = 0,  /* command OK */
> > +             STATUS_FAIL = 1,/* IMU communicated an error */
> > +             STATUS_CRIT = -1/* serial communication with IMU failed */
> > +     } cmd_status;
> > +     struct mutex lock;
> > +
> > +     /* Only accessed in behalf of 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;
> > +};
> > +
> > +static int bno055_sl_send_chunk(struct bno055_sl_priv *priv, u8 *data, int len)
> > +{
> > +     int ret;
> > +
> > +     dev_dbg(&priv->serdev->dev, "send (len: %d): %*ph", len, len, data);
> > +     ret = serdev_device_write(priv->serdev, data, len,
> > +                               msecs_to_jiffies(25));
> > +     if (ret < len)
> > +             return ret < 0 ? ret : -EIO;
>
> Break this up perhaps as will be easier to read.
>
>         if (ret < 0)
>                 return ret;
>
>         if (ret < len)
>                 return -EIO;
>
>         return 0;

OK

> > +     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_sl_do_send_cmd(struct bno055_sl_priv *priv,
> > +                              int read, int addr, int len, u8 *data)
> > +{
> > +     int ret;
> > +     int chunk_len;
> > +     u8 hdr[] = {0xAA, !!read, addr, len};
> > +
> > +     if (read) {
> > +             ret = bno055_sl_send_chunk(priv, hdr, 4);
> > +     } else {
> > +             ret = bno055_sl_send_chunk(priv, hdr, 2);
> > +             if (ret)
> > +                     goto fail;
> > +
> > +             usleep_range(2000, 3000);
> > +             ret = bno055_sl_send_chunk(priv, hdr + 2, 2);
> > +     }
> > +     if (ret)
> > +             goto fail;
> > +
> > +     if (data) {
> > +             while (len) {
> > +                     chunk_len = min(len, 2);
> > +                     usleep_range(2000, 3000);
> > +                     ret = bno055_sl_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 bno_sl_send_cmd(struct bno055_sl_priv *priv,
> > +                        int read, int addr, int len, u8 *data)
> > +{
> > +     const int retry_max = 5;
> > +     int retry = retry_max;
> > +     int ret = 0;
> > +
> > +     /*
> > +      * In case previous command was interrupted we still neet 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)
> > +                     goto exit;
> > +     }
> > +
> > +     /*
> > +      * 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.
> > +      */
> > +     while (retry--) {
> > +             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 - 1))
> > +                     dev_dbg(&priv->serdev->dev, "cmd retry: %d",
> > +                             retry_max - retry);
> > +             ret = bno055_sl_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;
> > +             } else if (!ret) {
> > +                     ret = -ETIMEDOUT;
> > +                     break;
> > +             }
> > +             ret = 0;
> > +
> > +             /*
> > +              * Poll if the IMU returns error (i.e busy), break if the IMU
> > +              * returns OK or if the serial communication broke
> > +              */
> > +             if (priv->cmd_status <= 0)
> > +                     break;
> > +     }
> > +
> > +exit:
> > +     if (ret)
> > +             return ret;
> > +     if (priv->cmd_status == STATUS_CRIT)
> > +             return -EIO;
> > +     if (priv->cmd_status == STATUS_FAIL)
> > +             return -EINVAL;
> > +     return 0;
> > +}
> > +
> > +static int bno055_sl_write_reg(void *context, const void *data, size_t count)
> > +{
> > +     int ret;
> > +     int reg;
> > +     u8 *write_data = (u8 *)data + 1;
> > +     struct bno055_sl_priv *priv = context;
> > +
> > +     if (count < 2) {
> > +             dev_err(&priv->serdev->dev, "Invalid write count %d", count);
> > +             return -EINVAL;
> > +     }
> > +
> > +     reg = ((u8 *)data)[0];
> > +     dev_dbg(&priv->serdev->dev, "wr reg 0x%x = 0x%x", reg, ((u8 *)data)[1]);
> > +     ret = bno_sl_send_cmd(priv, 0, reg, count - 1, write_data);
> > +
> > +     return ret;
> > +}
> > +
> > +static int bno055_sl_read_reg(void *context,
> > +                           const void *reg, size_t reg_size,
> > +                           void *val, size_t val_size)
> > +{
> > +     int ret;
> > +     int reg_addr;
> > +     struct bno055_sl_priv *priv = context;
> > +
> > +     if (reg_size != 1) {
>
> Can we plausibly hit this?  I would have though the regmap controls it
> and is set appropriately.  Hence safe to drop this check.

OK

> > +             dev_err(&priv->serdev->dev, "Invalid read regsize %d",
> > +                     reg_size);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (val_size > 128) {
> > +             dev_err(&priv->serdev->dev, "Invalid read valsize %d",
> > +                     val_size);
> > +             return -EINVAL;
> > +     }
> > +
> > +     reg_addr = ((u8 *)reg)[0];
> > +     dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
> > +     mutex_lock(&priv->lock);
> > +     priv->expected_data_len = val_size;
> > +     priv->response_buf = val;
> > +     mutex_unlock(&priv->lock);
> > +
> > +     ret = bno_sl_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_sl_handle_rx(struct bno055_sl_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;
> > +     complete(&priv->cmd_complete);
> > +     mutex_unlock(&priv->lock);
> > +}
> > +
> > +/*
> > + * Serdev receiver FSM. This tracks the serial communication and parse the
> > + * header. It pushes packets to bno055_sl_handle_rx(), eventually communicating
> > + * failures (i.e. malformed packets).
> > + * Idellay it doesn't know anything about upper layer (i.e. if this is the
>
> Ideally

Sure

>
> > + * 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_sl_receive_buf(struct serdev_device *serdev,
> > +                              const unsigned char *buf, size_t size)
> > +{
> > +     int status;
> > +     struct bno055_sl_priv *priv = serdev_device_get_drvdata(serdev);
> > +     int _size = size;
> > +
> > +     if (size == 0)
> > +             return 0;
> > +
> > +     dev_dbg(&priv->serdev->dev, "recv (len %d): %*ph ", size, 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_sl_handle_rx(priv, STATUS_CRIT);
> > +                     break;
> > +             }
> > +             priv->rx.type = buf[0];
> > +             priv->rx.state = RX_START;
> > +             size--;
> > +             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 (size == 0)
> > +                     break;
> > +
> > +             if (priv->rx.type == 0xEE) {
> > +                     if (size > 1) {
> > +                             dev_err(&priv->serdev->dev, "EE pkt. Extra data received");
> > +                             status = STATUS_CRIT;
> > +
> > +                     } else {
> > +                             status = (buf[0] == 1) ? STATUS_OK : STATUS_FAIL;
> > +                     }
> > +                     bno055_sl_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];
> > +                     size--;
> > +                     buf++;
> > +             }
> > +             fallthrough;
> > +
> > +     case RX_DATA:
> > +             /* Header parsed; now receiving packet data payload */
> > +             if (size == 0)
> > +                     break;
> > +
> > +             if (priv->rx.databuf_count + size > priv->rx.expected_len) {
> > +                     /*
> > +                      * This is a 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_sl_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 + size <= priv->expected_data_len))
> > +                     memcpy(priv->response_buf + priv->rx.databuf_count,
> > +                            buf, size);
> > +             mutex_unlock(&priv->lock);
> > +
> > +             priv->rx.databuf_count += size;
> > +
> > +             /*
> > +              * 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_sl_handle_rx(priv, STATUS_OK);
> > +                     priv->rx.state = RX_IDLE;
> > +             }
> > +             break;
> > +     }
> > +
> > +     return _size;
> > +}
> > +
> > +static const struct serdev_device_ops bno055_sl_serdev_ops = {
> > +     .receive_buf = bno055_sl_receive_buf,
> > +     .write_wakeup = serdev_device_write_wakeup,
> > +};
> > +
> > +static struct regmap_bus bno055_sl_regmap_bus = {
> > +     .write = bno055_sl_write_reg,
> > +     .read = bno055_sl_read_reg,
> > +};
> > +
> > +static int bno055_sl_probe(struct serdev_device *serdev)
> > +{
> > +     struct bno055_sl_priv *priv;
> > +     struct regmap *regmap;
> > +     int ret;
> > +     int irq = 0;
> > +
> > +     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_sl_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_sl_regmap_bus,
> > +                               priv, &bno055_regmap_config);
> > +     if (IS_ERR(regmap)) {
> > +             dev_err(&serdev->dev, "Unable to init register map");
> > +             return PTR_ERR(regmap);
> > +     }
> > +
> > +     if (serdev->dev.of_node) {
> If possible, use generic fw node functions from
> linux/property.h rather than of specific ones. It 'might' be possible
> to instantiate this from ACPI using the magic of PRP0001
> (which uses the dt bindings from an entry in the DSDT table in ACPI
> firmware).

OK

> > +             irq = of_irq_get(serdev->dev.of_node, 0);
> > +             if (irq == -EPROBE_DEFER)
> > +                     return irq;
> > +             if (irq <= 0) {
> > +                     dev_info(&serdev->dev,
> > +                              "Can't get IRQ resource (err %d)", irq);
> Isn't there an explicit errno for when it fails to get it because it
> isn't specified?  We want to catch that and error out on anything else.
>
> Afterall if someone specified an IRQ that doesn't work, then they don't
> want us to hid that fact.

Here is my mistake, sorry: I wrote code for handling data ready
interrupt, and I tried to hook it to a trigger in bno055.c.
Unfortunately I cannot get it working because the IMU firmware is too
old (and I really couldn't find a way to update it). So, since I
couldn't even test my code, I dropped interrupt handling/trigger code
for this series.. But I missed this chunk, that is actually a
leftover.. We don't use the IRQ at all (it could be used for event
like high G acceleration, but I have no support for them). So I would
just drop this

>
> > +                     irq = 0;
> > +             }
> > +     }
> > +
> > +     return bno055_probe(&serdev->dev, regmap, irq,
> > +                         BNO055_SL_XFER_BURST_BREAK_THRESHOLD);
> > +}
> > +
> > +static const struct of_device_id bno055_sl_of_match[] = {
> > +     { .compatible = "bosch,bno055-serial" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, bno055_sl_of_match);
> > +
> > +static struct serdev_device_driver bno055_sl_driver = {
> > +     .driver = {
> > +             .name = BNO055_SL_DRIVER_NAME,
> > +             .of_match_table = bno055_sl_of_match,
> > +     },
> > +     .probe = bno055_sl_probe,
> > +};
> > +module_serdev_device_driver(bno055_sl_driver);
> > +
> > +MODULE_AUTHOR("Andrea Merello <[e-mail]>");
> > +MODULE_DESCRIPTION("Bosch BNO055 serdev interface");
> > +MODULE_LICENSE("GPL v2");
>

Andy Shevchenko July 19, 2021, 11:55 a.m. UTC | #4

On Mon, Jul 19, 2021 at 10:49:54AM +0200, Andrea Merello wrote:
> Il giorno sab 17 lug 2021 alle ore 17:48 Jonathan Cameron
> <[e-mail]> ha scritto:
> > On Thu, 15 Jul 2021 16:17:42 +0200
> > Andrea Merello <[e-mail]> wrote:

...

> > > +/*
> > > + * Register writes cmd have the following format
> > > + * +------+------+-----+-----+----- ... ----+
> > > + * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
> > > + * +------+------+-----+-----+----- ... ----+
> > > + *
> > > + * Register write responses have the following format
> > > + * +------+----------+
> > > + * | 0xEE | ERROCODE |
> > > + * +------+----------+
> > > + *
> > > + * 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).
> > > + *
> > > + * BMU055 has been seen also failing to process commands in case we send them
> > > + * too close each other (or if it is somehow busy?)
> > > + *
> > > + * One idea would be to split data in chunks, and then wait 1-2mS between
> > > + * chunks (we hope not to exceed 30mS delay for any reason - which should
> > > + * be pretty a lot of time for us), and eventually retry in case the BNO055
> > > + * gets upset for any reason. This seems to work in avoiding the overflow
> > > + * errors, but indeed it seems slower than just perform a retry when an overflow
> > > + * error occur.
> > > + * 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.
> > > + *
> > > + * So, we workaround all this in the following way:
> > > + * In case of read we don't split the header but we rely on retries; This seems
> > > + * convenient for data read (where we TX only the hdr).
> > > + * For TX we split the transmission in 2-bytes chunks so that, we should not
> > > + * only avoid case 2 (which is still manageable), but we also hopefully avoid
> > > + * case 3, that would be by far worse.
> >
> > Nice docs and this sounds terrible!
> 
> Indeed.. If anyone has nicer ideas, or is aware about better
> workaround, I would really love to know...

This needs somebody to go thru data sheet and check for possibilities, what you
described above is not gonna fly. Okay, "in a robust way".

I can't believe there is nothing in the communication protocol that may
increase a robustness.

> > > + */

...

> > > +/* Read operation overhead:
> > > + * 4 bytes req + 2byte resp hdr
> > > + * 6 bytes = 60 bit (considering 1start + 1stop bits).
> > > + * 60/115200 = ~520uS
> > > + * In 520uS we could read back about 34 bytes that means 3 samples, this means
> > > + * that in case of scattered read in which the gap is 3 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.
> > > + * By experiment, it seems convenient to burst up to about 5/6-samples-long gap

Missed perial and entire comment needs proper style and space occupation ratio.

> > > + */

...

> > > +     enum {
> > > +             STATUS_OK = 0,  /* command OK */
> > > +             STATUS_FAIL = 1,/* IMU communicated an error */
> > > +             STATUS_CRIT = -1/* serial communication with IMU failed */

enum may be kernel doc described.

> > > +     } cmd_status;

...

> > > +static struct serdev_device_driver bno055_sl_driver = {
> > > +     .driver = {

> > > +             .name = BNO055_SL_DRIVER_NAME,

This is (semi-)ABI and preferably should be hard coded explicitly.

> > > +             .of_match_table = bno055_sl_of_match,
> > > +     },
> > > +     .probe = bno055_sl_probe,
> > > +};

Andrea Merello July 19, 2021, 12:59 p.m. UTC | #5

Il giorno lun 19 lug 2021 alle ore 13:56 Andy Shevchenko
<[e-mail]> ha scritto:
>
> On Mon, Jul 19, 2021 at 10:49:54AM +0200, Andrea Merello wrote:
> > Il giorno sab 17 lug 2021 alle ore 17:48 Jonathan Cameron
> > <[e-mail]> ha scritto:
> > > On Thu, 15 Jul 2021 16:17:42 +0200
> > > Andrea Merello <[e-mail]> wrote:
>
> ...
>
> > > > +/*
> > > > + * Register writes cmd have the following format
> > > > + * +------+------+-----+-----+----- ... ----+
> > > > + * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
> > > > + * +------+------+-----+-----+----- ... ----+
> > > > + *
> > > > + * Register write responses have the following format
> > > > + * +------+----------+
> > > > + * | 0xEE | ERROCODE |
> > > > + * +------+----------+
> > > > + *
> > > > + * 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).
> > > > + *
> > > > + * BMU055 has been seen also failing to process commands in case we send them
> > > > + * too close each other (or if it is somehow busy?)
> > > > + *
> > > > + * One idea would be to split data in chunks, and then wait 1-2mS between
> > > > + * chunks (we hope not to exceed 30mS delay for any reason - which should
> > > > + * be pretty a lot of time for us), and eventually retry in case the BNO055
> > > > + * gets upset for any reason. This seems to work in avoiding the overflow
> > > > + * errors, but indeed it seems slower than just perform a retry when an overflow
> > > > + * error occur.
> > > > + * 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.
> > > > + *
> > > > + * So, we workaround all this in the following way:
> > > > + * In case of read we don't split the header but we rely on retries; This seems
> > > > + * convenient for data read (where we TX only the hdr).
> > > > + * For TX we split the transmission in 2-bytes chunks so that, we should not
> > > > + * only avoid case 2 (which is still manageable), but we also hopefully avoid
> > > > + * case 3, that would be by far worse.
> > >
> > > Nice docs and this sounds terrible!
> >
> > Indeed.. If anyone has nicer ideas, or is aware about better
> > workaround, I would really love to know...
>
> This needs somebody to go thru data sheet and check for possibilities, what you
> described above is not gonna fly. Okay, "in a robust way".
>
> I can't believe there is nothing in the communication protocol that may
> increase a robustness.

The serial protocol is both described in the datasheet and in an
application note "BNO055UART interface". Both of them mention the fact
that  the IMU could just fail in processing the commands and responds
with a status message with the "overflow" error code. The application
note says this can happen because of an internal IMU buffer clearing
stuff not happening in time, and that you have to retry the command in
such case (which works for read commands, because after the header the
IMU will always respond with something).

They say nothing about the fact that the IMU could decide to respond
with an "overflow" status message when a write command is still being
TXed, even if it is not finished yet, but this actually happens (seen
at least after the 4-bytes header).

I think there is not much other information about this in datasheet
and application note. Besides, the message formats are also described
the comments in bno055_sl.c

Given that the IMU behaves like this, I could only see three possible
workarounds for managing write commands:
1 - be quick enough on RX side in catching the IMU overflow status
response before sending the next char, which seems unfeasible.
2 - be slow enough to let the IMU do its own stuff, which seems doable.
3 - let the mess happen and try to recover: when we get the IMU
overflow error then we might still being TXing; in this case we stop
and we wait for the IMU to complain for a malformed command, but I'm
unsure how the IMU could handle this: it will refuse the wrong
starting byte (unless our payload is 0xAA by chance), but then how
many bytes does it throw away? How may (malformed) commands would it
try to extract from a the garbage we TXed (how may complaints response
would we receive and need to wait) ? And what if there is something in
payload that resembles a valid command and got accepted? This seems
worse than workaround #2

What I meant: given this IMU behaviour, if someone has a better idea
about how to deal with it, I would listen :)

> > > > + */
>
> ...
>
> > > > +/* Read operation overhead:
> > > > + * 4 bytes req + 2byte resp hdr
> > > > + * 6 bytes = 60 bit (considering 1start + 1stop bits).
> > > > + * 60/115200 = ~520uS
> > > > + * In 520uS we could read back about 34 bytes that means 3 samples, this means
> > > > + * that in case of scattered read in which the gap is 3 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.
> > > > + * By experiment, it seems convenient to burst up to about 5/6-samples-long gap
>
> Missed perial and entire comment needs proper style and space occupation ratio.

Perial? But OK: text reflow and I see the 1st line for multilne
commend is not correct.

> > > > + */
>
> ...
>
> > > > +     enum {
> > > > +             STATUS_OK = 0,  /* command OK */
> > > > +             STATUS_FAIL = 1,/* IMU communicated an error */
> > > > +             STATUS_CRIT = -1/* serial communication with IMU failed */
>
> enum may be kernel doc described.

OK

> > > > +     } cmd_status;
>
> ...
>
> > > > +static struct serdev_device_driver bno055_sl_driver = {
> > > > +     .driver = {
>
> > > > +             .name = BNO055_SL_DRIVER_NAME,
>
> This is (semi-)ABI and preferably should be hard coded explicitly.

OK

> > > > +             .of_match_table = bno055_sl_of_match,
> > > > +     },
> > > > +     .probe = bno055_sl_probe,
> > > > +};
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Andy Shevchenko July 19, 2021, 2:15 p.m. UTC | #6

On Mon, Jul 19, 2021 at 02:59:30PM +0200, Andrea Merello wrote:
> Il giorno lun 19 lug 2021 alle ore 13:56 Andy Shevchenko
> <[e-mail]> ha scritto:
> >
> > On Mon, Jul 19, 2021 at 10:49:54AM +0200, Andrea Merello wrote:
> > > Il giorno sab 17 lug 2021 alle ore 17:48 Jonathan Cameron
> > > <[e-mail]> ha scritto:
> > > > On Thu, 15 Jul 2021 16:17:42 +0200
> > > > Andrea Merello <[e-mail]> wrote:

...

> > > > > +/*
> > > > > + * Register writes cmd have the following format
> > > > > + * +------+------+-----+-----+----- ... ----+
> > > > > + * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
> > > > > + * +------+------+-----+-----+----- ... ----+
> > > > > + *
> > > > > + * Register write responses have the following format
> > > > > + * +------+----------+
> > > > > + * | 0xEE | ERROCODE |
> > > > > + * +------+----------+
> > > > > + *
> > > > > + * 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).
> > > > > + *
> > > > > + * BMU055 has been seen also failing to process commands in case we send them
> > > > > + * too close each other (or if it is somehow busy?)
> > > > > + *
> > > > > + * One idea would be to split data in chunks, and then wait 1-2mS between
> > > > > + * chunks (we hope not to exceed 30mS delay for any reason - which should
> > > > > + * be pretty a lot of time for us), and eventually retry in case the BNO055
> > > > > + * gets upset for any reason. This seems to work in avoiding the overflow
> > > > > + * errors, but indeed it seems slower than just perform a retry when an overflow
> > > > > + * error occur.
> > > > > + * 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.
> > > > > + *
> > > > > + * So, we workaround all this in the following way:
> > > > > + * In case of read we don't split the header but we rely on retries; This seems
> > > > > + * convenient for data read (where we TX only the hdr).
> > > > > + * For TX we split the transmission in 2-bytes chunks so that, we should not
> > > > > + * only avoid case 2 (which is still manageable), but we also hopefully avoid
> > > > > + * case 3, that would be by far worse.
> > > >
> > > > Nice docs and this sounds terrible!
> > >
> > > Indeed.. If anyone has nicer ideas, or is aware about better
> > > workaround, I would really love to know...
> >
> > This needs somebody to go thru data sheet and check for possibilities, what you
> > described above is not gonna fly. Okay, "in a robust way".
> >
> > I can't believe there is nothing in the communication protocol that may
> > increase a robustness.
> 
> The serial protocol is both described in the datasheet and in an
> application note "BNO055UART interface". Both of them mention the fact
> that  the IMU could just fail in processing the commands and responds
> with a status message with the "overflow" error code. The application
> note says this can happen because of an internal IMU buffer clearing
> stuff not happening in time, and that you have to retry the command in
> such case (which works for read commands, because after the header the
> IMU will always respond with something).
> 
> They say nothing about the fact that the IMU could decide to respond
> with an "overflow" status message when a write command is still being
> TXed, even if it is not finished yet, but this actually happens (seen
> at least after the 4-bytes header).

(1)

> 
> I think there is not much other information about this in datasheet
> and application note. Besides, the message formats are also described
> the comments in bno055_sl.c
> 
> Given that the IMU behaves like this, I could only see three possible
> workarounds for managing write commands:
> 1 - be quick enough on RX side in catching the IMU overflow status
> response before sending the next char, which seems unfeasible.
> 2 - be slow enough to let the IMU do its own stuff, which seems doable.
> 3 - let the mess happen and try to recover: when we get the IMU
> overflow error then we might still being TXing; in this case we stop
> and we wait for the IMU to complain for a malformed command, but I'm
> unsure how the IMU could handle this: it will refuse the wrong
> starting byte (unless our payload is 0xAA by chance), but then how
> many bytes does it throw away? How may (malformed) commands would it
> try to extract from a the garbage we TXed (how may complaints response
> would we receive and need to wait) ? And what if there is something in
> payload that resembles a valid command and got accepted? This seems
> worse than workaround #2

I believe the #3 is the right thing to do actually.

The payload is up to 128 bytes and speed is fixed. I believe that firmware has
internally the state of the input processing. OTOH the UART is duplex and what
you need in the driver is to have a callback that will read whatever the answer
from the sensor will be at the time it appears.

Also note that Linux is not an RTOS and you may end up, maybe rarely, in the
case which resembles the #3 while using workaround #2.

On top of that you demolish the idea of using DMA with UART.

(Btw, AN012 [1] says 100ms is the write timeout for the next byte,
 and not 30ms.)

As AN012 rightfully pointed out the UART is _async_ interface, so I believe (1)
is covered by this, meaning that error respond may appear _at any time_ on the
(host-side) Rx line.

My personal take away is never ever use UART for this kind (*) of
communications and this sensor specifically.

*) time-based IPCs are doomed by definition in non-RTOS environments with UART
   hardware interface.

[1]: BST-BNO055-AN012-00 | Revision 1.0 | June 2015

> What I meant: given this IMU behaviour, if someone has a better idea
> about how to deal with it, I would listen :)
> 
> > > > > + */

...

> > > > > +/* Read operation overhead:
> > > > > + * 4 bytes req + 2byte resp hdr
> > > > > + * 6 bytes = 60 bit (considering 1start + 1stop bits).
> > > > > + * 60/115200 = ~520uS
> > > > > + * In 520uS we could read back about 34 bytes that means 3 samples, this means
> > > > > + * that in case of scattered read in which the gap is 3 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.
> > > > > + * By experiment, it seems convenient to burst up to about 5/6-samples-long gap
> >
> > Missed perial and entire comment needs proper style and space occupation ratio.
> 
> Perial? But OK: text reflow and I see the 1st line for multilne

Period.

> commend is not correct.
> 
> > > > > + */

Andrea Merello July 19, 2021, 3:07 p.m. UTC | #7

Il giorno lun 19 lug 2021 alle ore 16:15 Andy Shevchenko
<[e-mail]> ha scritto:
>
> On Mon, Jul 19, 2021 at 02:59:30PM +0200, Andrea Merello wrote:
> > Il giorno lun 19 lug 2021 alle ore 13:56 Andy Shevchenko
> > <[e-mail]> ha scritto:
> > >
> > > On Mon, Jul 19, 2021 at 10:49:54AM +0200, Andrea Merello wrote:
> > > > Il giorno sab 17 lug 2021 alle ore 17:48 Jonathan Cameron
> > > > <[e-mail]> ha scritto:
> > > > > On Thu, 15 Jul 2021 16:17:42 +0200
> > > > > Andrea Merello <[e-mail]> wrote:
>
> ...
>
> > > > > > +/*
> > > > > > + * Register writes cmd have the following format
> > > > > > + * +------+------+-----+-----+----- ... ----+
> > > > > > + * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
> > > > > > + * +------+------+-----+-----+----- ... ----+
> > > > > > + *
> > > > > > + * Register write responses have the following format
> > > > > > + * +------+----------+
> > > > > > + * | 0xEE | ERROCODE |
> > > > > > + * +------+----------+
> > > > > > + *
> > > > > > + * 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).
> > > > > > + *
> > > > > > + * BMU055 has been seen also failing to process commands in case we send them
> > > > > > + * too close each other (or if it is somehow busy?)
> > > > > > + *
> > > > > > + * One idea would be to split data in chunks, and then wait 1-2mS between
> > > > > > + * chunks (we hope not to exceed 30mS delay for any reason - which should
> > > > > > + * be pretty a lot of time for us), and eventually retry in case the BNO055
> > > > > > + * gets upset for any reason. This seems to work in avoiding the overflow
> > > > > > + * errors, but indeed it seems slower than just perform a retry when an overflow
> > > > > > + * error occur.
> > > > > > + * 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.
> > > > > > + *
> > > > > > + * So, we workaround all this in the following way:
> > > > > > + * In case of read we don't split the header but we rely on retries; This seems
> > > > > > + * convenient for data read (where we TX only the hdr).
> > > > > > + * For TX we split the transmission in 2-bytes chunks so that, we should not
> > > > > > + * only avoid case 2 (which is still manageable), but we also hopefully avoid
> > > > > > + * case 3, that would be by far worse.
> > > > >
> > > > > Nice docs and this sounds terrible!
> > > >
> > > > Indeed.. If anyone has nicer ideas, or is aware about better
> > > > workaround, I would really love to know...
> > >
> > > This needs somebody to go thru data sheet and check for possibilities, what you
> > > described above is not gonna fly. Okay, "in a robust way".
> > >
> > > I can't believe there is nothing in the communication protocol that may
> > > increase a robustness.
> >
> > The serial protocol is both described in the datasheet and in an
> > application note "BNO055UART interface". Both of them mention the fact
> > that  the IMU could just fail in processing the commands and responds
> > with a status message with the "overflow" error code. The application
> > note says this can happen because of an internal IMU buffer clearing
> > stuff not happening in time, and that you have to retry the command in
> > such case (which works for read commands, because after the header the
> > IMU will always respond with something).
> >
> > They say nothing about the fact that the IMU could decide to respond
> > with an "overflow" status message when a write command is still being
> > TXed, even if it is not finished yet, but this actually happens (seen
> > at least after the 4-bytes header).
>
> (1)
>
> >
> > I think there is not much other information about this in datasheet
> > and application note. Besides, the message formats are also described
> > the comments in bno055_sl.c
> >
> > Given that the IMU behaves like this, I could only see three possible
> > workarounds for managing write commands:
> > 1 - be quick enough on RX side in catching the IMU overflow status
> > response before sending the next char, which seems unfeasible.
> > 2 - be slow enough to let the IMU do its own stuff, which seems doable.
> > 3 - let the mess happen and try to recover: when we get the IMU
> > overflow error then we might still being TXing; in this case we stop
> > and we wait for the IMU to complain for a malformed command, but I'm
> > unsure how the IMU could handle this: it will refuse the wrong
> > starting byte (unless our payload is 0xAA by chance), but then how
> > many bytes does it throw away? How may (malformed) commands would it
> > try to extract from a the garbage we TXed (how may complaints response
> > would we receive and need to wait) ? And what if there is something in
> > payload that resembles a valid command and got accepted? This seems
> > worse than workaround #2
>
> I believe the #3 is the right thing to do actually.
>
> The payload is up to 128 bytes and speed is fixed. I believe that firmware has
> internally the state of the input processing. OTOH the UART is duplex and what
> you need in the driver is to have a callback that will read whatever the answer
> from the sensor will be at the time it appears.

I can add a reader thread that always process IMU data, but I doubt we
can make assumptions on the fact it will be triggered in time to avoid
to TX extra characters after IMU signals an overflow; this seems
really a RT stuff, and it would even depends by the USART HW (think
about USB-UART adaptors).

So, how could we implement #3 dealing correctly with the issue about
the IMU possiblly misinterpreting the bytes we might have already sent
out? What's if they are a valid command by chance? The problem here is
that this protocol has no any CRC or something like that to make sure
garbage is really thrown away.

> Also note that Linux is not an RTOS and you may end up, maybe rarely, in the
> case which resembles the #3 while using workaround #2.

Yes, in case we sleep more than 30mS we fail. I would say it's very
unlikely, but that's true.

Maybe if I add a reader thread to the workaround #2, considering we
sleep every two bytes, then we can know about IMU errors - maybe still
not before TXing any further data - but before we TX the bare minimum
data amount in order to produce an unintentionally valid write command
(should be 5 bytes).

Anyway it seems to me that there is no perfectly robust way to deal
with this IMU.. But, in real word scenarios, workaround #2 seem to
work well enough.

> On top of that you demolish the idea of using DMA with UART.

True, but we really have no rush for write commands, so that it
shouldn't be an issue indeed - and apart for the few bytes of the
calibration data, that we might discuss about, all other writes are so
short that DMA has probably no real advantage (OTOH read command
doesn't have any extra sleep, so we are fine with reading the IMU
measures).

> (Btw, AN012 [1] says 100ms is the write timeout for the next byte,
>  and not 30ms.)

Yes: there is an inconsistency here: the datasheet says 30mS, the AN
says 100mS. We should then consider not to exceed 30mS.

> As AN012 rightfully pointed out the UART is _async_ interface, so I believe (1)
> is covered by this, meaning that error respond may appear _at any time_ on the
> (host-side) Rx line.

Ah, that is an interesting interpretation :)

> My personal take away is never ever use UART for this kind (*) of
> communications and this sensor specifically.

Consider that this kind of devices are position dependant, e.g. you
want to fix them on your arms, or on your robot feet, etc. I2C is
electrically unsuitable for long off-board wires, while serial lines
should be by far better in this case.

> *) time-based IPCs are doomed by definition in non-RTOS environments with UART
>    hardware interface.

We have to live with this :/ But indeed having sensors that measure
real-world physical phenomena probably are a bit RT-ish anyway..

> [1]: BST-BNO055-AN012-00 | Revision 1.0 | June 2015
>
> > What I meant: given this IMU behaviour, if someone has a better idea
> > about how to deal with it, I would listen :)
> >
> > > > > > + */
>
> ...
>
> > > > > > +/* Read operation overhead:
> > > > > > + * 4 bytes req + 2byte resp hdr
> > > > > > + * 6 bytes = 60 bit (considering 1start + 1stop bits).
> > > > > > + * 60/115200 = ~520uS
> > > > > > + * In 520uS we could read back about 34 bytes that means 3 samples, this means
> > > > > > + * that in case of scattered read in which the gap is 3 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.
> > > > > > + * By experiment, it seems convenient to burst up to about 5/6-samples-long gap
> > >
> > > Missed perial and entire comment needs proper style and space occupation ratio.
> >
> > Perial? But OK: text reflow and I see the 1st line for multilne
>
> Period.

Ah, OK :)

> > commend is not correct.
> >
> > > > > > + */
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Patch

diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig
index 2bfed8df4554..6d2e8c9f85b7 100644
--- a/drivers/iio/imu/bno055/Kconfig
+++ b/drivers/iio/imu/bno055/Kconfig
@@ -5,3 +5,8 @@ 
 
 config BOSH_BNO055_IIO
 	tristate
+
+config BOSH_BNO055_SERIAL
+	tristate "Bosh BNO055 attached via serial bus"
+	depends on SERIAL_DEV_BUS
+	select BOSH_BNO055_IIO
diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile
index 15c5ddf8d648..b704b10b6bd1 100644
--- a/drivers/iio/imu/bno055/Makefile
+++ b/drivers/iio/imu/bno055/Makefile
@@ -4,3 +4,4 @@ 
 #
 
 obj-$(CONFIG_BOSH_BNO055_IIO) += bno055.o
+obj-$(CONFIG_BOSH_BNO055_SERIAL) += bno055_sl.o
diff --git a/drivers/iio/imu/bno055/bno055_sl.c b/drivers/iio/imu/bno055/bno055_sl.c
new file mode 100644
index 000000000000..9604d73d126c
--- /dev/null
+++ b/drivers/iio/imu/bno055/bno055_sl.c
@@ -0,0 +1,576 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Serial line interface for Bosh BNO055 IMU (via serdev).
+ * This file implements serial communication up to the register read/write
+ * level.
+ *
+ * Copyright (C) 2021 Istituto Italiano di Tecnologia
+ * Electronic Design Laboratory
+ * Written by Andrea Merello <[e-mail]>
+ *
+ * This driver is besed on
+ *	Plantower PMS7003 particulate matter sensor driver
+ *	Which is
+ *	Copyright (c) Tomasz Duszynski <[e-mail]>
+ */
+
+#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/of_irq.h>
+#include <linux/regmap.h>
+#include <linux/serdev.h>
+
+#include "bno055.h"
+
+#define BNO055_SL_DRIVER_NAME "bno055-sl"
+
+/*
+ * Register writes cmd have the following format
+ * +------+------+-----+-----+----- ... ----+
+ * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
+ * +------+------+-----+-----+----- ... ----+
+ *
+ * Register write responses have the following format
+ * +------+----------+
+ * | 0xEE | ERROCODE |
+ * +------+----------+
+ *
+ * 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).
+ *
+ * BMU055 has been seen also failing to process commands in case we send them
+ * too close each other (or if it is somehow busy?)
+ *
+ * One idea would be to split data in chunks, and then wait 1-2mS between
+ * chunks (we hope not to exceed 30mS delay for any reason - which should
+ * be pretty a lot of time for us), and eventually retry in case the BNO055
+ * gets upset for any reason. This seems to work in avoiding the overflow
+ * errors, but indeed it seems slower than just perform a retry when an overflow
+ * error occur.
+ * 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.
+ *
+ * So, we workaround all this in the following way:
+ * In case of read we don't split the header but we rely on retries; This seems
+ * convenient for data read (where we TX only the hdr).
+ * For TX we split the transmission in 2-bytes chunks so that, we should not
+ * only avoid case 2 (which is still manageable), but we also hopefully avoid
+ * case 3, that would be by far worse.
+ */
+
+/* Read operation overhead:
+ * 4 bytes req + 2byte resp hdr
+ * 6 bytes = 60 bit (considering 1start + 1stop bits).
+ * 60/115200 = ~520uS
+ * In 520uS we could read back about 34 bytes that means 3 samples, this means
+ * that in case of scattered read in which the gap is 3 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.
+ * By experiment, it seems convenient to burst up to about 5/6-samples-long gap
+ */
+
+#define BNO055_SL_XFER_BURST_BREAK_THRESHOLD 6
+
+struct bno055_sl_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 {
+		STATUS_OK = 0,  /* command OK */
+		STATUS_FAIL = 1,/* IMU communicated an error */
+		STATUS_CRIT = -1/* serial communication with IMU failed */
+	} cmd_status;
+	struct mutex lock;
+
+	/* Only accessed in behalf of 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;
+};
+
+static int bno055_sl_send_chunk(struct bno055_sl_priv *priv, u8 *data, int len)
+{
+	int ret;
+
+	dev_dbg(&priv->serdev->dev, "send (len: %d): %*ph", len, len, data);
+	ret = serdev_device_write(priv->serdev, data, len,
+				  msecs_to_jiffies(25));
+	if (ret < len)
+		return ret < 0 ? ret : -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_sl_do_send_cmd(struct bno055_sl_priv *priv,
+				 int read, int addr, int len, u8 *data)
+{
+	int ret;
+	int chunk_len;
+	u8 hdr[] = {0xAA, !!read, addr, len};
+
+	if (read) {
+		ret = bno055_sl_send_chunk(priv, hdr, 4);
+	} else {
+		ret = bno055_sl_send_chunk(priv, hdr, 2);
+		if (ret)
+			goto fail;
+
+		usleep_range(2000, 3000);
+		ret = bno055_sl_send_chunk(priv, hdr + 2, 2);
+	}
+	if (ret)
+		goto fail;
+
+	if (data) {
+		while (len) {
+			chunk_len = min(len, 2);
+			usleep_range(2000, 3000);
+			ret = bno055_sl_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 bno_sl_send_cmd(struct bno055_sl_priv *priv,
+			   int read, int addr, int len, u8 *data)
+{
+	const int retry_max = 5;
+	int retry = retry_max;
+	int ret = 0;
+
+	/*
+	 * In case previous command was interrupted we still neet 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)
+			goto exit;
+	}
+
+	/*
+	 * 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.
+	 */
+	while (retry--) {
+		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 - 1))
+			dev_dbg(&priv->serdev->dev, "cmd retry: %d",
+				retry_max - retry);
+		ret = bno055_sl_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;
+		} else if (!ret) {
+			ret = -ETIMEDOUT;
+			break;
+		}
+		ret = 0;
+
+		/*
+		 * Poll if the IMU returns error (i.e busy), break if the IMU
+		 * returns OK or if the serial communication broke
+		 */
+		if (priv->cmd_status <= 0)
+			break;
+	}
+
+exit:
+	if (ret)
+		return ret;
+	if (priv->cmd_status == STATUS_CRIT)
+		return -EIO;
+	if (priv->cmd_status == STATUS_FAIL)
+		return -EINVAL;
+	return 0;
+}
+
+static int bno055_sl_write_reg(void *context, const void *data, size_t count)
+{
+	int ret;
+	int reg;
+	u8 *write_data = (u8 *)data + 1;
+	struct bno055_sl_priv *priv = context;
+
+	if (count < 2) {
+		dev_err(&priv->serdev->dev, "Invalid write count %d", count);
+		return -EINVAL;
+	}
+
+	reg = ((u8 *)data)[0];
+	dev_dbg(&priv->serdev->dev, "wr reg 0x%x = 0x%x", reg, ((u8 *)data)[1]);
+	ret = bno_sl_send_cmd(priv, 0, reg, count - 1, write_data);
+
+	return ret;
+}
+
+static int bno055_sl_read_reg(void *context,
+			      const void *reg, size_t reg_size,
+			      void *val, size_t val_size)
+{
+	int ret;
+	int reg_addr;
+	struct bno055_sl_priv *priv = context;
+
+	if (reg_size != 1) {
+		dev_err(&priv->serdev->dev, "Invalid read regsize %d",
+			reg_size);
+		return -EINVAL;
+	}
+
+	if (val_size > 128) {
+		dev_err(&priv->serdev->dev, "Invalid read valsize %d",
+			val_size);
+		return -EINVAL;
+	}
+
+	reg_addr = ((u8 *)reg)[0];
+	dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
+	mutex_lock(&priv->lock);
+	priv->expected_data_len = val_size;
+	priv->response_buf = val;
+	mutex_unlock(&priv->lock);
+
+	ret = bno_sl_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_sl_handle_rx(struct bno055_sl_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;
+	complete(&priv->cmd_complete);
+	mutex_unlock(&priv->lock);
+}
+
+/*
+ * Serdev receiver FSM. This tracks the serial communication and parse the
+ * header. It pushes packets to bno055_sl_handle_rx(), eventually communicating
+ * failures (i.e. malformed packets).
+ * Idellay 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_sl_receive_buf(struct serdev_device *serdev,
+				 const unsigned char *buf, size_t size)
+{
+	int status;
+	struct bno055_sl_priv *priv = serdev_device_get_drvdata(serdev);
+	int _size = size;
+
+	if (size == 0)
+		return 0;
+
+	dev_dbg(&priv->serdev->dev, "recv (len %d): %*ph ", size, 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_sl_handle_rx(priv, STATUS_CRIT);
+			break;
+		}
+		priv->rx.type = buf[0];
+		priv->rx.state = RX_START;
+		size--;
+		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 (size == 0)
+			break;
+
+		if (priv->rx.type == 0xEE) {
+			if (size > 1) {
+				dev_err(&priv->serdev->dev, "EE pkt. Extra data received");
+				status = STATUS_CRIT;
+
+			} else {
+				status = (buf[0] == 1) ? STATUS_OK : STATUS_FAIL;
+			}
+			bno055_sl_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];
+			size--;
+			buf++;
+		}
+		fallthrough;
+
+	case RX_DATA:
+		/* Header parsed; now receiving packet data payload */
+		if (size == 0)
+			break;
+
+		if (priv->rx.databuf_count + size > priv->rx.expected_len) {
+			/*
+			 * This is a 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_sl_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 + size <= priv->expected_data_len))
+			memcpy(priv->response_buf + priv->rx.databuf_count,
+			       buf, size);
+		mutex_unlock(&priv->lock);
+
+		priv->rx.databuf_count += size;
+
+		/*
+		 * 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_sl_handle_rx(priv, STATUS_OK);
+			priv->rx.state = RX_IDLE;
+		}
+		break;
+	}
+
+	return _size;
+}
+
+static const struct serdev_device_ops bno055_sl_serdev_ops = {
+	.receive_buf = bno055_sl_receive_buf,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static struct regmap_bus bno055_sl_regmap_bus = {
+	.write = bno055_sl_write_reg,
+	.read = bno055_sl_read_reg,
+};
+
+static int bno055_sl_probe(struct serdev_device *serdev)
+{
+	struct bno055_sl_priv *priv;
+	struct regmap *regmap;
+	int ret;
+	int irq = 0;
+
+	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_sl_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_sl_regmap_bus,
+				  priv, &bno055_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&serdev->dev, "Unable to init register map");
+		return PTR_ERR(regmap);
+	}
+
+	if (serdev->dev.of_node) {
+		irq = of_irq_get(serdev->dev.of_node, 0);
+		if (irq == -EPROBE_DEFER)
+			return irq;
+		if (irq <= 0) {
+			dev_info(&serdev->dev,
+				 "Can't get IRQ resource (err %d)", irq);
+			irq = 0;
+		}
+	}
+
+	return bno055_probe(&serdev->dev, regmap, irq,
+			    BNO055_SL_XFER_BURST_BREAK_THRESHOLD);
+}
+
+static const struct of_device_id bno055_sl_of_match[] = {
+	{ .compatible = "bosch,bno055-serial" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bno055_sl_of_match);
+
+static struct serdev_device_driver bno055_sl_driver = {
+	.driver = {
+		.name = BNO055_SL_DRIVER_NAME,
+		.of_match_table = bno055_sl_of_match,
+	},
+	.probe = bno055_sl_probe,
+};
+module_serdev_device_driver(bno055_sl_driver);
+
+MODULE_AUTHOR("Andrea Merello <[e-mail]>");
+MODULE_DESCRIPTION("Bosch BNO055 serdev interface");
+MODULE_LICENSE("GPL v2");