Tuesday 30 March 2010 08:56:19 Dmitry Torokhov wrote:
On Mon, Mar 29, 2010 at 04:30:41PM +0200, Janusz Krzysztofik wrote:
The patch introduces a serio driver that supports a keyboard serial port found on the Amstrad Delta videophone board.
[snip]
+#define MAX_SCANCODE 0x84
Not needed anymore?
[snip]
printk(KERN_WARNING"Invalid stop bit in AMS keyboard"" data=0x%X\r\n", data);Consider switching top dev_err(), dev_warning(), etc. Also do not split text strings, even if you run over 80 column limit. BTW, why "\r\n" in the message?
[snip]
- serio = kmalloc(sizeof(struct serio), GFP_KERNEL);
kzalloc() please.
[snip]
snprintf(serio->phys, sizeof(serio->phys), "%s/serio0", "GPIO");strlcpy(serio->phys, "GPIO/serio0", sizeof(serio->phys)); ?
[snip]
- if (gpio_request(AMS_DELTA_GPIO_PIN_KEYBRD_DATA, "kbd-data")) {
printk(KERN_ERR "Couldn't request gpio pin for keyboard data");err = -EINVAL;Why do you mangle return value of gpio_request()? Just return what it reported. Same goes for request_irq() below.
[snip]
- /* enable keyboard */
- ams_delta_latch2_write(AMD_DELTA_LATCH2_KEYBRD_PWR,
AMD_DELTA_LATCH2_KEYBRD_PWR);This shoukd probably go into open() method of the serio port.
[snip]
- /* disable keyboard */
- ams_delta_latch2_write(AMD_DELTA_LATCH2_KEYBRD_PWR, 0);
And this into close().
Hi Dmitry, Thanks for your review time. I agree with all your comments and will address them in next version.
Regards, Janusz