On Wed, Jan 11, 2012 at 08:55:54AM +0100, Geert Uytterhoeven wrote:
> Hi Dmitry,
>
> On Sat, Dec 31, 2011 at 02:11, Dmitry Torokhov
> <[email protected]> wrote:
> > Convert the driver to standard spilt model arch-specific code registers
> > platform device to which driver code can bind later.
> >
> > Also request IRQ immediately upon binding to the device instead of doing
> > this when serio port is being opened.
> >
> > Signed-off-by: Dmitry Torokhov <[email protected]>
> > ---
> >
> > Not tested as I don't have the hardware...
>
> Unfortunately I also don't have the hardware.
> But it compiles. A few comments below.
>
> > diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
> > index ad10fec..27d3f76 100644
> > --- a/arch/m68k/q40/config.c
> > +++ b/arch/m68k/q40/config.c
>
> > +static struct platform_device q40_kbd_pdev = {
> > + .name = "q40kbd",
> > + .id = -1,
> > +};
> > +
> > +static __init int q40_add_kbd_device(void)
> > +{
> > + return platform_device_register(&q40_kbd_pdev);
>
> If you would use platform_device_register_simple(), you don't need the
> q40_kbd_pdev above, reducing memory consumption on non-Q40 platforms.
Isn't this file only compiled on q40 platforms?
>
> > +}
> > +arch_initcall(q40_add_kbd_device);
>
> For the Amiga platform drivers, I used device_initcall().
Won't it potentially race with initialization of q40kbd? It looks like
module_initcall is the same as device_initcall() when module is compiled
in. Given that q40kbd uses platform_dveice_probe() losing race might be
fatal.
>
> > diff --git a/drivers/input/serio/q40kbd.c b/drivers/input/serio/q40kbd.c
> > index 5eb84b3..1e61dd4 100644
> > --- a/drivers/input/serio/q40kbd.c
> > +++ b/drivers/input/serio/q40kbd.c
> >
> > +static void q40kbd_stop(struct q40kbd *q40kbd)
> > +{
>
> The q40kbd parameters is unused and can be removed.
>
> > -static int __devinit q40kbd_probe(struct platform_device *dev)
> > +static int __devinit q40kbd_probe(struct platform_device *pdev)
>
> > + serio_register_port(q40kbd->port);
>
> This cannot fail?
> I see it returns void, but serio_queue_event() inside can fail and thus
> won't propagate its error condition and code.
Yes, this is current limitation of serio.
>
> > -static int __devexit q40kbd_remove(struct platform_device *dev)
> > +static int __devexit q40kbd_remove(struct platform_device *pdev)
> > {
> > - serio_unregister_port(q40kbd_port);
> > + struct q40kbd *q40kbd = platform_get_drvdata(pdev);
> > +
> > + free_irq(Q40_IRQ_KEYBOARD, q40kbd);
> > +
> > + serio_unregister_port(q40kbd->port);
>
> Should the unregister be done before freeing the IRQ, i.e. reverse
> order compared to probe?
Unregister will most likely cause memory being freed; you don't want to
chance IRQ firing here.
>
> > + kfree(q40kbd);
> > +
> > + platform_set_drvdata(pdev, NULL);
> >
> > return 0;
> > }
>
> Thanks, it's a nice cleanup!
>
Thanks for lookign this over.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
