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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
