[RESEND] [PATCH] Input: add appleir USB driver

[RESEND] [PATCH] Input: add appleir USB driver

[Bastien Nocera]

This driver was originally written by James McKenzie, updated by
Greg Kroah-Hartman, further updated by myself, with suspend support
added.

More recent versions of the IR receiver are also supported through
a patch by Alex Karpenko. The patch also adds support for the 2nd
and 5th generation of the controller, and the menu key on newer
brushed metal remotes.

Tested on a MacbookAir1,1

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 Documentation/input/appleir.txt |   46 ++++
 drivers/hid/hid-apple.c         |    4 -
 drivers/hid/hid-core.c          |    7 +-
 drivers/hid/hid-ids.h           |    5 +-
 drivers/input/misc/Kconfig      |   13 +
 drivers/input/misc/Makefile     |    1 +
 drivers/input/misc/appleir.c    |  519 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 588 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/input/appleir.txt
 create mode 100644 drivers/input/misc/appleir.c

diff --git a/Documentation/input/appleir.txt b/Documentation/input/appleir.txt
new file mode 100644
index 0000000..db637fb
--- /dev/null
+++ b/Documentation/input/appleir.txt
@@ -0,0 +1,46 @@
+Apple IR receiver Driver (appleir)
+----------------------------------
+	Copyright (C) 2009 Bastien Nocera <hadess@hadess.net>
+
+The appleir driver is a kernel input driver to handle Apple's IR
+receivers (and associated remotes) in the kernel.
+
+The driver is an input driver which only handles "official" remotes
+as built and sold by Apple.
+
+Authors
+-------
+
+James McKenzie (original driver)
+Alex Karpenko (05ac:8242 support)
+Greg Kroah-Hartman (cleanups and original submission)
+Bastien Nocera (further cleanups, brushed metal "enter"
+button support and suspend support)
+
+Supported hardware
+------------------
+
+- All Apple laptops and desktops from 2005 onwards, except:
+  - the unibody Macbook (2009)
+  - Mac Pro (all versions)
+- Apple TV (all revisions prior to September 2010)
+
+The remote will only support the 6 (old white) or 7 (brushed metal) buttons
+of the remotes as sold by Apple. See the next section if you want to use
+other remotes or want to use lirc with the device instead of the kernel driver.
+
+Using lirc (native) instead of the kernel driver
+------------------------------------------------
+
+First, you will need to disable the kernel driver for the receiver.
+
+This can be achieved by passing quirks to the usbhid driver.
+The quirk line would be:
+usbhid.quirks=0x05ac:0x8242:0x40000010
+
+With 0x05ac being the vendor ID (Apple, you shouldn't need to change this)
+With 0x8242 being the product ID (check the output of lsusb for your hardware)
+And 0x10 being "HID_QUIRK_HIDDEV_FORCE" and 0x40000000 being "HID_QUIRK_NO_IGNORE"
+
+This should force the creation of a hiddev device for the receiver, and
+make it usable under lirc.
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index bba05d0..0059d5a 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -361,10 +361,6 @@ static void apple_remove(struct hid_device *hdev)
 }
 
 static const struct hid_device_id apple_devices[] = {
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL),
-		.driver_data = APPLE_HIDDEV | APPLE_IGNORE_HIDINPUT },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4),
-		.driver_data = APPLE_HIDDEV | APPLE_IGNORE_HIDINPUT },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE),
 		.driver_data = APPLE_MIGHTYMOUSE | APPLE_INVERT_HWHEEL },
 
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index baa25ad..abc5bd7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1244,8 +1244,6 @@ static const struct hid_device_id hid_blacklist[] = {
 #if defined(CONFIG_HID_ACRUX_FF) || defined(CONFIG_HID_ACRUX_FF_MODULE)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ACRUX, 0x0802) },
 #endif
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI) },
@@ -1577,6 +1575,11 @@ static const struct hid_device_id hid_ignore_list[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_AIPTEK, USB_DEVICE_ID_AIPTEK_24) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_AIRCABLE, USB_DEVICE_ID_AIRCABLE1) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ALCOR, USB_DEVICE_ID_ALCOR_USBRS232) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL2) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL3) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL5) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUS, USB_DEVICE_ID_ASUS_T91MT)},
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_LCM)},
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_LCM2)},
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 11af537..360a5ca 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -100,8 +100,11 @@
 #define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS   0x023b
 #define USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY	0x030a
 #define USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY	0x030b
