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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Thu, Aug 26, 2010 at 09:39:01PM +0530, Sundar R IYER wrote:
> 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.

Why would this matter? You chose spinlock/vs mutex mostly depending on
what code you need to protect (does it need to sleep or not), not the
caller's context.

> 
> > > +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.

I guess if you spun for ...hmm... 50 msecs you may just stop, re-enable
IRQ and return IRQ_HANDLED and hope for the better luck next time. It
does not do any good to keep spinning in the IRQ thread.

> 
> > 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.

10 msec latency for keypad is quite acceptable, so I'd get interrupt,
scheule a timer to be fired in 10 msec and have that timer check if
keypad has settled. If it hasn't I'd rearm the timer for another 10
msec.

> 
> > > +     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.
> 

PLanform author migth opt to do the initialization earlier, together
with teh rest of GPIO configuration so it is all ready by the time the
driver is loaded. I think we should give such an option.

> > > +     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.
> 

Thanks.

-- 
Dmitry