Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220613120534.36991-1-andrea.merello@iit.it> <20220613120534.36991-9-andrea.merello@iit.it>
In-Reply-To: <20220613120534.36991-9-andrea.merello@iit.it>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 13 Jun 2022 18:44:09 +0200
Message-ID: <CAHp75Vdh8kAH6540xCwzFh5uf=QMVTHC42a8pOgvkpObzjfD+w@mail.gmail.com>
Subject: Re: [v6 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
To:     Andrea Merello <andrea.merello@iit.it>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Matt Ranostay <matt.ranostay@konsulko.com>,
        Alexandru Ardelean <ardeleanalex@gmail.com>,
        jmondi <jacopo@jmondi.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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