-#define USB_DEVICE_ID_APPLE_ATV_IRCONTROL	0x8241
+#define USB_DEVICE_ID_APPLE_IRCONTROL	0x8240
+#define USB_DEVICE_ID_APPLE_IRCONTROL2	0x1440
+#define USB_DEVICE_ID_APPLE_IRCONTROL3	0x8241
 #define USB_DEVICE_ID_APPLE_IRCONTROL4	0x8242
+#define USB_DEVICE_ID_APPLE_IRCONTROL5	0x8243
 
 #define USB_VENDOR_ID_ASUS		0x0486
 #define USB_DEVICE_ID_ASUS_T91MT	0x0185
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 60de906..2f2f2e7 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -209,6 +209,19 @@ config INPUT_KEYSPAN_REMOTE
 	  To compile this driver as a module, choose M here: the module will
 	  be called keyspan_remote.
 
+config INPUT_APPLEIR
+	tristate "Apple infrared receiver (built in)"
+	depends on USB_ARCH_HAS_HCD
+	select USB
+	help
+	  Say Y here if you want to use a Apple infrared remote control. All
+	  the Apple computers from 2005 onwards include such a port, except
+	  the unibody Macbook (2009), and Mac Pros. This receiver is also
+	  used in the Apple TV set-top box prior to the 2010 model.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called appleir.
+
 config INPUT_POWERMATE
 	tristate "Griffin PowerMate and Contour Jog support"
 	depends on USB_ARCH_HAS_HCD
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 1fe1f6c..d5ef2b9 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_INPUT_ADXL34X)		+= adxl34x.o
 obj-$(CONFIG_INPUT_ADXL34X_I2C)		+= adxl34x-i2c.o
 obj-$(CONFIG_INPUT_ADXL34X_SPI)		+= adxl34x-spi.o
 obj-$(CONFIG_INPUT_APANEL)		+= apanel.o
+obj-$(CONFIG_INPUT_APPLEIR)		+= appleir.o
 obj-$(CONFIG_INPUT_ATI_REMOTE)		+= ati_remote.o
 obj-$(CONFIG_INPUT_ATI_REMOTE2)		+= ati_remote2.o
 obj-$(CONFIG_INPUT_ATLAS_BTNS)		+= atlas_btns.o
