Re: [PATCH] input: add support for Nomadik SKE keypad controller

[Sundar R IYER <sundar.iyer@stericsson.com>]

Hello Shubhra,

On Tue, Aug 24, 2010 at 08:37:16 +0200, Datta, Shubhrajyoti wrote:
> > + * ske_keypad_chip_init : init keypad controller configuration
> > + *
> > + * Enable Multi key press detection, auto scan mode
> > + */
> May consider making it an init function as called only at probe?

Okay. will do so.

> > +
> Nitpick : Bonus space

Sure.

> > +out_unregisterinput:
> > +     input_unregister_device(input);
> > +     input = NULL;
> > +     clk_disable(keypad->clk);
> > +out_freeinput:
> Unregister and then free you may want to have a relook.
> > +     input_free_device(input);

Hmm. Is there something wrong here? input_free_device() is being
called with a NULL after input_unregister_device(), which is okay.

> > +out_freekeypad:
> > +     kfree(keypad);
> > +out_freeclk:
> Kfree keypad and then access keypad->clk ?
> 
> > +     clk_put(keypad->clk);

Oops. It ought to be only clk.

> > +     clk_disable(keypad->clk);
> > +     clk_put(keypad->clk);
> What is the cpu you tested on? Just curious not a comment.

I have tested it on the U8500 platform.

Thanx for the review; I will re-post with the changes.

Regards,
Sundar