Is this a good first patch for enabling screen rotation?
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
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?
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@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.
participants (2)
-
Brock York -
Valdis.Kletnieks＠vt.edu