Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 15 Apr 2022 18:43:05 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Andrea Merello <andrea.merello@gmail.com>
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, Andrea Merello <andrea.merello@iit.it>
Subject: Re: [v4 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
Message-ID: <20220415184305.03805452@jic23-huawei>
In-Reply-To: <20220415130005.85879-9-andrea.merello@gmail.com>
References: <20220415130005.85879-1-andrea.merello@gmail.com>
        <20220415130005.85879-9-andrea.merello@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 15 Apr 2022 14:59:59 +0200
Andrea Merello <andrea.merello@gmail.com> wrote:

> From: Andrea Merello <andrea.merello@iit.it>
> 
> 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@iit.it>
Hi Andrea,

A few trivial things from me on this read through.

I haven't commented on a lot of the patches because I was happy with them.

Other than the small changes requested from me, we mostly need to get
device tree review of the binding and allow time for others to take
another look.

Thanks,

Jonathan


> ---

...

> +
> +static int bno055_read_simple_chan(struct iio_dev *indio_dev,
> +				   struct iio_chan_spec const *chan,
> +				   int *val, int *val2, long mask)
> +{
> +	struct bno055_priv *priv = iio_priv(indio_dev);
> +	__le16 raw_val;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(priv->regmap, chan->address,
> +				       &raw_val, sizeof(raw_val));
> +		if (ret < 0)
> +			return ret;
> +		*val = (s16)le16_to_cpu(raw_val);

Hmm. I'd ever so slightly prefer

sign_extend32(le16_to_cpu(raw_val), 15) as it makes it extremely clear
what is going on and hopefully the compiler can do a good job
of optimising it either way.  If you strongly prefer the current
approach I'll cope :)

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (priv->operation_mode != BNO055_OPR_MODE_AMG) {
> +			*val = 0;
> +		} else {
> +			ret = regmap_bulk_read(priv->regmap,
> +					       chan->address +
> +					       BNO055_REG_OFFSET_ADDR,
> +					       &raw_val, sizeof(raw_val));
> +			if (ret < 0)
> +				return ret;
> +			/*
> +			 * IMU reports sensor offests; IIO wants correction
> +			 * offset, thus we need the 'minus' here.
> +			 */
> +			*val = -(s16)le16_to_cpu(raw_val);
> +		}
> +		return IIO_VAL_INT;
...

> +}

...

> +
> +static int bno055_read_quaternion(struct iio_dev *indio_dev,
> +				  struct iio_chan_spec const *chan,
> +				  int size, int *vals, int *val_len,
> +				  long mask)
> +{
> +	struct bno055_priv *priv = iio_priv(indio_dev);
> +	__le16 raw_vals[4];
> +	int i, ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (size < 4)
> +			return -EINVAL;
> +		ret = regmap_bulk_read(priv->regmap,
> +				       BNO055_QUAT_DATA_W_LSB_REG,
> +				       raw_vals, sizeof(raw_vals));
> +		if (ret < 0)
> +			return ret;
> +		for (i = 0; i < 4; i++)
> +			vals[i] = (s16)le16_to_cpu(raw_vals[i]);
> +		*val_len = 4;
> +		return IIO_VAL_INT_MULTIPLE;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Table 3-31: 1 quaternion = 2^14 LSB */
> +		if (size < 2)
> +			return -EINVAL;
> +		vals[0] = 14;
> +		vals[1] = 1 << 14;
This looks odd.  As you are using FRACTIONAL_LOG2, I think that should
just be
vals[0] = 1;
vals[1] = 14;

> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +


...

> +
> +static void bno055_clk_disable(void *arg)
> +{
> +	clk_disable_unprepare((struct clk *)arg);
> +}

Hopefully one day: https://lore.kernel.org/all/CAHp75VdH4vGr57v6tfkRuxh-3agRKO8C08+DH8dsB1HnPfnz5Q@mail.gmail.com/
will get merged...  Until then, this is the best we can do.

> +
> +int bno055_probe(struct device *dev, struct regmap *regmap,
> +		 int xfer_burst_break_thr, bool sw_reset)
> +{
> +	const struct firmware *caldata;
See comment below. I think you need to set this to NULL here


> +
> +	ret = regmap_read(priv->regmap, BNO055_CHIP_ID_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != BNO055_CHIP_ID_MAGIC) {

We've run into this a few times recently.  Traditionally IIO has been very
restrictive on allowing drivers to probe if the Who Am I type values
don't match.  That causes problems for backwards compatibility in
device tree - e.g. (with made up compatible part number 055b :)
compatible = "bosch,bno055b", "bosch,bno055"

The viewpoint of the dt maintainers is that we should assume the
dt is correct and at most warn about missmatched IDs before trying
to carry on.  So to avoid hitting that again please relax this to a
warning and cross your fingers after this point if it doesn't match.
I'm fine on the firmware question because we know we are dealing
with buggy firmware.  Ideally we'll get some working firmware
additions at somepoint then we can just label the bad firmwares
and assume one less bug in the ones that don't match :)

> +		dev_err(dev, "Unrecognized chip ID 0x%x", val);
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * In case we haven't a HW reset pin, we can still reset the chip via
> +	 * register write. This is probably nonsense in case we can't even
> +	 * communicate with the chip or the chip isn't the one we expect (i.e.
> +	 * we don't write to unknown chips), so we perform SW reset only after
> +	 * chip magic ID check
> +	 */
> +	if (!priv->reset_gpio) {
> +		ret = bno055_system_reset(priv);
> +		if (ret)
> +			return ret;
> +	}
> +

...

> +	ret = regmap_bulk_read(priv->regmap, BNO055_UID_LOWER_REG,
> +			       priv->uid, BNO055_UID_LEN);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * This has nothing to do with the IMU firmware, this is for sensor
> +	 * calibration data.

I'd not say what it isn't.  What it is will be enough.

	/* Sensor calibration data */

> +	 */
> +	fw_name_buf = kasprintf(GFP_KERNEL,
> +				BNO055_FW_UID_NAME,
> +				BNO055_UID_LEN, priv->uid);
> +	if (!fw_name_buf)
> +		return -ENOMEM;
> +
> +	ret = request_firmware(&caldata, fw_name_buf, dev);
> +	kfree(fw_name_buf);
> +	if (ret)
> +		ret = request_firmware(&caldata, BNO055_FW_GENERIC_NAME, dev);
> +
> +	if (ret)
> +		dev_notice(dev, "Calibration file load failed. See instruction in kernel Documentation/iio/bno055.rst");

If no calibration files are found, is caldata guaranteed to have been set to NULL?
I'm not seeing that in the documentation, so even if it is true today it might not
be in future and I think that leaves you referencing a variable that may not
have been set.   Probably just set caldata_data = NULL where you define
the local variable at top of this function.

> +
> +	if (caldata) {
> +		caldata_data = caldata->data;
> +		caldata_size = caldata->size;
> +	}
> +	ret = bno055_init(priv, caldata_data, caldata_size);
> +	if (caldata)
> +		release_firmware(caldata);
> +	if (ret)
> +		return ret;
> +