Is this a good first patch for enabling screen rotation?

[twunknown@gmail.com (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.