Is this a good first patch for enabling screen rotation?

Is this a good first patch for enabling screen rotation?

[Brock York]

Hello 

This is my first patch and after speaking to a few kernel
devs at LCA2015 I thought I would ask here for some advice on
if I have followed the newbie guide correctly.

The patch is to get automatic screen rotation working on a
acer iconia w500 tablet.

The patch was made against linux-next branch next-20150129
and successfully built and ran.

Do you think this is a succificient patch to get accepted by the
platform-x86 maintainers? I used get_maintainer.pl and it suggested
the platform-x86 mailing list.

Thank you, any help is much appreciated.
Regards Brock York.

BEGIN PATCH BELOW------------
Subject: [PATCH] acer-wmi: Enable screen auto-rotation via udev

Emit udev event via kobject_uevent when accelerometer position
is updated. This is to allow userspace to grab the accelerometer
position and rotate the screen accordingly.

Swap the x and y values and negate the x value converting the
accelerometers coordinate system into the coordinate system
userspace udev expects.

Accelerometer		Expected
Coordinates		Coordinates
        x+		    y+
        ^		    ^
        |		    |
        |		    |
 y+<----/		    /---->x+
       /		   /
      z+		  z+

Screen Orientation
 ________
|        |
|        |
|________|

Tested on a Acer Iconia W500 tablet with Gnome 3

Signed-off-by: Brock York <twunknown@gmail.com>
---
 drivers/platform/x86/acer-wmi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 3ac29a1..d5678e4 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1491,12 +1491,14 @@ static int acer_gsensor_event(void)
 		return -1;
 
 	input_report_abs(acer_wmi_accel_dev, ABS_X,
-		(s16)out_obj->package.elements[0].integer.value);
+		-(s16)out_obj->package.elements[1].integer.value);
 	input_report_abs(acer_wmi_accel_dev, ABS_Y,
-		(s16)out_obj->package.elements[1].integer.value);
+		(s16)out_obj->package.elements[0].integer.value);
 	input_report_abs(acer_wmi_accel_dev, ABS_Z,
 		(s16)out_obj->package.elements[2].integer.value);
 	input_sync(acer_wmi_accel_dev);
+
+	kobject_uevent(&acer_wmi_accel_dev->dev.kobj, KOBJ_CHANGE);
 	return 0;
 }
 
-- 
2.2.2

Is this a good first patch for enabling screen rotation?

[Valdis.Kletnieks at vt.edu]

On Fri, 30 Jan 2015 00:45:30 +0800, Brock York said:

(Note, I don't have an Acer, nor am I an HID expert...  so take this
all with a grain of salt...)

> Swap the x and y values and negate the x value converting the
> accelerometers coordinate system into the coordinate system
> userspace udev expects.

This part looks fishy...

1) I wasn't aware that udev had a coordinate system.  I think you
meant that something *else* was using values that udev passed
to userspace.  You probably want to clarify what you meant.

2) There obviously was *some* consumer of the input_report_abs()
value before you added the kobject_uevent() call - convince us
that said consumer is OK with its axes being twiddled like this.

>  	input_report_abs(acer_wmi_accel_dev, ABS_X,
> -		(s16)out_obj->package.elements[0].integer.value);
> +		-(s16)out_obj->package.elements[1].integer.value);
>  	input_report_abs(acer_wmi_accel_dev, ABS_Y,
> -		(s16)out_obj->package.elements[1].integer.value);
> +		(s16)out_obj->package.elements[0].integer.value);

Other issues:

3) "when accelerometer position is updated."

Is that going to generate a flood of events if (for example) you're walking
with the device and it's swinging back and forth a bit?  It's one thing to
have HID devices generating a near-constant stream of updates for mouse
tracking and so on - it's a whole 'nother can of worms to additionally
bash udev with an update stream.

Perhaps udev events should be clamped to only trigger when it's time for
an actual axis swap?

