Re: [v4 12/14] iio: imu: add BNO055 serdev driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux