Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762225Ab2EQRiZ (ORCPT ); Thu, 17 May 2012 13:38:25 -0400 Received: from mail-pb0-f46.google.com ([209.85.160.46]:47772 "EHLO mail-pb0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762209Ab2EQRiW (ORCPT ); Thu, 17 May 2012 13:38:22 -0400 Date: Thu, 17 May 2012 10:38:14 -0700 From: Dmitry Torokhov To: Roland Stigge Cc: axel.lin@gmail.com, riyer@nvidia.com, michael.hennerich@analog.com, grant.likely@secretlab.ca, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kevin.wells@nxp.com, srinivas.bakki@nxp.com, devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com Subject: Re: [PATCH RESEND v3 1/2] input: keyboard: Add keys driver for the LPC32xx SoC Message-ID: <20120517173814.GA26948@core.coreip.homeip.net> References: <1337199955-26441-1-git-send-email-stigge@antcom.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1337199955-26441-1-git-send-email-stigge@antcom.de> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 16484 Lines: 547 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 > > --- > 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@40050000 { > + 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 > + * Roland Stigge > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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 "); > +MODULE_AUTHOR("Roland Stigge "); > +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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/