Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver
From: Andy Shevchenko
Date: Thu Jul 15 2021 - 12:50:19 EST
On Thu, Jul 15, 2021 at 5:21 PM Andrea Merello <andrea.merello@xxxxxxxxx> 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@xxxxxx>
> Cc: Andrea Merello <andrea.merello@xxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Vlad Dogaru <vlad.dogaru@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-iio@xxxxxxxxxxxxxxx
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@xxxxxx>
> + *
> + * 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