Message-ID: <20120517173814.GA26948@core.coreip.homeip.net>
Date:	Thu, 17 May 2012 10:38:14 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Roland Stigge <stigge@...com.de>
Cc:	axel.lin@...il.com, riyer@...dia.com, michael.hennerich@...log.com,
	grant.likely@...retlab.ca, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	kevin.wells@....com, srinivas.bakki@....com,
	devicetree-discuss@...ts.ozlabs.org, rob.herring@...xeda.com
Subject: Re: [PATCH RESEND v3 1/2] input: keyboard: Add keys driver for the
 LPC32xx SoC

Hi Roland,

On Wed, May 16, 2012 at 10:25:54PM +0200, Roland Stigge wrote:
> This patch adds a driver for the key scan interface of the LPC32xx SoC
> 
> Signed-off-by: Roland Stigge <stigge@...com.de>
> 
> ---
> Applies to v3.4-rc7
> 
> Please pick this patch for the input subsystem.
> 
> Changes since v2:
> * Fixed case of lpc32XX_* -> lpc32xx_*
> * Removed unnecessary casts
> * Removed unnecessary unlikely()
> * Use unsigned for key bits
> * Included Russell's lpc32xx_mod_states() algorithm change suggestion
> * Replaced (res == NULL) with (!res)
> * clk_enable() -> clk_prepare_enable()
> * Moved clk_prepare_enable() and clk_disable_unprepare() to input's
>   open()/close() callbacks to save power
> * Handle clk_prepare_enable() return value appropriately
> * DT: Use "keypad,num-rows", "keypad,num-columns" and "linux,keymap" properties
> 
> Thanks to Axel Lin and Russell King for reviewing!
> 
>  Documentation/devicetree/bindings/input/lpc32xx-key.txt |   28 +
>  drivers/input/keyboard/Kconfig                          |   10 
>  drivers/input/keyboard/Makefile                         |    1 
>  drivers/input/keyboard/lpc32xx-keys.c                   |  382 ++++++++++++++++
>  4 files changed, 421 insertions(+)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/input/lpc32xx-key.txt
> @@ -0,0 +1,28 @@
> +NXP LPC32xx Key Scan Interface
> +
> +Required Properties:
> +- compatible: Should be "nxp,lpc3220-key"
> +- reg: Physical base address of the controller and length of memory mapped
> +  region.
> +- interrupts: The interrupt number to the cpu.
> +- keypad,num-rows: Number of rows and columns, e.g. 1: 1x1, 6: 6x6
> +- keypad,num-columns: Must be equal to keypad,num-rows since LPC32xx only
> +  supports square matrices
> +- nxp,debounce-delay-ms: Debounce delay in ms
> +- nxp,scan-delay-ms: Repeated scan period in ms
> +- linux,keymap: the key-code to be reported when the key is pressed
> +  and released, see also
> +  Documentation/devicetree/bindings/input/matrix-keymap.txt
> +
> +Example:
> +
> +	key@...50000 {
> +		compatible = "nxp,lpc3220-key";
> +		reg = <0x40050000 0x1000>;
> +		interrupts = <54 0>;
> +		keypad,num-rows = <1>;
> +		keypad,num-columns = <1>;
> +		nxp,debounce-delay-ms = <3>;
> +		nxp,scan-delay-ms = <34>;
> +		linux,keymap = <0x00000002>;
> +	};
> --- linux-2.6.orig/drivers/input/keyboard/Kconfig
> +++ linux-2.6/drivers/input/keyboard/Kconfig
> @@ -318,6 +318,16 @@ config KEYBOARD_LOCOMO
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called locomokbd.
>  
> +config KEYBOARD_LPC32XX
> +	tristate "LPC32XX matrix key scanner support"
> +	depends on ARCH_LPC32XX
> +	help
> +	  Say Y here if you want to use NXP LPC32XX SoC key scanner interface,
> +	  connected to a key matrix.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called lpc32xx-keys.
> +
>  config KEYBOARD_MAPLE
>  	tristate "Maple bus keyboard"
>  	depends on SH_DREAMCAST && MAPLE
> --- linux-2.6.orig/drivers/input/keyboard/Makefile
> +++ linux-2.6/drivers/input/keyboard/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_KEYBOARD_HP7XX)		+= jornada
>  obj-$(CONFIG_KEYBOARD_LKKBD)		+= lkkbd.o
>  obj-$(CONFIG_KEYBOARD_LM8323)		+= lm8323.o
>  obj-$(CONFIG_KEYBOARD_LOCOMO)		+= locomokbd.o
> +obj-$(CONFIG_KEYBOARD_LPC32XX)		+= lpc32xx-keys.o
>  obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
>  obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
>  obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
> --- /dev/null
> +++ linux-2.6/drivers/input/keyboard/lpc32xx-keys.c
> @@ -0,0 +1,382 @@
> +/*
> + * NXP LPC32xx SoC Key Scan Interface
> + *
> + * Authors:
> + *    Kevin Wells <kevin.wells@....com>
> + *    Roland Stigge <stigge@...com.de>
> + *
> + * Copyright (C) 2010 NXP Semiconductors
> + * Copyright (C) 2012 Roland Stigge
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/irq.h>
> +#include <linux/pm.h>
> +#include <linux/platform_device.h>
> +#include <linux/input.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +/*
> + * Key scanner register offsets
> + */
> +#define LPC32XX_KS_DEB(x)			((x) + 0x00)
> +#define LPC32XX_KS_STATE_COND(x)		((x) + 0x04)
> +#define LPC32XX_KS_IRQ(x)			((x) + 0x08)
> +#define LPC32XX_KS_SCAN_CTL(x)			((x) + 0x0C)
> +#define LPC32XX_KS_FAST_TST(x)			((x) + 0x10)
> +#define LPC32XX_KS_MATRIX_DIM(x)		((x) + 0x14)
> +#define LPC32XX_KS_DATA(x, y)			((x) + 0x40 + ((y) << 2))
> +
> +#define LPC32XX_KSCAN_DEB_NUM_DEB_PASS(n)	((n) & 0xFF)
> +
> +#define LPC32XX_KSCAN_SCOND_IN_IDLE		0x0
> +#define LPC32XX_KSCAN_SCOND_IN_SCANONCE		0x1
> +#define LPC32XX_KSCAN_SCOND_IN_IRQGEN		0x2
> +#define LPC32XX_KSCAN_SCOND_IN_SCAN_MATRIX	0x3
> +
> +#define LPC32XX_KSCAN_IRQ_PENDING_CLR		0x1
> +
> +#define LPC32XX_KSCAN_SCTRL_SCAN_DELAY(n)	((n) & 0xFF)
> +
> +#define LPC32XX_KSCAN_FTST_FORCESCANONCE	0x1
> +#define LPC32XX_KSCAN_FTST_USE32K_CLK		0x2
> +
> +#define LPC32XX_KSCAN_MSEL_SELECT(n)		((n) & 0xF)
> +
> +/*
> + * Key scanner platform configuration structure
> + */
> +struct lpc32xx_kscan_cfg {
> +	u32	matrix_sz;	/* Size of matrix in XxY, ie. 3 = 3x3 */
> +	int	*keymap;	/* Pointer to key map for the scan matrix */
> +	u32	deb_clks;	/* Debounce clocks (based on 32KHz clock) */
> +	u32	scan_delay;	/* Scan delay (based on 32KHz clock) */
> +};