diff --git a/drivers/input/misc/appleir.c b/drivers/input/misc/appleir.c
new file mode 100644
index 0000000..3817a3c
--- /dev/null
+++ b/drivers/input/misc/appleir.c
@@ -0,0 +1,519 @@
+/*
+ * appleir: USB driver for the apple ir device
+ *
+ * Original driver written by James McKenzie
+ * Ported to recent 2.6 kernel versions by Greg Kroah-Hartman <gregkh@suse.de>
+ *
+ * Copyright (C) 2006 James McKenzie
+ * Copyright (C) 2008 Greg Kroah-Hartman <greg@kroah.com>
+ * Copyright (C) 2008 Novell Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation, version 2.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/usb/input.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/usb.h>
+#include <linux/usb/input.h>
+#include <asm/unaligned.h>
+#include <asm/byteorder.h>
+
+#define DRIVER_VERSION "v1.2"
+#define DRIVER_AUTHOR "James McKenzie"
+#define DRIVER_DESC "Apple infrared receiver driver"
+#define DRIVER_LICENSE "GPL"
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE(DRIVER_LICENSE);
+
+#define USB_VENDOR_ID_APPLE			0x05ac
+#define USB_DEVICE_ID_APPLE_IRCONTROL		0x8240
+#define USB_DEVICE_ID_APPLE_IRCONTROL2		0x1440
+#define USB_DEVICE_ID_APPLE_IRCONTROL3		0x8241
+#define USB_DEVICE_ID_APPLE_IRCONTROL4		0x8242
+#define USB_DEVICE_ID_APPLE_IRCONTROL5		0x8243
+
+#define URB_SIZE	32
+
+#define MAX_KEYS	9
+#define MAX_KEYS_MASK	(MAX_KEYS - 1)
+
+#define dbginfo(dev, format, arg...) do { if (debug) dev_info(dev , format , ## arg); } while (0)
+
+static int debug;
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "Enable extra debug messages and information");
+
+/* I have two devices both of which report the following */
+/* 25 87 ee 83 0a  	+  */
+/* 25 87 ee 83 0c  	-  */
+/* 25 87 ee 83 09	<< */
+/* 25 87 ee 83 06	>> */
+/* 25 87 ee 83 05	>" */
+/* 25 87 ee 83 03	menu */
+/* 26 00 00 00 00	for key repeat*/
+
+/* Thomas Glanzmann reports the following responses */
+/* 25 87 ee ca 0b	+  */
+/* 25 87 ee ca 0d	-  */
+/* 25 87 ee ca 08	<< */
+/* 25 87 ee ca 07	>> */
+/* 25 87 ee ca 04	>" */
+/* 25 87 ee ca 02 	menu */
+/* 26 00 00 00 00       for key repeat*/
+/* He also observes the following event sometimes */
+/* sent after a key is release, which I interpret */
+/* as a flat battery message */
+/* 25 87 e0 ca 06	flat battery */
+
+/* Alexandre Karpenko reports the following responses for Device ID 0x8242 */
+/* 25 87 ee 47 0b	+  */
+/* 25 87 ee 47 0d	-  */
+/* 25 87 ee 47 08	<< */
+/* 25 87 ee 47 07	>> */
+/* 25 87 ee 47 04	>" */
+/* 25 87 ee 47 02 	menu */
+/* 26 87 ee 47 ** 	for key repeat (** is the code of the key being held) */
+
+/* Bastien Nocera's "new" remote */
+/* 25 87 ee 91 5f	followed by
+ * 25 87 ee 91 05	gives you >"
+ *
+ * 25 87 ee 91 5c	followed by
+ * 25 87 ee 91 05	gives you the middle button */
+
+static const unsigned short appleir_key_table[] = {
+	KEY_RESERVED,
+	KEY_MENU,
+	KEY_PLAYPAUSE,
+	KEY_FORWARD,
+	KEY_BACK,
+	KEY_VOLUMEUP,
+	KEY_VOLUMEDOWN,
+	KEY_ENTER,
+	KEY_RESERVED,
+};
+
+struct appleir {
+	struct input_dev *input_dev;
+	unsigned short keymap[ARRAY_SIZE(appleir_key_table)];
+	u8 *data;
+	dma_addr_t dma_buf;
+	struct usb_device *usbdev;
+	unsigned int flags;
+	struct urb *urb;
+	struct timer_list key_up_timer;
+	int current_key;
+	int prev_key_idx;
+	char phys[32];
+};
+
+static DEFINE_MUTEX(appleir_mutex);
+
+enum {
+	APPLEIR_OPENED = 0x1,
+	APPLEIR_SUSPENDED = 0x2,
+};
+
+static struct usb_device_id appleir_ids[] = {
+	{ USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL) },
+	{ USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL2) },
+	{ USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL3) },
+	{ USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
+	{ USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL5) },
+	{}
+};
+MODULE_DEVICE_TABLE(usb, appleir_ids);
+
+static void dump_packet(struct appleir *appleir, char *msg, u8 *data, int len)
+{
+	int i;
+
+	printk(KERN_ERR "appleir: %s (%d bytes)", msg, len);
+
+	for (i = 0; i < len; ++i)
+		printk(" %02x", data[i]);
+	printk(" (should be command %d)\n", (data[4] >> 1) & MAX_KEYS_MASK);
+}
+
+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;
+	}
+}
+
+static void key_up(struct appleir *appleir, int key)
+{
+	dbginfo(&appleir->input_dev->dev, "key %d up\n", key);
+	input_report_key(appleir->input_dev, key, 0);
+	input_sync(appleir->input_dev);
+}
+
+static void key_down(struct appleir *appleir, int key)
+{
+	dbginfo(&appleir->input_dev->dev, "key %d down\n", key);
+	input_report_key(appleir->input_dev, key, 1);
+	input_sync(appleir->input_dev);
+}
+
+static void battery_flat(struct appleir *appleir)
+{
+	dev_err(&appleir->input_dev->dev, "possible flat battery?\n");
+}
+
+static void key_up_tick(unsigned long data)
+{
+	struct appleir *appleir = (struct appleir *)data;
+
+	if (appleir->current_key) {
+		key_up(appleir, appleir->current_key);
+		appleir->current_key = 0;
+	}
+}
+
+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);
+
+		/* 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;
+			return;
+		}
+	}
+
+	appleir->prev_key_idx = 0;
+
+	if (!memcmp(data, keyrepeat, sizeof(keyrepeat))) {
+		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);
+		return;
+	}
+
+	if (!memcmp(data, flatbattery, sizeof(flatbattery))) {
+		battery_flat(appleir);
+		/* Fall through */
+	}
+
+	dump_packet(appleir, "unknown packet", data, len);
+}
+
+static void appleir_urb(struct urb *urb)
+{
+	struct appleir *appleir = urb->context;
+	int status = urb->status;
+	int retval;
+
+	switch (status) {
+	case 0:
+		new_data(appleir, urb->transfer_buffer, urb->actual_length);
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		/* This urb is terminated, clean up */
+		dbginfo(&appleir->input_dev->dev, "%s - urb shutting down with status: %d", __func__,
+			urb->status);
+		return;
+	default:
+		dbginfo(&appleir->input_dev->dev, "%s - nonzero urb status received: %d", __func__,
+			urb->status);
+	}
+
+	retval = usb_submit_urb(urb, GFP_ATOMIC);
+	if (retval)
+		err("%s - usb_submit_urb failed with result %d", __func__,
+		    retval);
+}
+
+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)) {
+		r = -EIO;
+		goto fail;
+	}
+
+	appleir->flags |= APPLEIR_OPENED;
+
+	mutex_unlock(&appleir_mutex);
+
+	usb_autopm_put_interface(intf);
+
+	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);
+	}
+
+	appleir->flags &= ~APPLEIR_OPENED;
+
+	mutex_unlock(&appleir_mutex);
+}
+
+static int appleir_probe(struct usb_interface *intf,
+			 const struct usb_device_id *id)
+{
+	struct usb_device *dev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *endpoint;
+	struct appleir *appleir = NULL;
+	struct input_dev *input_dev;
+	int retval = -ENOMEM;
+	int i;
+
+	appleir = kzalloc(sizeof(struct appleir), GFP_KERNEL);
+	if (!appleir)
+		goto allocfail;
+
+	appleir->data = usb_alloc_coherent(dev, URB_SIZE, GFP_KERNEL,
+					 &appleir->dma_buf);
+	if (!appleir->data)
+		goto usbfail;
+
+	appleir->urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!appleir->urb)
+		goto urbfail;
+
+	appleir->usbdev = dev;
+
+	input_dev = input_allocate_device();
+	if (!input_dev)
+		goto inputfail;
+
+	appleir->input_dev = input_dev;
+
+	usb_make_path(dev, appleir->phys, sizeof(appleir->phys));
+	strlcpy(appleir->phys, "/input0", sizeof(appleir->phys));
+
+	input_dev->name = "Apple Infrared Remote Controller";
+	input_dev->phys = appleir->phys;
+	usb_to_input_id(dev, &input_dev->id);
+	input_dev->dev.parent = &intf->dev;
+	input_dev->keycode = appleir->keymap;
+	input_dev->keycodesize = sizeof(unsigned short);
+	input_dev->keycodemax = ARRAY_SIZE(appleir->keymap);
+
+	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP);
+
+	memcpy(appleir->keymap, appleir_key_table, sizeof(appleir->keymap));
+	for (i = 0; i < ARRAY_SIZE(appleir_key_table); i++)
+		set_bit(appleir->keymap[i], input_dev->keybit);
+	clear_bit(KEY_RESERVED, input_dev->keybit);
+
+	input_set_drvdata(input_dev, appleir);
+	input_dev->open = appleir_open;
+	input_dev->close = appleir_close;
+
+	endpoint = &intf->cur_altsetting->endpoint[0].desc;
+
+	usb_fill_int_urb(appleir->urb, dev,
+			 usb_rcvintpipe(dev, endpoint->bEndpointAddress),
+			 appleir->data, 8,
+			 appleir_urb, appleir, endpoint->bInterval);
+
+	appleir->urb->transfer_dma = appleir->dma_buf;
+	appleir->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+
+	setup_timer(&appleir->key_up_timer,
+		    key_up_tick, (unsigned long) appleir);
+
+	retval = input_register_device(appleir->input_dev);
+	if (retval)
+		goto inputfail;
+
+	usb_set_intfdata(intf, appleir);
+
+	return 0;
+
+inputfail:
+	input_free_device(appleir->input_dev);
+
+urbfail:
+	usb_free_urb(appleir->urb);
+
+usbfail:
+	usb_free_coherent(dev, URB_SIZE, appleir->data,
+			appleir->dma_buf);
+
+allocfail:
+	kfree(appleir);
+
+	return retval;
+}
+
+static void appleir_disconnect(struct usb_interface *intf)
+{
+	struct appleir *appleir = usb_get_intfdata(intf);
+
+	usb_set_intfdata(intf, NULL);
+	input_unregister_device(appleir->input_dev);
+	usb_free_urb(appleir->urb);
+	usb_free_coherent(interface_to_usbdev(intf), URB_SIZE,
+			appleir->data, appleir->dma_buf);
+	kfree(appleir);
+}
+
+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)
+		usb_kill_urb(appleir->urb);
+
+	appleir->flags |= APPLEIR_SUSPENDED;
+
+	mutex_unlock(&appleir_mutex);
+
+	return 0;
+}
+
+static int appleir_resume(struct usb_interface *interface)
+{
+	struct appleir *appleir;
+	int r = 0;
+
+	appleir = usb_get_intfdata(interface);
+
+	mutex_lock(&appleir_mutex);
+	if (appleir->flags & APPLEIR_OPENED) {
+		struct usb_endpoint_descriptor *endpoint;
+
+		endpoint = &interface->cur_altsetting->endpoint[0].desc;
+		usb_fill_int_urb(appleir->urb, appleir->usbdev,
+				 usb_rcvintpipe(appleir->usbdev, endpoint->bEndpointAddress),
+				 appleir->data, 8,
+				 appleir_urb, appleir, endpoint->bInterval);
+		appleir->urb->transfer_dma = appleir->dma_buf;
+		appleir->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
+
+		/* And reset the USB device */
+		if (usb_submit_urb(appleir->urb, GFP_ATOMIC))
+			r = -EIO;
+	}
+
+	appleir->flags &= ~APPLEIR_SUSPENDED;
+
+	mutex_unlock(&appleir_mutex);
+
+	return r;
+}
+
+static struct usb_driver appleir_driver = {
+	.name                 = "appleir",
+	.probe                = appleir_probe,
+	.disconnect           = appleir_disconnect,
+	.suspend              = appleir_suspend,
+	.resume               = appleir_resume,
+	.reset_resume         = appleir_resume,
+	.id_table             = appleir_ids,
+};
+
+static int __init appleir_init(void)
+{
+	return usb_register(&appleir_driver);
+}
+
+static void __exit appleir_exit(void)
+{
+	usb_deregister(&appleir_driver);
+}
+
+module_init(appleir_init);
+module_exit(appleir_exit);
-- 
1.7.2.2

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Dmitry Torokhov]

