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
