Date:   Sun, 24 Apr 2022 18:45:21 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Andrea Merello <andrea.merello@...il.com>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        Alexandru Ardelean <ardeleanalex@...il.com>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Andrea Merello <andrea.merello@....it>
Subject: Re: [v4 08/14] iio: imu: add Bosch Sensortec BNO055 core driver

On Tue, 19 Apr 2022 09:10:54 +0200
Andrea Merello <andrea.merello@...il.com> wrote:

> Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
> <jic23@...nel.org> ha scritto:
> >
> > On Fri, 15 Apr 2022 14:59:59 +0200
> > Andrea Merello <andrea.merello@...il.com> wrote:
> >  
> > > From: Andrea Merello <andrea.merello@....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@....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?

Oops. Not sure what I was smoking that day.

Moving back to declaration time is a good idea though.

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

Jonathan

Powered by blists - more mailing lists