Bastien,

On Thursday, September 30, 2010 03:57:55 am Bastien Nocera wrote:
> This driver was originally written by James McKenzie, updated by
> Greg Kroah-Hartman, further updated by myself, with suspend support
> added.
> 
> More recent versions of the IR receiver are also supported through
> a patch by Alex Karpenko. The patch also adds support for the 2nd
> and 5th generation of the controller, and the menu key on newer
> brushed metal remotes.
> 
> Tested on a MacbookAir1,1
> 

There are no changes from the last time you sent it, right? I'll try
to get the driver in .37 but after that we should probably move it
over to the new IR infrastructure...

Thanks.

-- 
Dmitry

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Bastien Nocera]

On Thu, 2010-09-30 at 08:47 -0700, Dmitry Torokhov wrote:
> Bastien,
> 
> On Thursday, September 30, 2010 03:57:55 am Bastien Nocera wrote:
> > This driver was originally written by James McKenzie, updated by
> > Greg Kroah-Hartman, further updated by myself, with suspend support
> > added.
> > 
> > More recent versions of the IR receiver are also supported through
> > a patch by Alex Karpenko. The patch also adds support for the 2nd
> > and 5th generation of the controller, and the menu key on newer
> > brushed metal remotes.
> > 
> > Tested on a MacbookAir1,1
> > 
> 
> There are no changes from the last time you sent it, right?

