Re: [PATCH] iio: accel: mxc4005: report orientation of accelerometer
From: Quentin Schulz
Date: Mon Jul 04 2022 - 12:50:08 EST
Hi Jonathan,
On 6/19/22 16:27, Jonathan Cameron wrote:
On Wed, 15 Jun 2022 13:02:40 +0200
Quentin Schulz <foss+kernel@xxxxxxxxx> wrote:
From: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
Hi Quentin,
Interesting / horribly ill defined little feature ;)
The accelerometer can report precise values for x, y and z accelerations
but it can also simply report its orientation on XY plane and Z axis.
Since the orientation of the device may be enough information for
userspace and allows to avoid expensive fusion algorithms, let's add
support for it.
The orientation register stores a 2b value for XY plane orientation:
between 225° and 315°, returns 0, between 315° and 45°, 1, between 45°
and 135°, 2 and between 135° and 225°, 3. We "round" those to 270°,
0°, 90° and 180° degrees.
Wow. The datasheet description of this very confusing...
I'm relieved I'm not the only one confused by this datasheet :)
One key thing is we need to be careful of is that tilt (x/y is
not always available - but rather shows the last, and probably
now garbage, value)
Being pedantic here, not garbage, but outdated. This register exists so
that the values aren't garbage (at the cost of being outdated). Except
this small notion, I agree on the statement.
For Z axis, the register bit returns 0 if facing the user, 1 otherwise,
which the driver translates to 0° and 180° respectively.
I assume facing up vs facing down? User might be lying on their
back in which case this description doesn't work. The datasheet
Correct, I was playing with the device while seated, hence the bias. But
yes, everything is relative to Earth gravity, so face up/down is a
better description indeed.
also talks about the case where g lies near the XY plane and hence
the z axis is horizontal.
Those values are proper if the accelerometer is mounted such that the
XYZ axes are as follows when the device is facing the user in portrait
mode (respecting the right-hand rule):
y
^
|
|
|
+----------> x
/
/
/
L
z
Since this information is very basic, imprecise (only 4 values for XY
plane and 2 for Z axis) and can be extrapolated from the actual,
precise, x, y and z acceleration values, it is not made available
through buffers.
A change in XY plane or Z axis orientation can also trigger an interrupt
but this feature is not added in this commit.
Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
---
drivers/iio/accel/mxc4005.c | 39 +++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index b3afbf064915..61f24058d239 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -20,6 +20,11 @@
#define MXC4005_IRQ_NAME "mxc4005_event"
#define MXC4005_REGMAP_NAME "mxc4005_regmap"
+#define MXC4005_REG_TILT_ORIENT 0x01
+#define MXC4005_REG_TILT_ORIENT_Z_MASK BIT(6)
I think you need to deal with BIT(7) as well.
+#define MXC4005_REG_TILT_ORIENT_XY_MASK GENMASK(5, 4)
+#define MXC4005_REG_TILT_ORIENT_XY_SHIFT 4
Don't define the shift, you can use FIELD_GET(MASK, val)
Wasn't aware of this neat macro, thanks for the heads up.
+
#define MXC4005_REG_XOUT_UPPER 0x03
#define MXC4005_REG_XOUT_LOWER 0x04
#define MXC4005_REG_YOUT_UPPER 0x05
@@ -96,6 +101,7 @@ static const struct attribute_group mxc4005_attrs_group = {
static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
{
switch (reg) {
+ case MXC4005_REG_TILT_ORIENT:
case MXC4005_REG_XOUT_UPPER:
case MXC4005_REG_XOUT_LOWER:
case MXC4005_REG_YOUT_UPPER:
@@ -214,6 +220,28 @@ static int mxc4005_read_raw(struct iio_dev *indio_dev,
int ret;
switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ switch (chan->type) {
+ case IIO_ROT:
+ ret = regmap_read(data->regmap, chan->address, val);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to read rotation\n");
+ return ret;
+ }
+
+ if (chan->channel2 == IIO_MOD_X_AND_Y) {
+ *val &= MXC4005_REG_TILT_ORIENT_XY_MASK;
+ *val >>= MXC4005_REG_TILT_ORIENT_XY_SHIFT;
FIELD_GET()
+ /* 00 = 270°; 01 = 0°; 10 = 90°; 11 = 180° */
+ *val = (360 + (*val - 1) * 90) % 360;
In event of tilt not being set (BIT (7)) I think you should return an error
code here. -EBUSY perhaps? To reflect the fact we don't have valid data.
Difficult to find the appropriate error code to return. It's not
technically busy, especially if the device stays in that position
forever, it'll never return a valid value.
+ } else {
+ *val &= MXC4005_REG_TILT_ORIENT_Z_MASK;
+ *val = *val ? 180 : 0;
Documentation for this is really confusing, as it refers to a circumstance
when it can be assumed to be horizontal, but then doesn't define it.
It might be a simple as tilt being set and thus indicating significant
acceleration due to gravity in the xy plane.
However, the Z orientation is still updated in that case...
I'm wondering if it's not an exclusive validity of axes. E.g. XY plane
is valid only when Z is not and vice-versa?
"The vertical/horizontal Z axis
orientation is determined by the same criteria used to determine the
XY-plane off-axis tilt angle" seems to indicate that the TILT bit
defines whether the Z axis is vertical or horizontal.
"When the XY plane
has a sufficiently small off-axis tilt angle, XY orientation detection
is valid (and continues to be updated), and the Z
axis is detected as horizontal" would mean Z is just not usable when XY
orientation is valid (because Z is horizontal and thus does not have a
big enough acceleration to be usable).
"When off-axis tilt angle exceeds the
threshold discussed above, the Z axis is detected as either vertical up
or vertical down, depending on the sign of the
Z axis acceleration output." could be interpreted as "when XY
orientation is invalid, Z orientation is either vertical up or down".
+ }
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_RAW:
switch (chan->type) {
case IIO_ACCEL:
@@ -287,11 +315,22 @@ static const unsigned long mxc4005_scan_masks[] = {
}, \
}
+#define MXC4005_CHANNEL_ORIENTATION(_axis) { \
+ .type = IIO_ROT, \
Hmm. Should this be rotation or inclination (so referenced
to gravity). Inclination is not particularly tightly defined but the
point is that it is relative to gravity - kind of a special case of
rot.
For the adis16209 we handled inclination and rotation. I think rotation
in that device corresponds to XY here. (though it's oddly defined for
X axis, whereas I'm fairly sure it should be Z - as rotation 'about'
z axis). The Z one here should I think be an inclination because it's not
about any particular axis.
We also have angle to confuse matters. In that case intent was 'between'
two things. Arguably all the uses of rot are as well, just that one of those
things is gravity or magnetic north. With hindsight I think we could have
gotten away with one of them, but hard to tidy up now.
You mentioned the three candidates I had in mind, but none seemed to
perfectly match (or maybe I'm just confused about the difference between
the three and just can't make up my mind) so I picked rotation because
the English term seemed closer to what I think those values represent?
In conclusion, what you have here I think is best described as
IIO_ROT about Z axis (the XY one)
I disagree, this would mean that having XY plane parallel to ground and
rotate the device along the Z axis (so XY plane staying parallel to
ground) should change the value of this IIO_ROT on Z axis, but it does
not if I'm not mistaken (I assume because the acceleration on X and Y
axes are too small because the axes are parallel to the ground).
But that also kind of highlights that IIO_ROT for Z as I've done it in
the patch probably isn't correct either and inclination would probably
match best?
I feel like this will be an interesting discussion :)
Cheers,
Quentin