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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <c7664fda936d36e0d916ae09dd554d2e@manjaro.org>
Date: Wed, 18 Sep 2024 11:27:31 +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

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.

[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