Hmm, this is not usedul if it is hidden in this driver. You need to
decide if you want to split it into a separate header file and place it
in include/linux/input/... or include/linux/platform_data/... so that it
can be used by the platform code, or make your driver pure DT driver and
have it depend on DT support and require all board carry DT bindings.

> +
> +struct lpc32xx_kscan_drv {
> +	struct input_dev *input;
> +	struct lpc32xx_kscan_cfg *kscancfg;
> +	struct clk *clk;
> +	void __iomem *kscan_base;
> +	int irq;
> +	u8 lastkeystates[8];
> +};
> +
> +static void lpc32xx_mod_states(struct lpc32xx_kscan_drv *kscandat, int off)
> +{
> +	u8 key;
> +	int j;
> +	unsigned changed, scancode, keycode;
> +
> +	key = readl(LPC32XX_KS_DATA(kscandat->kscan_base, off));
> +	changed = key ^ kscandat->lastkeystates[off];
> +	if (changed) {
> +		for (j = 0; j < kscandat->kscancfg->matrix_sz; j++) {
> +			if (changed & (1 << j)) {
> +				/* Key state changed, signal an event */
> +				scancode =
> +				    (j * kscandat->kscancfg->matrix_sz) + off;

Please use:

	scancode = MATRIX_SCAN_CODE(j, off, kscandat->row_shift);

BTW, instead of "j" and "off" maybe switch to "row" and "col" naming?

> +				keycode = kscandat->kscancfg->keymap[scancode];
> +				input_report_key(kscandat->input, keycode,
> +						 key & (1 << j));

You might consider emitting MSC_SCAN as well.

> +			}
> +		}
> +
> +		kscandat->lastkeystates[off] = key;
> +	}
> +}
> +
> +static irqreturn_t lpc32xx_kscan_irq(int irq, void *dev_id)
> +{
> +	int i;
> +	struct lpc32xx_kscan_drv *kscandat = dev_id;
> +
> +	for (i = 0; i < kscandat->kscancfg->matrix_sz; i++)
> +		lpc32xx_mod_states(kscandat, i);
> +
> +	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
> +
> +	input_sync(kscandat->input);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int lpc32xx_kscan_open(struct input_dev *dev)
> +{
> +	struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev);
> +
> +	return clk_prepare_enable(kscandat->clk);
> +}
> +
> +static void lpc32xx_kscan_close(struct input_dev *dev)
> +{
> +	struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev);
> +
> +	clk_disable_unprepare(kscandat->clk);
> +}
> +
> +#ifdef CONFIG_OF
> +static struct lpc32xx_kscan_cfg *lpc32xx_parse_dt(struct device *dev)
> +{
> +	struct lpc32xx_kscan_cfg *pdata;
> +	struct device_node *np = dev->of_node;
> +	int key_count;
> +	int i;
> +	u32 rows, columns;
> +	const __be32 *prop;
> +	int proplen;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "could not allocate memory for platform data\n");
> +		return NULL;
> +	}
> +
> +	of_property_read_u32(np, "keypad,num-rows", &rows);
> +	of_property_read_u32(np, "keypad,num-columns", &columns);
> +	if (!rows || rows != columns) {
> +		dev_err(dev, "rows and columns must be specified and be equal!\n");

Why?

> +		return NULL;
> +	}
> +
> +	pdata->matrix_sz = rows;
> +	key_count = rows * columns;
> +
> +	of_property_read_u32(np, "nxp,debounce-delay-ms", &pdata->deb_clks);
> +	of_property_read_u32(np, "nxp,scan-delay-ms", &pdata->scan_delay);
> +	if (!pdata->deb_clks || !pdata->scan_delay) {
> +		dev_err(dev, "debounce or scan delay not specified\n");
> +		return NULL;
> +	}
> +
> +	pdata->keymap = devm_kzalloc(dev, sizeof(int) * key_count, GFP_KERNEL);
> +	if (!pdata->keymap) {
> +		dev_err(dev, "could not allocate memory for keymap\n");
> +		return NULL;
> +	}
> +
> +	prop = of_get_property(np, "linux,keymap", &proplen);
> +	if (!prop) {
> +		dev_err(dev, "linux,keymap not specified\n");
> +		return NULL;
> +	}
> +
> +	if (proplen % sizeof(u32)) {
> +		dev_err(dev, "bad linux,keymap property size: %d\n", proplen);
> +		return NULL;
> +	}
> +	proplen /= sizeof(u32);
> +
> +	for (i = 0; i < proplen; i++) {
> +		u32 tmp = be32_to_cpup(prop + i);
> +		u32 row, column, key_code;
> +
> +		row = (tmp >> 24) & 0xff;
> +		column = (tmp >> 16) & 0xff;
> +		key_code = tmp & 0xffff;
> +		pdata->keymap[row * columns + column] = key_code;
> +	}

