Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver

Andy Shevchenko
2021-07-15T16:50:19Z

On Thu, Jul 15, 2021 at 5:21 PM Andrea Merello <andrea.merello@gmail.com> wrote:
>
> This patch adds a core driver for the BNO055 IMU from Bosch. This IMU
> can be connected via both serial and I2C busses; separate patches will
> add support for them.
>
> The driver supports "AMG" (Accelerometer, Magnetometer, Gyroscope) mode,
> that provides raw data from the said internal sensors, and a couple of
> "fusion" modes (i.e. the IMU also do calculations in order to provide
> euler angles, quaternions, linear acceleration and gravity measurements).
>
> In fusion modes the AMG data is still available (with some calibration
> refinements done by the IMU), but certain settings such as low pass
> filters cut-off frequency and sensors ranges are fixed, while in AMG mode
> they can be customized; this is why AMG mode can still be interesting.

...

> Signed-off-by: Andrea Merello <andrea.merello@iit.it>
> Cc: Andrea Merello <andrea.merello@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Vlad Dogaru <vlad.dogaru@intel.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org

Instead of polluting commit messages with this, use --to and --cc
parameters. You may utilize my script [1] which finds automatically to
whom to send (of course it allows manually to add more).

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

...

> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# driver for Bosh bmo055

Driver

Point of this comment actually is ..?

...

> +# Makefile for bosh bno055

Ditto.

...

> +// SPDX-License-Identifier: GPL-2.0-or-later

Is it the correct one, looking at the portions taken from other drivers?

> +/*
> + * IIO driver for Bosh BNO055 IMU
> + *
> + * Copyright (C) 2021 Istituto Italiano di Tecnologia
> + * Electronic Design Laboratory
> + * Written by Andrea Merello <andrea.merello@iit.it>
> + *
> + * Portions of this driver are taken from the BNO055 driver patch
> + * from Vlad Dogaru which is Copyright (c) 2016, Intel Corporation.
> + *
> + * This driver is also based on BMI160 driver, which is:
> + * Copyright (c) 2016, Intel Corporation.
> + * Copyright (c) 2019, Martin Kelly.
> + */

...

> +#include <linux/clk.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>

> +#include <linux/iio/iio.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>

Can you move this group to...

> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>

...be here?

> +#include "bno055.h"

...

> +#define BNO055_CALIB_STAT_MASK 3

GENMASK()

...

> +#define BNO055_UNIT_SEL_ANDROID BIT(7)

Android? What does this mean?

...

> +#define BNO055_CALDATA_LEN (BNO055_CALDATA_END - BNO055_CALDATA_START + 1)

Can you put just a plain number?

...

> +#define BNO055_ACC_CONFIG_LPF_MASK 0x1C

GENMASK() here and everywhere where it makes sense.

...

> +#define BNO055_UID_LEN (0xF)

Useless parentheses. If the LEN is a plain number, use decimal, if
it's limited by register width, use the form of (BIT(x) - 1). In such
a case it's easy to see how many bits are used for it.

...

> + int i;
> + int best_idx, best_delta, delta;
> + int first = 1;

Use reversed xmas tree order.

...

> + for (i = 0; i < len; i++) {
> + delta = abs(arr[i] - val);
> + if (first || delta < best_delta) {
> + best_delta = delta;
> + best_idx = i;
> + }
> + first = 0;
> + }

I think I saw this kind of snippet for the 100th time. Can it be
factored out to some helper and used by everyone?

...

> + if ((reg >= 0x8 && reg <= 0x3A) ||

Use names instead of values here and in similar places elsewhere.

> + /* when in fusion mode, config is updated by chip */
> + reg == BNO055_MAG_CONFIG_REG ||
> + reg == BNO055_ACC_CONFIG_REG ||
> + reg == BNO055_GYR_CONFIG_REG ||
> + (reg >= BNO055_CALDATA_START && reg <= BNO055_CALDATA_END))

Please, split this to 3 or more conditionals that are easier to read
(logically separated).
Same comment to the rest of the similar functions.

...

> + .selector_mask = 0xff,

GENMASK() ?

...

