Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core

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

On Wed, May 13, 2015 at 12:01:23AM +0300, Dmitry Eremin-Solenikov wrote:
> Hello,
> 
> 2015-05-12 23:21 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > Hi Dmitry,
> >
> > On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
> >> As LoCoMo is switching to new device model, adapt keyboard driver to
> >> support new locomo core driver.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >> ---
> 
> Thanks for the review.
> 
> >>  /* helper functions for reading the keyboard matrix */
> >> -static inline void locomokbd_charge_all(unsigned long membase)
> >> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
> >>  {
> >> -     locomo_writel(0x00FF, membase + LOCOMO_KSC);
> >> +     regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
> >>  }
> >>
> >> -static inline void locomokbd_activate_all(unsigned long membase)
> >> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
> >
> > Drop "inline"s from the .c file please.
> 
> Why?

Because compiler usually knows better whether a function should be
inlined or not as it keeps track of available registers, so leave the
decision to it.

> 
> >
> >>  {
> >> -     unsigned long r;
> >> -
> >> -     locomo_writel(0, membase + LOCOMO_KSC);
> >> -     r = locomo_readl(membase + LOCOMO_KIC);
> >> -     r &= 0xFEFF;
> >> -     locomo_writel(r, membase + LOCOMO_KIC);
> >> +     regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> >> +     regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> >>  }
> >>
> 
> [skipped]
> 
> >> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
> >>
> >>       input_set_drvdata(input_dev, locomokbd);
> >>
> >> -     memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
> >> +     memcpy(locomokbd->keycode,
> >> +                     locomokbd_keycode,
> >> +                     sizeof(locomokbd->keycode));
> >> +
> >> +     if (machine_is_collie())
> >> +             locomokbd->keycode[18] = KEY_HOME;
> >> +     else
> >> +             locomokbd->keycode[3] = KEY_HOME;
> >
> > This seems like a new addition. Ideally keymap twiddling shoudl be done
> > from userspace.
> 
> This fixes a hardware issue. Home key is wired differently on two
> devices using this driver.
> I'd prefer to have such setting in board file or in DTS in future,
> however that looks like an
> overkill. What would be your suggestion?

I am OK with doing this in driver, just as a separate patch please.

> 
> >>       /* attempt to get the interrupt */
> >> -     err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
> >> +     err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
> >> +                     "locomokbd", locomokbd);
> >
> > devm_request_irq()?
> >
> [skipped]
> 
> >> -static int locomokbd_remove(struct locomo_dev *dev)
> >> +static int locomokbd_remove(struct platform_device *dev)
> >>  {
> >> -     struct locomokbd *locomokbd = locomo_get_drvdata(dev);
> >> +     struct locomokbd *locomokbd = platform_get_drvdata(dev);
> >>
> >> -     free_irq(dev->irq[0], locomokbd);
> >> +     free_irq(locomokbd->irq, locomokbd);
> >
> > Is not needed with devm.
> 
> Not quite. There will be a possibility for the IRQ to happen after deleting
> a timer in locomokbd_remove() and before freeing the IRQ through the devres
> core. Oops.

Right, but if you make sure that device does not generate interrupts in
probe() until open() is called and do the same in close(), then it
should be OK.

> 
> >
> >>
> >>       del_timer_sync(&locomokbd->timer);
> >
> > Should likely to go into close().
> 
> Hmm. I will rethink this part, thank you.
> 
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int locomokbd_suspend(struct device *dev)
> >
> > Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
> 
> Fine, however I thought that #ifdef's here are a typical pattern.

It is up to subsystems, __maybe_unused provides better compile coverage.

Thanks.

-- 
Dmitry