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

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

[Andrey Skvortsov]

From: Ondřej Jirman <megi@xff.cz>

accelerometer is mounted the way x and z-axis are invereted, x and y
axis have to be spawed to match device orientation.
The mount matrix is based on PCB drawing and was tested on the device.

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";
 	};
 };
 
-- 
2.45.2

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

[Dragan Simic]

Hello Andrey,

On 2024-09-16 22:45, Andrey Skvortsov wrote:
> From: Ondřej Jirman <megi@xff.cz>
> 
> accelerometer is mounted the way x and z-axis are invereted, x and y
> axis have to be spawed to match device orientation.
> The mount matrix is based on PCB drawing and was tested on the device.

This commit summary should be copyedited for grammar and style.  If
you want, I can provide a copyedited version?

> 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";
>  	};
>  };

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

[Andrey Skvortsov]

Hi, Dragan,

On 24-09-16 23:08, Dragan Simic wrote:
> Hello Andrey,
> 
> On 2024-09-16 22:45, Andrey Skvortsov wrote:
> > From: Ondřej Jirman <megi@xff.cz>
> > 
> > accelerometer is mounted the way x and z-axis are invereted, x and y
> > axis have to be spawed to match device orientation.
> > The mount matrix is based on PCB drawing and was tested on the device.
> 
> This commit summary should be copyedited for grammar and style.  If
> you want, I can provide a copyedited version?

It would be helpful to avoid further grammar/style problems in the
commit message. Thanks in advance.

> 
> > 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";
> >  	};
> >  };

-- 
Best regards,
Andrey Skvortsov

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

[Dragan Simic]

Hello Andrey,

On 2024-09-17 19:56, Andrey Skvortsov wrote:
> On 24-09-16 23:08, Dragan Simic wrote:
>> On 2024-09-16 22:45, Andrey Skvortsov wrote:
>> > From: Ondřej Jirman <megi@xff.cz>
>> >
>> > accelerometer is mounted the way x and z-axis are invereted, x and y
>> > axis have to be spawed to match device orientation.
>> > The mount matrix is based on PCB drawing and was tested on the device.
>> 
>> This commit summary should be copyedited for grammar and style.  If
>> you want, I can provide a copyedited version?
> 
> It would be helpful to avoid further grammar/style problems in the
> commit message. Thanks in advance.

Alright, here's how it could be worded...  First, the patch summary
should use the common prefix, together with a bit of rewording, so
the patch summary should read like this:

   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

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.

>> > 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";

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

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

[Dragan Simic]

On 2024-09-18 11:27, Dragan Simic wrote:
> Hello Andrey,
> 
> On 2024-09-17 19:56, Andrey Skvortsov wrote:
>> On 24-09-16 23:08, Dragan Simic wrote:
>>> On 2024-09-16 22:45, Andrey Skvortsov wrote:
>>> > From: Ondřej Jirman <megi@xff.cz>
>>> >
>>> > accelerometer is mounted the way x and z-axis are invereted, x and y
>>> > axis have to be spawed to match device orientation.
>>> > The mount matrix is based on PCB drawing and was tested on the device.
>>> 
>>> This commit summary should be copyedited for grammar and style.  If
>>> you want, I can provide a copyedited version?
>> 
>> It would be helpful to avoid further grammar/style problems in the
>> commit message. Thanks in advance.
> 
> Alright, here's how it could be worded...  First, the patch summary
> should use the common prefix, together with a bit of rewording, so
> the patch summary should read like this:
> 
>   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
> 
> 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.
> 
>>> > 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";
> 
> Please also note the line indentation that was changed a bit.

Actually, unless my analysis is proven wrong, perhaps it would
be better if I'd submit this patch in its final form, because it
has diverged a lot from the original patch.  IIUC, Ondrej only
imported the original patch from somewhere, without some kind of
proper attribution. [4]

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

[4] 
https://xff.cz/kernels/6.9/patches/0221-arm64-dts-sun50i-a64-pinephone-Add-mount-matrix-for-.patch

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

[Andrey Skvortsov]

Hi Dragan,

On 24-09-18 11:27, Dragan Simic wrote:
> Hello Andrey,
> 
> 
>   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.

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

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.

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

> 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

-- 
Best regards,
Andrey Skvortsov

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

[Dragan Simic]

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

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

[Dragan Simic]

[Sorry for the noise, the webmail I'm using somehow managed to mess up
the recipients list.  Sending again with the list fixed.]

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

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

[Andrey Skvortsov]

On 24-09-18 13:27, Dragan Simic wrote:
> > > 
> > > 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.

I'll try to explain how I read sensor's documentation and what bothers me.

Picture 1.

up

         ^ Z
         !      
         ! X=Y=0
     ++++++++++ 
     !        ! 
     +--------+ 
         !      
         !      
         v      
       gravity 

down

Screen (drawn as ++++++++++) is looking upwards. Device is on the flat
surface. Sensor returns +1g, although gravity points into *apposite* direction.

Experiment:

When I put PinePhone like in the sensor's documentation with the screen
upwards (Picture 1), gravity and Z axis point into different
directions, I read positive values from the sensor. So sensor works as
it's described in the documentation.


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

how I read kernel documentation.

Picture 2.

up

     +--------+ 
     !        ! 
     ++++++++++ 
         !      
         !      
         v      
       gravity, Z

