Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210715141742.15072-1-andrea.merello@gmail.com>
 <20210715141742.15072-3-andrea.merello@gmail.com> <20210717163236.1553fbfa@jic23-huawei>
 <CAN8YU5NctVMPfNZn7ya-Jw7yE=NQDBq1aweWn0fX0Rp1p1P=aw@mail.gmail.com> <20210724180823.692b203f@jic23-huawei>
In-Reply-To: <20210724180823.692b203f@jic23-huawei>
Reply-To: andrea.merello@gmail.com
From:   Andrea Merello <andrea.merello@gmail.com>
Date:   Mon, 26 Jul 2021 16:36:49 +0200
Message-ID: <CAN8YU5PcrR-xM5A=3jd50=UaY9wWDJZGBqajmvM8Te1Ly14Hew@mail.gmail.com>
Subject: Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver
To:     Jonathan Cameron <jic23@kernel.org>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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 ?

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


> > > > +     &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?

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

Andrea