4) This should probably be 2 patches - one to swap the input_report_abs()
values, and a second to add the kobject_uevent() call.

5) I almost have to wonder if the input_report_abs() part is sufficient,
and udev events aren't needed?





-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150129/f1b72986/attachment.bin 

Is this a good first patch for enabling screen rotation?

[Brock York]

Hello Valdis

Sorry for such a late reply and thank you for such a quick reply.

On Thu, 2015-01-29 at 12:12 -0500, Valdis.Kletnieks at vt.edu wrote:
> On Fri, 30 Jan 2015 00:45:30 +0800, Brock York said:
> 
> (Note, I don't have an Acer, nor am I an HID expert...  so take this
> all with a grain of salt...)
> 
> > Swap the x and y values and negate the x value converting the
> > accelerometers coordinate system into the coordinate system
> > userspace udev expects.
> 
> This part looks fishy...
> 
> 1) I wasn't aware that udev had a coordinate system.  I think you
> meant that something *else* was using values that udev passed
> to userspace.  You probably want to clarify what you meant.

I haven't really explained this very well. It's not udev specifically
that is 'consuming' the output. From what my research has turned up is
that udev creates a event and then runs a program called accelerometer.
Which is part of the udev package. It then reads the coordinates from
the udev event and then modifies the event adding the orientation to the
event.

I've read the source code for accelerometer but I don't quite understand
it. So I guess I should probably look at the udev docs or contact the
udev developers. But from what I can understand it expects the
accelerometer axes to be how I described in the expected coordinates.
But first I should probably make sure the acceleromter is actually
correctly being read and etc.

> 2) There obviously was *some* consumer of the input_report_abs()
> value before you added the kobject_uevent() call - convince us
> that said consumer is OK with its axes being twiddled like this.
> 
> >  	input_report_abs(acer_wmi_accel_dev, ABS_X,
> > -		(s16)out_obj->package.elements[0].integer.value);
> > +		-(s16)out_obj->package.elements[1].integer.value);
> >  	input_report_abs(acer_wmi_accel_dev, ABS_Y,
> > -		(s16)out_obj->package.elements[1].integer.value);
> > +		(s16)out_obj->package.elements[0].integer.value);

Do you have any advice on what else could be consuming the output?
The only thing I know of would be udev. I'd like to test it
on another Acer laptop that has a acceleromter to see whether this
breaks things. But I don't have the hardware.

> Other issues:
> 
> 3) "when accelerometer position is updated."
> 
> Is that going to generate a flood of events if (for example) you're walking
> with the device and it's swinging back and forth a bit?  It's one thing to
> have HID devices generating a near-constant stream of updates for mouse
> tracking and so on - it's a whole 'nother can of worms to additionally
> bash udev with an update stream.
> 
> Perhaps udev events should be clamped to only trigger when it's time for
> an actual axis swap?

I was thinking this myself, whether I would be flooding udev. But it
means I would need to reimplement the functionality that is in
userspace. Which is determining what the orientation of the device is
and then emiting a udev event with the orientation. Would this be
acceptable though, to add code that would be duplicating what is
currently in userspace? 
Or do you mean just checking if the x or y axis go from negative to
positive and only then emiting a udev event?

> 
> 4) This should probably be 2 patches - one to swap the input_report_abs()
> values, and a second to add the kobject_uevent() call.

Thanks I was really wondering whether this is one or two logical
changes. So that clears it up thank you.

> 
> 5) I almost have to wonder if the input_report_abs() part is sufficient,
> and udev events aren't needed?

Well I had been waiting for automatic screen rotation to come to my
tablet for quite some time. After searching for a year or two now
I finally found a blog about the weetabs automatic screen rotation in
Gnome 3. It explained the basic idea behind how it works. So I read the
asus driver (which is what the weetab uses) and the only line missing
from the acer driver seemed to be the kobject_uevent() call. As soon as
I added that call automatic rotation started working. But the detected
screen orientation was wrong which is why I started twidling the x and y
coordinates.