Re: [PATCH] arm64: dts: sun50i-a64-pinephone: Add mount matrix for accelerometer

[Dragan Simic <dsimic@manjaro.org>]

Hello Andrey,

On 2024-09-18 13:00, Andrey Skvortsov wrote:
> On 24-09-18 11:27, Dragan Simic wrote:
>>   arm64: dts: allwinner: pinephone: Add mount matrix to accelerometer
>> 
>> The patch description should be reworded like this, reflown into
>> proper line lengths, of course:
>> 
>>   The way InvenSense MPU-6050 accelerometer is mounted on the
>>   user-facing side of the Pine64 PinePhone mainboard requires
>>   the accelerometer's x- and y-axis to be swapped, and the
>>   direction of the accelerometer's y-axis to be inverted.
>> 
>>   Rectify this by adding a mount-matrix to the accelerometer
>>   definition in the PinePhone dtsi file.
>> 
>>   [andrey: Picked the patch description provided by dsimic]
>>   Fixes: 91f480d40942 ("arm64: dts: allwinner: Add initial support for
>> Pine64 PinePhone")
>>   Cc: stable@vger.kernel.org
> 
> Thanks for the commit description, it's much better, than original
> one.

Thanks, I'm glad that you like it.

>> Please note the Fixes tag, which will submit this bugfix patch
>> for inclusion into the long-term/stable kernels.
>> 
>> Also note that the patch description corrects the way inversion
>> of the axis direction is described, which should also be corrected
>> in the patch itself, as described further below.
>> 
>> After going through the InvenSense MPU-6050 datasheet, [1] the
>> MPU-6050 evaluation board user guide, the PinePhone schematic,
>> the PinePhone mainboard component placement, [2] and the kernel
>> bindings documentation for mount-matrix, [3] I can conslude that
>> only the direction of the accelerometer's y-axis is inverted,
>> while the direction of the z-axis remain unchanged, according
>> to the right-hand rule.
> 
> yes, it looks so on the first glance, but in MPU-6050 datasheet there
> is also following information:
> 
>  7.8 Three-Axis MEMS Accelerometer with 16-bit ADCs and Signal
>  Conditioning
> 
>  When the device is placed on a flat surface, it will measure
>  0g on the X- and Y-axes and +1g on the Z-axis.
> 
> So sensors reports positive acceleration values for Z-axis, when
> the gravity points to Z-minus. I see the same on device. positive
> values are returned, when screen and IC point upwards (not the center
> for gravity).
> 
> In device tree mount-matrix documentation [3] there is
> 
>  users would likely expect a value of 9.81 m/s^2 upwards along the (z)
>  axis, i.e. out of the screen when the device is held with its screen
>  flat on the planets surface.
> 
> According to that, it looks like Z-axis here has to be inverted.

Yes, reporting +1 g on the z-axis with the device remaining stationary
on a level surface is the normal behavior, and the returned positive
value actually goes along with the quoted description from the kernel
documentation.  The z-axis of the MPU-6050 goes upward and out of the
screen, the way the MPU-6050 is placed inside the PinePhone.

> It applies to other axes as well. And because of that I came from
> (only Y-axis is inverted)
> 
> x' = -y
> y' =  x
> z' =  z
> 
> to inverted solution (Y-axis is kept, but X and Z are inverted).
> 
> x' =  y
> y' = -x
> z' = -z
> 
> probably should put this information into commit description.

Wouldn't inverting the direction of the z-axis go against the
above-quoted description from the kernel documentation?

>> > > > Signed-off-by: Ondrej Jirman <megi@xff.cz>
>> > > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
>> > > > ---
>> > > >  arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi | 3 +++
>> > > >  1 file changed, 3 insertions(+)
>> > > >
>> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
>> > > > b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
>> > > > index bc6af17e9267a..1da7506c38cd0 100644
>> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
>> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
>> > > > @@ -229,6 +229,9 @@ accelerometer@68 {
>> > > >  		interrupts = <7 5 IRQ_TYPE_EDGE_RISING>; /* PH5 */
>> > > >  		vdd-supply = <&reg_dldo1>;
>> > > >  		vddio-supply = <&reg_dldo1>;
>> > > > +		mount-matrix = "0", "1", "0",
>> > > > +				"-1", "0", "0",
>> > > > +				"0", "0", "-1";
>> > > >  	};
>> > > >  };
>> 
>> With the above-described analysis in mind, the mount-matrix
>> should be defined like this instead:
>> 
>> 		mount-matrix = "0", "1", "0",
>> 			       "-1", "0", "0",
>> 			       "0", "0", "1";
> 
> 
> x' =  0 * x + 1 * y + 0 * z =  y
> y' = -1 * x + 1 * y + 0 * z = -x
> z' =  0 * z + 0 * y + 1 * z =  z
> 
> your description says, that only Y-axis is inverted, but this matrix,
> imho, inverts original X axis as it was in original description.

The way it's specified, it actually swaps the x- and y-axis, and
inverts the direction of the y-axis, all that from the viewpoint
of the accelerometer, which matches the proposed patch description.
IOW, the description keeps the original names of the axes.

>> Please also note the line indentation that was changed a bit.
>> 
>> [1] https://rimgo.reallyaweso.me/vrBXQPq.png
>> [2] https://rimgo.reallyaweso.me/uTmT1pr.png
>> [3] 
>> https://www.kernel.org/doc/Documentation/devicetree/bindings/iio/mount-matrix.txt