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

[Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  drivers/input/keyboard/Kconfig     |   1 -
>  drivers/input/keyboard/locomokbd.c | 271 +++++++++++++++++++------------------
>  2 files changed, 143 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 106fbac..0a3d875 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -337,7 +337,6 @@ config KEYBOARD_LM8333
>  
>  config KEYBOARD_LOCOMO
>  	tristate "LoCoMo Keyboard Support"
> -	depends on SHARP_LOCOMO
>  	help
>  	  Say Y here if you are running Linux on a Sharp Zaurus Collie or Poodle based PDA
>  
> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> index c94d610..eed0a94 100644
> --- a/drivers/input/keyboard/locomokbd.c
> +++ b/drivers/input/keyboard/locomokbd.c
> @@ -23,37 +23,37 @@
>   *
>   */
>  
> -#include <linux/slab.h>
> -#include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/input.h>
> -#include <linux/delay.h>
> -#include <linux/device.h>
>  #include <linux/interrupt.h>
> -#include <linux/ioport.h>
> -
> -#include <asm/hardware/locomo.h>
> -#include <asm/irq.h>
> -
> -MODULE_AUTHOR("John Lenz <lenz@xxxxxxxxxxx>");
> -MODULE_DESCRIPTION("LoCoMo keyboard driver");
> -MODULE_LICENSE("GPL");
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/locomo.h>
>  
> -#define LOCOMOKBD_NUMKEYS	128
> +/* There is one minor difference between mappings on poodle and collie */
> +#include <asm/mach-types.h>
>  
>  #define KEY_ACTIVITY		KEY_F16
>  #define KEY_CONTACT		KEY_F18
>  #define KEY_CENTER		KEY_F15
>  
> +#define KB_ROWS			16
> +#define KB_COLS			8
> +#define LOCOMOKBD_NUMKEYS	(KB_ROWS * KB_COLS)
> +#define SCANCODE(c, r)		(((c)<<4) + (r) + 1)
> +
>  static const unsigned char
>  locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
>  	0, KEY_ESC, KEY_ACTIVITY, 0, 0, 0, 0, 0, 0, 0,				/* 0 - 9 */
> -	0, 0, 0, 0, 0, 0, 0, KEY_MENU, KEY_HOME, KEY_CONTACT,			/* 10 - 19 */
> +	0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, KEY_CONTACT,				/* 10 - 19 */
>  	0, 0, 0, 0, 0, 0, 0, 0, 0, 0,						/* 20 - 29 */
>  	0, 0, 0, KEY_CENTER, 0, KEY_MAIL, 0, 0, 0, 0,				/* 30 - 39 */
>  	0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RIGHT,					/* 40 - 49 */
>  	KEY_UP, KEY_LEFT, 0, 0, KEY_P, 0, KEY_O, KEY_I, KEY_Y, KEY_T,		/* 50 - 59 */
> -	KEY_E, KEY_W, 0, 0, 0, 0, KEY_DOWN, KEY_ENTER, 0, 0,			/* 60 - 69 */
> +	KEY_E, KEY_W, 0, 0, 0, 0, KEY_DOWN, KEY_KPENTER, 0, 0,			/* 60 - 69 */
>  	KEY_BACKSPACE, 0, KEY_L, KEY_U, KEY_H, KEY_R, KEY_D, KEY_Q, 0, 0,	/* 70 - 79 */
>  	0, 0, 0, 0, 0, 0, KEY_ENTER, KEY_RIGHTSHIFT, KEY_K, KEY_J,		/* 80 - 89 */
>  	KEY_G, KEY_F, KEY_X, KEY_S, 0, 0, 0, 0, 0, 0,				/* 90 - 99 */
> @@ -62,20 +62,14 @@ locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
>  	KEY_M, KEY_SPACE, KEY_V, KEY_APOSTROPHE, KEY_SLASH, 0, 0, 0		/* 120 - 128 */
>  };
>  
> -#define KB_ROWS			16
> -#define KB_COLS			8
> -#define KB_ROWMASK(r)		(1 << (r))
> -#define SCANCODE(c,r)		( ((c)<<4) + (r) + 1 )
> -
>  #define KB_DELAY		8
> -#define SCAN_INTERVAL		(HZ/10)
>  
>  struct locomokbd {
>  	unsigned char keycode[LOCOMOKBD_NUMKEYS];
>  	struct input_dev *input;
> -	char phys[32];
>  
> -	unsigned long base;
> +	struct regmap *regmap;
> +	int irq;
>  	spinlock_t lock;
>  
>  	struct timer_list timer;
> @@ -84,37 +78,33 @@ struct locomokbd {
>  };
>  
>  /* 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.

>  {
> -	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);
>  }
>  
> -static inline void locomokbd_activate_col(unsigned long membase, int col)
> +static inline void locomokbd_activate_col(struct locomokbd *locomokbd, int col)
>  {
>  	unsigned short nset;
>  	unsigned short nbset;
>  
> -	nset = 0xFF & ~(1 << col);
> +	nset = 0xFF & ~BIT(col);
>  	nbset = (nset << 8) + nset;
> -	locomo_writel(nbset, membase + LOCOMO_KSC);
> +	regmap_write(locomokbd->regmap, LOCOMO_KSC, nbset);
>  }
>  
> -static inline void locomokbd_reset_col(unsigned long membase, int col)
> +static inline void locomokbd_reset_col(struct locomokbd *locomokbd, int col)
>  {
>  	unsigned short nbset;
>  
> -	nbset = ((0xFF & ~(1 << col)) << 8) + 0xFF;
> -	locomo_writel(nbset, membase + LOCOMO_KSC);
> +	nbset = ((0xFF & ~BIT(col)) << 8) + 0xFF;
> +	regmap_write(locomokbd->regmap, LOCOMO_KSC, nbset);
>  }
>  
>  /*
> @@ -129,24 +119,25 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
>  	unsigned int row, col, rowd;
>  	unsigned long flags;
>  	unsigned int num_pressed;
> -	unsigned long membase = locomokbd->base;
> +	bool esc_pressed = false;
>  
>  	spin_lock_irqsave(&locomokbd->lock, flags);
>  
> -	locomokbd_charge_all(membase);
> +	locomokbd_charge_all(locomokbd);
>  
>  	num_pressed = 0;
>  	for (col = 0; col < KB_COLS; col++) {
> -
> -		locomokbd_activate_col(membase, col);
> +		udelay(KB_DELAY);
> +		locomokbd_activate_col(locomokbd, col);
>  		udelay(KB_DELAY);
>  
> -		rowd = ~locomo_readl(membase + LOCOMO_KIB);
> +		regmap_read(locomokbd->regmap, LOCOMO_KIB, &rowd);
> +		rowd = ~rowd;
>  		for (row = 0; row < KB_ROWS; row++) {
>  			unsigned int scancode, pressed, key;
>  
>  			scancode = SCANCODE(col, row);
> -			pressed = rowd & KB_ROWMASK(row);
> +			pressed = rowd & BIT(row);
>  			key = locomokbd->keycode[scancode];
>  
>  			input_report_key(locomokbd->input, key, pressed);
> @@ -158,29 +149,30 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
>  			/* The "Cancel/ESC" key is labeled "On/Off" on
>  			 * Collie and Poodle and should suspend the device
>  			 * if it was pressed for more than a second. */
> -			if (unlikely(key == KEY_ESC)) {
> -				if (!time_after(jiffies,
> -					locomokbd->suspend_jiffies + HZ))
> -					continue;
> -				if (locomokbd->count_cancel++
> -					!= (HZ/SCAN_INTERVAL + 1))
> -					continue;
> -				input_event(locomokbd->input, EV_PWR,
> -					KEY_SUSPEND, 1);
> -				locomokbd->suspend_jiffies = jiffies;
> -			} else
> -				locomokbd->count_cancel = 0;
> +			if (unlikely(key == KEY_ESC))
> +				esc_pressed = true;
>  		}
> -		locomokbd_reset_col(membase, col);
> +		locomokbd_reset_col(locomokbd, col);
>  	}
> -	locomokbd_activate_all(membase);
> +	locomokbd_activate_all(locomokbd);
>  
>  	input_sync(locomokbd->input);
>  
>  	/* if any keys are pressed, enable the timer */
>  	if (num_pressed)
> -		mod_timer(&locomokbd->timer, jiffies + SCAN_INTERVAL);
> +		mod_timer(&locomokbd->timer, jiffies + msecs_to_jiffies(100));
>  	else
> +		regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> +
> +
> +	if (esc_pressed && time_after(jiffies,
> +		    locomokbd->suspend_jiffies + msecs_to_jiffies(1000))) {
> +		if (locomokbd->count_cancel++ > (20)) {

Why parentheses around 20?

> +			input_event(locomokbd->input, EV_PWR,
> +					KEY_SUSPEND, 1);
> +			locomokbd->suspend_jiffies = jiffies;
> +		}
> +	} else
>  		locomokbd->count_cancel = 0;

