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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64a2bf8f1491e510e93cc4c121975fd9@manjaro.org>
Date: Wed, 18 Sep 2024 12:02:53 +0200
From: Dragan Simic <dsimic@...jaro.org>
To: Andrey Skvortsov <andrej.skvortzov@...il.com>
Cc: Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Ondřej Jirman <megi@....cz>
Subject: Re: [PATCH] arm64: dts: sun50i-a64-pinephone: Add mount matrix for
 accelerometer

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@....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@...r.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@....cz>
>>> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@...il.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