No, but given that I received no comments on it since I submitted it in
February, and again a couple of weeks ago, I thought it might need
somebody else to look into it.

>  I'll try
> to get the driver in .37 but after that we should probably move it
> over to the new IR infrastructure...

I'm not sure what that would bring us, nor do I have any experience in
using the ir-core. From what I understood, it still needed fleshing out.

Cheers

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Oliver Neukum]

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?


> +
> +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;
> +			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.

> +	}
> +
> +	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.
> +
> +	appleir->flags |= APPLEIR_SUSPENDED;
> +
> +	mutex_unlock(&appleir_mutex);
> +
> +	return 0;
> +}
> +
> +static int appleir_resume(struct usb_interface *interface)
> +{
> +	struct appleir *appleir;
> +	int r = 0;
> +
> +	appleir = usb_get_intfdata(interface);
> +
> +	mutex_lock(&appleir_mutex);
> +	if (appleir->flags & APPLEIR_OPENED) {
> +		struct usb_endpoint_descriptor *endpoint;
> +
> +		endpoint = &interface->cur_altsetting->endpoint[0].desc;
> +		usb_fill_int_urb(appleir->urb, appleir->usbdev,
> +				 usb_rcvintpipe(appleir->usbdev, endpoint->bEndpointAddress),
> +				 appleir->data, 8,
> +				 appleir_urb, appleir, endpoint->bInterval);
> +		appleir->urb->transfer_dma = appleir->dma_buf;
> +		appleir->urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +
> +		/* And reset the USB device */
> +		if (usb_submit_urb(appleir->urb, GFP_ATOMIC))

GFP_NOIO is sufficient.

> +static struct usb_driver appleir_driver = {
> +	.name                 = "appleir",
> +	.probe                = appleir_probe,
> +	.disconnect           = appleir_disconnect,
> +	.suspend              = appleir_suspend,
> +	.resume               = appleir_resume,
> +	.reset_resume         = appleir_resume,
> +	.id_table             = appleir_ids,

If you want runtime power management you need to define
supports_autosuspend.

	Regards
		Oliver

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Dmitry Torokhov]

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

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Oliver Neukum]

Am Dienstag, 5. Oktober 2010, 06:45:20 schrieb Dmitry Torokhov:
> > > +{
> > > +   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.

But what about the next opener? He'll get a completely spurious
key release event, as the next key is pressed.

> > > +           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.

I see. OK, this is covered.

	Regards
		Oliver

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Dmitry Torokhov]

On Tue, Oct 05, 2010 at 08:02:14AM +0200, Oliver Neukum wrote:
> Am Dienstag, 5. Oktober 2010, 06:45:20 schrieb Dmitry Torokhov:
> > > > +{
> > > > +   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.
> 
> But what about the next opener? He'll get a completely spurious
> key release event, as the next key is pressed.

How does the opening of a device handle relate to a device state?
Userspace should expect to see releases without presses (in case they
weren't the first client that opened the device).

But I guess we should reset the "last key" here, just in case.

> 
> > > > +           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.
> 
> I see. OK, this is covered.

Might be a good idea to reset "last key" here as well. No need to send
2nd release event later on.

-- 
Dmitry

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Oliver Neukum]

Am Dienstag, 5. Oktober 2010, 08:08:22 schrieb Dmitry Torokhov:
> > But what about the next opener? He'll get a completely spurious
> > key release event, as the next key is pressed.
> 
> How does the opening of a device handle relate to a device state?

It doesn't.

> Userspace should expect to see releases without presses (in case they
> weren't the first client that opened the device).

Yes, but based on sane timing. From the time stamps user space
would conclude that the key had been pressed at an unknown time
before open and released at the time the event indicates. Possibly
it would compute that the key had been held for at least hours.

	Regards
		Oliver

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Dmitry Torokhov]

On Tue, Oct 05, 2010 at 08:18:35AM +0200, Oliver Neukum wrote:
> Am Dienstag, 5. Oktober 2010, 08:08:22 schrieb Dmitry Torokhov:
> > > But what about the next opener? He'll get a completely spurious
> > > key release event, as the next key is pressed.
> > 
> > How does the opening of a device handle relate to a device state?
> 
> It doesn't.
> 
> > Userspace should expect to see releases without presses (in case they
> > weren't the first client that opened the device).
> 
> Yes, but based on sane timing. From the time stamps user space
> would conclude that the key had been pressed at an unknown time
> before open and released at the time the event indicates. Possibly
> it would compute that the key had been held for at least hours.
> 

I do not understand. If a client never seen the "press" event and only
saw "release" event it can't make any assumptions about timing. Press
could be a millisecond ago or an hour ago, it just does not know. And
such scenario can easily happen if the client is second to open the
device.

I think the only sane behavior for clients is to ignore release events
for keys that they did not see "down" event for.

- 
Dmitry

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Oliver Neukum]

Am Dienstag, 5. Oktober 2010, 17:55:52 schrieb Dmitry Torokhov:
> > Yes, but based on sane timing. From the time stamps user space
> > would conclude that the key had been pressed at an unknown time
> > before open and released at the time the event indicates. Possibly
> > it would compute that the key had been held for at least hours.
> > 
> 
> I do not understand. If a client never seen the "press" event and only
> saw "release" event it can't make any assumptions about timing. Press
> could be a millisecond ago or an hour ago, it just does not know. And
> such scenario can easily happen if the client is second to open the
> device.

As soon as you open a device you see key presses. If you see a release
without a press, the press must have happened before you opened.
So if you know when you opened and when a key is released, you'll
have a lower limit on how long the key must have been held down.

	Regards
		Oliver

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Dmitry Torokhov]

On Tue, Oct 05, 2010 at 06:07:36PM +0200, Oliver Neukum wrote:
> Am Dienstag, 5. Oktober 2010, 17:55:52 schrieb Dmitry Torokhov:
> > > Yes, but based on sane timing. From the time stamps user space
> > > would conclude that the key had been pressed at an unknown time
> > > before open and released at the time the event indicates. Possibly
> > > it would compute that the key had been held for at least hours.
> > > 
> > 
> > I do not understand. If a client never seen the "press" event and only
> > saw "release" event it can't make any assumptions about timing. Press
> > could be a millisecond ago or an hour ago, it just does not know. And
> > such scenario can easily happen if the client is second to open the
> > device.
> 
> As soon as you open a device you see key presses. If you see a release
> without a press, the press must have happened before you opened.
> So if you know when you opened and when a key is released, you'll
> have a lower limit on how long the key must have been held down.
> 

Yes. I still do not see the usefulness of this data.

-- 
Dmitry

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Jarod Wilson]

