| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VdLiBkg100UjFN36rW_vaOObOoJ_Mv9n=4LjSWb+dQWMw@mail.gmail.com>
Date: Wed, 27 Apr 2022 15:22:56 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Andrea Merello <andrea.merello@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
devicetree <devicetree@...r.kernel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Matt Ranostay <matt.ranostay@...sulko.com>,
Alexandru Ardelean <ardeleanalex@...il.com>,
jmondi <jacopo@...ndi.org>,
Andrea Merello <andrea.merello@....it>
Subject: Re: [v5 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
On Tue, Apr 26, 2022 at 3:11 PM Andrea Merello <andrea.merello@...il.com> wrote:
>
> From: Andrea Merello <andrea.merello@....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
Powered by blists - more mailing lists