> + if (res && res != -ERESTARTSYS) {

Shouldn't RESTARTSYS be handled on a regmap level?

...

> +/* must be called in configuration mode */
> +int bno055_calibration_load(struct bno055_priv *priv, const struct firmware *fw)
> +{
> + int i;
> + unsigned int tmp;
> + u8 cal[BNO055_CALDATA_LEN];
> + int read, tot_read = 0;
> + int ret = 0;
> + char *buf = kmalloc(fw->size + 1, GFP_KERNEL);
> +
> + if (!buf)
> + return -ENOMEM;
> +
> + memcpy(buf, fw->data, fw->size);

kmemdup() ?

> + buf[fw->size] = '\0';

> + for (i = 0; i < BNO055_CALDATA_LEN; i++) {
> + ret = sscanf(buf + tot_read, "%x%n",
> + &tmp, &read);
> + if (ret != 1 || tmp > 0xff) {
> + ret = -EINVAL;
> + goto exit;
> + }
> + cal[i] = tmp;
> + tot_read += read;
> + }

Sounds like NIH hex2bin().

> + dev_dbg(priv->dev, "loading cal data: %*ph", BNO055_CALDATA_LEN, cal);
> + ret = regmap_bulk_write(priv->regmap, BNO055_CALDATA_START,
> + cal, BNO055_CALDATA_LEN);
> +exit:
> + kfree(buf);
> + return ret;
> +}

...

> + int ret = bno055_reg_read(priv, reg, &hwval);
> +
> + if (ret)
> + return ret;

In all cases (esp. when resource allocations are involved) better to use

int ret;

ret = func();
if (foo)
return ret;

...

> + dev_dbg(priv->dev, "WR config - reg, mask, val: 0x%x, 0x%x, 0x%x",
> + reg, mask, hwval);

Why? Enable regmap trace events for this and drop these unneeded prints.

...

> + __le16 raw_val;

> + ret = regmap_bulk_read(priv->regmap, chan->address,
> + &raw_val, 2);

sizeof(raw_val)

and everywhere where similar cases are.

> + if (ret < 0)
> + return ret;

...

> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (chan->type == IIO_MAGN)
> + return bno055_get_mag_odr(priv, val, val2);
> + else
> + return -EINVAL;

Use usual pattern

if (!cond)
return ERRNO;
...
return bar;

...

> + for (i = 0; i < 4; i++)
> + vals[i] = (s16)le16_to_cpu(raw_vals[i]);

Extract this to be a helper like there are for u32 and u64.

...

> + vals[1] = 1 << 14;

BIT(14) But still magic.

...
> + switch (mask) {
> + case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> + return bno055_set_gyr_lpf(priv, val, val2);

default?

> + }

...

> + struct bno055_priv *priv = iio_priv(dev_to_iio_dev(dev));
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n",
> + (priv->operation_mode != BNO055_OPR_MODE_AMG) ? "20" :
> + "2 6 8 10 15 20 25 30");

IIO core should do this, besides the fact that it must use sysfs_emit().
Ditto for the similar.

...

> + if (sysfs_streq(buf, "amg"))
> + priv->operation_mode = BNO055_OPR_MODE_AMG;
> + else if (sysfs_streq(buf, "fusion"))
> + priv->operation_mode = BNO055_OPR_MODE_FUSION;
> + else if (sysfs_streq(buf, "fusion_fmc_off"))
> + priv->operation_mode = BNO055_OPR_MODE_FUSION_FMC_OFF;
> + else
> + return -EINVAL;

Wondering if you may use sysfs_match_string().

...

> + return res ? res : len;

ret, res, ... Be consistent!
Besides that the form

return ret ?: len;

is shorter and better.

...

> + static const char * const calib_status[] = {"bad", "barely enough",
> + "fair", "good"};

Please use better indentation

static char ... foo[] = {
{ a, b, c, d, }
};

> + for (size = 0, i = 0; i < BNO055_CALDATA_LEN; i++) {
> + ret = scnprintf(buf + size,
> + PAGE_SIZE - size, "%02x%c", data[i],
> + (i + 1 < BNO055_CALDATA_LEN) ? ' ' : '\n');
> + if (ret < 0)
> + return ret;
> + size += ret;
> + }

And if it's more than 4/3 kBytes (binary)?
Isn't it better to use the request_firmware() interface or something similar?
If IIO doesn't provide the common attributes for this it probably
should and it has to be a binary one.

...

> +static IIO_DEVICE_ATTR_RO(in_magn_sampling_frequency_available,
> + 0);

Definitely one line.

...

> + &iio_dev_attr_in_autocalibration_status_gyro.dev_attr.attr,
> + &iio_dev_attr_in_autocalibration_status_magn.dev_attr.attr,
> + &iio_dev_attr_in_calibration_data.dev_attr.attr,
> + NULL,

No comma for terminator line.

...