On Thu, Sep 30, 2010 at 04:51:30PM +0100, Bastien Nocera wrote:
> On Thu, 2010-09-30 at 08:47 -0700, Dmitry Torokhov wrote:
> > Bastien,
...
> >  I'll try
> > to get the driver in .37 but after that we should probably move it
> > over to the new IR infrastructure...
> 
> I'm not sure what that would bring us, nor do I have any experience in
> using the ir-core. From what I understood, it still needed fleshing out.

There are a few things that still need fleshing out, but nothing really
that should have any major impact on a potential port of this driver.

Off the top of my head, the main thing it would give you is the ability to
upload your own keymap for the receiver. Then someone wanting to use more
than 6 buttons or whatever it is might still be able to get the benefits
of in-kernel decoding with everything coming through the input layer,
rather than using lirc or the like.

-- 
Jarod Wilson
jarod@redhat.com

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Bastien Nocera]

On Thu, 2010-10-07 at 20:39 -0400, Jarod Wilson wrote:
> On Thu, Sep 30, 2010 at 04:51:30PM +0100, Bastien Nocera wrote:
> > On Thu, 2010-09-30 at 08:47 -0700, Dmitry Torokhov wrote:
> > > Bastien,
> ...
> > >  I'll try
> > > to get the driver in .37 but after that we should probably move it
> > > over to the new IR infrastructure...
> > 
> > I'm not sure what that would bring us, nor do I have any experience in
> > using the ir-core. From what I understood, it still needed fleshing out.
> 
> There are a few things that still need fleshing out, but nothing really
> that should have any major impact on a potential port of this driver.
> 
> Off the top of my head, the main thing it would give you is the ability to
> upload your own keymap for the receiver.

