Messages in this thread |  | | From | Andy Shevchenko <> | Date | Mon, 13 Jun 2022 18:44:09 +0200 | Subject | Re: [v6 08/14] iio: imu: add Bosch Sensortec BNO055 core driver |
| |
On Mon, Jun 13, 2022 at 2:05 PM <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.
...
> +config BOSCH_BNO055_IIO
Does it need _IIO suffix? Any name collision?
...
> +static int bno055_acc_lpf_vals[] = { > + 7, 810000, 15, 630000, 31, 250000, 62, 500000, > + 125, 0, 250, 0, 500, 0, 1000, 0
+ Comma?
> +};
...
> + /* G: 2, 4, 8, 16 */
Indentation of this comment is a bit off.
> +static int bno055_acc_range_vals[] = {1962, 3924, 7848, 15696};
Perhaps split this to 4 lines and put the comment on top of the third line?
...
> +static int bno055_gyr_scale_vals[] = { > + 125, 1877467, 250, 1877467, 500, 1877467, > + 1000, 1877467, 2000, 1877467
+ Comma?
> +};
...
> +#ifdef CONFIG_DEBUG_FS > + struct dentry *debugfs; > +#endif
...
> + /* > + * IMU reports sensor offests; IIO wants correction
offsets
> + * offsets, thus we need the 'minus' here. > + */
...
> + if (kstrtobool(buf, &en)) > + return -EINVAL;
Why shadow an actual error code(s)?
...
> + ret = kstrtoul(buf, 10, &val); > + if (ret) > + return ret;
Here it's done properly (see just above).
...
> +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);
Shouldn't we report the potential error here? It's not directly related to debugfs, but something which is not directly related.
> +}
...
> +static IIO_DEVICE_ATTR(fusion_enable, 0644, > + bno055_fusion_enable_show, > + bno055_fusion_enable_store, 0);
IIO_DEVICE_ATTR_RW()
> +static IIO_DEVICE_ATTR(in_magn_calibration_fast_enable, 0644, > + bno055_fmc_enable_show, > + bno055_fmc_enable_store, 0); > + > +static IIO_DEVICE_ATTR(in_accel_range_raw, 0644, > + bno055_in_accel_range_show, > + bno055_in_accel_range_store, 0);
Ditto for above.
...
> + /* > + * All chans are made up 1 16-bit sample, except for quaternion that is
channels
> + * made up 4 16-bit values. > + * For us the quaternion CH is just like 4 regular CHs. > + * If our 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. > + */
-- With Best Regards, Andy Shevchenko
|  |