LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Andrea Merello <andrea.merello@gmail.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Rob Herring <robh+dt@kernel.org>,
	Matt Ranostay <matt.ranostay@konsulko.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>,
	Andrea Merello <andrea.merello@iit.it>
Subject: Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver
Date: Sat, 31 Jul 2021 19:01:03 +0100	[thread overview]
Message-ID: <20210731190103.6e2a3d41@jic23-huawei> (raw)
In-Reply-To: <CAN8YU5PcrR-xM5A=3jd50=UaY9wWDJZGBqajmvM8Te1Ly14Hew@mail.gmail.com>

On Mon, 26 Jul 2021 16:36:49 +0200
Andrea Merello <andrea.merello@gmail.com> wrote:

> just a few of in-line comment below; OK for all the rest of your
> comment, thanks!
> 
> > > > > +static int bno055_reg_write(struct bno055_priv *priv,
> > > > > +                         unsigned int reg, unsigned int val)
> > > > > +{
> > > > > +     int res = regmap_write(priv->regmap, reg, val);
> > > > > +
> > > > > +     if (res && res != -ERESTARTSYS) {  
> > > >
> > > > I think Andy asked about these, so I won't repeat...
> > > > Nice to get rid of those and just be able to make the regmap calls inline though...  
> > >
> > > Ok for inline. I've just answered in another mail to Andy's comments
> > > for the rest.  
> 
> Indeed, so far I couldn't understand what do you really mean. Should I
> move those check+dev_err() inside the regmap core layer ?

Better to move any necessary print to the caller of the reg_write() where any
error message can often give more information.  Adding wrappers just to print
an error message normally mostly serves to make the code a little harder to
review.

> 
> > > > > +     /*
> > > > > +      * Start in fusion mode (all data available), but with magnetometer auto
> > > > > +      * calibration switched off, in order not to overwrite magnetometer
> > > > > +      * calibration data in case one want to keep it untouched.  
> > > >
> > > > Why might you? good to have a default that is what people most commonly want...
> > > > If there is a usecase for this then it may be better to have a 'disable autocalibration
> > > > and manually reload a fixed calibration' path.  
> > >
> > > I'm not sure whether disabling autocalibration for magnetometer is
> > > just a matter of saving some power, or whether this has the purpose of
> > > carefully doing the calibration far from magnetic disturbances,
> > > avoiding screwing the calibration every time you briefly pass by a
> > > piece of iron. I think I found some clues for this second
> > > interpretation poking on the internet, but I don't know whether they
> > > were right.  
> >
> > It's possible if the calibration routines have much faster response than
> > you'd normally expect.  
> 
> This HW function is called "Fast Magnetometer Calibration".. But I
> don't know how fast is it..

Nice  - got to love informative datasheets :)

> 
> 
> > > > > +     &iio_dev_attr_in_accel_filter_low_pass_3db_frequency_available.dev_attr.attr,
> > > > > +     &iio_dev_attr_in_anglvel_range_available.dev_attr.attr,  
> > > >
> > > > Hmm. Range typically maps to something else (normally scale, but these smart
> > > > sensors can do weird things)  
> > >
> > > Here the scaling doesn't change, just the range. I *think* that by
> > > changing range you also get better or worse precision.  
> >
> > oh goody.  Make sure the default is maximum range + when you document this
> > we will have to be careful to make it clear we don't want this to be used in
> > drivers where scale is an option.  Perhaps we just put it in a device
> > specific ABI file.
> >  
> 
> The default is to run the IMU with fusion mode enabled; in this mode
> those parameters are locked by the HW to a given value (which is not
> the maximum e.g. in case of accelerometer range).
> 
> If the user disables the fusion mode, then those parameters become
> tweakable, but shouldn't they just remain at their previous values
> (the one set by fusion mode), unless the user change also them?

That makes sense to me.

> 
> I.o.w the only chance we have for assigning them a "default" value is
> when the fusion is switched off, but this would mean that switching
> off fusion mode also has a side effect on those values (which I'm
> unsure if we really want to happen).

Thanks for the explanation.  Ok. Fine to have the range here, but please
sanity check we have appropriate ABI documentation in the main ABI doc
Documentation/ABI/testing/sysfs-bus-iio.
One thing to think about is how range would generalize.   These sensors are
symmetric, but not all are - a range that is higher in postive than negative
is definitely possible.  Perhaps we need to name it to make it clear we are
talking magnitude here? 

Ideally it should state something about range should be used only when it has no
affect on scaling.  Hopefully we'll not get a device where it has an affect
but it's non linear as that doesn't make any sense.  I'd imagine a range
control in hardware either is proportional to the scale, or has no affect on
it as here.

Thanks,

Jonathan

> 
> Andrea


  reply	other threads:[~2021-07-31 17:58 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-15 14:17 [PATCH 0/4] Add support for Bosch BNO055 IMU Andrea Merello
2021-07-15 14:17 ` [PATCH 1/4] iio: add modifiers for linear acceleration Andrea Merello
2021-07-17 14:32   ` Jonathan Cameron
2021-07-15 14:17 ` [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver Andrea Merello
2021-07-15 16:49   ` Andy Shevchenko
2021-07-16  9:19     ` Andrea Merello
2021-07-16 12:39       ` Andy Shevchenko
2021-07-19  7:12         ` Andrea Merello
2021-07-19 10:33         ` Andrea Merello
2021-07-19  9:02     ` Andrea Merello
2021-07-19 11:48       ` Andy Shevchenko
2021-07-19 13:13         ` Andrea Merello
2021-07-16  7:24   ` Alexandru Ardelean
2021-07-16  9:49     ` Andrea Merello
2021-07-17 15:32   ` Jonathan Cameron
2021-07-19  8:30     ` Andrea Merello
2021-07-24 17:08       ` Jonathan Cameron
2021-07-26 14:36         ` Andrea Merello
2021-07-31 18:01           ` Jonathan Cameron [this message]
2021-08-04 10:06             ` Andrea Merello
2021-08-04 16:50               ` Jonathan Cameron
2021-08-04 19:27                 ` Andy Shevchenko
2021-07-15 14:17 ` [PATCH 3/4] dt-bindings: iio: imu: add bosch BNO055 serdev driver bindings Andrea Merello
2021-07-17 15:39   ` Jonathan Cameron
2021-07-19  8:44     ` Andrea Merello
2021-07-15 14:17 ` [PATCH 4/4] iio: imu: add BNO055 serdev driver Andrea Merello
2021-07-15 17:08   ` kernel test robot
2021-07-15 18:14   ` kernel test robot
2021-07-17 15:50   ` Jonathan Cameron
2021-07-19  8:49     ` Andrea Merello
2021-07-19 11:55       ` Andy Shevchenko
2021-07-19 12:59         ` Andrea Merello
2021-07-19 14:15           ` Andy Shevchenko
2021-07-19 15:07             ` Andrea Merello
2021-10-28 10:18 ` [v2 00/10] Add support for Bosch BNO055 IMU Andrea Merello
2021-10-28 10:18   ` [v2 01/10] utils_macro: introduce find_closest_unsorted() Andrea Merello
2021-10-28 10:25     ` Andy Shevchenko
2021-11-08 11:05       ` Andrea Merello
2021-10-28 10:18   ` [v2 02/10] iio: document linear acceleration modifiers Andrea Merello
2021-10-28 10:31     ` Andy Shevchenko
2021-11-09  7:48       ` Andrea Merello
2021-10-28 10:40     ` Jonathan Cameron
2021-11-09  8:00       ` Andrea Merello
2021-11-09 17:00         ` Jonathan Cameron
2021-10-28 10:18   ` [v2 03/10] iio: document euler angles modifiers Andrea Merello
2021-10-28 10:33     ` Andy Shevchenko
2021-10-28 10:41     ` Jonathan Cameron
2021-11-09  8:15       ` Andrea Merello
2021-11-09 17:03         ` Jonathan Cameron
2021-10-28 10:18   ` [v2 04/10] iio: add modifiers for linear acceleration Andrea Merello
2021-10-28 10:45     ` Jonathan Cameron
2021-11-09  9:58       ` Andrea Merello
2021-11-09 17:05         ` Jonathan Cameron
2021-10-28 10:18   ` [v2 05/10] iio: add modifers for pitch, yaw, roll Andrea Merello
2021-10-28 10:47     ` Jonathan Cameron
2021-10-28 10:18   ` [v2 06/10] iio: document bno055 private sysfs attributes Andrea Merello
2021-10-28 11:04     ` Jonathan Cameron
2021-11-09 10:22       ` Andrea Merello
2021-11-14 16:20         ` Jonathan Cameron
2022-01-04 11:42           ` Andrea Merello
2022-01-15 15:27             ` Jonathan Cameron
2022-01-17  9:37               ` Andrea Merello
2021-10-28 10:18   ` [v2 07/10] iio: imu: add Bosch Sensortec BNO055 core driver Andrea Merello
2021-10-28 13:31     ` Jonathan Cameron
2021-11-09 11:52       ` Andrea Merello
2021-11-14 16:33         ` Jonathan Cameron
2021-10-28 10:18   ` [v2 08/10] dt-bindings: iio: imu: add documentation for Bosch BNO055 bindings Andrea Merello
2021-10-28 12:25     ` Rob Herring
2021-10-28 10:18   ` [v2 09/10] iio: imu: add BNO055 serdev driver Andrea Merello
2021-10-28 12:31     ` Jonathan Cameron
2021-11-09 15:33       ` Andrea Merello
2021-11-14 16:37         ` Jonathan Cameron
2021-10-29  7:09     ` kernel test robot
2021-10-29 12:59     ` kernel test robot
2021-10-28 10:18   ` [v2 10/10] iio: imu: add BNO055 I2C driver Andrea Merello
2021-10-28 11:10     ` Jonathan Cameron
2021-11-11 10:12       ` Andrea Merello
2021-11-14 16:38         ` Jonathan Cameron
2021-10-28 22:04     ` Randy Dunlap
2021-11-09 11:56       ` Andrea Merello
2021-11-09 15:47         ` Randy Dunlap
2021-11-09 18:21           ` Joe Perches
2021-11-09 19:11             ` Randy Dunlap
2021-11-09 20:46               ` Joe Perches
2021-11-09 21:20                 ` Randy Dunlap
2021-10-29 13:30     ` kernel test robot
2021-10-28 10:35   ` [v2 00/10] Add support for Bosch BNO055 IMU Jonathan Cameron
2021-10-28 10:33     ` 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=20210731190103.6e2a3d41@jic23-huawei \
    --to=jic23@kernel.org \
    --cc=andrea.merello@gmail.com \
    --cc=andrea.merello@iit.it \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matt.ranostay@konsulko.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver' \
    /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

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