If you use matrix_keypad_build_keymap() most of this code will go away.

> +
> +	return pdata;
> +}
> +#else
> +static struct lpc32xx_kscan_cfg *lpc32xx_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
> +static int __devinit lpc32xx_kscan_probe(struct platform_device *pdev)
> +{
> +	struct lpc32xx_kscan_drv *kscandat;
> +	struct resource *res;
> +	int retval, i, keynum;
> +
> +	kscandat = kzalloc(sizeof(struct lpc32xx_kscan_drv), GFP_KERNEL);
> +	if (!kscandat) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get platform I/O memory\n");
> +		retval = -EBUSY;
> +		goto out1;
> +	}
> +
> +	kscandat->kscan_base = devm_request_and_ioremap(&pdev->dev, res);

I would prefer you did not mix devm_* and nonmanaged resources in the
driver; stick with nonmanaged ones.

> +	if (kscandat->kscan_base == NULL) {
> +		dev_err(&pdev->dev, "failed to request and remap I/O memory\n");
> +		retval = -EBUSY;
> +		goto out1;
> +	}
> +
> +	/* Get the key scanner clock */
> +	kscandat->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(kscandat->clk)) {
> +		dev_err(&pdev->dev, "failed to get clock\n");
> +		retval = -ENODEV;

Do not clobber retval:

		retval = PTR_ERR(kscandat->clk);

BTW, can we call it "error" pleasei since you return "0" explicitly on
success?

> +		goto out1;
> +	}
> +
> +	kscandat->irq = platform_get_irq(pdev, 0);
> +	if ((kscandat->irq < 0) || (kscandat->irq >= NR_IRQS)) {
> +		dev_err(&pdev->dev, "failed to get platform irq\n");
> +		retval = -EINVAL;
> +		goto out2;
> +	}
> +	retval = request_irq(kscandat->irq, lpc32xx_kscan_irq,
> +			     0, pdev->name, kscandat);
> +	if (retval) {
> +		dev_err(&pdev->dev, "failed to request irq\n");
> +		goto out2;
> +	}
> +
> +	kscandat->input = input_allocate_device();
> +	if (kscandat->input == NULL) {
> +		dev_err(&pdev->dev, "failed to allocate device\n");
> +		retval = -ENOMEM;
> +		goto out3;
> +	}
> +
> +	if (pdev->dev.of_node)
> +		kscandat->kscancfg = lpc32xx_parse_dt(&pdev->dev);
> +	else
> +		kscandat->kscancfg = pdev->dev.platform_data;
> +	if (!kscandat->kscancfg) {
> +		dev_err(&pdev->dev, "failed to get platform data\n");
> +		retval = -EINVAL;
> +		goto out4;
> +	}
> +
> +	platform_set_drvdata(pdev, kscandat);
> +
> +	/* Setup key input */
> +	kscandat->input->evbit[0]	= BIT_MASK(EV_KEY);
> +	kscandat->input->name		= pdev->name;
> +	kscandat->input->phys		= "matrix-keys/input0";
> +	kscandat->input->dev.parent	=  &pdev->dev;
> +	kscandat->input->id.vendor	= 0x0001;
> +	kscandat->input->id.product	= 0x0001;
> +	kscandat->input->id.version	= 0x0100;
> +	kscandat->input->open		= &lpc32xx_kscan_open;