If one branch has curly braces the other should have them too.

>  
>  	spin_unlock_irqrestore(&locomokbd->lock, flags);
> @@ -192,18 +184,18 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
>  static irqreturn_t locomokbd_interrupt(int irq, void *dev_id)
>  {
>  	struct locomokbd *locomokbd = dev_id;
> -	u16 r;
> +	unsigned int r;
>  
> -	r = locomo_readl(locomokbd->base + LOCOMO_KIC);
> +
> +	regmap_read(locomokbd->regmap, LOCOMO_KIC, &r);
>  	if ((r & 0x0001) == 0)
>  		return IRQ_HANDLED;
>  
> -	locomo_writel(r & ~0x0100, locomokbd->base + LOCOMO_KIC); /* Ack */
> +	/* Mask and Ack */
> +	regmap_write(locomokbd->regmap, LOCOMO_KIC, r & ~0x110);
>  
> -	/** wait chattering delay **/
> -	udelay(100);
> +	mod_timer(&locomokbd->timer, jiffies + msecs_to_jiffies(1));
>  
> -	locomokbd_scankeyboard(locomokbd);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -220,47 +212,37 @@ static void locomokbd_timer_callback(unsigned long data)
>  static int locomokbd_open(struct input_dev *dev)
>  {
>  	struct locomokbd *locomokbd = input_get_drvdata(dev);
> -	u16 r;
> -	
> -	r = locomo_readl(locomokbd->base + LOCOMO_KIC) | 0x0010;
> -	locomo_writel(r, locomokbd->base + LOCOMO_KIC);
> -	return 0;
> +
> +	return regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
>  }
>  
>  static void locomokbd_close(struct input_dev *dev)
>  {
>  	struct locomokbd *locomokbd = input_get_drvdata(dev);
> -	u16 r;
> -	
> -	r = locomo_readl(locomokbd->base + LOCOMO_KIC) & ~0x0010;
> -	locomo_writel(r, locomokbd->base + LOCOMO_KIC);
> +
> +	regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x0);
>  }
>  
> -static int locomokbd_probe(struct locomo_dev *dev)
> +static int locomokbd_probe(struct platform_device *dev)
>  {
>  	struct locomokbd *locomokbd;
>  	struct input_dev *input_dev;
>  	int i, err;
>  
> -	locomokbd = kzalloc(sizeof(struct locomokbd), GFP_KERNEL);
> -	input_dev = input_allocate_device();
> -	if (!locomokbd || !input_dev) {
> -		err = -ENOMEM;
> -		goto err_free_mem;
> -	}
> +	locomokbd = devm_kzalloc(&dev->dev, sizeof(struct locomokbd),
> +			GFP_KERNEL);
> +	if (!locomokbd)
> +		return -ENOMEM;
>  
> -	/* try and claim memory region */
> -	if (!request_mem_region((unsigned long) dev->mapbase,
> -				dev->length,
> -				LOCOMO_DRIVER_NAME(dev))) {
> -		err = -EBUSY;
> -		printk(KERN_ERR "locomokbd: Can't acquire access to io memory for keyboard\n");
> -		goto err_free_mem;
> -	}
> +	locomokbd->regmap = dev_get_regmap(dev->dev.parent, NULL);
> +	if (!locomokbd->regmap)
> +		return -EINVAL;
>  
> -	locomo_set_drvdata(dev, locomokbd);
> +	locomokbd->irq = platform_get_irq(dev, 0);
> +	if (locomokbd->irq < 0)
> +		return -ENXIO;
>  
> -	locomokbd->base = (unsigned long) dev->mapbase;
> +	platform_set_drvdata(dev, locomokbd);
>  
>  	spin_lock_init(&locomokbd->lock);
>  
> @@ -270,11 +252,13 @@ static int locomokbd_probe(struct locomo_dev *dev)
>  
>  	locomokbd->suspend_jiffies = jiffies;
>  
> -	locomokbd->input = input_dev;
> -	strcpy(locomokbd->phys, "locomokbd/input0");
> +	input_dev = input_allocate_device();

devm_input_allocate_device()?

> +	if (!input_dev)
> +		return -ENOMEM;
>  
> +	locomokbd->input = input_dev;
>  	input_dev->name = "LoCoMo keyboard";
> -	input_dev->phys = locomokbd->phys;
> +	input_dev->phys = "locomokbd/input0";
>  	input_dev->id.bustype = BUS_HOST;
>  	input_dev->id.vendor = 0x0001;
>  	input_dev->id.product = 0x0001;
> @@ -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.

> +
>  	for (i = 0; i < LOCOMOKBD_NUMKEYS; i++)
> -		set_bit(locomokbd->keycode[i], input_dev->keybit);
> -	clear_bit(0, input_dev->keybit);
> +		input_set_capability(input_dev, EV_KEY, locomokbd->keycode[i]);
> +	input_set_capability(input_dev, EV_PWR, KEY_SUSPEND);
> +	__set_bit(EV_REP, input_dev->evbit);
> +
> +	regmap_write(locomokbd->regmap, LOCOMO_KCMD, 1);
> +	regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x0);
> +	regmap_write(locomokbd->regmap, LOCOMO_KIC, 0x0);
>  
>  	/* 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()?

>  	if (err) {
> -		printk(KERN_ERR "locomokbd: Can't get irq for keyboard\n");
> -		goto err_release_region;
> +		dev_err(&dev->dev, "locomokbd: Can't get irq for keyboard\n");
> +		goto err_free_mem;
>  	}
>  
>  	err = input_register_device(locomokbd->input);
> @@ -309,54 +307,71 @@ static int locomokbd_probe(struct locomo_dev *dev)
>  
>  	return 0;
>  
> - err_free_irq:
> -	free_irq(dev->irq[0], locomokbd);
> - err_release_region:
> -	release_mem_region((unsigned long) dev->mapbase, dev->length);
> -	locomo_set_drvdata(dev, NULL);
> - err_free_mem:
> +err_free_irq:
> +	free_irq(locomokbd->irq, locomokbd);
> +err_free_mem:
>  	input_free_device(input_dev);
> -	kfree(locomokbd);
>  
>  	return err;
>  }
>  
> -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.

>  
>  	del_timer_sync(&locomokbd->timer);

Should likely to go into close().

>  
>  	input_unregister_device(locomokbd->input);

Is not needed with devm.

> -	locomo_set_drvdata(dev, NULL);
>  
> -	release_mem_region((unsigned long) dev->mapbase, dev->length);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int locomokbd_suspend(struct device *dev)

Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.

> +{
> +	struct locomokbd *locomokbd = dev_get_drvdata(dev);
> +
> +	regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x0);
>  
> -	kfree(locomokbd);
> +	del_timer_sync(&locomokbd->timer);
>  
>  	return 0;
>  }
>  
> -static struct locomo_driver keyboard_driver = {
> -	.drv = {
> -		.name = "locomokbd"
> +static int locomokbd_resume(struct device *dev)

__maybe_unused as well.

> +{
> +	struct locomokbd *locomokbd = dev_get_drvdata(dev);
> +
> +	regmap_write(locomokbd->regmap, LOCOMO_KCMD, 1);
> +	regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> +	regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> +	regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> +
> +	locomokbd_scankeyboard(locomokbd);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(locomo_kbd_pm, locomokbd_suspend, locomokbd_resume);
> +#define LOCOMO_KBD_PM	(&locomo_kbd_pm)
> +#else
> +#define LOCOMO_KBD_PM	NULL
> +#endif

Just do

static SIMPLE_DEV_PM_OPS(locomo_kbd_pm,
			 locomokbd_suspend, locomokbd_resume);


outside of #ifdef, it will produce the right thing (an empty structure).

> +
> +static struct platform_driver locomokbd_driver = {
> +	.driver = {
> +		.name	= "locomo-kbd",
> +		.pm	= LOCOMO_KBD_PM,

		.pm	= &locomo_kbd_pm;
>  	},
> -	.devid	= LOCOMO_DEVID_KEYBOARD,
>  	.probe	= locomokbd_probe,
>  	.remove	= locomokbd_remove,
>  };
>  
> -static int __init locomokbd_init(void)
> -{
> -	return locomo_driver_register(&keyboard_driver);
> -}
> -
> -static void __exit locomokbd_exit(void)
> -{
> -	locomo_driver_unregister(&keyboard_driver);
> -}
> +module_platform_driver(locomokbd_driver);
>  
> -module_init(locomokbd_init);
> -module_exit(locomokbd_exit);
> +MODULE_AUTHOR("John Lenz <lenz@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("LoCoMo keyboard driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:locomo-kbd");
> -- 
> 2.1.4
> 

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html