On Tue, 9 Nov 2021 16:33:44 +0100
Andrea Merello [off-list ref] wrote:

...

quoted

quoted

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

Why the local variable?  

size variable gets modified, so we cache the value to return in case of success.

Ah - missed that as unusual way around.  I'd modify the local variable instead
and perhaps call it something like remaining to reflect how it is being used?

quoted

quoted

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