On Tue, 9 Nov 2021 16:33:44 +0100
Andrea Merello <andrea.merello@xxxxxxxxx> wrote:
...
> > > +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?
>
> > > +
> > > + 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;
> > > +}
