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

[Jonathan Cameron]

On Tue, 26 Apr 2022 11:28:53 +0200
Andrea Merello <andrea.merello@xxxxxxxxx> wrote:

> Il giorno dom 24 apr 2022 alle ore 19:37 Jonathan Cameron
> <jic23@xxxxxxxxxx> ha scritto:
> >
> > On Tue, 19 Apr 2022 09:10:54 +0200
> > Andrea Merello <andrea.merello@xxxxxxxxx> wrote:
> >  
> > > Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
> > > <jic23@xxxxxxxxxx> ha scritto:  
> > > >
> > > > On Fri, 15 Apr 2022 14:59:59 +0200
> > > > Andrea Merello <andrea.merello@xxxxxxxxx> wrote:
> > > >  
> > > > > From: Andrea Merello <andrea.merello@xxxxxx>
> > > > >
> > > > > 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@xxxxxx>  
> 
> [...]
> 
> > > >  
> > > > > +
> > > > > +     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..  
> >
> > Hmm. I  wonder if we can get the best of both worlds.  Given there
> > is a WHOAMI and these very rarely / never take the value of all 0's or all 1's
> > (what you'd see with a wiring error) maybe we can sanity check against
> > those to provide the hardware self-test element.  Then accept any
> > 'sane' value of WHOAMI, but with a warning?  
> 
> While trying to do this and testing it, I've realized that indeed when
> the BUS is broken (e.g. incorrect wiring) the probe() fails even
> earlier. When we are unable to communicate with the device, this is
> caught by the lower layer protocols (e.g. I2C sees no ACK, I suppose),
> so there is no need to fail here; the IIO device doesn't eventually
> pop up anyway.

Ah. Good point.  I was thinking we had SPI which is the one where a lack
of reply is harder to detect.  For I2C we are definitely fine and
I guess the serial protocol protects against this as well.

Great that indeed makes things simpler.

Jonathan


> 
> So, I now revert my previous request to keep a check to bail out for
> crazy IDs here :) ; I'd say we can just relax the check to just a
> warning as you said before, without the need for checking for 0x00 and
> 0xff..
> 
> > Jonathan
> >
> >