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

Andy Shevchenko
2022-02-21T20:28:37Z

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.

> 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