You could already change the keymap...

>  Then someone wanting to use more
> than 6 buttons

But only for the 6 buttons supported.

>  or whatever it is might still be able to get the benefits
> of in-kernel decoding with everything coming through the input layer,
> rather than using lirc or the like.

I see very little benefits myself (I mean, unless you're adamant that
you want to use a remote that's not intended for that computer, but we
already have lirc user-space driver for doing that), and it would remove
metadata from the events, so we couldn't do "remote filtering" (each
remote has a semi-random UID, which gets passed in with the events).

When the appleir patch was finally accepted (which means I need to get
off my arse and do the changes Oliver mentioned), working on UID
filtering is next on my list of TODOs.

Cheers

Re: [RESEND] [PATCH] Input: add appleir USB driver

[Jarod Wilson]

On Thu, Nov 18, 2010 at 04:18:03AM +0000, Bastien Nocera wrote:
> On Thu, 2010-10-07 at 20:39 -0400, Jarod Wilson wrote:
> > On Thu, Sep 30, 2010 at 04:51:30PM +0100, Bastien Nocera wrote:
> > > On Thu, 2010-09-30 at 08:47 -0700, Dmitry Torokhov wrote:
> > > > Bastien,
> > ...
> > > >  I'll try
> > > > to get the driver in .37 but after that we should probably move it
> > > > over to the new IR infrastructure...
> > > 
> > > I'm not sure what that would bring us, nor do I have any experience in
> > > using the ir-core. From what I understood, it still needed fleshing out.
> > 
> > There are a few things that still need fleshing out, but nothing really
> > that should have any major impact on a potential port of this driver.
> > 
> > Off the top of my head, the main thing it would give you is the ability to
> > upload your own keymap for the receiver.
> 
> You could already change the keymap...

