lkml.org 
[lkml]   [2022]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [v5 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
On Tue, Apr 26, 2022 at 3:11 PM Andrea Merello <andrea.merello@gmail.com> wrote:
>
> From: Andrea Merello <andrea.merello@iit.it>
>
> Add the 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

does

> 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

sensor?
sensors'?

> be customized; this is why AMG mode can still be interesting.

...

> +#include <linux/bitmap.h>
> +#include <linux/clk.h>
> +#include <linux/bitfield.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>

Keep them ordered?

...

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

Ditto.

...

> +#define BNO055_FW_NAME "bno055-caldata"
> +#define BNO055_FW_EXT ".dat"
> +#define BNO055_FW_UID_NAME BNO055_FW_NAME "-%*phN" BNO055_FW_EXT

I believe this should be _FMT prefix, since it's not a name (I don't
think %*phN is a part of the actual file name).

> +#define BNO055_FW_GENERIC_NAME (BNO055_FW_NAME BNO055_FW_EXT)

And these macros of macros look unreadable, can we just hardcode
format and generic name?

...

> +#define BNO055_ATTR_VALS(...) \
> + .vals = (int[]){ __VA_ARGS__}, \
> + .len = ARRAY_SIZE(((int[]){__VA_ARGS__}))

Not sure this adds any readability to the code. Can we simply have an
array of int for each case with the explicit ARRAY_SIZE() calls?

...

> +/*
> + * Theoretically the IMU should return data in a given (i.e. fixed) unit
> + * regardless the range setting. This happens for the accelerometer, but not for

regardless of

> + * the gyroscope; the gyroscope range setting affects the scale.
> + * This is probably due to this[0] bug.
> + * For this reason we map the internal range setting onto the standard IIO scale
> + * attribute for gyro.
> + * Since the bug[0] may be fixed in future, we check for the IMU FW version and
> + * eventually warn the user.
> + * Currently we just don't care about "range" attributes for gyro.
> + *
> + * [0] https://community.bosch-sensortec.com/t5/MEMS-sensors-forum/BNO055-Wrong-sensitivity-resolution-in-datasheet/td-p/10266
> + */

...

> + /* calibration data may be updated by the IMU */
> + if (reg >= BNO055_CALDATA_START && reg <= BNO055_CALDATA_END)
> + return true;
> +
> + return false;

Can be done in one return statement w/o branch, but probably OK taking
into account other related functions.

> +}

...

> + {
> + .range_min = 0,
> + .range_max = 0x7f * 2,
> + .selector_reg = BNO055_PAGESEL_REG,
> + .selector_mask = GENMASK(7, 0),
> + .selector_shift = 0,
> + .window_start = 0,
> + .window_len = 0x80

In all cases like this, keep the trailing comma.

> + },

...

> + msleep(20);

Perhaps a comment why so long sleep is needed.

...

> +static int bno055_system_reset(struct bno055_priv *priv)
> +{
> + int ret;
> +
> + if (priv->reset_gpio) {
> + gpiod_set_value_cansleep(priv->reset_gpio, 0);
> + usleep_range(5000, 10000);
> + gpiod_set_value_cansleep(priv->reset_gpio, 1);
> + } else {
> + if (!priv->sw_reset)
> + return 0;

Can be unified with the above 'else':

} else if (...) {
return 0;
} else {

> + ret = regmap_write(priv->regmap, BNO055_SYS_TRIGGER_REG,
> + BNO055_SYS_TRIGGER_RST_SYS);
> + if (ret)
> + return ret;
> + }
> +
> + regcache_drop_region(priv->regmap, 0x0, 0xff);
> + usleep_range(650000, 700000);
> +
> + return 0;
> +}

...

> +exit:

exit_unlock: ?

> + mutex_unlock(&priv->lock);
> + return ret;
> +}

...

> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
> + .repeat = IIO_MOD_##_axis == IIO_MOD_QUATERNION ? 4 : 0 \

+ Comma.

> + }, \

...

> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + .scan_index = -1

+ Comma.

> + },

...

> + /*
> + * We always get a request in INT_PLUS_MICRO, but we
> + * take care of the micro part only when we really have
> + * non-integer tables. This prevents 32-bit overflow with
> + * larger integers contained in integer tables.
> + */
> + req_val = val;
> + if (attr->type != IIO_VAL_INT) {


> + if (val > 2147)
> + val = 2147;

min() ?

It seems it's not used outside of this branch, so this min() can be
integrated into below without corrupting val itself.

> + len /= 2;
> + req_val = val * 1000000 + val2;
> + }

...

> + if (first || delta < best_delta) {

In such cases checking 'first' last might slightly be better.

> + best_delta = delta;
> + hwval = i;
> + first = false;
> + }

...

