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