Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220415130005.85879-1-andrea.merello@gmail.com>
 <20220415130005.85879-9-andrea.merello@gmail.com> <20220415184305.03805452@jic23-huawei>
 <CAN8YU5Mz--8R2oE=bgok_JdM6NNW8m2h5_V8LZSocFnaa-PADA@mail.gmail.com> <20220424184521.3f5a9d18@jic23-huawei>
In-Reply-To: <20220424184521.3f5a9d18@jic23-huawei>
Reply-To: andrea.merello@gmail.com
From:   Andrea Merello <andrea.merello@gmail.com>
Date:   Tue, 26 Apr 2022 11:28:53 +0200
Message-ID: <CAN8YU5OB5A0m3gQ0J-PTiEdcSTY_KXONK4V6sjmFEyyK0bmVmw@mail.gmail.com>
Subject: Re: [v4 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Matt Ranostay <matt.ranostay@konsulko.com>,
        Alexandru Ardelean <ardeleanalex@gmail.com>,
        Jacopo Mondi <jacopo@jmondi.org>,
        Andrea Merello <andrea.merello@iit.it>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Il giorno dom 24 apr 2022 alle ore 19:37 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> On Tue, 19 Apr 2022 09:10:54 +0200
> Andrea Merello <andrea.merello@gmail.com> wrote:
>
> > 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>

[...]

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

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