Tuesday 30 March 2010 08:56:19 Dmitry Torokhov wrote:

On Mon, Mar 29, 2010 at 04:30:41PM +0200, Janusz Krzysztofik wrote:

quoted

The patch introduces a serio driver that supports a keyboard serial port
found on the Amstrad Delta videophone board.

[snip]

quoted

+#define MAX_SCANCODE 0x84

Not needed anymore?

[snip]

quoted

+		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]

quoted

+	serio = kmalloc(sizeof(struct serio), GFP_KERNEL);

kzalloc() please.

[snip]

quoted

+		snprintf(serio->phys, sizeof(serio->phys), "%s/serio0", "GPIO");

strlcpy(serio->phys, "GPIO/serio0", sizeof(serio->phys)); ?

[snip]

quoted

+	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]

quoted

+	/* 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]

quoted

+	/* 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