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

[Dmitry Eremin-Solenikov]

Hello,

2015-05-12 23:21 GMT+03:00 Dmitry Torokhov <dmitry.torokhov at 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 at 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?

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

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

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

[skipped the rest]

-- 
With best wishes
Dmitry