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

[Andy Shevchenko]

On Thu, Feb 17, 2022 at 5:27 PM Andrea Merello <andrea.merello@xxxxxxxxx> 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.

>  drivers/iio/imu/bno055/bno055_sl.c | 557 +++++++++++++++++++++++++++++

Can we use the suffix _ser instead of _sl?

...

> +config BOSCH_BNO055_SERIAL
> +       tristate "Bosch BNO055 attached via serial bus"

Serial is too broad, it can cover a lot of buses, can we be more specific?

...

> +       help
> +         Enable this to support Bosch BNO055 IMUs attached via serial bus.

Ditto.

...

> +struct bno055_sl_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_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?

> +       } cmd_status;
> +       struct mutex lock;
> +
> +       /* Only accessed in RX callback context. No lock needed. */
> +       struct {
> +               enum {
> +                       RX_IDLE,
> +                       RX_START,
> +                       RX_DATA

+ Comma.

> +               } state;
> +               int databuf_count;
> +               int expected_len;
> +               int type;
> +       } rx;
> +
> +       /* Never accessed in behalf of RX callback context. No lock needed */
> +       bool cmd_stale;
> +};

...

> +       dev_dbg(&priv->serdev->dev, "send (len: %d): %*ph", len, len, data);

Not a fan of this and similar. Can't you introduce a trace events for
this driver?

...

> +       ret = serdev_device_write(priv->serdev, data, len,
> +                                 msecs_to_jiffies(25));

One line?

...

> +       int i = 0;

> +       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).

...

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

...

> +               if (retry != (retry_max - 1))
> +                       dev_dbg(&priv->serdev->dev, "cmd retry: %d",
> +                               retry_max - retry);

This is an invariant to the loop.

> +               ret = bno055_sl_do_send_cmd(priv, read, addr, len, data);
> +               if (ret)
> +                       continue;

...

> +               if (ret == -ERESTARTSYS) {
> +                       priv->cmd_stale = true;
> +                       return -ERESTARTSYS;

> +               } else if (!ret) {

Redundant 'else'

> +                       return -ETIMEDOUT;
> +               }

Also {} can be dropped.

...

> +               if (priv->cmd_status == STATUS_OK)
> +                       return 0;

> +               else if (priv->cmd_status == STATUS_CRIT)

Redundant 'else'

> +                       return -EIO;

...

> +static int bno055_sl_write_reg(void *context, const void *_data, size_t count)
> +{
> +       u8 *data = (u8 *)_data;

Why casting?

...

> +       if (val_size > 128) {
> +               dev_err(&priv->serdev->dev, "Invalid read valsize %d",
> +                       val_size);

One line?

> +               return -EINVAL;
> +       }

...

> +       reg_addr = ((u8 *)reg)[0];

This looks ugly.
Can't you supply the data struct pointer instead of void pointer?

...

> +       if (serdev_device_set_baudrate(serdev, 115200) != 115200) {

Is it limitation / requirement by the hardware? Otherwise it should
come from DT / ACPI.

...

> +       ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);

Ditto.

...

> +       regmap = devm_regmap_init(&serdev->dev, &bno055_sl_regmap_bus,
> +                                 priv, &bno055_regmap_config);

> +       if (IS_ERR(regmap)) {
> +               dev_err(&serdev->dev, "Unable to init register map");
> +               return PTR_ERR(regmap);
> +       }

return dev_err_probe();

-- 
With Best Regards,
Andy Shevchenko