> +/*
> + * Reads len samples from the HW, stores them in buf starting from buf_idx,
> + * and applies mask to cull (skip) unneeded samples.
> + * Updates buf_idx incrementing with the number of stored samples.
> + * Samples from HW are xferred into buf, then in-place copy on buf is

transferred

> + * performed in order to cull samples that need to be skipped.
> + * This avoids copies of the first samples until we hit the 1st sample to skip,
> + * and also avoids having an extra bounce buffer.
> + * buf must be able to contain len elements inspite of how many samples we are

in spite of

> + * going to cull.
> + */

...

> + /* All chans are made up 1 16bit sample, except for quaternion

16-bit

Multi-line comment style. And be consistent!

> + * that is made up 4 16-bit values.
> + * For us the quaternion CH is just like 4 regular CHs.
> + * If out read starts past the quaternion make sure to adjust the
> + * starting offset; if the quaternion is contained in our scan then
> + * make sure to adjust the read len.

Your lines here like a drunk person. use the space more monotonically.

> + */

...

> + n = ((start_ch + i) == BNO055_SCAN_QUATERNION) ? 4 : 1;

Too many parentheses.

...

> + for (j = 0; j < n; j++)
> + dst[j] = src[j];

NIH memcpy()

...

> + __le16 chans[(BNO055_GRAVITY_DATA_Z_LSB_REG -
> + BNO055_ACC_DATA_X_LSB_REG) / 2];

Can you define separately what's inside square brackets?

...

> + while (!finish) {
> + end = find_next_zero_bit(iio_dev->active_scan_mask,
> + iio_dev->masklength, start);
> + if (end == iio_dev->masklength) {
> + finish = true;

NIH for_each_clear_bit().

> + } else {
> + next = find_next_bit(iio_dev->active_scan_mask,
> + iio_dev->masklength, end);
> + if (next == iio_dev->masklength) {
> + finish = true;

Ditto.

> + } else {
> + quat = ((next > BNO055_SCAN_QUATERNION) &&
> + (end <= BNO055_SCAN_QUATERNION)) ? 3 : 0;
> + thr_hit = (next - end + quat) >
> + priv->xfer_burst_break_thr;
> + }
> + }
> +
> + if (thr_hit || finish) {
> + mask = *iio_dev->active_scan_mask >> xfer_start;
> + ret = bno055_scan_xfer(priv, xfer_start,
> + end - xfer_start,
> + mask, buf.chans, &buf_idx);
> + if (ret)
> + goto done;
> + xfer_start = next;
> + }
> + start = next;
> + }

...

> + /* base name + separator + UID + ext + zero */
> + char fw_name_buf[sizeof(BNO055_FW_NAME BNO055_FW_EXT) +
> + BNO055_UID_LEN * 2 + 1 + 1];

Perhaps devm_kasprintf()?

...

> + memset(priv, 0, sizeof(*priv));

Why?!

...

> + if (IS_ERR(rst) && (PTR_ERR(rst) != -EPROBE_DEFER)) {
> + dev_err(dev, "Failed to get reset GPIO");
> + return PTR_ERR(rst);
> + }

if (IS_ERR(...))
return dev_err_probe(...);

...

> + if (IS_ERR(priv->clk) && (PTR_ERR(priv->clk) != -EPROBE_DEFER)) {
> + dev_err(dev, "Failed to get CLK");
> + return PTR_ERR(priv->clk);
> + }

Ditto.

...

> + clk_prepare_enable(priv->clk);

Missed clk_disabled_unprepare() from all below error paths.
Use devm_add_action_or_reset() approach.

...

> + dev_dbg(dev, "Found BMO055 chip");

Useless noise and LOC.

...

> + dev_info(dev, "unique ID: %*ph", BNO055_UID_LEN, priv->uid);

Can it be printed somewhere together with firmware revision?

...

> + sprintf(fw_name_buf, BNO055_FW_NAME "-%*phN" BNO055_FW_EXT,

Simply define a format string as FW_FMT somewhere above and use it here.

> + BNO055_UID_LEN, priv->uid);

...

> + dev_notice(dev, "Failed to load calibration data firmware file; this has nothing to do with IMU main firmware.");
> + dev_notice(dev, "You can calibrate your IMU (look for 'in_autocalibration_status*' files in sysfs) and then copy 'in_calibration_data' to your firmware file");

'\n' exists on purpose.

...

> +#include <linux/device.h>

No user of this.

> +#include <linux/regmap.h>
> +
> +int bno055_probe(struct device *dev, struct regmap *regmap, int irq,
> + int xfer_burst_break_thr);
> +extern const struct regmap_config bno055_regmap_config;

--
With Best Regards,
Andy Shevchenko