diff mbox series

[v3,11/13] iio: imu: add BNO055 serdev driver

Message ID 20220217162710.33615-12-andrea.merello@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for Bosch BNO055 IMU | expand

Commit Message

Andrea Merello Feb. 17, 2022, 4:27 p.m. UTC
This path adds a serdev driver for communicating to a BNO055 IMU via
serial bus, and it enables the BNO055 core driver to work in this
scenario.

Signed-off-by: Andrea Merello <andrea.merello@iit.it>
---
 drivers/iio/imu/bno055/Kconfig     |  10 +
 drivers/iio/imu/bno055/Makefile    |   1 +
 drivers/iio/imu/bno055/bno055_sl.c | 557 +++++++++++++++++++++++++++++
 3 files changed, 568 insertions(+)
 create mode 100644 drivers/iio/imu/bno055/bno055_sl.c

Comments

kernel test robot Feb. 17, 2022, 7:47 p.m. UTC | #1
Hi Andrea,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linux/master linus/master v5.17-rc4 next-20220217]
[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/20220218-002932
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220218/202202180353.lRcys4tc-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.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/98d7db4486f0404718da9e85ae13da54d757104b
        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/20220218-002932
        git checkout 98d7db4486f0404718da9e85ae13da54d757104b
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/iio/imu/bno055/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/iio/imu/bno055/bno055.c:285:5: warning: no previous prototype for 'bno055_calibration_load' [-Wmissing-prototypes]
     285 | int bno055_calibration_load(struct bno055_priv *priv, const u8 *data, int len)
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/printk.h:555,
                    from include/linux/kernel.h:29,
                    from include/linux/clk.h:13,
                    from drivers/iio/imu/bno055/bno055.c:18:
   drivers/iio/imu/bno055/bno055.c: In function 'bno055_calibration_load':
>> drivers/iio/imu/bno055/bno055.c:288:36: warning: format '%zu' expects argument of type 'size_t', but argument 4 has type 'int' [-Wformat=]
     288 |                 dev_dbg(priv->dev, "Invalid calibration file size %zu (expected %d)",
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
     166 |         _dynamic_func_call(fmt,__dynamic_dev_dbg,               \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                              ^~~~~~~
   drivers/iio/imu/bno055/bno055.c:288:17: note: in expansion of macro 'dev_dbg'
     288 |                 dev_dbg(priv->dev, "Invalid calibration file size %zu (expected %d)",
         |                 ^~~~~~~
   drivers/iio/imu/bno055/bno055.c:288:69: note: format string is defined here
     288 |                 dev_dbg(priv->dev, "Invalid calibration file size %zu (expected %d)",
         |                                                                   ~~^
         |                                                                     |
         |                                                                     long unsigned int
         |                                                                   %u
   drivers/iio/imu/bno055/bno055.c: At top level:
>> drivers/iio/imu/bno055/bno055.c:748:5: warning: no previous prototype for 'bno055_sysfs_attr_avail' [-Wmissing-prototypes]
     748 | int bno055_sysfs_attr_avail(struct bno055_priv *priv, struct bno055_sysfs_attr *attr,
         |     ^~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/imu/bno055/bno055.c:1203:5: warning: no previous prototype for 'bno055_debugfs_reg_access' [-Wmissing-prototypes]
    1203 | int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/device.h:15,
                    from drivers/iio/imu/bno055/bno055_sl.c:18:
   drivers/iio/imu/bno055/bno055_sl.c: In function 'bno055_sl_read_reg':
>> drivers/iio/imu/bno055/bno055_sl.c:305:45: warning: format '%d' expects argument of type 'int', but argument 3 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     305 |                 dev_err(&priv->serdev->dev, "Invalid read valsize %d",
         |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                              ^~~
   include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt'
     144 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                        ^~~~~~~
   drivers/iio/imu/bno055/bno055_sl.c:305:17: note: in expansion of macro 'dev_err'
     305 |                 dev_err(&priv->serdev->dev, "Invalid read valsize %d",
         |                 ^~~~~~~
   drivers/iio/imu/bno055/bno055_sl.c:305:68: note: format string is defined here
     305 |                 dev_err(&priv->serdev->dev, "Invalid read valsize %d",
         |                                                                   ~^
         |                                                                    |
         |                                                                    int
         |                                                                   %ld
   In file included from include/linux/printk.h:555,
                    from include/asm-generic/bug.h:22,
                    from arch/alpha/include/asm/bug.h:23,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/alpha/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:55,
                    from include/linux/swait.h:7,
                    from include/linux/completion.h:12,
                    from drivers/iio/imu/bno055/bno055_sl.c:17:
   drivers/iio/imu/bno055/bno055_sl.c:311:37: warning: format '%d' expects argument of type 'int', but argument 5 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     311 |         dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
         |                                     ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
     166 |         _dynamic_func_call(fmt,__dynamic_dev_dbg,               \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                              ^~~~~~~
   drivers/iio/imu/bno055/bno055_sl.c:311:9: note: in expansion of macro 'dev_dbg'
     311 |         dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
         |         ^~~~~~~
   drivers/iio/imu/bno055/bno055_sl.c:311:56: note: format string is defined here
     311 |         dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
         |                                                       ~^
         |                                                        |
         |                                                        int
         |                                                       %ld
   In file included from include/linux/printk.h:555,
                    from include/asm-generic/bug.h:22,
                    from arch/alpha/include/asm/bug.h:23,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:13,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/alpha/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:78,
                    from include/linux/spinlock.h:55,
                    from include/linux/swait.h:7,
                    from include/linux/completion.h:12,
                    from drivers/iio/imu/bno055/bno055_sl.c:17:
   drivers/iio/imu/bno055/bno055_sl.c: In function 'bno055_sl_receive_buf':
>> drivers/iio/imu/bno055/bno055_sl.c:387:37: warning: field width specifier '*' expects argument of type 'int', but argument 5 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
     387 |         dev_dbg(&priv->serdev->dev, "recv (len %zu): %*ph ", size, size, buf);
         |                                     ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:134:29: note: in definition of macro '__dynamic_func_call'
     134 |                 func(&id, ##__VA_ARGS__);               \
         |                             ^~~~~~~~~~~
   include/linux/dynamic_debug.h:166:9: note: in expansion of macro '_dynamic_func_call'
     166 |         _dynamic_func_call(fmt,__dynamic_dev_dbg,               \
         |         ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:9: note: in expansion of macro 'dynamic_dev_dbg'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:155:30: note: in expansion of macro 'dev_fmt'
     155 |         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                              ^~~~~~~
   drivers/iio/imu/bno055/bno055_sl.c:387:9: note: in expansion of macro 'dev_dbg'
     387 |         dev_dbg(&priv->serdev->dev, "recv (len %zu): %*ph ", size, size, buf);
         |         ^~~~~~~
   drivers/iio/imu/bno055/bno055_sl.c:387:55: note: format string is defined here
     387 |         dev_dbg(&priv->serdev->dev, "recv (len %zu): %*ph ", size, size, buf);
         |                                                      ~^~
         |                                                       |
         |                                                       int


vim +305 drivers/iio/imu/bno055/bno055_sl.c

   295	
   296	static int bno055_sl_read_reg(void *context,
   297				      const void *reg, size_t reg_size,
   298				      void *val, size_t val_size)
   299	{
   300		int ret;
   301		int reg_addr;
   302		struct bno055_sl_priv *priv = context;
   303	
   304		if (val_size > 128) {
 > 305			dev_err(&priv->serdev->dev, "Invalid read valsize %d",
   306				val_size);
   307			return -EINVAL;
   308		}
   309	
   310		reg_addr = ((u8 *)reg)[0];
   311		dev_dbg(&priv->serdev->dev, "rd reg 0x%x (len %d)", reg_addr, val_size);
   312		mutex_lock(&priv->lock);
   313		priv->expected_data_len = val_size;
   314		priv->response_buf = val;
   315		mutex_unlock(&priv->lock);
   316	
   317		ret = bno_sl_send_cmd(priv, 1, reg_addr, val_size, NULL);
   318	
   319		mutex_lock(&priv->lock);
   320		priv->response_buf = NULL;
   321		mutex_unlock(&priv->lock);
   322	
   323		return ret;
   324	}
   325	
   326	/*
   327	 * Handler for received data; this is called from the reicever callback whenever
   328	 * it got some packet from the serial bus. The status tell us whether the
   329	 * packet is valid (i.e. header ok && received payload len consistent wrt the
   330	 * header). It's now our responsability to check whether this is what we
   331	 * expected, of whether we got some unexpected, yet valid, packet.
   332	 */
   333	static void bno055_sl_handle_rx(struct bno055_sl_priv *priv, int status)
   334	{
   335		mutex_lock(&priv->lock);
   336		switch (priv->expect_response) {
   337		case CMD_NONE:
   338			dev_warn(&priv->serdev->dev, "received unexpected, yet valid, data from sensor");
   339			mutex_unlock(&priv->lock);
   340			return;
   341	
   342		case CMD_READ:
   343			priv->cmd_status = status;
   344			if (status == STATUS_OK &&
   345			    priv->rx.databuf_count != priv->expected_data_len) {
   346				/*
   347				 * If we got here, then the lower layer serial protocol
   348				 * seems consistent with itself; if we got an unexpected
   349				 * amount of data then signal it as a non critical error
   350				 */
   351				priv->cmd_status = STATUS_FAIL;
   352				dev_warn(&priv->serdev->dev, "received an unexpected amount of, yet valid, data from sensor");
   353			}
   354			break;
   355	
   356		case CMD_WRITE:
   357			priv->cmd_status = status;
   358			break;
   359		}
   360	
   361		priv->expect_response = CMD_NONE;
   362		complete(&priv->cmd_complete);
   363		mutex_unlock(&priv->lock);
   364	}
   365	
   366	/*
   367	 * Serdev receiver FSM. This tracks the serial communication and parse the
   368	 * header. It pushes packets to bno055_sl_handle_rx(), eventually communicating
   369	 * failures (i.e. malformed packets).
   370	 * Ideally it doesn't know anything about upper layer (i.e. if this is the
   371	 * packet we were really expecting), but since we copies the payload into the
   372	 * receiver buffer (that is not valid when i.e. we don't expect data), we
   373	 * snoop a bit in the upper layer..
   374	 * Also, we assume to RX one pkt per time (i.e. the HW doesn't send anything
   375	 * unless we require to AND we don't queue more than one request per time).
   376	 */
   377	static int bno055_sl_receive_buf(struct serdev_device *serdev,
   378					 const unsigned char *buf, size_t size)
   379	{
   380		int status;
   381		struct bno055_sl_priv *priv = serdev_device_get_drvdata(serdev);
   382		int remaining = size;
   383	
   384		if (size == 0)
   385			return 0;
   386	
 > 387		dev_dbg(&priv->serdev->dev, "recv (len %zu): %*ph ", size, size, buf);
   388		switch (priv->rx.state) {
   389		case RX_IDLE:
   390			/*
   391			 * New packet.
   392			 * Check for its 1st byte, that identifies the pkt type.
   393			 */
   394			if (buf[0] != 0xEE && buf[0] != 0xBB) {
   395				dev_err(&priv->serdev->dev,
   396					"Invalid packet start %x", buf[0]);
   397				bno055_sl_handle_rx(priv, STATUS_CRIT);
   398				break;
   399			}
   400			priv->rx.type = buf[0];
   401			priv->rx.state = RX_START;
   402			remaining--;
   403			buf++;
   404			priv->rx.databuf_count = 0;
   405			fallthrough;
   406	
   407		case RX_START:
   408			/*
   409			 * Packet RX in progress, we expect either 1-byte len or 1-byte
   410			 * status depending by the packet type.
   411			 */
   412			if (remaining == 0)
   413				break;
   414	
   415			if (priv->rx.type == 0xEE) {
   416				if (remaining > 1) {
   417					dev_err(&priv->serdev->dev, "EE pkt. Extra data received");
   418					status = STATUS_CRIT;
   419	
   420				} else {
   421					status = (buf[0] == 1) ? STATUS_OK : STATUS_FAIL;
   422				}
   423				bno055_sl_handle_rx(priv, status);
   424				priv->rx.state = RX_IDLE;
   425				break;
   426	
   427			} else {
   428				/*priv->rx.type == 0xBB */
   429				priv->rx.state = RX_DATA;
   430				priv->rx.expected_len = buf[0];
   431				remaining--;
   432				buf++;
   433			}
   434			fallthrough;
   435	
   436		case RX_DATA:
   437			/* Header parsed; now receiving packet data payload */
   438			if (remaining == 0)
   439				break;
   440	
   441			if (priv->rx.databuf_count + remaining > priv->rx.expected_len) {
   442				/*
   443				 * This is a inconsistency in serial protocol, we lost
   444				 * sync and we don't know how to handle further data
   445				 */
   446				dev_err(&priv->serdev->dev, "BB pkt. Extra data received");
   447				bno055_sl_handle_rx(priv, STATUS_CRIT);
   448				priv->rx.state = RX_IDLE;
   449				break;
   450			}
   451	
   452			mutex_lock(&priv->lock);
   453			/*
   454			 * NULL e.g. when read cmd is stale or when no read cmd is
   455			 * actually pending.
   456			 */
   457			if (priv->response_buf &&
   458			    /*
   459			     * Snoop on the upper layer protocol stuff to make sure not
   460			     * to write to an invalid memory. Apart for this, let's the
   461			     * upper layer manage any inconsistency wrt expected data
   462			     * len (as long as the serial protocol is consistent wrt
   463			     * itself (i.e. response header is consistent with received
   464			     * response len.
   465			     */
   466			    (priv->rx.databuf_count + remaining <= priv->expected_data_len))
   467				memcpy(priv->response_buf + priv->rx.databuf_count,
   468				       buf, remaining);
   469			mutex_unlock(&priv->lock);
   470	
   471			priv->rx.databuf_count += remaining;
   472	
   473			/*
   474			 * Reached expected len advertised by the IMU for the current
   475			 * packet. Pass it to the upper layer (for us it is just valid).
   476			 */
   477			if (priv->rx.databuf_count == priv->rx.expected_len) {
   478				bno055_sl_handle_rx(priv, STATUS_OK);
   479				priv->rx.state = RX_IDLE;
   480			}
   481			break;
   482		}
   483	
   484		return size;
   485	}
   486	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 18, 2022, 5:20 a.m. UTC | #2
Hi Andrea,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linux/master linus/master v5.17-rc4 next-20220217]
[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/20220218-002932
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220218/202202181307.Lb4f99qg-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/98d7db4486f0404718da9e85ae13da54d757104b
        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/20220218-002932
        git checkout 98d7db4486f0404718da9e85ae13da54d757104b
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/iio/imu/bno055/bno055.c:285:5: warning: no previous prototype for 'bno055_calibration_load' [-Wmissing-prototypes]
     285 | int bno055_calibration_load(struct bno055_priv *priv, const u8 *data, int len)
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/imu/bno055/bno055.c:748:5: warning: no previous prototype for 'bno055_sysfs_attr_avail' [-Wmissing-prototypes]
     748 | int bno055_sysfs_attr_avail(struct bno055_priv *priv, struct bno055_sysfs_attr *attr,
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/imu/bno055/bno055.c:1203:5: warning: no previous prototype for 'bno055_debugfs_reg_access' [-Wmissing-prototypes]
    1203 | int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/imu/bno055/bno055.c: In function 'bno055_show_fw_version':
>> drivers/iio/imu/bno055/bno055.c:1235:2: error: implicit declaration of function 'kfree'; did you mean 'vfree'? [-Werror=implicit-function-declaration]
    1235 |  kfree(buf);
         |  ^~~~~
         |  vfree
   cc1: some warnings being treated as errors


vim +1235 drivers/iio/imu/bno055/bno055.c

16cbcd24402827b Andrea Merello 2022-02-17  1213  
16cbcd24402827b Andrea Merello 2022-02-17  1214  static ssize_t bno055_show_fw_version(struct file *file, char __user *userbuf,
16cbcd24402827b Andrea Merello 2022-02-17  1215  				      size_t count, loff_t *ppos)
16cbcd24402827b Andrea Merello 2022-02-17  1216  {
16cbcd24402827b Andrea Merello 2022-02-17  1217  	struct bno055_priv *priv = file->private_data;
16cbcd24402827b Andrea Merello 2022-02-17  1218  	int rev, ver;
16cbcd24402827b Andrea Merello 2022-02-17  1219  	char *buf;
16cbcd24402827b Andrea Merello 2022-02-17  1220  	int ret;
16cbcd24402827b Andrea Merello 2022-02-17  1221  
16cbcd24402827b Andrea Merello 2022-02-17  1222  	ret = regmap_read(priv->regmap, BNO055_SW_REV_LSB_REG, &rev);
16cbcd24402827b Andrea Merello 2022-02-17  1223  	if (ret)
16cbcd24402827b Andrea Merello 2022-02-17  1224  		return ret;
16cbcd24402827b Andrea Merello 2022-02-17  1225  
16cbcd24402827b Andrea Merello 2022-02-17  1226  	ret = regmap_read(priv->regmap, BNO055_SW_REV_MSB_REG, &ver);
16cbcd24402827b Andrea Merello 2022-02-17  1227  	if (ret)
16cbcd24402827b Andrea Merello 2022-02-17  1228  		return ret;
16cbcd24402827b Andrea Merello 2022-02-17  1229  
16cbcd24402827b Andrea Merello 2022-02-17  1230  	buf = kasprintf(GFP_KERNEL, "ver: 0x%x, rev: 0x%x\n", ver, rev);
16cbcd24402827b Andrea Merello 2022-02-17  1231  	if (!buf)
16cbcd24402827b Andrea Merello 2022-02-17  1232  		return -ENOMEM;
16cbcd24402827b Andrea Merello 2022-02-17  1233  
16cbcd24402827b Andrea Merello 2022-02-17  1234  	ret = simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
16cbcd24402827b Andrea Merello 2022-02-17 @1235  	kfree(buf);
16cbcd24402827b Andrea Merello 2022-02-17  1236  
16cbcd24402827b Andrea Merello 2022-02-17  1237  	return ret;
16cbcd24402827b Andrea Merello 2022-02-17  1238  }
16cbcd24402827b Andrea Merello 2022-02-17  1239  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Andy Shevchenko Feb. 21, 2022, 8:27 p.m. UTC | #3
On Thu, Feb 17, 2022 at 5:27 PM Andrea Merello <andrea.merello@gmail.com> wrote:
>
> This path adds a serdev driver for communicating to a BNO055 IMU via
> serial bus, and it enables the BNO055 core driver to work in this
> scenario.

>  drivers/iio/imu/bno055/bno055_sl.c | 557 +++++++++++++++++++++++++++++

Can we use the suffix _ser instead of _sl?

...

> +config BOSCH_BNO055_SERIAL
> +       tristate "Bosch BNO055 attached via serial bus"

Serial is too broad, it can cover a lot of buses, can we be more specific?

...

> +       help
> +         Enable this to support Bosch BNO055 IMUs attached via serial bus.

Ditto.

...

> +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 cmd_status - represent the status of a command sent to the HW.
> +        * @STATUS_OK:   The command executed successfully.
> +        * @STATUS_FAIL: The command failed: HW responded with an error.
> +        * @STATUS_CRIT: The command failed: the serial communication failed.
> +        */
> +       enum {
> +               STATUS_OK = 0,
> +               STATUS_FAIL = 1,
> +               STATUS_CRIT = -1

+ Comma and sort them by value?
For the second part is an additional question, why negative?

> +       } cmd_status;
> +       struct mutex lock;
> +
> +       /* Only accessed in RX callback context. No lock needed. */
> +       struct {
> +               enum {
> +                       RX_IDLE,
> +                       RX_START,
> +                       RX_DATA

+ Comma.

> +               } state;
> +               int databuf_count;
> +               int expected_len;
> +               int type;
> +       } rx;
> +
> +       /* Never accessed in behalf of RX callback context. No lock needed */
> +       bool cmd_stale;
> +};

...

> +       dev_dbg(&priv->serdev->dev, "send (len: %d): %*ph", len, len, data);

Not a fan of this and similar. Can't you introduce a trace events for
this driver?

...

> +       ret = serdev_device_write(priv->serdev, data, len,
> +                                 msecs_to_jiffies(25));

One line?

...

> +       int i = 0;

> +       while (1) {
> +               ret = bno055_sl_send_chunk(priv, hdr + i * 2, 2);
> +               if (ret)
> +                       goto fail;
> +
> +               if (i++ == 1)
> +                       break;
> +               usleep_range(2000, 3000);
> +       }

The infinite loops are hard to read and understand.
Can you convert it to the regular while or for one?

Also, this looks like a repetition of something (however it seems that
it's two sequencial packets to send).

...

> +       const int retry_max = 5;
> +       int retry = retry_max;

> +       while (retry--) {

Instead simply use

unsigned int retries = 5;

do {
  ...
} while (--retries);

which is much better to understand.

...

> +               if (retry != (retry_max - 1))
> +                       dev_dbg(&priv->serdev->dev, "cmd retry: %d",
> +                               retry_max - retry);

This is an invariant to the loop.

> +               ret = bno055_sl_do_send_cmd(priv, read, addr, len, data);
> +               if (ret)
> +                       continue;

...

> +               if (ret == -ERESTARTSYS) {
> +                       priv->cmd_stale = true;
> +                       return -ERESTARTSYS;

> +               } else if (!ret) {

Redundant 'else'

> +                       return -ETIMEDOUT;
> +               }

Also {} can be dropped.

...

> +               if (priv->cmd_status == STATUS_OK)
> +                       return 0;

> +               else if (priv->cmd_status == STATUS_CRIT)

Redundant 'else'

> +                       return -EIO;

...

> +static int bno055_sl_write_reg(void *context, const void *_data, size_t count)
> +{
> +       u8 *data = (u8 *)_data;

Why casting?

...

> +       if (val_size > 128) {
> +               dev_err(&priv->serdev->dev, "Invalid read valsize %d",
> +                       val_size);

One line?

> +               return -EINVAL;
> +       }

...

> +       reg_addr = ((u8 *)reg)[0];

This looks ugly.
Can't you supply the data struct pointer instead of void pointer?

...

> +       if (serdev_device_set_baudrate(serdev, 115200) != 115200) {

Is it limitation / requirement by the hardware? Otherwise it should
come from DT / ACPI.

...

> +       ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);

Ditto.

...

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

return dev_err_probe();
Andrea Merello March 22, 2022, 10:37 a.m. UTC | #4
Some inline comments, OK for the rest.

Il giorno lun 21 feb 2022 alle ore 21:28 Andy Shevchenko
<andy.shevchenko@gmail.com> ha scritto:
>
> On Thu, Feb 17, 2022 at 5:27 PM Andrea Merello <andrea.merello@gmail.com> wrote:
> >
> > This path adds a serdev driver for communicating to a BNO055 IMU via
> > serial bus, and it enables the BNO055 core driver to work in this
> > scenario.

[...]

> > +       /**
> > +        * enum cmd_status - represent the status of a command sent to the HW.
> > +        * @STATUS_OK:   The command executed successfully.
> > +        * @STATUS_FAIL: The command failed: HW responded with an error.
> > +        * @STATUS_CRIT: The command failed: the serial communication failed.
> > +        */
> > +       enum {
> > +               STATUS_OK = 0,
> > +               STATUS_FAIL = 1,
> > +               STATUS_CRIT = -1
>
> + Comma and sort them by value?
> For the second part is an additional question, why negative?

For STATUS_CRIT, being a (bad) error, a negative value seemed
appropriate to me. STATUS_OK is zero as usual, but maybe STATUS_FAIL
should be negative also? It is some legal protocol status (unlike the
STATUS_CRIT mess), in this sense I may consider it not an error, but
still our command failed (because the IMU politely didn't accept it),
so it's an error in this sense...

I may just let all of them implicit if you prefer.

[...]

> > +       while (1) {
> > +               ret = bno055_sl_send_chunk(priv, hdr + i * 2, 2);
> > +               if (ret)
> > +                       goto fail;
> > +
> > +               if (i++ == 1)
> > +                       break;
> > +               usleep_range(2000, 3000);
> > +       }
>
> The infinite loops are hard to read and understand.
> Can you convert it to the regular while or for one?
>
> Also, this looks like a repetition of something (however it seems that
> it's two sequencial packets to send).

Maybe it's worth to unroll then?

> ...
>
> > +       const int retry_max = 5;
> > +       int retry = retry_max;
>
> > +       while (retry--) {
>
> Instead simply use
>
> unsigned int retries = 5;
>
> do {
>   ...
> } while (--retries);
>
> which is much better to understand.

OK, but still have the const var for the max (see below)

> ...
>
> > +               if (retry != (retry_max - 1))
> > +                       dev_dbg(&priv->serdev->dev, "cmd retry: %d",
> > +                               retry_max - retry);
>
> This is an invariant to the loop.

why? This triggers at all retries, not at the first attempt i.e. it
prints only if this doesn't succeed at the first time. Indeed what
seems wrong to me is that you need -1 also in the dev_dbg() argument
to produce correct text.

[...]

>
> > +       reg_addr = ((u8 *)reg)[0];
>
> This looks ugly.
> Can't you supply the data struct pointer instead of void pointer?

I confirm that it's ugly :)

Not sure about what you exactly meant, sorry; what I can do is to
introduce a local and split this ugly loc, as done in
bno055_sl_write_reg(). Is this what you are suggesting?

> ...
>
> > +       if (serdev_device_set_baudrate(serdev, 115200) != 115200) {
>
> Is it limitation / requirement by the hardware? Otherwise it should
> come from DT / ACPI.

 It's a requirement. Not sure it's really by the HW; possibly it's
statically set in device firmware.

> ...
>
> > +       ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
>
> Ditto.

Ditto :)

[...]
diff mbox series

Patch

diff --git a/drivers/iio/imu/bno055/Kconfig b/drivers/iio/imu/bno055/Kconfig
index d0ab3221fba5..8a83639ad2a9 100644
--- a/drivers/iio/imu/bno055/Kconfig
+++ b/drivers/iio/imu/bno055/Kconfig
@@ -2,3 +2,13 @@ 
 
 config BOSCH_BNO055_IIO
 	tristate
+
+config BOSCH_BNO055_SERIAL
+	tristate "Bosch BNO055 attached via serial bus"
+	depends on SERIAL_DEV_BUS
+	select BOSCH_BNO055_IIO
+	help
+	  Enable this to support Bosch BNO055 IMUs attached via serial bus.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called bno055_sl.
diff --git a/drivers/iio/imu/bno055/Makefile b/drivers/iio/imu/bno055/Makefile
index 56cc4de60a7e..416f0ff96de5 100644
--- a/drivers/iio/imu/bno055/Makefile
+++ b/drivers/iio/imu/bno055/Makefile
@@ -1,3 +1,4 @@ 
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_BOSCH_BNO055_IIO) += bno055.o
+obj-$(CONFIG_BOSCH_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..5af44f151389
--- /dev/null
+++ b/drivers/iio/imu/bno055/bno055_sl.c
@@ -0,0 +1,557 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Serial line interface for Bosh BNO055 IMU (via serdev).
+ * This file implements serial communication up to the register read/write
+ * level.
+ *
+ * Copyright (C) 2021 Istituto Italiano di Tecnologia
+ * Electronic Design Laboratory
+ * Written by Andrea Merello <andrea.merello@iit.it>
+ *
+ * This driver is besed on
+ *	Plantower PMS7003 particulate matter sensor driver
+ *	Which is
+ *	Copyright (c) Tomasz Duszynski <tduszyns@gmail.com>
+ */
+
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/serdev.h>
+
+#include "bno055.h"
+
+/*
+ * Register writes cmd have the following format
+ * +------+------+-----+-----+----- ... ----+
+ * | 0xAA | 0xOO | REG | LEN | payload[LEN] |
+ * +------+------+-----+-----+----- ... ----+
+ *
+ * Register write responses have the following format
+ * +------+----------+
+ * | 0xEE | ERROCODE |
+ * +------+----------+
+ *
+ * .. except when writing the SYS_RST bit (i.e. triggering a system reset); in
+ * case the IMU accepts the command, then it resets without responding. We don't
+ * handle this (yet) here (so we inform the common bno055 code not to perform
+ * sw resets - bno055 on serial bus basically requires the hw reset pin).
+ *
+ * Register read have the following format
+ * +------+------+-----+-----+
+ * | 0xAA | 0xO1 | REG | LEN |
+ * +------+------+-----+-----+
+ *
+ * Successful register read response have the following format
+ * +------+-----+----- ... ----+
+ * | 0xBB | LEN | payload[LEN] |
+ * +------+-----+----- ... ----+
+ *
+ * Failed register read response have the following format
+ * +------+--------+
+ * | 0xEE | ERRCODE|  (ERRCODE always > 1)
+ * +------+--------+
+ *
+ * Error codes are
+ * 01: OK
+ * 02: read/write FAIL
+ * 04: invalid address
+ * 05: write on RO
+ * 06: wrong start byte
+ * 07: bus overrun
+ * 08: len too high
+ * 09: len too low
+ * 10: bus RX byte timeout (timeout is 30mS)
+ *
+ *
+ * **WORKAROUND ALERT**
+ *
+ * Serial communication seems very fragile: the BNO055 buffer seems to overflow
+ * very easy; BNO055 seems able to sink few bytes, then it needs a brief pause.
+ * On the other hand, it is also picky on timeout: if there is a pause > 30mS in
+ * between two bytes then the transaction fails (IMU internal RX FSM resets).
+ *
+ * BNO055 has been seen also failing to process commands in case we send them
+ * too close each other (or if it is somehow busy?)
+ *
+ * In particular I saw these scenarios:
+ * 1) If we send 2 bytes per time, then the IMU never(?) overflows.
+ * 2) If we send 4 bytes per time (i.e. the full header), then the IMU could
+ *    overflow, but it seem to sink all 4 bytes, then it returns error.
+ * 3) If we send more than 4 bytes, the IMU could overflow, and I saw it sending
+ *    error after 4 bytes are sent; we have troubles in synchronizing again,
+ *    because we are still sending data, and the IMU interprets it as the 1st
+ *    byte of a new command.
+ *
+ * While we must avoid case 3, we could send 4 bytes per time and eventually
+ * retry in case of failure; this seemed convenient for reads (which requires
+ * TXing exactly 4 bytes), however it has been seen that, depending by the IMU
+ * settings (e.g. LPF), failures became less or more frequent; in certain IMU
+ * configurations they are very rare, but in certain others we keeps failing
+ * even after like 30 retries.
+ *
+ * So, we just split TXes in [2-bytes + delay] steps, and still keep an eye on
+ * the IMU response; in case it overflows (which is now unlikely), we retry.
+ */
+
+/*
+ * Read operation overhead:
+ *  4 bytes req + 2byte resp hdr.
+ *  6 bytes = 60 bit (considering 1start + 1stop bits).
+ *  60/115200 = ~520uS + about 2500mS dealay -> ~3mS
+ * In 3mS we could read back about 34 bytes that means 17 samples, this means
+ * that in case of scattered read in which the gap is 17 samples or less it is
+ * still convenient to go for a burst.
+ * We have to take into account also IMU response time - IMU seems to be often
+ * reasonably quick to respond, but sometimes it seems to be in some "critical
+ * section" in which it delays handling of serial protocol. Because of this we
+ * round-up to 22, which is the max number of samples, always bursting indeed.
+ */
+#define BNO055_SL_XFER_BURST_BREAK_THRESHOLD 22
+
+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 cmd_status - represent the status of a command sent to the HW.
+	 * @STATUS_OK:   The command executed successfully.
+	 * @STATUS_FAIL: The command failed: HW responded with an error.
+	 * @STATUS_CRIT: The command failed: the serial communication failed.
+	 */
+	enum {
+		STATUS_OK = 0,
+		STATUS_FAIL = 1,
+		STATUS_CRIT = -1
+	} cmd_status;
+	struct mutex lock;
+
+	/* Only accessed in RX callback context. No lock needed. */
+	struct {
+		enum {
+			RX_IDLE,
+			RX_START,
+			RX_DATA
+		} state;
+		int databuf_count;
+		int expected_len;
+		int type;
+	} rx;
+
+	/* Never accessed in behalf of RX callback context. No lock needed */
+	bool cmd_stale;
+};
+
+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 < 0)
+		return ret;
+
+	if (ret < len)
+		return -EIO;
+
+	return 0;
+}
+
+/*
+ * Sends a read or write command.
+ * 'data' can be NULL (used in read case). 'len' parameter is always valid; in
+ * case 'data' is non-NULL then it must match 'data' size.
+ */
+static int bno055_sl_do_send_cmd(struct bno055_sl_priv *priv,
+				 bool read, int addr, int len, u8 *data)
+{
+	u8 hdr[] = {0xAA, read, addr, len};
+	int chunk_len;
+	int i = 0;
+	int ret;
+
+	while (1) {
+		ret = bno055_sl_send_chunk(priv, hdr + i * 2, 2);
+		if (ret)
+			goto fail;
+
+		if (i++ == 1)
+			break;
+		usleep_range(2000, 3000);
+	}
+
+	if (read)
+		return 0;
+
+	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,
+			   bool 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)
+			return -EIO;
+	}
+
+	/*
+	 * Try to convince the IMU to cooperate.. as explained in the comments
+	 * at the top of this file, the IMU could also refuse the command (i.e.
+	 * it is not ready yet); retry in this case.
+	 */
+	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) {
+			return -ETIMEDOUT;
+		}
+
+		if (priv->cmd_status == STATUS_OK)
+			return 0;
+		else if (priv->cmd_status == STATUS_CRIT)
+			return -EIO;
+
+		/* loop in case priv->cmd_status == STATUS_FAIL */
+	}
+
+	if (ret < 0)
+		return ret;
+	if (priv->cmd_status == STATUS_FAIL)
+		return -EINVAL;
+	return 0;
+}
+
+static int bno055_sl_write_reg(void *context, const void *_data, size_t count)
+{
+	u8 *data = (u8 *)_data;
+	struct bno055_sl_priv *priv = context;
+
+	if (count < 2) {
+		dev_err(&priv->serdev->dev, "Invalid write count %zu", count);
+		return -EINVAL;
+	}
+
+	dev_dbg(&priv->serdev->dev, "wr reg 0x%x = 0x%x", data[0], data[1]);
+	return bno_sl_send_cmd(priv, 0, data[0], count - 1, data + 1);
+}
+
+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 (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).
+ * Ideally it doesn't know anything about upper layer (i.e. if this is the
+ * packet we were really expecting), but since we copies the payload into the
+ * receiver buffer (that is not valid when i.e. we don't expect data), we
+ * snoop a bit in the upper layer..
+ * Also, we assume to RX one pkt per time (i.e. the HW doesn't send anything
+ * unless we require to AND we don't queue more than one request per time).
+ */
+static int bno055_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 remaining = size;
+
+	if (size == 0)
+		return 0;
+
+	dev_dbg(&priv->serdev->dev, "recv (len %zu): %*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;
+		remaining--;
+		buf++;
+		priv->rx.databuf_count = 0;
+		fallthrough;
+
+	case RX_START:
+		/*
+		 * Packet RX in progress, we expect either 1-byte len or 1-byte
+		 * status depending by the packet type.
+		 */
+		if (remaining == 0)
+			break;
+
+		if (priv->rx.type == 0xEE) {
+			if (remaining > 1) {
+				dev_err(&priv->serdev->dev, "EE pkt. Extra data received");
+				status = STATUS_CRIT;
+
+			} else {
+				status = (buf[0] == 1) ? STATUS_OK : STATUS_FAIL;
+			}
+			bno055_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];
+			remaining--;
+			buf++;
+		}
+		fallthrough;
+
+	case RX_DATA:
+		/* Header parsed; now receiving packet data payload */
+		if (remaining == 0)
+			break;
+
+		if (priv->rx.databuf_count + remaining > priv->rx.expected_len) {
+			/*
+			 * This is 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 + remaining <= priv->expected_data_len))
+			memcpy(priv->response_buf + priv->rx.databuf_count,
+			       buf, remaining);
+		mutex_unlock(&priv->lock);
+
+		priv->rx.databuf_count += remaining;
+
+		/*
+		 * Reached expected len advertised by the IMU for the current
+		 * packet. Pass it to the upper layer (for us it is just valid).
+		 */
+		if (priv->rx.databuf_count == priv->rx.expected_len) {
+			bno055_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;
+
+	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);
+	}
+
+	return bno055_probe(&serdev->dev, regmap,
+			    BNO055_SL_XFER_BURST_BREAK_THRESHOLD, false);
+}
+
+static const struct of_device_id bno055_sl_of_match[] = {
+	{ .compatible = "bosch,bno055" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bno055_sl_of_match);
+
+static struct serdev_device_driver bno055_sl_driver = {
+	.driver = {
+		.name = "bno055-sl",
+		.of_match_table = bno055_sl_of_match,
+	},
+	.probe = bno055_sl_probe,
+};
+module_serdev_device_driver(bno055_sl_driver);
+
+MODULE_AUTHOR("Andrea Merello <andrea.merello@iit.it>");
+MODULE_DESCRIPTION("Bosch BNO055 serdev interface");
+MODULE_LICENSE("GPL v2");