Re: [v4 08/14] iio: imu: add Bosch Sensortec BNO055 core driver

[Andrea Merello <andrea.merello@gmail.com>]

Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> 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

Ok, good! As usual, just a few inline comments, ok for the rest.

> > +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

Hum. I'm confused here: I think I did set it to NULL (is some later
LOC) in V2, but you argued against it, because hopefully
request_firmware() does it by itself.
https://www.spinics.net/lists/linux-iio/msg64896.html

What has changed or what I've missed? Was your original point just to
move the NULL assignment back at declaration time?

>
> > +
> > +     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 :)

To be honest my point wasn't about the correctness of the DT at all..

I've hit this several times when I was switching my test board from
serial to i2c and vice-versa, because I made wrong connections or I
forgot to switch FPGA image (which contains the serial IP here). I got
my test script failing because the IIO device didn't pop up at all,
which is better than getting e.g. random data. In the real world
people may have less chance to have to worry about this, but they may
when e.g. they have an RPi and a hand-wired IMU.

.. IOW I'm seeing this as a hardware self-test rather than a SW
check.. But if the DT thing makes this a no-go, then I can live with
the warning, and e.g. by making my script to check the kernel log..