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