On Mon, 8 Apr 2013, Benjamin Tissoires wrote:
> This driver was originally written by James McKenzie, updated by
> Greg Kroah-Hartman, further updated by Bastien Nocera, with suspend
> support added.
> I ported it to the HID subsystem, in order to simplify it a litle
> and allow lirc to use it through hiddev.
>
> 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-by: Fabien André <[email protected]>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> Hi guys,
>
> this is the re-spin of Bastien's patch submitted last november.
> I ported it to use the hid subsystem, reducing the size of the driver
> to 355 lines instead of 527.
Thanks. Generally, I like the idea of appleir being on the hid bus.
Bastien, could you please provide your Ack (and most importantly also
Tested-by?).
[ ... snip ... ]
> +static void dump_packet(struct appleir *appleir, char *msg, u8 *data, int
> len)
> +{
> + appleir_dbg(appleir->hid,
> + "appleir: %s (%d bytes) %*ph (should be command %d)\n",
> + msg, len, len, data, (data[4] >> 1) & KEY_MASK);
> +}
Can't we just use HID debugfs interface for this kind of debugging output?
> +static int get_key(int data)
> +{
> + /*
> + * The indexes of the keys are computed like this:
> + * 0x00 or 0x01 ( ) key: 0 -> KEY_RESERVED
> + * 0x02 or 0x03 ( menu ) key: 1 -> KEY_MENU
> + * 0x04 or 0x05 ( >" ) key: 2 -> KEY_PLAYPAUSE
> + * 0x06 or 0x07 ( >> ) key: 3 -> KEY_FORWARD
> + * 0x08 or 0x09 ( << ) key: 4 -> KEY_BACK
> + * 0x0a or 0x0b ( + ) key: 5 -> KEY_VOLUMEUP
> + * 0x0c or 0x0d ( - ) key: 6 -> KEY_VOLUMEDOWN
> + * 0x0e or 0x0f ( ) key: 7 -> KEY_RESERVED
> + * 0x50 or 0x51 ( ) key: -8 -> KEY_RESERVED
> + * 0x52 or 0x53 ( ) key: -9 -> KEY_RESERVED
> + * 0x54 or 0x55 ( ) key: -10 -> KEY_RESERVED
> + * 0x56 or 0x57 ( ) key: -11 -> KEY_RESERVED
> + * 0x58 or 0x59 ( ) key: -12 -> KEY_RESERVED
> + * 0x5a or 0x5b ( ) key: -13 -> KEY_RESERVED
> + * 0x5c or 0x5d ( middle ) key: -14 -> KEY_ENTER
> + * 0x5e or 0x5f ( >" ) key: -15 -> KEY_PLAYPAUSE
> + */
> + int key = (data >> 1) & KEY_MASK;
> +
> + if ((data & TWO_PACKETS_MASK))
> + /* Part of a 2 packet-command */
> + key = -key;
I think all the shuffling with negative indices would justify an
explanatory comment; it's quite confusing on a first sight.
[ ... snip ... ]
> +static int appleir_raw_event(struct hid_device *hid, struct hid_report
> *report,
> + u8 *data, int len)
> +{
> + struct appleir *appleir = hid_get_drvdata(hid);
> + static const u8 keydown[] = { 0x25, 0x87, 0xee };
> + static const u8 keyrepeat[] = { 0x26, };
> + static const u8 flatbattery[] = { 0x25, 0x87, 0xe0 };
> + unsigned long flags;
> +
> + if (debug)
> + dump_packet(appleir, "received", data, len);
> +
> + if (len != 5)
> + goto out;
> +
> + if (!memcmp(data, keydown, sizeof(keydown))) {
> + int index;
> +
> + spin_lock_irqsave(&appleir->lock, flags);
> + /* If we already have a key down, take it up before marking
> + this one down */
Could you please make this comment in-line with kernel comments coding
style?
Thanks, Benjamin.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
