From: Jonathan Cameron <jic23@kernel.org>
To: Andrea Merello <andrea.merello@gmail.com>
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>
Subject: Re: [v4 08/14] iio: imu: add Bosch Sensortec BNO055 core driver
Date: Sun, 24 Apr 2022 18:45:21 +0100 [thread overview]
Message-ID: <20220424184521.3f5a9d18@jic23-huawei> (raw)
In-Reply-To: <CAN8YU5Mz--8R2oE=bgok_JdM6NNW8m2h5_V8LZSocFnaa-PADA@mail.gmail.com>
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>
> > 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
next prev parent reply other threads:[~2022-04-24 17:37 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-15 12:59 [v4 00/14] Add support for Bosch BNO055 IMU Andrea Merello
2022-04-15 12:59 ` [v4 01/14] iio: add modifiers for linear acceleration Andrea Merello
2022-04-15 12:59 ` [v4 02/14] iio: document linear acceleration modifiers Andrea Merello
2022-04-15 12:59 ` [v4 03/14] iio: event_monitor: add " Andrea Merello
2022-04-16 8:37 ` Andy Shevchenko
2022-04-15 12:59 ` [v4 04/14] iio: add modifers for pitch, yaw, roll Andrea Merello
2022-04-16 8:38 ` Andy Shevchenko
2022-04-15 12:59 ` [v4 05/14] iio: document pitch, yaw, roll modifiers Andrea Merello
2022-04-15 12:59 ` [v4 06/14] iio: event_monitor: add pitch, yaw and " Andrea Merello
2022-04-15 12:59 ` [v4 07/14] iio: add support for binary attributes Andrea Merello
2022-04-15 12:59 ` [v4 08/14] iio: imu: add Bosch Sensortec BNO055 core driver Andrea Merello
2022-04-15 17:43 ` Jonathan Cameron
2022-04-19 7:10 ` Andrea Merello
2022-04-24 17:45 ` Jonathan Cameron [this message]
2022-04-26 9:28 ` Andrea Merello
2022-04-28 18:45 ` Jonathan Cameron
2022-04-15 13:00 ` [v4 09/14] iio: document bno055 private sysfs attributes Andrea Merello
2022-04-15 13:00 ` [v4 10/14] iio: document "serial_number" sysfs attribute Andrea Merello
2022-04-19 7:41 ` Lars-Peter Clausen
2022-04-24 17:46 ` Jonathan Cameron
2022-04-15 13:00 ` [v4 11/14] dt-bindings: iio: imu: add documentation for Bosch BNO055 bindings Andrea Merello
2022-04-25 22:08 ` Rob Herring
2022-04-15 13:00 ` [v4 12/14] iio: imu: add BNO055 serdev driver Andrea Merello
2022-04-15 16:48 ` Jonathan Cameron
2022-04-16 8:44 ` Andy Shevchenko
2022-04-19 7:48 ` Andrea Merello
2022-04-19 8:29 ` Andy Shevchenko
2022-04-20 7:22 ` Andrea Merello
2022-04-15 13:00 ` [v4 13/14] iio: imu: add BNO055 I2C driver Andrea Merello
2022-04-15 16:49 ` Jonathan Cameron
2022-04-15 13:00 ` [v4 14/14] docs: iio: add documentation for BNO055 driver Andrea Merello
2022-04-16 8:40 ` [v4 00/14] Add support for Bosch BNO055 IMU Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220424184521.3f5a9d18@jic23-huawei \
--to=jic23@kernel.org \
--cc=andrea.merello@gmail.com \
--cc=andrea.merello@iit.it \
--cc=andy.shevchenko@gmail.com \
--cc=ardeleanalex@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jacopo@jmondi.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matt.ranostay@konsulko.com \
--cc=mchehab+huawei@kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).