hilkbd: Kill compiler warning and fix comment dyslexia
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 10/33 hilkbd: Kill compiler warning and fix comment dyslexia
hilkbd: Kill compiler warning and fix comment dyslexia
--- drivers/input/keyboard/hilkbd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/keyboard/hilkbd.c +1 -1
@@ -52,7 +52,7 @@ MODULE_LICENSE("GPL v2");#elif defined(CONFIG_HP300 ) -#define HILBASE 0xf0428000/* HP300 (m86k) port address */ +#define HILBASE 0xf0428000UL/* HP300 (m68k) port address */ #define HIL_DATA 0x1#define HIL_CMD 0x3#define HIL_IRQ 2
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
parisc-linux mailing list
[email protected]
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
A message that is not in the archive (sent off-list or not yet received): [email protected]
Dmitry Torokhov
maintainerRe: [patch 04/33] m68k: Atari keyboard and mouse support.
Hi,
Review
Quoted 9 lines · Geert Uytterhoeven
(quoted diff) +9
-0+ +/* These could be settable by some ioctl() in future... */ +static unsigned int key_repeat_delay = DEFAULT_KEYB_REP_DELAY; +static unsigned int key_repeat_rate = DEFAULT_KEYB_REP_RATE; + +static unsigned char rep_scancode; +static struct timer_list atakeyb_rep_timer = { + .function = atakeyb_rep, +};
Is there a problem with repeat implementation in input core that requres custom-made repeater here?
Review
Quoted 2 lines
(quoted diff) +2
-0+ + // handle_scancode(scancode, !break_flag);
The code looks a bit dirty (old code commented out, C++ style comments).
Review
Quoted 7 lines
(quoted diff) +7
-0+ + +int atari_kbdrate(struct kbd_repeat *k) +{ + if (k->delay > 0) { + /* convert from msec to jiffies */ + key_repeat_delay = (k->delay * HZ + 500) / 1000;
If this is really needed - msecs_to_jiffies().
Review
Quoted 11 lines
(quoted diff) +10
-0+config KEYBOARD_ATARI + tristate "Atari keyboard" + depends on ATARI + select ATARI_KBD_CORE + help + Say Y here if you are running Linux on any Atari and have a keyboard + attached. + + To compile this driver as a module, choose M here: the + module will be called atakbd.
Can we spell it out: atarikbd or atari_kbd?
Review
Quoted 6 lines
(quoted diff) +6
-0+ + if (!(atakbd_dev = input_allocate_device())) + return -ENOMEM; + + // need to init core driver if not already done so + if (atari_keyb_init())
Memory leak
Quoted 7 lines
(quoted diff) +7
-0+ return -ENODEV; + + atakbd_dev->name = "Atari Keyboard"; + atakbd_dev->phys = "atakbd/input0"; + atakbd_dev->id.bustype = BUS_ATARI; + atakbd_dev->id.vendor = 0x0001; + atakbd_dev->id.product = 0x0001;
Review
Quoted 10 lines
(quoted diff) +10
-0+ atakbd_dev->id.version = 0x0100; + + atakbd_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); + atakbd_dev->keycode = atakbd_keycode; + atakbd_dev->keycodesize = sizeof(unsigned char); + atakbd_dev->keycodemax = ARRAY_SIZE(atakbd_keycode); + + for (i = 1; i < 0x72; i++) { + atakbd_keycode[i] = i; + set_bit(atakbd_keycode[i], atakbd_dev->keybit);
It looks like this driver is not using standard input event codes. If Roman does not want to adjust keymaps on Amiga and Atari that should be handled in legacy keyboard driver (drivers/char/keyboard.c). As it is programs using /dev/input/eventX have no chance of working.
Review
Quoted 3 lines
(quoted diff) +3
-0+ } + + input_register_device(atakbd_dev);
Error handling.
Review
Quoted 5 lines
(quoted diff) +5
-0+ +static int mouse_threshold[2] = {2,2}; + +#ifdef __MODULE__ +MODULE_PARM(mouse_threshold, "2i");
MODULE_PARM is so 20th century...
Review
Quoted 5 lines
(quoted diff) +5
-0+#endif +#ifdef FIXED_ATARI_JOYSTICK +extern int atari_mouse_buttons; +#endif +static int atamouse_used = 0;
Initialization is not needed. Moreover the counter is not needed.
Review
Quoted 5 lines
(quoted diff) +5
-0+ +static int atamouse_open(struct input_dev *dev) +{ + if (atamouse_used++) + return 0;
No need to count, input core takes care of this.
Review
Quoted 10 lines
(quoted diff) +10
-0+ +static int __init atamouse_init(void) +{ + if (!MACH_IS_ATARI || !ATARIHW_PRESENT(ST_MFP)) + return -ENODEV; + + if (!(atamouse_dev = input_allocate_device())) + return -ENOMEM; + + if (!(atari_keyb_init()))
Memory leak
Quoted 6 lines
(quoted diff) +6
-0+ return -ENODEV; + + atamouse_dev->name = "Atari mouse"; + atamouse_dev->phys = "atamouse/input0"; + atamouse_dev->id.bustype = BUS_ATARI; + atamouse_dev->id.vendor = 0x0001;
Review
Quoted 10 lines
(quoted diff) +10
-0+ atamouse_dev->id.product = 0x0002; + atamouse_dev->id.version = 0x0100; + + atamouse_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REL); + atamouse_dev->relbit[0] = BIT(REL_X) | BIT(REL_Y); + atamouse_dev->keybit[LONG(BTN_LEFT)] = BIT(BTN_LEFT) | BIT(BTN_MIDDLE) | BIT(BTN_RIGHT); + atamouse_dev->open = atamouse_open; + atamouse_dev->close = atamouse_close; + + input_register_device(atamouse_dev);
Error handling.
Review
Quoted 2 lines
(quoted diff) +2
-0+ + printk(KERN_INFO "input: %s at keyboard ACIA\n", atamouse_dev->name);
Input core already logs every input device registered, do we need to repeat it in the driver?
Signature · 1 line
Dmitry
Re: [patch 04/33] m68k: Atari keyboard and mouse support.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+static unsigned char rep_scancode; +static struct timer_list atakeyb_rep_timer = { + .function = atakeyb_rep, +};Is there a problem with repeat implementation in input core that requres custom-made repeater here?
Apparently not - the homebrew repeater has not been active pretty much since I started to work on Atari in 2.6... time to kill that code.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + // handle_scancode(scancode, !break_flag);The code looks a bit dirty (old code commented out, C++ style comments).
You got that right.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ + +int atari_kbdrate(struct kbd_repeat *k) +{ + if (k->delay > 0) { + /* convert from msec to jiffies */ + key_repeat_delay = (k->delay * HZ + 500) / 1000;If this is really needed -
msecs_to_jiffies().
That has to go as well (together with the repeater).
Quoted 14 lines
Review
Quoted 12 lines
(quoted diff) +10
-0+config KEYBOARD_ATARI + tristate "Atari keyboard" + depends on ATARI + select ATARI_KBD_CORE + help + Say Y here if you are running Linux on any Atari and have a keyboard + attached. + + To compile this driver as a module, choose M here: the + module will be called atakbd.Can we spell it out: atarikbd or
atari_kbd?
Could do that, yes. Christoph suggested to fold everythig into atakeyb.c so modularization would go away here.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ + if (!(atakbd_dev = input_allocate_device())) + return -ENOMEM; + + // need to init core driver if not already done so + if (atari_keyb_init())Memory leak
How so? If the core has been initialized already this will just return ...
Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ for (i = 1; i < 0x72; i++) { + atakbd_keycode[i] = i; + set_bit(atakbd_keycode[i], atakbd_dev->keybit);It looks like this driver is not using standard input event codes. If Roman does not want to adjust keymaps on Amiga and Atari that should be handled in legacy keyboard driver (
drivers/char/keyboard.c). As it is programs using/dev/input/eventXhave no chance of working.
The translation map should not have been overwritten like above, is that what you mean?
My original patch didn't have that bit; scancodes were translated to input keycodes using atakbd_keycode[scancode] instead. I'll have that reverted...
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ } + + input_register_device(atakbd_dev);Error handling.
Oops.
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+ +static int mouse_threshold[2] = {2,2}; + +#ifdef __MODULE__ +MODULE_PARM(mouse_threshold, "2i");
MODULE_PARMis so 20th century...
Never tested, sorry.
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ +static int atamouse_open(struct input_dev *dev) +{ + if (atamouse_used++) + return 0;No need to count, input core takes care of this.
OK; I'll delete this.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + printk(KERN_INFO "input: %s at keyboard ACIA\n", atamouse_dev->name);Input core already logs every input device registered, do we need to repeat it in the driver?
Not really ... I'll merge atakeyb.c, atakbd.c and atamouse.c and send a new patch to Geert.
Thanks,
Michael
Re: [patch 04/33] m68k: Atari keyboard and mouse support.
Hi,
Quoted 5 lines · Michael Schmitz
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ for (i = 1; i < 0x72; i++) { + atakbd_keycode[i] = i; + set_bit(atakbd_keycode[i], atakbd_dev->keybit);It looks like this driver is not using standard input event codes.
Actually it does, it just sort of works because Atari generates mostly AT keycodes.
Quoted 4 lines
Quoted 4 lines
If
Roman does not want to adjust keymaps on Amiga and Atari that should be handled in legacy keyboard driver (drivers/char/keyboard.c). As it is programs using/dev/input/eventXhave no chance of working.
I explained already at a earlier occasion, why this "generic" keycode thing is broken by design, which makes connecting multiple keyboards with different mappings impossible.
bye, Roman
Dmitry Torokhov
maintainerRe: [patch 04/33] m68k: Atari keyboard and mouse support.
Hi Roman,
Quoted 4 lines · Roman Zippel
Hi,
On Thu, 3 May 2007, Michael Schmitz wrote:
Quoted 9 lines
Quoted 5 lines
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ for (i = 1; i < 0x72; i++) { + atakbd_keycode[i] = i; + set_bit(atakbd_keycode[i], atakbd_dev->keybit);It looks like this driver is not using standard input event codes.
Actually it does, it just sort of works because Atari generates mostly AT keycodes.
But not the input core codes, does it?
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
If
Roman does not want to adjust keymaps on Amiga and Atari that should be handled in legacy keyboard driver (drivers/char/keyboard.c). As it is programs using/dev/input/eventXhave no chance of working.I explained already at a earlier occasion, why this "generic" keycode thing is broken by design, which makes connecting multiple keyboards with different mappings impossible.
No, I don't think so. Your points were:
1) You did not want to adjust your legacy keymap on Amiga
2) You want userspace programs to know how to program scancodes for every type of keyboard and have different keymaps for different type of keyboards (So you need to have n_kbd_types * n_international_mappings keymaps).
Answering 1) I sent you once a patch for review that would have amikbd send raw scancode events in additional to standard input events and drivers/char/keyboard.c pick these raw events on amiga so both legacy driver and evdev could work, but I never heard from you.
As far as 2) goes I think it is better to have unified keyboard map across different types of keyboards and then overlay internatinalization/other settings. And you still have per-keyboard configurability as you can change scancode->keycode mapping on a per-device basis via evdev ioctl.
Signature · 1 line
Dmitry
Re: [patch 04/33] m68k: Atari keyboard and mouse support.
Hi,
Quoted 8 lines · Dmitry Torokhov
Quoted 4 lines
I explained already at a earlier occasion, why this "generic" keycode thing is broken by design, which makes connecting multiple keyboards with different mappings impossible.
No, I don't think so. Your points were:
1) You did not want to adjust your legacy keymap on Amiga
Adjusting wouldn't be really a problem, if it had some value...
Quoted 4 lines
2) You want userspace programs to know how to program scancodes for every type of keyboard and have different keymaps for different type of keyboards (So you need to have
n_kbd_types*n_international_mappingskeymaps).
I never said that. Many keyboard _types_ need a separate key mapping. Localization is a completely different problem (and could be solved via separate localization tables).
Most of it can be solved in userspace and we wouldn't have to enumerate every possible single key the kernel never cares about in <linux/input.h>.
Quoted 5 lines
As far as 2) goes I think it is better to have unified keyboard map across different types of keyboards and then overlay internatinalization/other settings. And you still have per-keyboard configurability as you can change
scancode->keycodemapping on a per-device basis via evdev ioctl.
You still completely ignore the problem of how said application should properly support multiple keyboard mappings...
bye, Roman
Dmitry Torokhov
maintainerRe: [patch 04/33] m68k: Atari keyboard and mouse support.
Quoted 20 lines · Roman Zippel
Hi,
Quoted 8 lines · Dmitry Torokhov
Quoted 4 lines
I explained already at a earlier occasion, why this "generic" keycode thing is broken by design, which makes connecting multiple keyboards with different mappings impossible.
No, I don't think so. Your points were:
1) You did not want to adjust your legacy keymap on Amiga
Adjusting wouldn't be really a problem, if it had some value...
Quoted 4 lines
2) You want userspace programs to know how to program scancodes for every type of keyboard and have different keymaps for different type of keyboards (So you need to have
n_kbd_types*n_international_mappingskeymaps).
Quoted 6 lines
I never said that. Many keyboard _types_ need a separate key mapping. Localization is a completely different problem (and could be solved via separate localization tables).
Most of it can be solved in userspace and we wouldn't have to enumerate every possible single key the kernel never cares about in <linux/input.h>.
I am not sure that solving it all in userspace is right solution. Consider a sleep button. It can be hooked up via ACPI, located on AT keyboard, USB keyboard or even on a remote control or some userspace daemon getting data from the network. If kernel just passes raw data to userspace then userspace programs need to know all these potential sources and handle them separately. But with unified input device interface userspace program only needs to monotor appearance of new event devices, latch onto them and wait for EV_KEY/KEY_SLEEP. And it is not much burden for the kernel because kernel already interfaces with all these devices. In fact there probably savings because kernel uses single interface layer.
Quoted 9 lines
Quoted 5 lines
As far as 2) goes I think it is better to have unified keyboard map across different types of keyboards and then overlay internatinalization/other settings. And you still have per-keyboard configurability as you can change
scancode->keycodemapping on a per-device basis via evdev ioctl.You still completely ignore the problem of how said application should properly support multiple keyboard mappings...
I am afraid this statement is too vague, we need to discuss specific scenarios...
Consider this scenario:
You use 2 PS/2 keyboards with your laptop - built-in and external one on a separate serio port. Your built-in has media keys generating some non-standard scancodes. The external one also has same media keys but generating different set of keycodes. With unified input events you can "fix" your keboards to generate same input events and the rest of your stack does not care.
Signature · 1 line
Dmitry
Re: [patch 04/33] m68k: Atari keyboard and mouse support.
Hi,
Quoted 8 lines · Dmitry Torokhov
Quoted 6 lines
I never said that. Many keyboard _types_ need a separate key mapping. Localization is a completely different problem (and could be solved via separate localization tables).
Most of it can be solved in userspace and we wouldn't have to enumerate every possible single key the kernel never cares about in <linux/input.h>.I am not sure that solving it all in userspace is right solution.
What userspace are you talking about? There is one main user - X, for it and all the rest you could provide a simple input library.
Quoted 10 lines
Consider a sleep button. It can be hooked up via ACPI, located on AT keyboard, USB keyboard or even on a remote control or some userspace daemon getting data from the network. If kernel just passes raw data to userspace then userspace programs need to know all these potential sources and handle them separately. But with unified input device interface userspace program only needs to monotor appearance of new event devices, latch onto them and wait for EV_KEY/KEY_SLEEP. And it is not much burden for the kernel because kernel already interfaces with all these devices. In fact there probably savings because kernel uses single interface layer.
If the kernel would do it properly, you would have a point...
Quoted 7 lines
Quoted 3 lines
You still completely ignore the problem of how said application should properly support multiple keyboard mappings...
I am afraid this statement is too vague, we need to discuss specific scenarios...
Quoted 8 lines
Consider this scenario:
You use 2 PS/2 keyboards with your laptop - built-in and external one on a separate serio port. Your built-in has media keys generating some non-standard scancodes. The external one also has same media keys but generating different set of keycodes. With unified input events you can "fix" your keboards to generate same input events and the rest of your stack does not care.
That's not the problem. Your keycode mapping has no idea about alternative mappings. How does the application know that "Shift"+"2" may mean '"' or '@'?
bye, Roman
Dmitry Torokhov
maintainerRe: [patch 04/33] m68k: Atari keyboard and mouse support.
In reply to Michael SchmitzHi Michael,
Quoted 10 lines · Michael Schmitz
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ + if (!(atakbd_dev = input_allocate_device())) + return -ENOMEM; + + // need to init core driver if not already done so + if (atari_keyb_init())Memory leak
Quoted 2 lines
How so? If the core has been initialized already this will just return ...
You just allocated atakbd_dev. If atari_keyb_init() fails you leak it.
Quoted 9 lines
Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ for (i = 1; i < 0x72; i++) { + atakbd_keycode[i] = i; + set_bit(atakbd_keycode[i], atakbd_dev->keybit);It looks like this driver is not using standard input event codes. If Roman does not want to adjust keymaps on Amiga and Atari that should be handled in legacy keyboard driver (
drivers/char/keyboard.c). As it is programs using/dev/input/eventXhave no chance of working.
Quoted 6 lines
The translation map should not have been overwritten like above, is that what you mean?
My original patch didn't have that bit; scancodes were translated to input keycodes using atakbd_keycode[scancode] instead. I'll have that reverted...
Does KEY_1 actually maps to scancode 2 on atari?
Thanks,
Signature · 1 line
Dmitry
Re: [patch 04/33] m68k: Atari keyboard and mouse support.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ // need to init core driver if not already done so + if (atari_keyb_init())Memory leak
How so? If the core has been initialized already this will just return ...
You just allocated
atakbd_dev. Ifatari_keyb_init()fails you leak it.
I see. Won't probably happen, but there's no reason to take a risk.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
It looks like this driver is not using standard input event codes. If Roman does not want to adjust keymaps on Amiga and Atari that should be handled in legacy keyboard driver (
drivers/char/keyboard.c). As it is programs using/dev/input/eventXhave no chance of working.
Quoted 8 lines
Quoted 6 lines
The translation map should not have been overwritten like above, is that what you mean?
My original patch didn't have that bit; scancodes were translated to input keycodes using atakbd_keycode[scancode] instead. I'll have that reverted...Does
KEY_1actually maps to scancode 2 on atari?
It sure does. For most of the keys, the identity mapping is in fact OK. There's a lot of keys where this breaks, though. Plus the notorious scancode 0 :-)
Michael