Re: [PATCH/RFC?] usb/input: Add support for fn key on Apple PowerBooks

Dmitry Torokhov
2006-01-13T04:12:10Z

On Wednesday 11 January 2006 19:08, Michael Hanselmann wrote:
> On Thu, Jan 12, 2006 at 10:41:40AM +1100, Benjamin Herrenschmidt wrote:
> > 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?

> #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