Message-ID: <7f562418-f56c-07bb-27b4-d2e54834ef2f@theobroma-systems.com>
Date:   Mon, 4 Jul 2022 18:49:54 +0200
From:   Quentin Schulz <quentin.schulz@...obroma-systems.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Quentin Schulz <foss+kernel@...il.net>
Cc:     lars@...afoo.de, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: accel: mxc4005: report orientation of accelerometer

Hi Jonathan,

On 6/19/22 16:27, Jonathan Cameron wrote:
> On Wed, 15 Jun 2022 13:02:40 +0200
> Quentin Schulz <foss+kernel@...il.net> wrote:
> 
>> From: Quentin Schulz <quentin.schulz@...obroma-systems.com>
>>
> 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@...obroma-systems.com>
>> ---
>>   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

Powered by blists - more mailing lists