Re: [PATCH v2 1/5] input: add driver for tnetv107x on-chip keypad controller

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

Hi Cyril,

Thank you for making changes. Some more comments below.

On Thu, Sep 16, 2010 at 03:54:40PM -0400, Cyril Chemparathy wrote:
> This patch adds support for tnetv107x's on-chip keypad controller.
> 
> Signed-off-by: Cyril Chemparathy <cyril@ti.com>
> ---
>  drivers/input/keyboard/Kconfig            |    9 +
>  drivers/input/keyboard/Makefile           |    1 +
>  drivers/input/keyboard/tnetv107x-keypad.c |  329 +++++++++++++++++++++++++++++
>  3 files changed, 339 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/keyboard/tnetv107x-keypad.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 9cc488d..df1facb 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -424,6 +424,15 @@ config KEYBOARD_OMAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called omap-keypad.
>  
> +config KEYBOARD_TNETV107X
> +	tristate "TI TNETV107X keypad support"
> +	depends on ARCH_DAVINCI_TNETV107X
> +	help
> +	  Say Y here if you want to use the TNETV107X keypad.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called tnetv107x-keypad.
> +
>  config KEYBOARD_TWL4030
>  	tristate "TI TWL4030/TWL5030/TPS659x0 keypad support"
>  	depends on TWL4030_CORE
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 504b591..dc04518 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
>  obj-$(CONFIG_KEYBOARD_STMPE)		+= stmpe-keypad.o
>  obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
> +obj-$(CONFIG_KEYBOARD_TNETV107X)	+= tnetv107x-keypad.o
>  obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
>  obj-$(CONFIG_KEYBOARD_W90P910)		+= w90p910_keypad.o
> diff --git a/drivers/input/keyboard/tnetv107x-keypad.c b/drivers/input/keyboard/tnetv107x-keypad.c
> new file mode 100644
> index 0000000..c262cb4
> --- /dev/null
> +++ b/drivers/input/keyboard/tnetv107x-keypad.c
> @@ -0,0 +1,329 @@
> +/*
> + * Texas Instruments TNETV107X Keypad Driver
> + *
> + * Copyright (C) 2010 Texas Instruments
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/input/matrix_keypad.h>
> +
> +#define BITS(x)			(BIT(x) - 1)
> +
> +#define KEYPAD_ROWS		9
> +#define KEYPAD_COLS		9
> +
> +#define DEBOUNCE_MIN		BIT(10)
> +#define DEBOUNCE_MAX		BITS(30)
> +
> +struct keypad_regs {
> +	u32	rev;
> +	u32	mode;
> +	u32	mask;
> +	u32	pol;
> +	u32	dclock;
> +	u32	rclock;
> +	u32	stable_cnt;
> +	u32	in_en;
> +	u32	out;
> +	u32	out_en;
> +	u32	in;
> +	u32	lock;
> +	u32	pres[3];
> +};
> +
> +#define keypad_read(kp, reg)		__raw_readl(&(kp)->regs->reg)
> +#define keypad_write(kp, reg, val)	__raw_writel(val, &(kp)->regs->reg)
> +
> +struct keypad_data {
> +	struct input_dev		*input_dev;
> +	struct resource			*res;
> +	struct keypad_regs __iomem	*regs;
> +	struct clk			*clk;
> +	struct device			*dev;
> +	u32				irq_press;
> +	u32				irq_release;
> +	int				rows, cols, row_shift;
> +	int				debounce_ms, active_low;
> +	unsigned short			*keycodes;
> +	u32				prev_keys[3];
> +};
> +
> +static irqreturn_t keypad_irq(int irq, void *data)
> +{
> +	struct keypad_data *kp = (struct keypad_data *)data;

No need to cast.

> +	int i, bit, val, row, col, code;
> +	u32 curr_keys[3];
> +	u32 change;
> +
> +	memset(curr_keys, 0, sizeof(curr_keys));
> +	if (irq == kp->irq_press)
> +		for (i = 0; i < 3; i++)
> +			curr_keys[i] = keypad_read(kp, pres[i]);
> +
> +	for (i = 0; i < 3; i++) {
> +		change = curr_keys[i] ^ kp->prev_keys[i];
> +
> +		while (change) {
> +			bit     = fls(change) - 1;
> +			change ^= BIT(bit);
> +			val     = curr_keys[i] & BIT(bit);
> +			bit    += i * 32;
> +			row     = bit / KEYPAD_COLS;
> +			col     = bit % KEYPAD_COLS;
> +
> +			code = MATRIX_SCAN_CODE(row, col, kp->row_shift);
> +			input_event(kp->input_dev, EV_MSC, MSC_SCAN, code);
> +			input_report_key(kp->input_dev, kp->keycodes[code],
> +					 val);
> +		}
> +	}
> +	input_sync(kp->input_dev);
> +	memcpy(kp->prev_keys, curr_keys, sizeof(curr_keys));
> +
> +	if (irq == kp->irq_press)
> +		keypad_write(kp, lock, 0); /* Allow hardware updates */
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int keypad_start(struct input_dev *dev)
> +{
> +	struct keypad_data *kp = input_get_drvdata(dev);
> +	unsigned long mask, debounce, clk_rate_khz;
> +	int error;
> +
> +	clk_enable(kp->clk);
> +	clk_rate_khz = clk_get_rate(kp->clk) / 1000;
> +
> +	/* Initialize device registers */
> +	keypad_write(kp, mode, 0);
> +
> +	mask  = BITS(kp->rows) << KEYPAD_COLS;
> +	mask |= BITS(kp->cols);
> +	keypad_write(kp, mask, ~mask);
> +
> +	keypad_write(kp, pol, kp->active_low ? 0 : 0x3ffff);
> +	debounce = kp->debounce_ms * clk_rate_khz;
> +	debounce = min(debounce, DEBOUNCE_MAX);
> +	debounce = max(debounce, DEBOUNCE_MIN);

	debounce = clamp(debounce, DEBOUNCE_MIN, DEBOUNCE_MAX);

Also I think defining DEBOUNCE_MIN and DEBOUNCE_MAX via BIT() is
confusing (I was expecting to see some bitwise ops); just use a
constant.

> +	keypad_write(kp, dclock, debounce);
> +	keypad_write(kp, rclock, 4 * debounce);
> +	keypad_write(kp, stable_cnt, 3);
> +	keypad_write(kp, in_en, 1);
> +
> +	error = request_irq(kp->irq_press, keypad_irq, 0, "key-press", kp);
> +	if (error < 0) {
> +		dev_err(kp->dev, "Could not allocate keypad press key irq\n");
> +		clk_disable(kp->clk);
> +		return error;
> +	}
> +
> +	error = request_irq(kp->irq_release, keypad_irq, 0, "key-release", kp);
> +	if (error < 0) {
> +		dev_err(kp->dev, "Could not allocate keypad release key irq\n");
> +		free_irq(kp->irq_press, kp);
> +		clk_disable(kp->clk);
> +		return error;
> +	}
> +

Request IRQ should go into keypad_probe(). The split is as follows - if
possible probe() should leave the device fully ready (all needed
resources acquired) but inactive, and open() should just activate the
device.

> +	return 0;
> +}
> +
> +static void keypad_stop(struct input_dev *dev)
> +{
> +	struct keypad_data *kp = input_get_drvdata(dev);
> +
> +	free_irq(kp->irq_press, kp);
> +	free_irq(kp->irq_release, kp);
> +	clk_disable(kp->clk);
> +}
> +
> +static int __devinit keypad_probe(struct platform_device *pdev)
> +{
> +	const struct matrix_keypad_platform_data *pdata;
> +	const struct matrix_keymap_data *keymap_data;
> +	struct device *dev = &pdev->dev;
> +	struct keypad_data *kp;
> +	int error = 0, sz;
> +	u32 rev = 0;
> +
> +	pdata = pdev->dev.platform_data;
> +	if (!pdata) {
> +		dev_err(dev, "cannot find device data\n");
> +		return -EINVAL;
> +	}
> +
> +	keymap_data = pdata->keymap_data;
> +	if (!keymap_data) {
> +		dev_err(dev, "cannot find keymap data\n");
> +		return -EINVAL;
> +	}
> +
> +	kp = kzalloc(sizeof(struct keypad_data), GFP_KERNEL);
> +	if (!kp) {
> +		dev_err(dev, "cannot allocate device info\n");
> +		return -ENOMEM;
> +	}
> +
> +	kp->dev  = dev;
> +	kp->rows = pdata->num_row_gpios;
> +	kp->cols = pdata->num_col_gpios;
> +	kp->row_shift = get_count_order(kp->cols);
> +	platform_set_drvdata(pdev, kp);
> +
> +	sz = (kp->rows << kp->row_shift) * sizeof(*kp->keycodes);
> +	kp->keycodes = kzalloc(sz, GFP_KERNEL);
> +	if (!kp->keycodes) {
> +		dev_err(dev, "cannot allocate keymap info\n");
> +		error = -ENOMEM;
> +		goto error;
> +	}

Why don't you allocate keymap together with the keypad structure:

struct keypad_data {
	...
	u32				prev_keys[3];
	unsigned short			keycodes[];
};

	keymap_size = (pdata->num_row_gpios <<
			get_order(pdata->num_col_gpios)) *
			sizeof(unsigned short);
	kp = kzalloc(sizeof(struct keypad_data) + keymap_size, GFP_KERNEL);
	...

> +
> +	kp->irq_press   = platform_get_irq_byname(pdev, "press");
> +	kp->irq_release = platform_get_irq_byname(pdev, "release");
> +	if (kp->irq_press < 0 || kp->irq_release < 0) {
> +		dev_err(dev, "cannot determine device interrupts\n");
> +		error = -ENODEV;
> +		goto error;
> +	}
> +
> +	kp->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!kp->res) {
> +		dev_err(dev, "cannot determine register area\n");
> +		error = -ENODEV;
> +		goto error;
> +	}
> +
> +	if (!request_mem_region(kp->res->start, resource_size(kp->res),
> +				pdev->name)) {
> +		dev_err(dev, "cannot claim register memory\n");
> +		kp->res = NULL;
> +		error = -EINVAL;
> +		goto error;
> +	}
> +
> +	kp->regs = ioremap(kp->res->start, resource_size(kp->res));
> +	if (!kp->regs) {
> +		dev_err(dev, "cannot map register memory\n");
> +		error = -ENOMEM;
> +		goto error;
> +	}
> +
> +	kp->clk = clk_get(dev, NULL);
> +	if (!kp->clk) {
> +		dev_err(dev, "cannot claim device clock\n");
> +		error = -EINVAL;
> +		goto error;
> +	}
> +
> +	kp->input_dev = input_allocate_device();
> +	if (!kp->input_dev) {
> +		dev_err(dev, "cannot allocate input device\n");
> +		error = -ENOMEM;
> +		goto error;
> +	}
> +	input_set_drvdata(kp->input_dev, kp);
> +
> +	kp->input_dev->name	  = pdev->name;
> +	kp->input_dev->dev.parent = &pdev->dev;
> +	kp->input_dev->open	  = keypad_start;
> +	kp->input_dev->close	  = keypad_stop;
> +	kp->input_dev->evbit[0]	  = BIT_MASK(EV_KEY);
> +	if (!pdata->no_autorepeat)
> +		kp->input_dev->evbit[0] |= BIT_MASK(EV_REP);
> +
> +	clk_enable(kp->clk);
> +	rev = keypad_read(kp, rev);
> +	kp->input_dev->id.bustype = BUS_HOST;
> +	kp->input_dev->id.product = ((rev >>  8) & 0x07);
> +	kp->input_dev->id.version = ((rev >> 16) & 0xfff);
> +	clk_disable(kp->clk);
> +
> +	kp->input_dev->keycode     = kp->keycodes;
> +	kp->input_dev->keycodesize = sizeof(*kp->keycodes);
> +	kp->input_dev->keycodemax  = kp->rows << kp->row_shift;
> +
> +	matrix_keypad_build_keymap(keymap_data, kp->row_shift, kp->keycodes,
> +				   kp->input_dev->keybit);
> +
> +	input_set_capability(kp->input_dev, EV_MSC, MSC_SCAN);
> +
> +	error = input_register_device(kp->input_dev);
> +	if (error < 0) {
> +		dev_err(dev, "Could not register input device\n");
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	if (kp->input_dev)
> +		input_free_device(kp->input_dev);
> +	if (kp->clk)
> +		clk_put(kp->clk);
> +	if (kp->regs)
> +		iounmap(kp->regs);
> +	if (kp->res)
> +		release_mem_region(kp->res->start, resource_size(kp->res));

I generally prefer having multiple error path labels instead of checking
the state.

> +	platform_set_drvdata(pdev, NULL);
> +	kfree(kp->keycodes);
> +	kfree(kp);
> +	return error;
> +}
> +
> +static int __devexit keypad_remove(struct platform_device *pdev)
> +{
> +	struct keypad_data *kp = platform_get_drvdata(pdev);
> +
> +	input_unregister_device(kp->input_dev);
> +	clk_put(kp->clk);
> +	iounmap(kp->regs);
> +	release_mem_region(kp->res->start, resource_size(kp->res));
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(kp->keycodes);
> +	kfree(kp);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver keypad_driver = {
> +	.probe		= keypad_probe,
> +	.remove		= keypad_remove,
> +	.driver.name	= "tnetv107x-keypad",
> +	.driver.owner	= THIS_MODULE,
> +};
> +
> +static int __init keypad_init(void)
> +{
> +	return platform_driver_register(&keypad_driver);
> +}
> +
> +static void __exit keypad_exit(void)
> +{
> +	platform_driver_unregister(&keypad_driver);
> +}
> +
> +module_init(keypad_init);
> +module_exit(keypad_exit);
> +
> +MODULE_AUTHOR("Cyril Chemparathy");
> +MODULE_DESCRIPTION("TNETV107X Keypad Driver");
> +MODULE_ALIAS("platform: tnetv107x-keypad");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.0.4
> 

Thanks.

-- 
Dmitry