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 [off-list ref]:

quoted

Hi Dmitry,

On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:

quoted

As LoCoMo is switching to new device model, adapt keyboard driver to
support new locomo core driver.

Signed-off-by: Dmitry Eremin-Solenikov <redacted>
---

Thanks for the review.

quoted

quoted

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

quoted

quoted

 {
-     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]

quoted

quoted

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

quoted

quoted

      /* 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]

quoted

quoted

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

quoted

quoted

      del_timer_sync(&locomokbd->timer);

Should likely to go into close().

Hmm. I will rethink this part, thank you.

quoted

quoted

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