Hello Dmitry,
Thanx for the review.
On Thu, Aug 26, 2010 at 17:49:08 +0200, Dmitry Torokhov wrote:
> > + int (*init)(void);
> > + int (*exit)(void);
> > + struct matrix_keymap_data *keymap_data;
>
> const?
Okay.
> > + struct input_dev *input;
> > + struct ske_keypad_platform_data *board;
>
> const?
Okay.
> > +DEFINE_MUTEX(ske_keypad_lock);
>
> Why isn't this lock part of ske_keypad structure?
>
Will move it in.
> > +
> > + mutex_lock(&ske_keypad_lock);
>
> Should probably be a spinlock.
>
Since I am using a threaded_irq, I used mutex over spinlock.
> > +static int __init ske_keypad_chip_init(struct ske_keypad *keypad)
>
> This must be __devinit, not __init since it is called from __devinit
> code.
Sure.
> > + /* while away till the SKEx registers are stable and can be read */
> > + while (readl(keypad->reg_base + SKE_CR) & SKE_KPASON)
> > + cpu_relax();
>
> I'd rather this loop had a bound. We do not want to completely wedge IRQ
> thread if something goes wrong.
>
My main concern is: what do I return from here in case of a timeout. For
the HW, this flag is always high as long as any key is pressed and the
auto scan active. For a multi key press, this flag has to be waited upon
before reading the data registers. Please advise on how do I return from
here.
> BTW, it this the only reason why we are using threaded IRQ here? What is
> the expected time for the registers to settle? Might we use a timer to
> postpone the read (I think that threaded IRQs are very convenient and
> quite often the best solution but hard IRQ + timer is probably easier on
> resources unless leads to complicated logic).
Yes. The expected settling time for the register is ~10msc when the key
is pressed to almost nil when the key is released. Please note that
these are just initial figures which I probed for a rough a data. But,
even for the timer, I dont know when to start reading because, a key
press might still betriggering an auto scan.
> > + struct resource *res = NULL;
>
> Why does it need to be initialized?
Okay. will remove it.
> > + struct ske_keypad_platform_data *plat = pdev->dev.platform_data;
>
> const.
Okay.
> > + dev_err(&pdev->dev, "failed to allocate keypad memory\n");
>
> ret = -ENOMEM. Presonally I prefer these to be called err or error and
> explicitely "return 0" in success path.
Will add it up.
> > +
> > + if (!keypad->board->init) {
> > + dev_err(&pdev->dev, "NULL board initialization helper\n");
>
> Could be checked earlier. Also, does it have to be an error?
>
The platform code takes care to request the GPIOs and the GPIO configurations.
Hence I return error in case the board configurations dont get completed.
> > + free_irq(keypad->irq, keypad);
>
>
> You need to free IRQ before unregistering the device or you may end up
> referencing free memory.
Okay.
>
> Where is the call to board->exit()?
>
Yes. I did miss that.
> > +static int __init ske_keypad_init(void)
> > +{
> > + return platform_driver_register(&ske_keypad_driver);
>
> Maybe switch to platform_driver_probe() since it is unlikely the
> device would appear after driver initialized?
>
Okay.
Regards,
Sundar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
