On Fri, 09 Mar 2007 21:58:23 +0100, René van Paassen <[EMAIL PROTECTED]> wrote:
[I'm cc-ing this to Dmitry because of input_sync() in a timer
context below.]
> Well, Петр, Linux is a pretty international effort after all. But for
> now, for the sake of getting this driver where it belongs, I robbed my
> co-developer Cédric and myself from our accents aigus. I am using a
> different e-mail client, let's see whether this patch survives the trip:
René, I wish you accepted the argument about the difference between
LANG=C and book English (the latter has characters outside of ASCII
even if we do not talk names, like the "sha"). But failing that, the
practical agreement is valuable too, thanks.
Actually, I asked Linus a couple of years ago to decree that kernel
is written in UTF-8 and thus put all this behind us, but he turned
me down.
BTW, I saw message from Larry where he complains about line-wrapping,
but the patch looks fine to me. Perhaps gmail does something wrong.
About the patch... I think it can be improved a fraction here:
> +static char *pLeft = "left";
> - if (strcmp(buf, "left") == 0) {
> + if (strncmp(buf, pLeft, 4) == 0) {
I would do this:
+static char pleft[] = "left";
- if (strcmp(buf, "left") == 0) {
+ if (strncmp(buf, pleft, sizeof(pleft)-1) == 0) {
Granted, my version includes the hated sizeof of data, but at
least it's not a naked constant.
Moving on...
> @@ -1407,7 +1595,7 @@
> if (aiptek == NULL)
> return 0;
>
> - aiptek->newSetting.programmableDelay = (int)simple_strtol(buf, NULL,
> 10);
> + aiptek->newSetting.programmableDelay = (int)simple_strtol(buf, 0, 10);
> return count;
This is just wrong. We aren't writing in C++.
Even if Greg accepts this, someone is going to revert it right away.
> @@ -1964,6 +2167,7 @@
> struct input_dev *inputdev;
> struct input_handle *inputhandle;
> struct list_head *node, *next;
> + // char path[64 + 1];
> int i;
> int speeds[] = { 0,
> AIPTEK_PROGRAMMABLE_DELAY_50,
What is this supposed to be?
> @@ -2111,6 +2337,29 @@
> aiptek->urb->transfer_dma = aiptek->data_dma;
> aiptek->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>
> + /* Register the tablet as an Input Device
> + */
> + input_register_device(aiptek->inputdev);
> +
> + /* We now will look for the evdev device which is mapped to
> + * the tablet. The partial name is kept in the link list of
> + * input_handles associated with this input device.
> + * What identifies an evdev input_handler is that it begins
> + * with 'event', continues with a digit, and that in turn
> + * is mapped to /{devfs}/input/eventN.
> + */
> + inputdev = aiptek->inputdev;
> + list_for_each_safe(node, next, &inputdev->h_list) {
> + inputhandle = to_handle(node);
> + if (strncmp(inputhandle->name, "event", 5) == 0) {
> + strcpy(aiptek->features.inputPath,
> + inputhandle->name);
> + break;
> + }
> + }
This is something I wanted to discuss with aiptek developers, because
of races walking h_list outside of input core itself, where you do not
have access to core's locks. This cannot be safe.
Is it feasible to get rid of this feature altogether?
> @@ -2173,6 +2410,23 @@
> return -ENOMEM;
> }
>
> +/* Activity checking thread. If a sufficient period of inactivity is
> detected,
> + the tablet's proximity is reset. */
> +static void activity_check(unsigned long data)
> +{
> + struct aiptek *aiptek = (struct aiptek *) data;
> +
> + /* This timer is set *after* handling input for the table, and
> + cleared *before* handling it. Am guessing that we never run
> + concurrently the module code. */
> +
> + /* info("aiptek: timeout proximity\n"); */
> +
> + /* apparently over-due. Reset the misc key, no tool, no proximity */
> + input_report_abs(aiptek->inputdev, ABS_MISC, 0);
> + input_sync(aiptek->inputdev);
> +}
This concerns me a bit. I am not 100% sure that input_sync and friends
are safe to run from several CPUs. Usually, nobody is bitten by this,
because they are delivered from a single IRQ. If you start doing this
from a timer, it's not longer true. Dmitry must bless this practice
(cc-ed). Perhas you want a spinlock here.
> @@ -2190,6 +2444,9 @@
> {
> struct aiptek *aiptek = usb_get_intfdata(intf);
>
> + /* First remove any timer. */
> + del_timer(&aiptek->activityCheck);
> +
> /* Disassociate driver's struct with usb interface
> */
> usb_set_intfdata(intf, NULL);
I'd use del_timer_sync just in case.
Greetings,
-- Pete
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
