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

[Andrea Merello <andrea.merello@gmail.com>]

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 :)

[...]