| Message ID | 20240916204521.2033218-1-andrej.skvortzov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: sun50i-a64-pinephone: Add mount matrix for accelerometer | expand |
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 = <®_dldo1>; > vddio-supply = <®_dldo1>; > + mount-matrix = "0", "1", "0", > + "-1", "0", "0", > + "0", "0", "-1"; > }; > };
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 = <®_dldo1>; > > vddio-supply = <®_dldo1>; > > + mount-matrix = "0", "1", "0", > > + "-1", "0", "0", > > + "0", "0", "-1"; > > }; > > };
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 = <®_dldo1>; >> > vddio-supply = <®_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
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 = <®_dldo1>; >>> > vddio-supply = <®_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
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 = <®_dldo1>; > > > > vddio-supply = <®_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
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 = <®_dldo1>; >> > > > vddio-supply = <®_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
[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 = <®_dldo1>; >> > > > vddio-supply = <®_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
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
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
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
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/
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 = <®_dldo1>; vddio-supply = <®_dldo1>; + mount-matrix = "0", "1", "0", + "-1", "0", "0", + "0", "0", "-1"; }; };