Re: [PATCH/RFC?] usb/input: Add support for fn key on Apple PowerBooks
From: Dmitry Torokhov <hidden>
Date: 2006-01-13 04:12:10
Also in:
lkml
On Wednesday 11 January 2006 19:08, Michael Hanselmann wrote:
On Thu, Jan 12, 2006 at 10:41:40AM +1100, Benjamin Herrenschmidt wrote:quoted
I don't understand the comment above ? You are adding this to all struct hid_device ? There can be only one of those keyboards plugged at one point in time, I don't think there is any problem having the above static in the driver rather than in the hid_device structure.While I don't agree on that, I still modified the patch.
I disagree as well, but if we get semantics right we can merge it in this form and adjust later.
+static int usbhid_pb_fnmode = 1; +module_param_named(pb_fnmode, usbhid_pb_fnmode, int, 0644); +MODULE_PARM_DESC(usbhid_pb_fnmode, + "Mode of fn key on PowerBooks (0 = disabled, " + "1 = fkeyslast, 2 = fkeysfirst)"); +#endif +
That should be "MODULE_PARM_DESC(pb_fn_mode, ...)". Also, since this is for compatibility with ADB, why do we have 3 options? Doesn't ADB have only 2?
quoted hunk ↗ jump to hunk
#define map_abs(c) do { usage->code = c; usage->type = EV_ABS; bit = input->absbit; max = ABS_MAX; } while (0) #define map_rel(c) do { usage->code = c; usage->type = EV_REL; bit = input->relbit; max = REL_MAX; } while (0) #define map_key(c) do { usage->code = c; usage->type = EV_KEY; bit = input->keybit; max = KEY_MAX; } while (0)@@ -73,6 +135,80 @@ static struct { #define map_key_clear(c) do { map_key(c); clear_bit(c, bit); } while (0) #define map_ff_effect(c) do { set_bit(c, input->ffbit); } while (0) +static inline struct hidinput_key_translation *find_translation(
I thought is was agreed that we'd avoid "inlines" in .c files?
+ struct hidinput_key_translation *table, u16 from)
+{
+ struct hidinput_key_translation *trans;
+
+ /* Look for the translation */
+ for(trans = table; trans->from && (trans->from != from); trans++);
+
+ return (trans->from?trans:NULL);
+}I'd prefer liberal amount of spaces applied here </extreme nitpick mode>
+
+static int hidinput_pb_event(struct hid_device *hid, struct input_dev *input,
+ struct hid_usage *usage, __s32 value)
+{
+#ifdef CONFIG_USB_HIDINPUT_POWERBOOK
+ struct hidinput_key_translation *trans;
+
+ if (usage->code == KEY_FN) {
+ if (value) hid->quirks |= HID_QUIRK_POWERBOOK_FN_ON;
+ else hid->quirks &= ~HID_QUIRK_POWERBOOK_FN_ON;
+
+ input_event(input, usage->type, usage->code, value);
+
+ return 1;
+ }
+
+ if (usbhid_pb_fnmode) {
+ int try_translate, do_translate;
+
+ trans = find_translation(powerbook_fn_keys, usage->code);
+ if (trans) {
+ if (test_bit(usage->code, usbhid_pb_fn))
+ do_translate = 1;
+ else if (trans->flags & POWERBOOK_FLAG_FKEY)
+ do_translate =
+ (usbhid_pb_fnmode == 2 && (hid->quirks & HID_QUIRK_POWERBOOK_FN_ON)) ||
+ (usbhid_pb_fnmode == 1 && !(hid->quirks & HID_QUIRK_POWERBOOK_FN_ON));
+ else
+ do_translate = (hid->quirks & HID_QUIRK_POWERBOOK_FN_ON);
+
+ if (do_translate) {
+ if (value)
+ set_bit(usage->code, usbhid_pb_fn);
+ else
+ clear_bit(usage->code, usbhid_pb_fn);
+
+ input_event(input, usage->type, trans->to, value);
+
+ return 1;
+ }
+ }
+
+ try_translate = test_bit(usage->code, usbhid_pb_numlock)?1:
+ test_bit(LED_NUML, input->led);
+ if (try_translate) {Isn't this the same as if (test_bit(usage->code, usbhid_pb_numlock) || test_bit(LED_NUML, input->led)) but harder to read? -- Dmitry