You could change the mapping for the 6 buttons, but new scancode to
keycode mappings wouldn't be possible. That's assuming the driver actually
does raw IR decode -- the scancodes I see in the key tables in the driver
definitely match decoded NEC that I get when using the Apple remote with
another raw IR receiver (mceusb). Reading through the code though, it
appears the IR signal is decoded in hardware and only the resulting
scancodes are passed along... I wonder if it passes along scancodes for
all NEC signals or if its doing filtering... I have sufficiently equipped
Apple hardware of my own that I suppose I really ought to prod.

> >  Then someone wanting to use more
> > than 6 buttons
> 
> But only for the 6 buttons supported.
> 
> >  or whatever it is might still be able to get the benefits
> > of in-kernel decoding with everything coming through the input layer,
> > rather than using lirc or the like.
> 
> I see very little benefits myself (I mean, unless you're adamant that
> you want to use a remote that's not intended for that computer, but we
> already have lirc user-space driver for doing that),

I don't see why we shouldn't be able to do it in-kernel too, without the
need for any userspace daemon. The bigger win though is probably
de-duplication of keyup/keydown/timer logic. I had similar duplication in
the imon driver (which is also scancode-based), and the code is smaller
and cleaner after being adapted to use ir/rc-core functions.

> and it would remove
> metadata from the events, so we couldn't do "remote filtering" (each
> remote has a semi-random UID, which gets passed in with the events).

No it wouldn't. The lsb of the decoded NEC pulse train contains the ID,
I'm able to see it just fine with other raw IR receivers and some
experimental patches we're kicking around on linux-media.

> When the appleir patch was finally accepted (which means I need to get
> off my arse and do the changes Oliver mentioned), working on UID
> filtering is next on my list of TODOs.

I've got an implementation here using the in-kernel NEC decoder, an Apple
remote any an mceusb transceiver that works. Its not terribly *elegant*,
but it does work. (Executive summary: if not doing pairing, zero out the
pair ID and use the full 32-bit scancode with a default keytable that has
0x00 in the pair ID position. If doing pairing, pass that byte unmodified,
and upload a keymap that matches).

-- 
Jarod Wilson
jarod@redhat.com