On Mon, Jun 13, 2022 at 2:05 PM <[email protected]> wrote:
>
> From: Andrea Merello <[email protected]>
>
> 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
Few inline comments, OK for the rest.
>...
>
>> + /* 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?
Not sure what you mean here, sorry. May you elaborate or provide an example, please?
>...
>
>> +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.
The error eventually comes out from something that has nothing to do with debugs per se (i.e. the devm stuff), but it will only affect debugfs indeed.
Assuming that we don't want to make the whole driver fail in case debugfs stuff fails (see last part of the comment above debugfs_create_file() implementation), and given that the devm_add_action_or_reset(), should indeed "reset" in case of failure (i.e. we should be in a clean situation anyway), I would say it should be OK not to propagate the error and let things go on.
However we can add a dev_warn() to report what happened.