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

From: Dragan Simic
Date: Wed Sep 18 2024 - 06:03:06 EST

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@xxxxxx>
>
> 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@xxxxxxxxxxxxxxx

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@xxxxxx>
> Signed-off-by: Andrey Skvortsov <andrej.skvortzov@xxxxxxxxx>
> ---
> 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