down

Screen (drawn as ++++++++++) is looking downwards ("its screen flat on
the planets surface"). Gravity and Z axis point into the same
direction and it's expected to read positive value.

Notice, that Z-axis on Picture 1 and Picture 2 point into different
directions to get positive values.


Experiment: 
Now, I go to the real device and check how the sensor actually works.

When I put PinePhone like is described in the kernel documentation
with the screen downwards, "screen flat on the planets surface"
(Picture 2), gravity and Z axis point into the same direction, but I
read negative values from the sensor. So the sensor works not as
expected by kernel documentation, when I understand documentation
correctly. Z-axis inversion comes from this.

> > 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?
See my comments above.


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

-- 
Best regards,
Andrey Skvortsov

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

[Andrey Skvortsov]

Hi Dragan,


On 24-09-18 16:41, Andrey Skvortsov wrote:
> On 24-09-18 13:27, Dragan Simic wrote:

> > > 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.
> > > 
> 
> how I read kernel documentation.

Hm, I think I misunderstand this part in kernel
documentation and you were correct.

> Picture 2.
> 
> up
> 
>      +--------+ 
>      !        ! 
>      ++++++++++ 
>          !      
>          !      
>          v      
>        gravity, Z
> 
> down
> 
> Screen (drawn as ++++++++++) is looking downwards ("its screen flat on
> the planets surface"). Gravity and Z axis point into the same
> direction and it's expected to read positive value.


Sorry, for the noise.


> Actually, unless my analysis is proven wrong, perhaps it would
> be better if I'd submit this patch in its final form, because it
> has diverged a lot from the original patch.  IIUC, Ondrej only
> imported the original patch from somewhere, without some kind of
> proper attribution. [4]

please, submit your version of this patch. I'd be glad to review it (I
think, I've already did)

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

> [4] https://xff.cz/kernels/6.9/patches/0221-arm64-dts-sun50i-a64-pinephone-Add-mount-matrix-for-.patch


-- 
Best regards,
Andrey Skvortsov

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

[Dragan Simic]

Hello Andrey,

On 2024-09-18 17:58, Andrey Skvortsov wrote:
> On 24-09-18 16:41, Andrey Skvortsov wrote:
>> On 24-09-18 13:27, Dragan Simic wrote:
> 
>> > > 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.
>> > >
>> 
>> how I read kernel documentation.
> 
> Hm, I think I misunderstand this part in kernel
> documentation and you were correct.
> 
>> Picture 2.
>> 
>> up
>> 
>>      +--------+
>>      !        !
>>      ++++++++++
>>          !
>>          !
>>          v
>>        gravity, Z
>> 
>> down
>> 
>> Screen (drawn as ++++++++++) is looking downwards ("its screen flat on
>> the planets surface"). Gravity and Z axis point into the same
>> direction and it's expected to read positive value.
> 
> Sorry, for the noise.

Oh, no worries at all.  It's always good to discuss and iron out
any kinks, to eliminate any possible doubt.

The entire concept of how the values on the z-axis are read is
a bit confusing indeed.  When the device is stationary on a level
surface, with the screen pointing upwards, it's like the device is
defying the Earth's gravity pull.  Well, not actually the device,
but the surface it's resting on, :) but you get the point.

>> Actually, unless my analysis is proven wrong, perhaps it would
>> be better if I'd submit this patch in its final form, because it
>> has diverged a lot from the original patch.  IIUC, Ondrej only
>> imported the original patch from somewhere, without some kind of
>> proper attribution. [4]
> 
> please, submit your version of this patch. I'd be glad to review it (I
> think, I've already did)

Yes, you basically already did that. :)  Thanks, I'll send my version
of the patch in the next few hours, with proper attribution included
for you and Ondrej, of course.

>>> [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
> 
>> [4] 
>> https://xff.cz/kernels/6.9/patches/0221-arm64-dts-sun50i-a64-pinephone-Add-mount-matrix-for-.patch

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

[Dragan Simic]

Hello Andrey,

On 2024-09-19 20:34, Dragan Simic wrote:
> On 2024-09-18 17:58, Andrey Skvortsov wrote:
>> On 24-09-18 13:27, Dragan Simic wrote:
>>> Actually, unless my analysis is proven wrong, perhaps it would
>>> be better if I'd submit this patch in its final form, because it
>>> has diverged a lot from the original patch.  IIUC, Ondrej only
>>> imported the original patch from somewhere, without some kind of
>>> proper attribution. [4]
>>> 
>>> [4] 
>>> https://xff.cz/kernels/6.9/patches/0221-arm64-dts-sun50i-a64-pinephone-Add-mount-matrix-for-.patch>>
>> 
>> please, submit your version of this patch. I'd be glad to review it (I
>> think, I've already did)
> 
> Yes, you basically already did that. :)  Thanks, I'll send my version
> of the patch in the next few hours, with proper attribution included
> for you and Ondrej, of course.

It would be great if you could provide your Reviewed-by tag.

For future reference, i.e. for anyone reading the mailing list
archive at some point in the future, here's the link to my version
of this patch:

https://lore.kernel.org/linux-sunxi/129f0c754d071cca1db5d207d9d4a7bd9831dff7.1726773282.git.dsimic@manjaro.org/