> + /*
> + * IMU reports sensor offests; IIO wants correction

offsets

> + * offset, thus we need the 'minus' here.
> + */

...

> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (chan->type != IIO_MAGN)
> + return -EINVAL;

> + else

redundant.

> + return bno055_get_regmask(priv, val, val2,
> + BNO055_MAG_CONFIG_REG,
> + BNO055_MAG_CONFIG_ODR_MASK,
> + &bno055_mag_odr);

...

> + for (i = 0; i < bno055_acc_range.len; i++)
> + len += sysfs_emit_at(buf, len, "%d%c", bno055_acc_range.vals[i],
> + (i == bno055_acc_range.len - 1) ? '\n' : ' ');

You may move the condition out of the loop.

...

> + int ret = 0;

Redundant assignment, see below.

> + if (indio_dev->active_scan_mask &&
> + !bitmap_empty(indio_dev->active_scan_mask, _BNO055_SCAN_MAX))
> + return -EBUSY;
> +
> + if (sysfs_streq(buf, "0")) {
> + ret = bno055_operation_mode_set(priv, BNO055_OPR_MODE_AMG);

return bno055_operation_mode_set(...);

> + } else {

...and drop this with the following decreasing indentation.

> + /*
> + * Coming from AMG means the FMC was off, just switch to fusion
> + * but don't change anything that doesn't belong to us (i.e let
> + * FMC stay off.
> + * Coming from any other fusion mode means we don't need to do
> + * anything.
> + */
> + if (priv->operation_mode == BNO055_OPR_MODE_AMG)
> + ret = bno055_operation_mode_set(priv, BNO055_OPR_MODE_FUSION_FMC_OFF);
> + }
> + if (ret)
> + return ret;
> + return len;

...

> + if (count < BNO055_CALDATA_LEN || pos != 0)

Drop ' != 0' part ?

> + return -EINVAL;

...

> +unlock:

exit_unlock: to be consistent?

> + mutex_unlock(&priv->lock);
> + return ret;

...

> + /*
> + * Walk the bitmap and eventually perform several transfers.
> + * Bitmap ones-fileds that are separated by gaps <= xfer_burst_break_thr

ones-fields ?

> + * will be included in same transfer.
> + * Every time the bitmap contains a gap wider than xfer_burst_break_thr
> + * then we split the transfer, skipping the gap.
> + */

...

> + /*
> + * First transfer will start from the beginnig of the first

beginning

> + * ones-field in the bitmap
> + */
> + if (first)
> + xfer_start = start;
> +
> + /*
> + * We found the next ones-field; check whether to include it in
> + * the current transfer or not (i.e. let's perform the current
> + * transfer and prepare for another one).
> + */

> + if (!first) {

'else {' ?


> + }

...

> +static void bno055_clk_disable(void *arg)
> +{
> + clk_disable_unprepare((struct clk *)arg);

Redundant casting.

> +}

...

> + const u8 *caldata_data = NULL;

> + int caldata_size = 0;

No need. See below.

...

> + if (priv->reset_gpio) {
> + usleep_range(5000, 10000);
> + gpiod_set_value_cansleep(priv->reset_gpio, 1);
> + usleep_range(650000, 750000);

> + } else {
> + if (!sw_reset)

} else if () {

> + dev_warn(dev, "No usable reset method; IMU may be unreliable");
> + }

...

> + /*
> + * Seems that stock FW version conains a bug (see comment at the

the stock
contains

> + * beginning of this file) that causes the anglvel scale to be changed
> + * depending by the chip range setting. We workaround this, but we don't

on the chip

> + * know what other FW version might do..

versions

One period at the end is enough, or add another one if there is a
continuation below.

> + */

...

> + ret = request_firmware(&caldata, fw_name_buf, dev);
> + kfree(fw_name_buf);
> + if (ret)
> + ret = request_firmware(&caldata, BNO055_FW_GENERIC_NAME, dev);

> +

Can be removed to group all related checks together.

> + if (ret)
> + dev_notice(dev, "Calibration file load failed. See instruction in kernel Documentation/iio/bno055.rst");
> +
> + if (caldata) {
> + caldata_data = caldata->data;
> + caldata_size = caldata->size;
> + }
> + ret = bno055_init(priv, caldata_data, caldata_size);

> + if (caldata)
> + release_firmware(caldata);

> + if (ret)
> + return ret;

Can be rewritten in a form of

if (caldata) {
ret = bno055_init();
release_firmware(...);
} else {
ret = bno055_init();
}
if (ret)
return ret;

?

...

> +#include <linux/regmap.h>

Missed types.h.

> +struct device;
> +int bno055_probe(struct device *dev, struct regmap *regmap,
> + int xfer_burst_break_thr, bool sw_reset);
> +extern const struct regmap_config bno055_regmap_config;

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2022-04-27 15:27    [W:0.143 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site