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
