[E3-hacking] [RFC][PATCH v2 4/5] input: serio: add support for Amstrad Delta serial keyboard port

[Janusz Krzysztofik]

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