On Mon, Oct 04, 2010 at 09:17:41PM +0200, Oliver Neukum wrote:
> Am Donnerstag, 30. September 2010, 12:57:55 schrieb Bastien Nocera:
>
> > +static int get_key(int data)
> > +{
> > + switch (data) {
> > + case 0x02:
> > + case 0x03:
> > + /* menu */
> > + return 1;
> > + case 0x04:
> > + case 0x05:
> > + /* >" */
> > + return 2;
> > + case 0x06:
> > + case 0x07:
> > + /* >> */
> > + return 3;
> > + case 0x08:
> > + case 0x09:
> > + /* << */
> > + return 4;
> > + case 0x0a:
> > + case 0x0b:
> > + /* + */
> > + return 5;
> > + case 0x0c:
> > + case 0x0d:
> > + /* - */
> > + return 6;
> > + case 0x5c:
> > + /* Middle button, on newer remotes,
> > + * part of a 2 packet-command */
> > + return -7;
> > + default:
> > + return -1;
> > + }
>
> Is this really better than a table lookup?
>
I think this driver should simply be converted to sparse keymap. It will
also make sure that keymap can be changed from userspace.
> > +static void new_data(struct appleir *appleir, u8 *data, int len)
> > +{
> > + static const u8 keydown[] = { 0x25, 0x87, 0xee };
> > + static const u8 keyrepeat[] = { 0x26, };
> > + static const u8 flatbattery[] = { 0x25, 0x87, 0xe0 };
> > +
> > + if (debug)
> > + dump_packet(appleir, "received", data, len);
> > +
> > + if (len != 5)
> > + return;
> > +
> > + if (!memcmp(data, keydown, sizeof(keydown))) {
> > + int index;
> > +
> > + /* If we already have a key down, take it up before marking
> > + this one down */
> > + if (appleir->current_key)
> > + key_up(appleir, appleir->current_key);
>
> This races with the timer. You can end up with a key reported up twice.
>
> > + /* Handle dual packet commands */
> > + if (appleir->prev_key_idx > 0)
> > + index = appleir->prev_key_idx;
> > + else
> > + index = get_key(data[4]);
> > +
> > + if (index > 0) {
> > + appleir->current_key = appleir->keymap[index];
> > +
> > + key_down(appleir, appleir->current_key);
> > + /* Remote doesn't do key up, either pull them up, in the test
> > + above, or here set a timer which pulls them up after 1/8 s */
> > + mod_timer(&appleir->key_up_timer, jiffies + HZ / 8);
> > + appleir->prev_key_idx = 0;
> > + return;
> > + } else if (index == -7) {
> > + /* Remember key for next packet */
> > + appleir->prev_key_idx = 0 - index;
So it alwas sets prev_key_index to 7 which is ">>". What about the
middle button?
> > + return;
> > + }
> > + }
>
>
> > +static int appleir_open(struct input_dev *dev)
> > +{
> > + struct appleir *appleir = input_get_drvdata(dev);
> > + struct usb_interface *intf = usb_ifnum_to_if(appleir->usbdev, 0);
> > + int r;
> > +
> > + r = usb_autopm_get_interface(intf);
> > + if (r) {
> > + dev_err(&intf->dev,
> > + "%s(): usb_autopm_get_interface() = %d\n", __func__, r);
> > + return r;
> > + }
> > +
> > + mutex_lock(&appleir_mutex);
> > +
> > + if (usb_submit_urb(appleir->urb, GFP_ATOMIC)) {
>
> No need for ATOMIC
>
> > + r = -EIO;
> > + goto fail;
> > + }
> > +
> > + appleir->flags |= APPLEIR_OPENED;
> > +
> > + mutex_unlock(&appleir_mutex);
> > +
> > + usb_autopm_put_interface(intf);
>
> This does not enable remote wakeup. Why? Looks like a bug.
>
> > + return 0;
> > +fail:
> > + mutex_unlock(&appleir_mutex);
> > + usb_autopm_put_interface(intf);
> > + return r;
> > +}
> > +
> > +static void appleir_close(struct input_dev *dev)
> > +{
> > + struct appleir *appleir = input_get_drvdata(dev);
> > +
> > + mutex_lock(&appleir_mutex);
> > +
> > + if (!(appleir->flags & APPLEIR_SUSPENDED)) {
> > + usb_kill_urb(appleir->urb);
> > + del_timer_sync(&appleir->key_up_timer);
>
> You can close with a key unreleased.
I think this is handled by input core. We forcibly send release events
when device is disconnected; this takes care of surprise disconnect case.
OTOH if input_dev->close() is called that means that there are no more
listeners for the events so the fact that a key is still pressed is not
interesting to anyone.
>
> > + }
> > +
> > + appleir->flags &= ~APPLEIR_OPENED;
> > +
> > + mutex_unlock(&appleir_mutex);
> > +}
> > +
>
>
> > +static int appleir_suspend(struct usb_interface *interface,
> > + pm_message_t message)
> > +{
> > + struct appleir *appleir = usb_get_intfdata(interface);
> > +
> > + mutex_lock(&appleir_mutex);
> > + if (appleir->flags & APPLEIR_OPENED)
>
> Why bother checking?
>
> > + usb_kill_urb(appleir->urb);
>
> If the system goes to sleep you'd better report a pressed key
> as released here and kill the timer.
Input core sends "release" events upon resume so we should be OK.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
