Re: [PATCH] iio: accel: mxc4005: report orientation of accelerometer

[Jonathan Cameron]

On Mon, 4 Jul 2022 18:49:54 +0200
Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx> wrote:

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

Probably 100s of people world wide :)

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

True. It's a fun corner.  We return -EBUSY in other cases where action of
some type is needed for it to free up.  We aren't implying a retry will get
you anywhere (unlikely EAGAIN).  To be honest, I'm fine with any error code
you fancy, but I think it should be an error code.

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

Maybe...  I get the feeling that scattering liberal comments through the
code along the lines of 'we think this is how it works' with datasheet
references might be the best we can do.

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

I'm confused. Why would XY be parallel to ground?  I'm struggling to get
my head around it, but I though for rotation in the x-y plane it's z that
is always parallel to the ground (or near enough that we can sensibly estimate
what is going on in the xy plane)
(I spent lots of time in my youth working with the Geometric Algebra version of this
stuff where it's the XY bivector, but that's a different story and probably
not helpful here :)

> 
> 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'm lost...  Best way forward here is probably for you to propose what
you think is the best we have with a lot of description and we see if that
aligns with my thoughts on how this should work (and anyone else who
cares to join in ;)

Foggy brain today I'm afraid...

Jonathan
> 
> I feel like this will be an interesting discussion :)
> 
> Cheers,
> Quentin