No need for '&'.

> +	kscandat->input->close		= &lpc32xx_kscan_close;

> +	keynum = kscandat->kscancfg->matrix_sz * kscandat->kscancfg->matrix_sz;
> +	for (i = 0; i < keynum; i++)
> +		__set_bit(kscandat->kscancfg->keymap[i],
> +			kscandat->input->keybit);

This should use matrix_keypad_build_keymap() with support for OF maps
(see 'next' branch in my tree).

> +
> +	input_set_drvdata(kscandat->input, kscandat);
> +	input_set_capability(kscandat->input, EV_MSC, MSC_SCAN);
> +
> +	retval = input_register_device(kscandat->input);
> +	if (retval) {
> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		goto out4;
> +	}
> +
> +	/* Configure the key scanner */
> +	writel(kscandat->kscancfg->deb_clks,
> +		LPC32XX_KS_DEB(kscandat->kscan_base));
> +	writel(kscandat->kscancfg->scan_delay,
> +		LPC32XX_KS_SCAN_CTL(kscandat->kscan_base));
> +	writel(LPC32XX_KSCAN_FTST_USE32K_CLK,
> +		LPC32XX_KS_FAST_TST(kscandat->kscan_base));
> +	writel(kscandat->kscancfg->matrix_sz,
> +		LPC32XX_KS_MATRIX_DIM(kscandat->kscan_base));
> +	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
> +
> +	return 0;
> +
> +out4:
> +	input_free_device(kscandat->input);
> +out3:
> +	free_irq(kscandat->irq, pdev);
> +out2:
> +	clk_put(kscandat->clk);
> +out1:
> +	kfree(kscandat);
> +
> +	return retval;
> +}
> +
> +static int __devexit lpc32xx_kscan_remove(struct platform_device *pdev)
> +{
> +	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
> +
> +	free_irq(kscandat->irq, pdev);
> +	input_unregister_device(kscandat->input);
> +	clk_put(kscandat->clk);
> +	kfree(kscandat);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM

Please use CONFIG_PM_SLEEP since you do not use runtime PM.

> +static int lpc32xx_kscan_suspend(struct platform_device *pdev,
> +	pm_message_t state)

Please convert to dev_pm_ops structure instead of legacy PM methods.

> +{
> +	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
> +
> +	/* Clear IRQ and disable clock */
> +	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
> +	clk_disable_unprepare(kscandat->clk);
> +
> +	return 0;
> +}
> +
> +static int lpc32xx_kscan_resume(struct platform_device *pdev)
> +{
> +	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
> +
> +	/* Enable clock and clear IRQ */
> +	clk_prepare_enable(kscandat->clk);
> +	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
> +
> +	return 0;
> +}
> +#else
> +#define lpc32xx_kscan_suspend	NULL
> +#define lpc32xx_kscan_resume	NULL
> +#endif

Use "static SIMPLE_DEV_PM_OPS()" outside of #ifdef block. You won't need
defining lpc32xx_kscan_suspend/lpc32xx_kscan_resume to NULL.

> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lpc32xx_kscan_match[] = {
> +	{ .compatible = "nxp,lpc3220-key" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, lpc32xx_kscan_match);
> +#endif
> +
> +static struct platform_driver lpc32xx_kscan_driver = {
> +	.probe		= lpc32xx_kscan_probe,
> +	.remove		= __devexit_p(lpc32xx_kscan_remove),
> +	.suspend	= lpc32xx_kscan_suspend,
> +	.resume		= lpc32xx_kscan_resume,

These 2 should go away.

> +	.driver		= {
> +		.name	= "lpc32xx_keys",
> +		.of_match_table = of_match_ptr(lpc32xx_kscan_match),

		.pm	= lpc32xx_kscan_pm_ops,
		.owner	= THIS_MODULE,
> +	}
> +};
> +
> +module_platform_driver(lpc32xx_kscan_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kevin Wells <kevin.wells@....com>");
> +MODULE_AUTHOR("Roland Stigge <stigge@...com.de>");
> +MODULE_DESCRIPTION("Key scanner driver for LPC32XX devices");

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists