LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrea Merello <andrea.merello@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
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: Tue, 9 Nov 2021 11:22:27 +0100	[thread overview]
Message-ID: <CAN8YU5Orbbzq-eDxmrR00xHwXQ=0LU2G3_yEtHGMkbVhmdcqgg@mail.gmail.com> (raw)
In-Reply-To: <20211028120405.6ffb01d1@jic23-huawei>

Few inline  comments; ok for the rest.

Il giorno gio 28 ott 2021 alle ore 12:59 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> On Thu, 28 Oct 2021 12:18:36 +0200
> Andrea Merello <andrea.merello@gmail.com> wrote:
>
> > This patch adds ABI documentation for bno055 driver private sysfs
> > attributes.
>
> Hohum. As normal I dislike custom attributes but reality is these
> don't map to anything 'standard' and I don't want them getting adopted
> in places where the 'standard' approach works.
>
> So thinking a bit more on this, I wonder if we can fit it into standard
> ABI.
>
> We can't use the normal range specification method of
> _scale because it's internal to the device and the output reading is
> unaffected.  The range specification via _raw_available would let us know
> the range, but it is not writeable so..
>
> A control that changes the internal scaling of the sensor in a fashion
> that is not visible to the outside world maps to calibscale.  Whilst
> that was intended for little tweaks to the input signal (often front
> end amplifier gain tweak) it works here.  It doesn't map through to
> anything userspace is expected to apply.  That combined with
> _raw_available to let us know what the result is should work?
>
> What do you think of that approach?  It's obviously a little more complex
> to handle in the driver, but it does map to existing ABI and avoids
> custom attributes etc.

If I read the ABI documentation, then I would say that calibscale has
nothing to do with this, but I think you have obviously a better
feeling than me about what calibscale is really for. To be honest I've
probably not a clear idea about what calibscale is indeed...

In general, I would say that is better to stick to standard attributes
when possible, and of course to avoid having the same thing mapped on
random custom attributes in each driver, but IMO only up to the extent
which doesn't  force something that is really something different to
map on a standard thing just because of the sake of having as much
standard things as possible... But all this is probably quite obvious,
and it all depends on the above (i.e. is it calibscale fitting well in
your opinion?) .. Up to you on this one..

BTW I'm missing why this should complicate the driver.. I guess I'll
find out if I'll implement it :)

> >
> > Signed-off-by: Andrea Merello <andrea.merello@iit.it>
> > ---
> >  .../ABI/testing/sysfs-bus-iio-bno055          | 84 +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-bno055
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-bno055 b/Documentation/ABI/testing/sysfs-bus-iio-bno055
> > new file mode 100644
> > index 000000000000..930a70c5a858
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-bno055
> > @@ -0,0 +1,84 @@
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_accel_range
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Range for acceleration readings in G. Note that this does not
> > +             affects the scale (which should be used when changing the
> > +             maximum and minimum readable value affects also the reading
> > +             scaling factor).
>
> Having this in G but the sensor output in m/s^2 seems inconsistent.
>
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_anglvel_range
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Range for angular velocity readings in dps. Note that this does
> > +             not affects the scale (which should be used when changing the
> > +             maximum and minimum readable value affects also the reading
> > +             scaling factor).
>
> Again, units need to match or this is going to be really confusing.
>
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_accel_range_available
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             List of allowed values for in_accel_range attribute
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_anglvel_range_available
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             List of allowed values for in_anglvel_range attribute
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/fast_magnetometer_calibration_enable
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Can be 1 or 0. Enables/disables the "Fast Magnetometer
> > +             Calibration" HW function.
>
> Naming needs to be consistent with the ABI.  This is a channel type specific function
> and to match existing calibration related ABI naming it would be.
>
> in_magn_calibration_fast_enable
>
> Some of the others need renaming in a similar way.
>
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/fusion_enable
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Can be 1 or 0. Enables/disables the "sensor fusion" (a.k.a.
> > +             NDOF) HW function.
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_calibration_data
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Reports the binary calibration data blob for the IMU sensors.
>
> Why in_ ?  What channels does this apply to?
>
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_autocalibration_status_accel
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Can be "Idle", "Bad", "Barely enough", "Fair", or "Good".
> > +             Report the autocalibration status for the accelerometer sensor.
>
> For interfaces that really don't have any chance of generalising this one is terrible.
> Any hope at all of mapping this to something numeric?
>
> in_accel_calibration_auto_status
>
>
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_autocalibration_status_gyro
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Can be "Idle", "Bad", "Barely enough", "Fair", or "Good".
> > +             Reports the autocalibration status for the gyroscope sensor.
>
> in_angvel_calibration_auto_status
> etc.
>
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_autocalibration_status_magn
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Can be "Idle", "Bad", "Barely enough", "Fair", or "Good".
> > +             Reports the autocalibration status for the magnetometer sensor.
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/in_autocalibration_status_sys
> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Can be "Idle", "Bad", "Barely enough", "Fair", or "Good".
> > +             Reports the status for the IMU overall autocalibration.
> > +
> > +What:                /sys/bus/iio/devices/iio:deviceX/unique_id
>
> Hmm. So normally we just dump these in the kernel log.  I guess you need it
> here to associate a calibration blob with a particular sensor?

Well, it was originally in kernel log, but putting in an attribute was
one of the changes that has been requested for V2.
It is needed by the user who copies the calibration data to the
calibration file, in order for her/him to be able to properly name it
(in case of more than 1 sensor on the same setup).

> We could put it in label, but that would stop us using that for things like
> positioning of the sensor.  So perhaps this is something that we should add
> to the main ABI doc.  Probably as serial_number rather than unique ID though.

OK, for renaming to "serial_number". I'm not sure they are
conceptually the same thing, but I think it works anyway.
Of course I can move its doc to the main file. Do you want a separate
patch for this?

> > +KernelVersion:       5.15
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             16-bytes, 2-digits-per-byte, HEX-string representing the sensor
> > +             unique ID number.
>

  reply	other threads:[~2021-11-09 10:22 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
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 [this message]
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='CAN8YU5Orbbzq-eDxmrR00xHwXQ=0LU2G3_yEtHGMkbVhmdcqgg@mail.gmail.com' \
    --to=andrea.merello@gmail.com \
    --cc=andrea.merello@iit.it \
    --cc=andy.shevchenko@gmail.com \
    --cc=ardeleanalex@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jacopo@jmondi.org \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matt.ranostay@konsulko.com \
    --cc=mchehab+huawei@kernel.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [v2 06/10] iio: document bno055 private sysfs attributes' \
    /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).