kernelnewbies.kernelnewbies.org archive mirror
 help / color / mirror / Atom feed
From: Valdis.Kletnieks@vt.edu (Valdis.Kletnieks at vt.edu)
To: kernelnewbies@lists.kernelnewbies.org
Subject: Is this a good first patch for enabling screen rotation?
Date: Thu, 29 Jan 2015 12:12:23 -0500	[thread overview]
Message-ID: <5684.1422551543@turing-police.cc.vt.edu> (raw)
In-Reply-To: Your message of "Fri, 30 Jan 2015 00:45:30 +0800." <1422549930-32443-1-git-send-email-twunknown@gmail.com>

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 

  reply	other threads:[~2015-01-29 17:12 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-29 16:45 Is this a good first patch for enabling screen rotation? Brock York
2015-01-29 17:12 ` Valdis.Kletnieks at vt.edu [this message]
2015-02-11 15:33   ` Brock York

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5684.1422551543@turing-police.cc.vt.edu \
    --to=valdis.kletnieks@vt.edu \
    --cc=kernelnewbies@lists.kernelnewbies.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).