Re: [v2 06/10] iio: document bno055 private sysfs attributes

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: [v2 06/10] iio: document bno055 private sysfs attributes
Date: Sat, 22 Jan 2022 18:08:43 +0000	[thread overview]
Message-ID: <20220122180843.2fa8ca0c@jic23-huawei> (raw)
In-Reply-To: <CAN8YU5OT44Wz813tKA62-Dvq3=VoTcoyVE__5UuRw+i7+B7i8w@mail.gmail.com>

On Mon, 17 Jan 2022 10:37:33 +0100
Andrea Merello <andrea.merello@gmail.com> wrote:

> Trivial inline comments below. Beside that, I've found another
> pleasing issue with this "range" thing on this device..
> 
> One one hand, things seem to always work as we discussed for the
> accelerometer (i.e. range doesn't affect the scale; the HW always
> provides readings in the same scale, but with different range and
> precision) on the other hand, the gyroscope behavior depends by the
> internal IMU firmware version.. great..

*sigh* :)

> 
> Stock firmware has a bug[0], so that the "range" gyroscope registers
> do change the scale indeed. AFAICT stock firmware is the one you find
> in most (all?) breakout boards, which are usually available (and which
> I'm using right now for this driver mainlining attempt). Upgrading
> firmware looks like a rather obscure process that AFAICT can be done
> only in some specific USB-stick demo-board ("shuttle board") or with
> maybe with FAE assistance on custom developed boards [1] (i.e. maybe
> can be done by some professional user; I would say not for most
> people).
> 
> So, I'm now wondering how to handle this... I really want to support
> the stock FW, which seems the most widespread, and the one I have
> right now; I'd say this means: the accelerometer thing will still work
> as we discussed (i.e. the range attribute thing), while the gyro will
> have writeable scale, and a (ro) scale_available attrib. But what
> about the gyro range thing? Should I drop it, or keep it as
> informative read-only?

I'd be cynical and for initial version at least, just hide it as 'too
complex' with a comment in the driver code on why.

> 
> Then I could also support the new firmware (which I cannot test right
> now with my actual breakout board, but I might see whether I could get
> a board with an updated IMU), keeping also the current driver behavior
> (i.e. range stuff).
> 
> But the question is: in either cases (new vs old fw) should the
> non-necessary attributes disappear or they may just be RO or locked
> (i.e. scale_available for new FW and range stuff for the old one)?

If they don't have meaning then they should disappear, but it would
also be valid to have the 'broken' one be read only if there is
an appropriate value.

> 
> Any thoughts and advice on this whole thing would be very welcome :)
> my current inclination anyway now tends to be: go on supporting only
> the stock FW (i.e. the board I have here now) and eventually add
> support for the new fw later on, after merge.

Sounds sensible - but.... Make sure you check the firmware version
number (I hope it has one) and print a warning at least if you get
one that you have strong reason to believe will handle this differently
from whatever the driver is supporting.

This is definitely going to be a case for detailed comments in
the driver code so that we can 'recall' what on earth was
going on here in N years time!

> 
> [0] https://community.bosch-sensortec.com/t5/MEMS-sensors-forum/BNO055-Wrong-sensitivity-resolution-in-datasheet/td-p/10266
> [1] https://community.bosch-sensortec.com/t5/MEMS-sensors-forum/BNO055-Software-Version/td-p/14001
> 
> > > I've looked at other iio sysfs attributes in the DOC.  It seems  that
> > > "thesh" and "roc" attributes allows for both preprocessed and raw
> > > data: I found e.g. "<type>[Y][_name]_<raw|input>_thresh_value", but
> > > the related "what" entries written above all seem to omit both "_raw"
> > > and "_input"; I don't understand why.  
> >
> > Excellent point.  That documentation is garbage.  Events are meant
> > to pick it up implicitly from the related channel _raw or _input.
> > I don't remember them ever having raw or input in their naming but
> > it's possible they did right at the beginning before the ABI was anywhere
> > near stable.  Gah. I dread to think how long that that has been wrong.  
> 
> Ok, great :)
> 
> > So I think range_raw postfix is the best bet.  
> 
> Will go with this, thanks.
> 
> > Jonathan
> >
> >
> >
> >
> >  
> > >  
> >  
> > >
> > > Andrea  
> >  