From: Jonathan Cameron <jic23@kernel.org>
To: <andrea.merello@iit.it>
Cc: <mchehab+huawei@kernel.org>, <linux-iio@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
<lars@metafoo.de>, <robh+dt@kernel.org>,
<andy.shevchenko@gmail.com>, <matt.ranostay@konsulko.com>,
<ardeleanalex@gmail.com>, <jacopo@jmondi.org>
Subject: Re: [v6 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
Date: Sat, 18 Jun 2022 18:26:18 +0100 [thread overview]
Message-ID: <20220618182618.4da1df69@jic23-huawei> (raw)
In-Reply-To: <20220613120534.36991-9-andrea.merello@iit.it>
On Mon, 13 Jun 2022 14:05:28 +0200
<andrea.merello@iit.it> 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 does 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>
> ---
Hi Andrea,
A few trivial things from me this time seeing as you are going to be doing a v7.
Thanks,
Jonathan
> +
> +static ssize_t bno055_get_calib_status(struct device *dev, char *buf, int which)
> +{
> + struct bno055_priv *priv = iio_priv(dev_to_iio_dev(dev));
> + int calib;
> + int ret;
> + int val;
> +
> + if (priv->operation_mode == BNO055_OPR_MODE_AMG ||
> + (priv->operation_mode == BNO055_OPR_MODE_FUSION_FMC_OFF &&
> + which == BNO055_CALIB_STAT_MAGN_SHIFT)) {
> + calib = 0;
> + } else {
> + mutex_lock(&priv->lock);
> + ret = regmap_read(priv->regmap, BNO055_CALIB_STAT_REG, &val);
> + mutex_unlock(&priv->lock);
> +
> + if (ret)
> + return -EIO;
> +
> + calib = ((val >> which) & BNO055_CALIB_STAT_MASK) + 1;
Hmm. This is an unsual field get. I wonder if it is good idea to
have what looks like a field mask in BNO055_CALIB_STAT_MASK but isn't
really because we are applying it to a shifted value. Maybe we
are better off just encoding it directly here as GENMASK(1, 0)
> + }
> +
> + return sysfs_emit(buf, "%d\n", calib);
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
I'm not particularly keen on ifdef fun in c files. How bad is it if we
just don't bother and use __maybe_unused. debug_fs_createfile is visible to
the compiler in !CONFIG_DEBUG_FS so it should manage to remove everything
except *_debugfs_reg_access() but that is trivial in size anyway.
> +static int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct bno055_priv *priv = iio_priv(iio_dev);
> +
> + if (readval)
> + return regmap_read(priv->regmap, reg, readval);
> + else
> + return regmap_write(priv->regmap, reg, writeval);
> +}
> +
> +static ssize_t bno055_show_fw_version(struct file *file, char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct bno055_priv *priv = file->private_data;
> + int rev, ver;
> + char *buf;
> + int ret;
> +
> + ret = regmap_read(priv->regmap, BNO055_SW_REV_LSB_REG, &rev);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(priv->regmap, BNO055_SW_REV_MSB_REG, &ver);
> + if (ret)
> + return ret;
> +
> + buf = kasprintf(GFP_KERNEL, "ver: 0x%x, rev: 0x%x\n", ver, rev);
> + if (!buf)
> + return -ENOMEM;
> +
> + ret = simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
> + kfree(buf);
> +
> + return ret;
> +}
> +
> +static const struct file_operations bno055_fw_version_ops = {
> + .open = simple_open,
> + .read = bno055_show_fw_version,
> + .llseek = default_llseek,
> + .owner = THIS_MODULE,
> +};
> +
> +static void bno055_debugfs_remove(void *debugfs)
> +{
> + debugfs_remove((struct dentry *)debugfs);
> +}
> +
> +static void bno055_debugfs_init(struct iio_dev *iio_dev)
> +{
> + struct bno055_priv *priv = iio_priv(iio_dev);
> +
> + priv->debugfs = debugfs_create_file("firmware_version", 0400,
> + iio_get_debugfs_dentry(iio_dev),
> + priv, &bno055_fw_version_ops);
> +
> + devm_add_action_or_reset(priv->dev, bno055_debugfs_remove, priv->debugfs);
> +}
> +#else
> +static void bno055_debugfs_init(struct iio_dev *iio_dev)
> +{
> +}
> +
> +static int bno055_debugfs_reg_access(struct iio_dev *iio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + return 0;
> +}
> +#endif
> +
...
> +
> +static irqreturn_t bno055_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *iio_dev = pf->indio_dev;
> + struct bno055_priv *priv = iio_priv(iio_dev);
> + int xfer_start, start, end, prev_end;
> + unsigned long mask;
> + int quat_extra_len;
> + bool first = true;
> + int buf_idx = 0;
> + bool thr_hit;
> + int ret;
> +
> + mutex_lock(&priv->lock);
> +
> + /*
> + * Walk the bitmap and eventually perform several transfers.
> + * Bitmap ones-fields that are separated by gaps <= xfer_burst_break_thr
> + * 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.
> + */
> + for_each_set_bitrange(start, end, iio_dev->active_scan_mask,
> + iio_dev->masklength) {
> + /*
> + * First transfer will start from the beginning of the first
> + * ones-field in the bitmap
> + */
> + if (first) {
> + xfer_start = start;
> + } else {
> + /*
> + * 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).
> + */
> +
> + /*
> + * In case the zeros-gap contains the quaternion bit,
> + * then its length is actually 4 words instead of 1
> + * (i.e. +3 wrt other channels).
> + */
> + quat_extra_len = ((start > BNO055_SCAN_QUATERNION) &&
you could reduce the scope of quat_extra_len and thr_hit, perhaps
to just within the for loop. Perhaps it's not worth bothering though. Up to you.
> + (prev_end <= BNO055_SCAN_QUATERNION)) ? 3 : 0;
> +
> + /* If the gap is wider than xfer_burst_break_thr then.. */
> + thr_hit = (start - prev_end + quat_extra_len) >
> + priv->xfer_burst_break_thr;
> +
> + /*
> + * .. transfer all the data up to the gap. Then set the
> + * next transfer start index at right after the gap
> + * (i.e. at the start of this ones-field).
> + */
> + if (thr_hit) {
> + mask = *iio_dev->active_scan_mask >> xfer_start;
> + ret = bno055_scan_xfer(priv, xfer_start,
> + prev_end - xfer_start,
> + mask, priv->buf.chans, &buf_idx);
> + if (ret)
> + goto done;
> + xfer_start = start;
> + }
> + }
> + first = false;
> + prev_end = end;
> + }
> +
> + /*
> + * We finished walking the bitmap; no more gaps to check for. Just
> + * perform the current transfer.
> + */
> + mask = *iio_dev->active_scan_mask >> xfer_start;
> + ret = bno055_scan_xfer(priv, xfer_start,
> + prev_end - xfer_start,
> + mask, priv->buf.chans, &buf_idx);
Check ret and if it's an error don't push the data.
> +
> + iio_push_to_buffers_with_timestamp(iio_dev, &priv->buf, pf->timestamp);
> +done:
> + mutex_unlock(&priv->lock);
> + iio_trigger_notify_done(iio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +int bno055_probe(struct device *dev, struct regmap *regmap,
> + int xfer_burst_break_thr, bool sw_reset)
> +{
...
> +
> + /*
> + * The stock FW version contains a bug (see comment at the beginning of
> + * this file) that causes the anglvel scale to be changed depending on
> + * the chip range setting. We workaround this, but we don't know what
> + * other FW versions might do.
> + */
> + if (ver != 0x3 || rev != 0x11)
> + dev_warn(dev, "Untested firmware version. Anglvel scale may not work as expected\n");
> +
> + ret = regmap_bulk_read(priv->regmap, BNO055_UID_LOWER_REG,
> + priv->uid, BNO055_UID_LEN);
> + if (ret)
> + return ret;
> +
> + /* Sensor calibration data */
> + fw_name_buf = kasprintf(GFP_KERNEL,
> + BNO055_FW_UID_FMT,
> + BNO055_UID_LEN, priv->uid);
No need to break this into so many lines. Can get it on two without
passing 80 chars.
next prev parent reply other threads:[~2022-06-18 17:17 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-13 12:05 [v6 00/14] Add support for Bosch BNO055 IMU andrea.merello
2022-06-13 12:05 ` [v6 01/14] iio: add modifiers for linear acceleration andrea.merello
2022-06-13 12:05 ` [v6 02/14] iio: document linear acceleration modifiers andrea.merello
2022-06-13 12:05 ` [v6 03/14] iio: event_monitor: add " andrea.merello
2022-06-13 12:05 ` [v6 04/14] iio: add modifers for pitch, yaw, roll andrea.merello
2022-06-13 12:05 ` [v6 05/14] iio: document pitch, yaw, roll modifiers andrea.merello
2022-06-13 12:05 ` [v6 06/14] iio: event_monitor: add pitch, yaw and " andrea.merello
2022-06-13 12:05 ` [v6 07/14] iio: add support for binary attributes andrea.merello
2022-06-13 12:05 ` [v6 08/14] iio: imu: add Bosch Sensortec BNO055 core driver andrea.merello
2022-06-13 16:44 ` Andy Shevchenko
2022-06-14 9:11 ` Andrea Merello
2022-06-14 10:58 ` Andy Shevchenko
2022-06-14 12:15 ` Andrea Merello
2022-06-14 15:10 ` Andy Shevchenko
2022-06-14 15:27 ` Andrea Merello
2022-06-18 17:26 ` Jonathan Cameron [this message]
2022-06-13 12:05 ` [v6 09/14] iio: document bno055 private sysfs attributes andrea.merello
2022-06-13 12:05 ` [v6 10/14] iio: document "serialnumber" sysfs attribute andrea.merello
2022-06-13 12:05 ` [v6 11/14] dt-bindings: iio/imu: Add Bosch BNO055 andrea.merello
2022-06-13 12:05 ` [v6 12/14] iio: imu: add BNO055 serdev driver andrea.merello
2022-06-13 16:52 ` Andy Shevchenko
2022-06-15 20:57 ` kernel test robot
2022-06-15 21:13 ` Andy Shevchenko
2022-06-15 21:15 ` Andy Shevchenko
2022-07-03 1:52 ` kernel test robot
2022-06-13 12:05 ` [v6 13/14] iio: imu: add BNO055 I2C driver andrea.merello
2022-06-13 12:05 ` [v6 14/14] docs: iio: add documentation for BNO055 driver andrea.merello
2022-07-03 7:58 ` kernel test robot
2022-07-03 13:11 ` Bagas Sanjaya
2022-07-03 14:00 ` Andy Shevchenko
2022-07-04 1:21 ` Bagas Sanjaya
2022-07-04 3:40 ` [PATCH v2] Documentation: bno055: separate SPDX identifier and page title Bagas Sanjaya
2022-07-04 19:49 ` Andy Shevchenko
2022-07-05 1:13 ` Bagas Sanjaya
2022-07-05 9:02 ` Andy Shevchenko
2022-07-13 16:09 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220618182618.4da1df69@jic23-huawei \
--to=jic23@kernel.org \
--cc=andrea.merello@iit.it \
--cc=andy.shevchenko@gmail.com \
--cc=ardeleanalex@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jacopo@jmondi.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matt.ranostay@konsulko.com \
--cc=mchehab+huawei@kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).