Il giorno ven 15 apr 2022 alle ore 18:40 Jonathan Cameron
<jic23@xxxxxxxxxx> ha scritto:
>
> On Fri, 15 Apr 2022 15:00:03 +0200
> Andrea Merello <andrea.merello@xxxxxxxxx> wrote:
>
> > From: Andrea Merello <andrea.merello@xxxxxx>
> >
> > 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@xxxxxx>
> Hi Andrea
>
> A few really trivial things in here from me.
Few inline comments below; OK for all indeed.
> > +struct bno055_ser_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_CRIT: The command failed: the serial communication failed.
> > + * @STATUS_OK: The command executed successfully.
> > + * @STATUS_FAIL: The command failed: HW responded with an error.
> > + */
> > + enum {
> > + STATUS_CRIT = -1,
> > + STATUS_OK = 0,
> > + STATUS_FAIL = 1,
> > + } cmd_status;
>
> Locks need documentation to say what scope they cover. In this case
> I think it is most but not quite all of this structure.
I admit my comments here are awkward: I've put a couple of comment
that indicate what doesn't need the lock.. I'll change to do the
reverse (comment on what need the lock)
> See comment on completion below.
>
> > + 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;
> > +};
>
[...]
> > + }
> > + break;
> > +
> > + case CMD_WRITE:
> > + priv->cmd_status = status;
> > + break;
> > + }
> > +
> > + priv->expect_response = CMD_NONE;
> > + complete(&priv->cmd_complete);
>
> I argued with myself a bit on whether the complete() should be inside the lock
> or not - but then concluded it doesn't really matter and moving it out is
> probably premature optimisation... Maybe it's worth moving it out simply
> so that it's clear the lock isn't needed to protect it, or am I missing something?
It should make no real difference to move the complete() out of the lock.
I think I put it inside the lock because of the (paranoid, and
hopefully not really required - would mean we have been too strict
with completion timeout) reinit_completion(). On serdev rx handler
side (i.e. bno055_ser_handle_rx()) we clear expect_response and
complete(), on the other side (bno055_ser_send_cmd()) we set
expect_response and clear spurious completed state, before issuing the
command and waiting for outcome. This looks symmetric, but those two
shouldn't really race in practice.
> > + mutex_unlock(&priv->lock);
> > +}
> > +
> > +/*
