On Fri, 15 Apr 2022 14:59:59 +0200
Andrea Merello <[email protected]> wrote:
> From: Andrea Merello <[email protected]>
>
> 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 <[email protected]>
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
> ---
...
> +
> +static int bno055_read_simple_chan(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct bno055_priv *priv = iio_priv(indio_dev);
> + __le16 raw_val;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_bulk_read(priv->regmap, chan->address,
> + &raw_val, sizeof(raw_val));
> + if (ret < 0)
> + return ret;
> + *val = (s16)le16_to_cpu(raw_val);
Hmm. I'd ever so slightly prefer
sign_extend32(le16_to_cpu(raw_val), 15) as it makes it extremely clear
what is going on and hopefully the compiler can do a good job
of optimising it either way. If you strongly prefer the current
approach I'll cope :)
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_OFFSET:
> + if (priv->operation_mode != BNO055_OPR_MODE_AMG) {
> + *val = 0;
> + } else {
> + ret = regmap_bulk_read(priv->regmap,
> + chan->address +
> + BNO055_REG_OFFSET_ADDR,
> + &raw_val, sizeof(raw_val));
> + if (ret < 0)
> + return ret;
> + /*
> + * IMU reports sensor offests; IIO wants correction
> + * offset, thus we need the 'minus' here.
> + */
> + *val = -(s16)le16_to_cpu(raw_val);
> + }
> + return IIO_VAL_INT;
...
> +}
...
> +
> +static int bno055_read_quaternion(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int size, int *vals, int *val_len,
> + long mask)
> +{
> + struct bno055_priv *priv = iio_priv(indio_dev);
> + __le16 raw_vals[4];
> + int i, ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (size < 4)
> + return -EINVAL;
> + ret = regmap_bulk_read(priv->regmap,
> + BNO055_QUAT_DATA_W_LSB_REG,
> + raw_vals, sizeof(raw_vals));
> + if (ret < 0)
> + return ret;
> + for (i = 0; i < 4; i++)
> + vals[i] = (s16)le16_to_cpu(raw_vals[i]);
> + *val_len = 4;
> + return IIO_VAL_INT_MULTIPLE;
> + case IIO_CHAN_INFO_SCALE:
> + /* Table 3-31: 1 quaternion = 2^14 LSB */
> + if (size < 2)
> + return -EINVAL;
> + vals[0] = 14;
> + vals[1] = 1 << 14;
This looks odd. As you are using FRACTIONAL_LOG2, I think that should
just be
vals[0] = 1;
vals[1] = 14;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
...
> +
> +static void bno055_clk_disable(void *arg)
> +{
> + clk_disable_unprepare((struct clk *)arg);
> +}
Hopefully one day: https://lore.kernel.org/all/CAHp75VdH4vGr57v6tfkRuxh-3agRKO8C08+DH8dsB1HnPfnz5Q@mail.gmail.com/
will get merged... Until then, this is the best we can do.
> +
> +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
> +
> + 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 :)
> + dev_err(dev, "Unrecognized chip ID 0x%x", val);
> + return -ENODEV;
> + }
> +
> + /*
> + * In case we haven't a HW reset pin, we can still reset the chip via
> + * register write. This is probably nonsense in case we can't even
> + * communicate with the chip or the chip isn't the one we expect (i.e.
> + * we don't write to unknown chips), so we perform SW reset only after
> + * chip magic ID check
> + */
> + if (!priv->reset_gpio) {
> + ret = bno055_system_reset(priv);
> + if (ret)
> + return ret;
> + }
> +
...
> + ret = regmap_bulk_read(priv->regmap, BNO055_UID_LOWER_REG,
> + priv->uid, BNO055_UID_LEN);
> + if (ret)
> + return ret;
> +
> + /*
> + * This has nothing to do with the IMU firmware, this is for sensor
> + * calibration data.
I'd not say what it isn't. What it is will be enough.
/* Sensor calibration data */
> + */
> + fw_name_buf = kasprintf(GFP_KERNEL,
> + BNO055_FW_UID_NAME,
> + BNO055_UID_LEN, priv->uid);
> + if (!fw_name_buf)
> + return -ENOMEM;
> +
> + ret = request_firmware(&caldata, fw_name_buf, dev);
> + kfree(fw_name_buf);
> + if (ret)
> + ret = request_firmware(&caldata, BNO055_FW_GENERIC_NAME, dev);
> +
> + if (ret)
> + dev_notice(dev, "Calibration file load failed. See instruction in kernel Documentation/iio/bno055.rst");
If no calibration files are found, is caldata guaranteed to have been set to NULL?
I'm not seeing that in the documentation, so even if it is true today it might not
be in future and I think that leaves you referencing a variable that may not
have been set. Probably just set caldata_data = NULL where you define
the local variable at top of this function.
> +
> + if (caldata) {
> + caldata_data = caldata->data;
> + caldata_size = caldata->size;
> + }
> + ret = bno055_init(priv, caldata_data, caldata_size);
> + if (caldata)
> + release_firmware(caldata);
> + if (ret)
> + return ret;
> +
Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
<[email protected]> ha scritto:
>
> On Fri, 15 Apr 2022 14:59:59 +0200
> Andrea Merello <[email protected]> wrote:
>
> > From: Andrea Merello <[email protected]>
> >
> > 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 <[email protected]>
> 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..
On Tue, 19 Apr 2022 09:10:54 +0200
Andrea Merello <[email protected]> wrote:
> Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
> <[email protected]> ha scritto:
> >
> > On Fri, 15 Apr 2022 14:59:59 +0200
> > Andrea Merello <[email protected]> wrote:
> >
> > > From: Andrea Merello <[email protected]>
> > >
> > > 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 <[email protected]>
> > 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
Il giorno dom 24 apr 2022 alle ore 19:37 Jonathan Cameron
<[email protected]> ha scritto:
>
> On Tue, 19 Apr 2022 09:10:54 +0200
> Andrea Merello <[email protected]> wrote:
>
> > Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
> > <[email protected]> ha scritto:
> > >
> > > On Fri, 15 Apr 2022 14:59:59 +0200
> > > Andrea Merello <[email protected]> wrote:
> > >
> > > > From: Andrea Merello <[email protected]>
> > > >
> > > > 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 <[email protected]>
[...]
> > >
> > > > +
> > > > + 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
>
>
On Tue, 26 Apr 2022 11:28:53 +0200
Andrea Merello <[email protected]> wrote:
> Il giorno dom 24 apr 2022 alle ore 19:37 Jonathan Cameron
> <[email protected]> ha scritto:
> >
> > On Tue, 19 Apr 2022 09:10:54 +0200
> > Andrea Merello <[email protected]> wrote:
> >
> > > Il giorno ven 15 apr 2022 alle ore 19:35 Jonathan Cameron
> > > <[email protected]> ha scritto:
> > > >
> > > > On Fri, 15 Apr 2022 14:59:59 +0200
> > > > Andrea Merello <[email protected]> wrote:
> > > >
> > > > > From: Andrea Merello <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
>
> [...]
>
> > > >
> > > > > +
> > > > > + 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
> >
> >