* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-06 12:48 [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller Sundar Iyer
@ 2010-09-06 15:07 ` Datta, Shubhrajyoti
2010-09-06 15:16 ` Sundar R IYER
2010-09-07 6:37 ` Dmitry Torokhov
2010-09-07 14:01 ` Trilok Soni
2 siblings, 1 reply; 14+ messages in thread
From: Datta, Shubhrajyoti @ 2010-09-06 15:07 UTC (permalink / raw)
To: Sundar Iyer; +Cc: linux-input, STEricsson_nomadik_linux
Hi Sundar,
> -----Original Message-----
> From: linux-input-owner@vger.kernel.org [mailto:linux-input-
> owner@vger.kernel.org] On Behalf Of Sundar Iyer
> Sent: Monday, September 06, 2010 6:18 PM
> To: dmitry.torokhov@gmail.com
> Cc: linux-input@vger.kernel.org; STEricsson_nomadik_linux@list.st.com;
> Sundar Iyer; Linus Walleij
> Subject: [PATCH v3 1/1] input: add support for Nomadik SKE keypad
> controller
>
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Signed-off-by: Sundar Iyer <sundar.iyer@stericsson.com>
> ---
>
> Changes:
> v3:
> - fixed Dmitry's comments assorted comments,
> - re-orged the IRQ to include a timer instead of cpu_relax()
>
> arch/arm/plat-nomadik/include/plat/ske.h | 54 ++++
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/nomadik-ske-keypad.c | 449
> +++++++++++++++++++++++++++
> 4 files changed, 514 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/plat-nomadik/include/plat/ske.h
> create mode 100644 drivers/input/keyboard/nomadik-ske-keypad.c
>
> diff --git a/arch/arm/plat-nomadik/include/plat/ske.h b/arch/arm/plat-
> nomadik/include/plat/ske.h
> new file mode 100644
> index 0000000..27aeadb
> --- /dev/null
> +++ b/arch/arm/plat-nomadik/include/plat/ske.h
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + *
> + * License Terms: GNU General Public License v2
> + * Author: Naveen Kumar Gaddipati <naveen.gaddipati@stericsson.com>
> + *
> + * ux500 Scroll key and Keypad Encoder (SKE) header
> + */
> +
> +#ifndef __SKE_H
> +#define __SKE_H
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +/* register definitions for SKE peripheral */
> +#define SKE_CR 0x00
> +#define SKE_VAL0 0x04
> +#define SKE_VAL1 0x08
> +#define SKE_DBCR 0x0C
> +#define SKE_IMSC 0x10
> +#define SKE_RIS 0x14
> +#define SKE_MIS 0x18
> +#define SKE_ICR 0x1C
> +#define SKE_ASR0 0x20
> +#define SKE_ASR1 0x24
> +#define SKE_ASR2 0x28
> +#define SKE_ASR3 0x2C
> +
> +#define SKE_NUM_ASRX_REGISTERS (4)
> +
> +/*
> + * Keypad module
> + */
> +
> +/**
> + * struct keypad_platform_data - structure for platform specific data
> + * @init: pointer to keypad init function
> + * @exit: pointer to keypad deinitialisation function
> + * @keymap_data: matrix scan code table for keycodes
> + * @krow: maximum number of rows
> + * @kcol: maximum number of columns
> + * @debounce_ms: platform specific debounce time
> + * @no_autorepeat: flag for auto repetition
> + */
> +struct ske_keypad_platform_data {
> + int (*init)(void);
> + int (*exit)(void);
> + struct matrix_keymap_data *keymap_data;
> + u8 krow;
> + u8 kcol;
> + u8 debounce_ms;
> + bool no_autorepeat;
> +};
> +#endif /*__SKE_KPD_H*/
> diff --git a/drivers/input/keyboard/Kconfig
> b/drivers/input/keyboard/Kconfig
> index 4f30048..d63f566 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -327,6 +327,16 @@ config KEYBOARD_NEWTON
> To compile this driver as a module, choose M here: the
> module will be called newtonkbd.
>
> +config KEYBOARD_NOMADIK
> + tristate "ST-Ericsson Nomadik SKE keyboard"
> + depends on PLAT_NOMADIK
> + help
> + Say Y here if you want to use a keypad provided on the SKE
> controller
> + used on the Ux500 and Nomadik platforms
> +
> + To compile this driver as a module, choose M here: the
> + module will be called nmk-ske-keypad.
> +
> config KEYBOARD_OPENCORES
> tristate "OpenCores Keyboard Controller"
> help
> diff --git a/drivers/input/keyboard/Makefile
> b/drivers/input/keyboard/Makefile
> index 8ac01fb..c13809c 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX) += matrix_keypad.o
> obj-$(CONFIG_KEYBOARD_MAX7359) += max7359_keypad.o
> obj-$(CONFIG_KEYBOARD_MCS) += mcs_touchkey.o
> obj-$(CONFIG_KEYBOARD_NEWTON) += newtonkbd.o
> +obj-$(CONFIG_KEYBOARD_NOMADIK) += nomadik-ske-keypad.o
> obj-$(CONFIG_KEYBOARD_OMAP) += omap-keypad.o
> obj-$(CONFIG_KEYBOARD_OMAP4) += omap4-keypad.o
> obj-$(CONFIG_KEYBOARD_OPENCORES) += opencores-kbd.o
> diff --git a/drivers/input/keyboard/nomadik-ske-keypad.c
> b/drivers/input/keyboard/nomadik-ske-keypad.c
> new file mode 100644
> index 0000000..1697181
> --- /dev/null
> +++ b/drivers/input/keyboard/nomadik-ske-keypad.c
> @@ -0,0 +1,449 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + *
> + * Author: Naveen Kumar G <naveen.gaddipati@stericsson.com> for ST-
> Ericsson
> + * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
> + *
> + * License terms:GNU General Public License (GPL) version 2
> + *
> + * Keypad controller driver for the SKE (Scroll Key Encoder) module used
> in
> + * the Nomadik 8815 and Ux500 platforms.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +
> +#include <plat/ske.h>
> +
> +/* SKE_CR bits */
> +#define SKE_KPMLT (0x1 << 6)
> +#define SKE_KPCN (0x7 << 3)
> +#define SKE_KPASEN (0x1 << 2)
> +#define SKE_KPASON (0x1 << 7)
> +
> +/* SKE_IMSC bits */
> +#define SKE_KPIMA (0x1 << 2)
> +
> +/* SKE_ICR bits */
> +#define SKE_KPICS (0x1 << 3)
> +#define SKE_KPICA (0x1 << 2)
> +
> +/* SKE_RIS bits */
> +#define SKE_KPRISA (0x1 << 2)
> +
> +#define SKE_KEYPAD_ROW_SHIFT 3
> +#define SKE_KPD_KEYMAP_SIZE (8 * 8)
> +
> +/**
> + * struct ske_keypad - data structure used by keypad driver
> + * @irq: irq no
> + * @reg_base: ske regsiters base address
> + * @input: pointer to input device object
> + * @board: keypad platform device
> + * @keymap: matrix scan code table for keycodes
> + * @clk: clock structure pointer
> + */
> +struct ske_keypad {
> + int irq;
> + void __iomem *reg_base;
> + struct input_dev *input;
> + const struct ske_keypad_platform_data *board;
> + unsigned short keymap[SKE_KPD_KEYMAP_SIZE];
> + struct clk *clk;
> + spinlock_t ske_keypad_lock;
> + struct timer_list timer;
> +};
> +
> +static void ske_keypad_set_bits(struct ske_keypad *keypad, u16 addr,
> + u8 mask, u8 data)
> +{
> + u32 ret;
> +
> + spin_lock(&keypad->ske_keypad_lock);
> +
> + ret = readl(keypad->reg_base + addr);
> + ret &= ~mask;
> + ret |= data;
> + writel(ret, keypad->reg_base + addr);
> +
> + spin_unlock(&keypad->ske_keypad_lock);
> +}
> +
> +/*
> + * ske_keypad_chip_init : init keypad controller configuration
> + *
> + * Enable Multi key press detection, auto scan mode
> + */
> +static int __devinit ske_keypad_chip_init(struct ske_keypad *keypad)
> +{
> + u32 value;
> + int timeout = keypad->board->debounce_ms;
> +
> + /* check SKE_RIS to be 0 */
> + while ((readl(keypad->reg_base + SKE_RIS) != 0x00000000) && timeout-
> -)
> + cpu_relax();
> + if (!timeout)
> + return -EINVAL;
> +
> + /*
> + * set debounce value
> + * keypad dbounce is configured in DBCR[15:8]
> + * dbounce value in steps of 32/32.768 ms
> + */
> + spin_lock(&keypad->ske_keypad_lock);
> + value = readl(keypad->reg_base + SKE_DBCR);
> + value = value & 0xff;
> + value |= ((keypad->board->debounce_ms * 32000)/32768) << 8;
> + writel(value, keypad->reg_base + SKE_DBCR);
> + spin_unlock(&keypad->ske_keypad_lock);
> +
> + /* enable multi key detection */
> + ske_keypad_set_bits(keypad, SKE_CR, 0x0, SKE_KPMLT);
> +
> + /*
> + * set up the number of columns
> + * KPCN[5:3] defines no. of keypad columns to be auto scanned
> + */
> + value = (keypad->board->kcol - 1) << 3;
> + ske_keypad_set_bits(keypad, SKE_CR, SKE_KPCN, value);
> +
> + /* clear keypad interrupt for auto(and pending SW) scans */
> + ske_keypad_set_bits(keypad, SKE_ICR, 0x0, SKE_KPICA | SKE_KPICS);
> +
> + /* un-mask keypad interrupts */
> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
> +
> + /* enable automatic scan */
> + ske_keypad_set_bits(keypad, SKE_CR, 0x0, SKE_KPASEN);
> +
> + return 0;
> +}
> +
> +static void ske_keypad_read_data(struct ske_keypad *keypad)
> +{
> + struct input_dev *input = keypad->input;
> + u16 status;
> + int col = 0, row = 0, code;
> + int ske_asr, ske_ris, key_pressed, i;
> +
> + /*
> + * Read the auto scan registers
> + *
> + * Each SKE_ASRx (x=0 to x=3) contains two row values.
> + * lower byte contains row value for coloumn 2*x,
> + * upper byte contains row value for column 2*x + 1
> + */
> + for (i = 0; i < SKE_NUM_ASRX_REGISTERS; i++) {
> + ske_asr = readl(keypad->reg_base + SKE_ASR0 + (4 * i));
> + if (!ske_asr)
> + continue;
> +
> + /* now that ASRx is zero, find out the coloumn x and row y*/
> + if (ske_asr & 0xff) {
> + col = i * 2;
> + status = ske_asr & 0xff;
> + } else {
> + col = (i * 2) + 1;
> + status = (ske_asr & 0xff00) >> 8;
> + }
> +
> + /* find out the row */
> + row = __ffs(status);
> +
> + code = MATRIX_SCAN_CODE(row, col, SKE_KEYPAD_ROW_SHIFT);
> + ske_ris = readl(keypad->reg_base + SKE_RIS);
> + key_pressed = ske_ris & SKE_KPRISA;
> +
> + input_event(input, EV_MSC, MSC_SCAN, code);
> + input_report_key(input, keypad->keymap[code], key_pressed);
> + input_sync(input);
> + }
> + return;
> +}
> +
> +static void ske_keypad_timer(unsigned long data)
> +{
> + struct platform_device *pdev = (struct platform_device *) data;
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int ske_kpason;
> + int timeout = keypad->board->debounce_ms;
> +
> + ske_kpason = readl(keypad->reg_base + SKE_CR) & SKE_KPASON;
> + if (ske_kpason) {
> + mod_timer(&keypad->timer, jiffies +
> msecs_to_jiffies(timeout));
> + return;
> + }
> +
> + /* read data registers and report event */
> + ske_keypad_read_data(keypad);
> +
> + return;
> +}
> +
> +static irqreturn_t ske_keypad_irq(int irq, void *dev_id)
> +{
> + struct ske_keypad *keypad = dev_id;
> + int timeout = keypad->board->debounce_ms;
> +
> + /* disable auto scan interrupt; mask the interrupt generated */
> + ske_keypad_set_bits(keypad, SKE_IMSC, ~SKE_KPIMA, 0x0);
> + ske_keypad_set_bits(keypad, SKE_ICR, 0x0, SKE_KPICA);
> +
> + /*
> + * if KPASON is not ready, we cannot read data registers
> + * so wait for a debounce period; if still not settled,
> + * we fire a timer and exit the IRQ
> + */
> + while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--)
> + cpu_relax();
> + if (!timeout) {
> + mod_timer(&keypad->timer, jiffies +
> msecs_to_jiffies(timeout));
> + goto out;
> + }
> +
> + /*
> + * KPASON behaved nicely and we can read data registers and report
> event
> + */
> + ske_keypad_read_data(keypad);
> +
> +out:
> + /* enable auto scan interrupts */
> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int __devinit ske_keypad_probe(struct platform_device *pdev)
> +{
> + struct ske_keypad *keypad;
> + struct resource *res;
> + struct input_dev *input;
> + struct clk *clk;
> + void __iomem *reg_base;
> + int ret;
> + int irq;
> + const struct ske_keypad_platform_data *plat = pdev-
> >dev.platform_data;
> +
> + if (!plat) {
> + dev_err(&pdev->dev, "invalid keypad platform data\n");
> + ret = -EINVAL;
> + goto out_ret;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get keypad irq\n");
> + ret = -EINVAL;
> + goto out_ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "missing platform resources\n");
> + ret = -EINVAL;
> + goto out_ret;
> + }
> +
> + res = request_mem_region(res->start, resource_size(res), pdev-
> >name);
> + if (!res) {
> + dev_err(&pdev->dev, "failed to request I/O memory\n");
> + ret = -EBUSY;
> + goto out_ret;
> + }
> +
> + reg_base = ioremap(res->start, resource_size(res));
> + if (!reg_base) {
> + dev_err(&pdev->dev, "failed to remap I/O memory\n");
> + ret = -ENXIO;
> + goto out_freerequest_memregions;
> + }
> +
> + clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "failed to clk_get\n");
> + ret = PTR_ERR(clk);
> + goto out_freeioremap;
> + }
> +
> + /* resources are sane; we begin allocation */
> + keypad = kzalloc(sizeof(struct ske_keypad), GFP_KERNEL);
> + if (!keypad) {
> + dev_err(&pdev->dev, "failed to allocate keypad memory\n");
> + ret = -ENOMEM;
> + goto out_freeclk;
> + }
> +
> + input = input_allocate_device();
> + if (!input) {
> + dev_err(&pdev->dev, "failed to input_allocate_device\n");
> + ret = -ENOMEM;
> + goto out_freekeypad;
> + }
> +
> + input->id.bustype = BUS_HOST;
> + input->name = "ux500-ske-keypad";
> + input->dev.parent = &pdev->dev;
> +
> + input->keycode = keypad->keymap;
> + input->keycodesize = sizeof(keypad->keymap[0]);
> + input->keycodemax = ARRAY_SIZE(keypad->keymap);
> +
> + input_set_capability(input, EV_MSC, MSC_SCAN);
> +
> + __set_bit(EV_KEY, input->evbit);
> + if (!plat->no_autorepeat)
> + __set_bit(EV_REP, input->evbit);
> +
> + matrix_keypad_build_keymap(plat->keymap_data, SKE_KEYPAD_ROW_SHIFT,
> + input->keycode, input->keybit);
> +
> + ret = input_register_device(input);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "unable to register input device: %d\n", ret);
> + goto out_freeinput;
> + }
> +
> + keypad->irq = irq;
> + keypad->board = plat;
> + keypad->input = input;
> + keypad->reg_base = reg_base;
> + keypad->clk = clk;
> +
> + /* allocations are sane, we begin HW initialization */
> + clk_enable(keypad->clk);
> +
> + /* go through board initialiazation helpers */
> + if (keypad->board->init) {
Did not understand this
> + keypad->board->init();
> +
> + ret = ske_keypad_chip_init(keypad);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "unable to init keypad hardware\n");
> + goto out_unregisterinput;
> + }
> +
> + spin_lock_init(&keypad->ske_keypad_lock);
> + setup_timer(&keypad->timer, ske_keypad_timer, (unsigned long)pdev);
> +
> + ret = request_threaded_irq(keypad->irq, NULL, ske_keypad_irq,
> + IRQF_ONESHOT, "ske-keypad", keypad);
> + if (ret) {
> + dev_err(&pdev->dev, "allocate irq %d failed\n", keypad->irq);
> + goto out_unregisterinput;
> + }
> +
> + device_init_wakeup(&pdev->dev, true);
> +
> + platform_set_drvdata(pdev, keypad);
> +
> + return 0;
> +
> +out_unregisterinput:
> + input_unregister_device(input);
> + input = NULL;
> + clk_disable(keypad->clk);
> +out_freeinput:
> + input_free_device(input);
> +out_freekeypad:
> + kfree(keypad);
> +out_freeclk:
> + clk_put(clk);
> +out_freeioremap:
> + iounmap(reg_base);
> +out_freerequest_memregions:
> + release_mem_region(res->start, resource_size(res));
> +out_ret:
> + return ret;
> +}
> +
> +static int __devexit ske_keypad_remove(struct platform_device *pdev)
> +{
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM,
> 0);
> +
> + del_timer_sync(&keypad->timer);
> + free_irq(keypad->irq, keypad);
> + input_unregister_device(keypad->input);
> +
> + clk_disable(keypad->clk);
> + clk_put(keypad->clk);
> +
> + if (keypad->board->exit())
> + keypad->board->exit();
if (keypad->board->exit)
keypad->board->exit();
What does this exit function do?
Are you sure the exit function does not need the keypad clock?
> +
> + iounmap(keypad->reg_base);
> + release_mem_region(res->start, resource_size(res));
> + kfree(keypad);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int ske_keypad_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int irq = platform_get_irq(pdev, 0);
> +
> + if (device_may_wakeup(dev))
> + enable_irq_wake(irq);
> + else
> + ske_keypad_set_bits(keypad, SKE_IMSC, ~SKE_KPIMA, 0x0);
> +
> + return 0;
> +}
> +
> +static int ske_keypad_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int irq = platform_get_irq(pdev, 0);
> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(irq);
> + else
> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ske_keypad_dev_pm_ops = {
> + .suspend = ske_keypad_suspend,
> + .resume = ske_keypad_resume,
> +};
> +#endif
> +
> +struct platform_driver ske_keypad_driver = {
> + .driver = {
> + .name = "nmk-ske-keypad",
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &ske_keypad_dev_pm_ops,
> +#endif
> + },
> + .probe = ske_keypad_probe,
> + .remove = __devexit_p(ske_keypad_remove),
> +};
> +
> +static int __init ske_keypad_init(void)
> +{
> + return platform_driver_probe(&ske_keypad_driver, ske_keypad_probe);
> +}
> +module_init(ske_keypad_init);
> +
> +static void __exit ske_keypad_exit(void)
> +{
> + platform_driver_unregister(&ske_keypad_driver);
> +}
> +module_exit(ske_keypad_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Naveen Kumar G/Sundar Iyer");
> +MODULE_DESCRIPTION("Nomadik Scroll-Key-Encoder Keypad Driver");
> +MODULE_ALIAS("platform:nomadik-ske-keypad");
> --
> 1.7.2.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-06 15:07 ` Datta, Shubhrajyoti
@ 2010-09-06 15:16 ` Sundar R IYER
2010-09-06 15:41 ` Datta, Shubhrajyoti
0 siblings, 1 reply; 14+ messages in thread
From: Sundar R IYER @ 2010-09-06 15:16 UTC (permalink / raw)
To: Datta, Shubhrajyoti; +Cc: linux-input, STEricsson_nomadik_linux
Hi,
>-----Original Message-----
>From: Datta, Shubhrajyoti [mailto:shubhrajyoti@ti.com]
>> + /* go through board initialiazation helpers */
>> + if (keypad->board->init) {
>Did not understand this
>-----Original Message-----
>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> > > +
>> > > + if (!keypad->board->init) {
>> > > + dev_err(&pdev->dev, "NULL board initialization helper\n");
>> >
>> > Could be checked earlier. Also, does it have to be an error?
>> >
>>
>> The platform code takes care to request the GPIOs and the GPIO configurations.
>> Hence I return error in case the board configurations dont get completed.
>>
>
>PLanform author migth opt to do the initialization earlier, together
>with teh rest of GPIO configuration so it is all ready by the time the
>driver is loaded. I think we should give such an option.
I interpreted Dmitry's above suggestion as allowing the driver to proceed in case platform
Initialization is not done.
The [PATCH 2/2] ux500: add platform data for Nomadik SKE keypad controller adds this platform
data which is configuration of the GPIO pins like this.
+/*
+ * ske_set_gpio_row: request and set gpio rows */ static int
+ske_set_gpio_row(int gpio) {
+ int ret;
+
+ ret = gpio_request(gpio, "ske-kp");
+ if (ret < 0) {
+ pr_err("ske_set_gpio_row: gpio request failed\n");
+ return ret;
+ }
+
+ ret = gpio_direction_output(gpio, 1);
+ if (ret < 0) {
+ pr_err("ske_set_gpio_row: gpio direction failed\n");
+ gpio_free(gpio);
+ }
+
+ return ret;
+}
+
+/*
+ * ske_kp_init - enable the gpio configuration */ static int
+ske_kp_init(void) {
+ int ret, i;
+
+ for (i = 0; i < SKE_KPD_MAX_ROWS; i++) {
+ ret = ske_set_gpio_row(ske_kp_rows[i]);
+ if (ret < 0) {
+ pr_err("ske_kp_init: failed init\n");
+ return ret;
+ }
+ }
+
+ return 0;
+}
And hence I need to call this board init to make sure that I get the GPIO lines
for my device.
>> +
>> + if (keypad->board->exit())
>> + keypad->board->exit();
>What does this exit function do?
>Are you sure the exit function does not need the keypad clock?
>
Although I haven't added a exit function to the platform data, but from the platform
code posted from above, it is clear that there are no controller dependencies for the
board exit function.
Regards,
Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-06 15:16 ` Sundar R IYER
@ 2010-09-06 15:41 ` Datta, Shubhrajyoti
2010-09-06 15:43 ` Sundar R IYER
0 siblings, 1 reply; 14+ messages in thread
From: Datta, Shubhrajyoti @ 2010-09-06 15:41 UTC (permalink / raw)
To: Sundar R IYER; +Cc: linux-input, STEricsson_nomadik_linux
> -----Original Message-----
> From: Sundar R IYER [mailto:sundar.iyer@stericsson.com]
> Sent: Monday, September 06, 2010 8:46 PM
> To: Datta, Shubhrajyoti
> Cc: linux-input@vger.kernel.org; STEricsson_nomadik_linux
> Subject: RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad
> controller
>
> Hi,
>
> >-----Original Message-----
> >From: Datta, Shubhrajyoti [mailto:shubhrajyoti@ti.com]
> >> + /* go through board initialiazation helpers */
> >> + if (keypad->board->init) {
> >Did not understand this
>
> >-----Original Message-----
> >From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> >> > > +
> >> > > + if (!keypad->board->init) {
> >> > > + dev_err(&pdev->dev, "NULL board initialization
> helper\n");
> >> >
> >> > Could be checked earlier. Also, does it have to be an error?
> >> >
> >>
> >> The platform code takes care to request the GPIOs and the GPIO
> configurations.
> >> Hence I return error in case the board configurations dont get
> completed.
> >>
> >
> >PLanform author migth opt to do the initialization earlier, together
> >with teh rest of GPIO configuration so it is all ready by the time the
> >driver is loaded. I think we should give such an option.
>
> I interpreted Dmitry's above suggestion as allowing the driver to proceed
> in case platform
> Initialization is not done.
>
> The [PATCH 2/2] ux500: add platform data for Nomadik SKE keypad controller
> adds this platform
> data which is configuration of the GPIO pins like this.
>
> +/*
> + * ske_set_gpio_row: request and set gpio rows */ static int
> +ske_set_gpio_row(int gpio) {
> + int ret;
> +
> + ret = gpio_request(gpio, "ske-kp");
> + if (ret < 0) {
> + pr_err("ske_set_gpio_row: gpio request failed\n");
> + return ret;
> + }
> +
> + ret = gpio_direction_output(gpio, 1);
> + if (ret < 0) {
> + pr_err("ske_set_gpio_row: gpio direction failed\n");
> + gpio_free(gpio);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * ske_kp_init - enable the gpio configuration */ static int
> +ske_kp_init(void) {
> + int ret, i;
> +
> + for (i = 0; i < SKE_KPD_MAX_ROWS; i++) {
> + ret = ske_set_gpio_row(ske_kp_rows[i]);
> + if (ret < 0) {
> + pr_err("ske_kp_init: failed init\n");
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
>
> And hence I need to call this board init to make sure that I get the GPIO
> lines
> for my device.
>
> >> +
> >> + if (keypad->board->exit())
> >> + keypad->board->exit();
> >What does this exit function do?
> >Are you sure the exit function does not need the keypad clock?
if (keypad->board->exit)
keypad->board->exit();
Is this you intended?
> >
>
> Although I haven't added a exit function to the platform data, but from
> the platform
> code posted from above, it is clear that there are no controller
> dependencies for the
> board exit function.
>
> Regards,
> Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-06 15:41 ` Datta, Shubhrajyoti
@ 2010-09-06 15:43 ` Sundar R IYER
0 siblings, 0 replies; 14+ messages in thread
From: Sundar R IYER @ 2010-09-06 15:43 UTC (permalink / raw)
To: Datta, Shubhrajyoti; +Cc: linux-input, STEricsson_nomadik_linux
Hi again,
>-----Original Message-----
>From: Datta, Shubhrajyoti [mailto:shubhrajyoti@ti.com]
>
> if (keypad->board->exit)
> keypad->board->exit();
>Is this you intended?
Oops. I wronged this one.
Anyways, I will wait for Dmitry's further comments before I spin a v4 of this.
Regards,
Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-06 12:48 [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller Sundar Iyer
2010-09-06 15:07 ` Datta, Shubhrajyoti
@ 2010-09-07 6:37 ` Dmitry Torokhov
2010-09-07 6:49 ` Sundar R IYER
2010-09-07 14:01 ` Trilok Soni
2 siblings, 1 reply; 14+ messages in thread
From: Dmitry Torokhov @ 2010-09-07 6:37 UTC (permalink / raw)
To: Sundar Iyer; +Cc: linux-input, STEricsson_nomadik_linux, Linus Walleij
Hi Sundar,
On Mon, Sep 06, 2010 at 06:18:04PM +0530, Sundar Iyer wrote:
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Signed-off-by: Sundar Iyer <sundar.iyer@stericsson.com>
> ---
>
> Changes:
> v3:
> - fixed Dmitry's comments assorted comments,
> - re-orged the IRQ to include a timer instead of cpu_relax()
>
Thank you for making the changes.
> arch/arm/plat-nomadik/include/plat/ske.h | 54 ++++
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/nomadik-ske-keypad.c | 449 +++++++++++++++++++++++++++
> 4 files changed, 514 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/plat-nomadik/include/plat/ske.h
> create mode 100644 drivers/input/keyboard/nomadik-ske-keypad.c
>
> diff --git a/arch/arm/plat-nomadik/include/plat/ske.h b/arch/arm/plat-nomadik/include/plat/ske.h
> new file mode 100644
> index 0000000..27aeadb
> --- /dev/null
> +++ b/arch/arm/plat-nomadik/include/plat/ske.h
> @@ -0,0 +1,54 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + *
> + * License Terms: GNU General Public License v2
> + * Author: Naveen Kumar Gaddipati <naveen.gaddipati@stericsson.com>
> + *
> + * ux500 Scroll key and Keypad Encoder (SKE) header
> + */
> +
> +#ifndef __SKE_H
> +#define __SKE_H
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +/* register definitions for SKE peripheral */
> +#define SKE_CR 0x00
> +#define SKE_VAL0 0x04
> +#define SKE_VAL1 0x08
> +#define SKE_DBCR 0x0C
> +#define SKE_IMSC 0x10
> +#define SKE_RIS 0x14
> +#define SKE_MIS 0x18
> +#define SKE_ICR 0x1C
> +#define SKE_ASR0 0x20
> +#define SKE_ASR1 0x24
> +#define SKE_ASR2 0x28
> +#define SKE_ASR3 0x2C
> +
> +#define SKE_NUM_ASRX_REGISTERS (4)
> +
> +/*
> + * Keypad module
> + */
> +
> +/**
> + * struct keypad_platform_data - structure for platform specific data
> + * @init: pointer to keypad init function
> + * @exit: pointer to keypad deinitialisation function
> + * @keymap_data: matrix scan code table for keycodes
> + * @krow: maximum number of rows
> + * @kcol: maximum number of columns
> + * @debounce_ms: platform specific debounce time
> + * @no_autorepeat: flag for auto repetition
> + */
> +struct ske_keypad_platform_data {
> + int (*init)(void);
> + int (*exit)(void);
> + struct matrix_keymap_data *keymap_data;
const please.
> + u8 krow;
> + u8 kcol;
> + u8 debounce_ms;
> + bool no_autorepeat;
> +};
> +#endif /*__SKE_KPD_H*/
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 4f30048..d63f566 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -327,6 +327,16 @@ config KEYBOARD_NEWTON
> To compile this driver as a module, choose M here: the
> module will be called newtonkbd.
>
> +config KEYBOARD_NOMADIK
> + tristate "ST-Ericsson Nomadik SKE keyboard"
> + depends on PLAT_NOMADIK
> + help
> + Say Y here if you want to use a keypad provided on the SKE controller
> + used on the Ux500 and Nomadik platforms
> +
> + To compile this driver as a module, choose M here: the
> + module will be called nmk-ske-keypad.
> +
> config KEYBOARD_OPENCORES
> tristate "OpenCores Keyboard Controller"
> help
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 8ac01fb..c13809c 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX) += matrix_keypad.o
> obj-$(CONFIG_KEYBOARD_MAX7359) += max7359_keypad.o
> obj-$(CONFIG_KEYBOARD_MCS) += mcs_touchkey.o
> obj-$(CONFIG_KEYBOARD_NEWTON) += newtonkbd.o
> +obj-$(CONFIG_KEYBOARD_NOMADIK) += nomadik-ske-keypad.o
> obj-$(CONFIG_KEYBOARD_OMAP) += omap-keypad.o
> obj-$(CONFIG_KEYBOARD_OMAP4) += omap4-keypad.o
> obj-$(CONFIG_KEYBOARD_OPENCORES) += opencores-kbd.o
> diff --git a/drivers/input/keyboard/nomadik-ske-keypad.c b/drivers/input/keyboard/nomadik-ske-keypad.c
> new file mode 100644
> index 0000000..1697181
> --- /dev/null
> +++ b/drivers/input/keyboard/nomadik-ske-keypad.c
> @@ -0,0 +1,449 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + *
> + * Author: Naveen Kumar G <naveen.gaddipati@stericsson.com> for ST-Ericsson
> + * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
> + *
> + * License terms:GNU General Public License (GPL) version 2
> + *
> + * Keypad controller driver for the SKE (Scroll Key Encoder) module used in
> + * the Nomadik 8815 and Ux500 platforms.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +
> +#include <plat/ske.h>
> +
> +/* SKE_CR bits */
> +#define SKE_KPMLT (0x1 << 6)
> +#define SKE_KPCN (0x7 << 3)
> +#define SKE_KPASEN (0x1 << 2)
> +#define SKE_KPASON (0x1 << 7)
> +
> +/* SKE_IMSC bits */
> +#define SKE_KPIMA (0x1 << 2)
> +
> +/* SKE_ICR bits */
> +#define SKE_KPICS (0x1 << 3)
> +#define SKE_KPICA (0x1 << 2)
> +
> +/* SKE_RIS bits */
> +#define SKE_KPRISA (0x1 << 2)
> +
> +#define SKE_KEYPAD_ROW_SHIFT 3
> +#define SKE_KPD_KEYMAP_SIZE (8 * 8)
> +
> +/**
> + * struct ske_keypad - data structure used by keypad driver
> + * @irq: irq no
> + * @reg_base: ske regsiters base address
> + * @input: pointer to input device object
> + * @board: keypad platform device
> + * @keymap: matrix scan code table for keycodes
> + * @clk: clock structure pointer
> + */
> +struct ske_keypad {
> + int irq;
> + void __iomem *reg_base;
> + struct input_dev *input;
> + const struct ske_keypad_platform_data *board;
> + unsigned short keymap[SKE_KPD_KEYMAP_SIZE];
> + struct clk *clk;
> + spinlock_t ske_keypad_lock;
> + struct timer_list timer;
> +};
> +
> +static void ske_keypad_set_bits(struct ske_keypad *keypad, u16 addr,
> + u8 mask, u8 data)
> +{
> + u32 ret;
> +
> + spin_lock(&keypad->ske_keypad_lock);
> +
> + ret = readl(keypad->reg_base + addr);
> + ret &= ~mask;
> + ret |= data;
> + writel(ret, keypad->reg_base + addr);
> +
> + spin_unlock(&keypad->ske_keypad_lock);
> +}
> +
> +/*
> + * ske_keypad_chip_init : init keypad controller configuration
> + *
> + * Enable Multi key press detection, auto scan mode
> + */
> +static int __devinit ske_keypad_chip_init(struct ske_keypad *keypad)
> +{
> + u32 value;
> + int timeout = keypad->board->debounce_ms;
> +
> + /* check SKE_RIS to be 0 */
> + while ((readl(keypad->reg_base + SKE_RIS) != 0x00000000) && timeout--)
> + cpu_relax();
> + if (!timeout)
> + return -EINVAL;
> +
> + /*
> + * set debounce value
> + * keypad dbounce is configured in DBCR[15:8]
> + * dbounce value in steps of 32/32.768 ms
> + */
> + spin_lock(&keypad->ske_keypad_lock);
> + value = readl(keypad->reg_base + SKE_DBCR);
> + value = value & 0xff;
> + value |= ((keypad->board->debounce_ms * 32000)/32768) << 8;
> + writel(value, keypad->reg_base + SKE_DBCR);
> + spin_unlock(&keypad->ske_keypad_lock);
> +
> + /* enable multi key detection */
> + ske_keypad_set_bits(keypad, SKE_CR, 0x0, SKE_KPMLT);
> +
> + /*
> + * set up the number of columns
> + * KPCN[5:3] defines no. of keypad columns to be auto scanned
> + */
> + value = (keypad->board->kcol - 1) << 3;
> + ske_keypad_set_bits(keypad, SKE_CR, SKE_KPCN, value);
> +
> + /* clear keypad interrupt for auto(and pending SW) scans */
> + ske_keypad_set_bits(keypad, SKE_ICR, 0x0, SKE_KPICA | SKE_KPICS);
> +
> + /* un-mask keypad interrupts */
> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
> +
> + /* enable automatic scan */
> + ske_keypad_set_bits(keypad, SKE_CR, 0x0, SKE_KPASEN);
> +
> + return 0;
> +}
> +
> +static void ske_keypad_read_data(struct ske_keypad *keypad)
> +{
> + struct input_dev *input = keypad->input;
> + u16 status;
> + int col = 0, row = 0, code;
> + int ske_asr, ske_ris, key_pressed, i;
> +
> + /*
> + * Read the auto scan registers
> + *
> + * Each SKE_ASRx (x=0 to x=3) contains two row values.
> + * lower byte contains row value for coloumn 2*x,
> + * upper byte contains row value for column 2*x + 1
> + */
> + for (i = 0; i < SKE_NUM_ASRX_REGISTERS; i++) {
> + ske_asr = readl(keypad->reg_base + SKE_ASR0 + (4 * i));
> + if (!ske_asr)
> + continue;
> +
> + /* now that ASRx is zero, find out the coloumn x and row y*/
> + if (ske_asr & 0xff) {
> + col = i * 2;
> + status = ske_asr & 0xff;
> + } else {
> + col = (i * 2) + 1;
> + status = (ske_asr & 0xff00) >> 8;
> + }
> +
> + /* find out the row */
> + row = __ffs(status);
> +
> + code = MATRIX_SCAN_CODE(row, col, SKE_KEYPAD_ROW_SHIFT);
> + ske_ris = readl(keypad->reg_base + SKE_RIS);
> + key_pressed = ske_ris & SKE_KPRISA;
> +
> + input_event(input, EV_MSC, MSC_SCAN, code);
> + input_report_key(input, keypad->keymap[code], key_pressed);
> + input_sync(input);
> + }
> + return;
No naked returns at the end of void functions please.
> +}
> +
> +static void ske_keypad_timer(unsigned long data)
> +{
> + struct platform_device *pdev = (struct platform_device *) data;
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int ske_kpason;
> + int timeout = keypad->board->debounce_ms;
> +
> + ske_kpason = readl(keypad->reg_base + SKE_CR) & SKE_KPASON;
> + if (ske_kpason) {
> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout));
> + return;
> + }
> +
> + /* read data registers and report event */
> + ske_keypad_read_data(keypad);
> +
> + return;
> +}
> +
> +static irqreturn_t ske_keypad_irq(int irq, void *dev_id)
> +{
> + struct ske_keypad *keypad = dev_id;
> + int timeout = keypad->board->debounce_ms;
> +
> + /* disable auto scan interrupt; mask the interrupt generated */
> + ske_keypad_set_bits(keypad, SKE_IMSC, ~SKE_KPIMA, 0x0);
> + ske_keypad_set_bits(keypad, SKE_ICR, 0x0, SKE_KPICA);
> +
> + /*
> + * if KPASON is not ready, we cannot read data registers
> + * so wait for a debounce period; if still not settled,
> + * we fire a timer and exit the IRQ
> + */
> + while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--)
> + cpu_relax();
> + if (!timeout) {
> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout));
timeout is 0 here, I do not think that's what you want.
> + goto out;
> + }
> +
> + /*
> + * KPASON behaved nicely and we can read data registers and report event
> + */
> + ske_keypad_read_data(keypad);
> +
> +out:
> + /* enable auto scan interrupts */
> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
You sure you want do do it here even when SKE_KPASON is set and timer is
pending? I'd expect re-enabling interrupts from the timer.
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int __devinit ske_keypad_probe(struct platform_device *pdev)
> +{
> + struct ske_keypad *keypad;
> + struct resource *res;
> + struct input_dev *input;
> + struct clk *clk;
> + void __iomem *reg_base;
> + int ret;
> + int irq;
> + const struct ske_keypad_platform_data *plat = pdev->dev.platform_data;
> +
> + if (!plat) {
> + dev_err(&pdev->dev, "invalid keypad platform data\n");
> + ret = -EINVAL;
> + goto out_ret;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get keypad irq\n");
> + ret = -EINVAL;
> + goto out_ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "missing platform resources\n");
> + ret = -EINVAL;
> + goto out_ret;
> + }
> +
> + res = request_mem_region(res->start, resource_size(res), pdev->name);
> + if (!res) {
> + dev_err(&pdev->dev, "failed to request I/O memory\n");
> + ret = -EBUSY;
> + goto out_ret;
> + }
> +
> + reg_base = ioremap(res->start, resource_size(res));
> + if (!reg_base) {
> + dev_err(&pdev->dev, "failed to remap I/O memory\n");
> + ret = -ENXIO;
> + goto out_freerequest_memregions;
> + }
> +
> + clk = clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "failed to clk_get\n");
> + ret = PTR_ERR(clk);
> + goto out_freeioremap;
> + }
> +
> + /* resources are sane; we begin allocation */
> + keypad = kzalloc(sizeof(struct ske_keypad), GFP_KERNEL);
> + if (!keypad) {
> + dev_err(&pdev->dev, "failed to allocate keypad memory\n");
> + ret = -ENOMEM;
> + goto out_freeclk;
> + }
> +
> + input = input_allocate_device();
> + if (!input) {
> + dev_err(&pdev->dev, "failed to input_allocate_device\n");
> + ret = -ENOMEM;
> + goto out_freekeypad;
> + }
> +
> + input->id.bustype = BUS_HOST;
> + input->name = "ux500-ske-keypad";
> + input->dev.parent = &pdev->dev;
> +
> + input->keycode = keypad->keymap;
> + input->keycodesize = sizeof(keypad->keymap[0]);
> + input->keycodemax = ARRAY_SIZE(keypad->keymap);
> +
> + input_set_capability(input, EV_MSC, MSC_SCAN);
> +
> + __set_bit(EV_KEY, input->evbit);
> + if (!plat->no_autorepeat)
> + __set_bit(EV_REP, input->evbit);
> +
> + matrix_keypad_build_keymap(plat->keymap_data, SKE_KEYPAD_ROW_SHIFT,
> + input->keycode, input->keybit);
> +
> + ret = input_register_device(input);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "unable to register input device: %d\n", ret);
> + goto out_freeinput;
> + }
I'd probably move registration past requesting IRQ - it will simplify
error handling I think. It is safe to send events through input device
as long as it has been allocated with input_allocate_device() even if it
has not been registered yet.
> +
> + keypad->irq = irq;
> + keypad->board = plat;
> + keypad->input = input;
> + keypad->reg_base = reg_base;
> + keypad->clk = clk;
> +
> + /* allocations are sane, we begin HW initialization */
> + clk_enable(keypad->clk);
> +
> + /* go through board initialiazation helpers */
> + if (keypad->board->init) {
> + keypad->board->init();
I do not think you actually compiled this - closing brace is missing
(well, braces are actually not needed at all).
> +
> + ret = ske_keypad_chip_init(keypad);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "unable to init keypad hardware\n");
> + goto out_unregisterinput;
> + }
> +
> + spin_lock_init(&keypad->ske_keypad_lock);
> + setup_timer(&keypad->timer, ske_keypad_timer, (unsigned long)pdev);
> +
> + ret = request_threaded_irq(keypad->irq, NULL, ske_keypad_irq,
> + IRQF_ONESHOT, "ske-keypad", keypad);
> + if (ret) {
> + dev_err(&pdev->dev, "allocate irq %d failed\n", keypad->irq);
> + goto out_unregisterinput;
> + }
> +
> + device_init_wakeup(&pdev->dev, true);
> +
> + platform_set_drvdata(pdev, keypad);
> +
> + return 0;
> +
> +out_unregisterinput:
> + input_unregister_device(input);
> + input = NULL;
> + clk_disable(keypad->clk);
> +out_freeinput:
> + input_free_device(input);
> +out_freekeypad:
> + kfree(keypad);
> +out_freeclk:
> + clk_put(clk);
> +out_freeioremap:
> + iounmap(reg_base);
> +out_freerequest_memregions:
> + release_mem_region(res->start, resource_size(res));
> +out_ret:
> + return ret;
> +}
> +
> +static int __devexit ske_keypad_remove(struct platform_device *pdev)
> +{
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> + del_timer_sync(&keypad->timer);
> + free_irq(keypad->irq, keypad);
> + input_unregister_device(keypad->input);
> +
> + clk_disable(keypad->clk);
> + clk_put(keypad->clk);
> +
> + if (keypad->board->exit())
You are already aware of the issue here...
> + keypad->board->exit();
> +
> + iounmap(keypad->reg_base);
> + release_mem_region(res->start, resource_size(res));
> + kfree(keypad);
platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int ske_keypad_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int irq = platform_get_irq(pdev, 0);
> +
> + if (device_may_wakeup(dev))
> + enable_irq_wake(irq);
> + else
> + ske_keypad_set_bits(keypad, SKE_IMSC, ~SKE_KPIMA, 0x0);
> +
> + return 0;
> +}
> +
> +static int ske_keypad_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int irq = platform_get_irq(pdev, 0);
> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(irq);
> + else
> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ske_keypad_dev_pm_ops = {
> + .suspend = ske_keypad_suspend,
> + .resume = ske_keypad_resume,
> +};
> +#endif
> +
> +struct platform_driver ske_keypad_driver = {
> + .driver = {
> + .name = "nmk-ske-keypad",
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &ske_keypad_dev_pm_ops,
> +#endif
> + },
> + .probe = ske_keypad_probe,
> + .remove = __devexit_p(ske_keypad_remove),
> +};
> +
> +static int __init ske_keypad_init(void)
> +{
> + return platform_driver_probe(&ske_keypad_driver, ske_keypad_probe);
> +}
> +module_init(ske_keypad_init);
> +
> +static void __exit ske_keypad_exit(void)
> +{
> + platform_driver_unregister(&ske_keypad_driver);
> +}
> +module_exit(ske_keypad_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Naveen Kumar G/Sundar Iyer");
> +MODULE_DESCRIPTION("Nomadik Scroll-Key-Encoder Keypad Driver");
> +MODULE_ALIAS("platform:nomadik-ske-keypad");
> --
> 1.7.2.dirty
>
--
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-07 6:37 ` Dmitry Torokhov
@ 2010-09-07 6:49 ` Sundar R IYER
0 siblings, 0 replies; 14+ messages in thread
From: Sundar R IYER @ 2010-09-07 6:49 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, STEricsson_nomadik_linux, Linus WALLEIJ
Hi Dmitry,
>-----Original Message-----
>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>> + struct matrix_keymap_data *keymap_data;
>
>const please.
Yes. I think I have missed this. You had pointed out this earlier also; but I think the re-spin
lost them.
>No naked returns at the end of void functions please.
Okay.
>> +}
>> +
>> +static void ske_keypad_timer(unsigned long data)
>> +{
>> + struct platform_device *pdev = (struct platform_device *) data;
>> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
>> + int ske_kpason;
>> + int timeout = keypad->board->debounce_ms;
>> +
>> + ske_kpason = readl(keypad->reg_base + SKE_CR) & SKE_KPASON;
>> + if (ske_kpason) {
>> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout));
>> + return;
>> + }
>> +
>> + /* read data registers and report event */
>> + ske_keypad_read_data(keypad);
>> +
>> + return;
>> +}
>> +
>> +static irqreturn_t ske_keypad_irq(int irq, void *dev_id)
>> +{
>> + struct ske_keypad *keypad = dev_id;
>> + int timeout = keypad->board->debounce_ms;
>> +
>> + /* disable auto scan interrupt; mask the interrupt generated */
>> + ske_keypad_set_bits(keypad, SKE_IMSC, ~SKE_KPIMA, 0x0);
>> + ske_keypad_set_bits(keypad, SKE_ICR, 0x0, SKE_KPICA);
>> +
>> + /*
>> + * if KPASON is not ready, we cannot read data registers
>> + * so wait for a debounce period; if still not settled,
>> + * we fire a timer and exit the IRQ
>> + */
>> + while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--)
>> + cpu_relax();
>> + if (!timeout) {
>> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout));
>
>timeout is 0 here, I do not think that's what you want.
>
Sorry. I will replace it with keypad->board->debounce_ms.
>> + goto out;
>> + }
>> +
>> + /*
>> + * KPASON behaved nicely and we can read data registers and report event
>> + */
>> + ske_keypad_read_data(keypad);
>> +
>> +out:
>> + /* enable auto scan interrupts */
>> + ske_keypad_set_bits(keypad, SKE_IMSC, 0x0, SKE_KPIMA);
>
>You sure you want do do it here even when SKE_KPASON is set and timer is
>pending? I'd expect re-enabling interrupts from the timer.
>
I did have that originally as you pointed out. But I ran into an issue where multi-key presses
were lost in a case of continued key press.
>I'd probably move registration past requesting IRQ - it will simplify
>error handling I think. It is safe to send events through input device
>as long as it has been allocated with input_allocate_device() even if it
>has not been registered yet.
>
Okay. Will do so.
>> + /* go through board initialiazation helpers */
>> + if (keypad->board->init) {
>> + keypad->board->init();
>
>I do not think you actually compiled this - closing brace is missing
>(well, braces are actually not needed at all).
Ohh yes, Linus W pointed me out. These are last minute fixups, which I lost. Sorry for that
>You are already aware of the issue here...
>
Yes.
Thanx for your review,
Regards,
Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-06 12:48 [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller Sundar Iyer
2010-09-06 15:07 ` Datta, Shubhrajyoti
2010-09-07 6:37 ` Dmitry Torokhov
@ 2010-09-07 14:01 ` Trilok Soni
2010-09-08 12:55 ` Sundar R IYER
2 siblings, 1 reply; 14+ messages in thread
From: Trilok Soni @ 2010-09-07 14:01 UTC (permalink / raw)
To: Sundar Iyer
Cc: dmitry.torokhov, linux-input, STEricsson_nomadik_linux, Linus Walleij
Hi Sundar,
On 9/6/2010 6:18 PM, Sundar Iyer wrote:
> Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> Signed-off-by: Sundar Iyer <sundar.iyer@stericsson.com>
> ---
More description about the keypad controller please.
>
> Changes:
> v3:
> - fixed Dmitry's comments assorted comments,
> - re-orged the IRQ to include a timer instead of cpu_relax()
>
> arch/arm/plat-nomadik/include/plat/ske.h | 54 ++++
> drivers/input/keyboard/Kconfig | 10 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/nomadik-ske-keypad.c | 449 +++++++++++++++++++++++++++
> 4 files changed, 514 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/plat-nomadik/include/plat/ske.h
Is it Onchip on the NomaDik or over PMIC?
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +/* register definitions for SKE peripheral */
> +#define SKE_CR 0x00
> +#define SKE_VAL0 0x04
> +#define SKE_VAL1 0x08
> +#define SKE_DBCR 0x0C
> +#define SKE_IMSC 0x10
> +#define SKE_RIS 0x14
> +#define SKE_MIS 0x18
> +#define SKE_ICR 0x1C
> +#define SKE_ASR0 0x20
> +#define SKE_ASR1 0x24
> +#define SKE_ASR2 0x28
> +#define SKE_ASR3 0x2C
> +
> +#define SKE_NUM_ASRX_REGISTERS (4)
Why they need to be in the header? These registers #define can be moved to .c file.
> diff --git a/drivers/input/keyboard/nomadik-ske-keypad.c b/drivers/input/keyboard/nomadik-ske-keypad.c
> new file mode 100644
> index 0000000..1697181
> --- /dev/null
> +++ b/drivers/input/keyboard/nomadik-ske-keypad.c
> @@ -0,0 +1,449 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + *
> + * Author: Naveen Kumar G <naveen.gaddipati@stericsson.com> for ST-Ericsson
> + * Author: Sundar Iyer <sundar.iyer@stericsson.com> for ST-Ericsson
> + *
> + * License terms:GNU General Public License (GPL) version 2
> + *
> + * Keypad controller driver for the SKE (Scroll Key Encoder) module used in
> + * the Nomadik 8815 and Ux500 platforms.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +
> +#include <plat/ske.h>
> +
> +/* SKE_CR bits */
> +#define SKE_KPMLT (0x1 << 6)
> +#define SKE_KPCN (0x7 << 3)
> +#define SKE_KPASEN (0x1 << 2)
> +#define SKE_KPASON (0x1 << 7)
> +
> +/* SKE_IMSC bits */
> +#define SKE_KPIMA (0x1 << 2)
> +
> +/* SKE_ICR bits */
> +#define SKE_KPICS (0x1 << 3)
> +#define SKE_KPICA (0x1 << 2)
> +
> +/* SKE_RIS bits */
> +#define SKE_KPRISA (0x1 << 2)
> +
> +#define SKE_KEYPAD_ROW_SHIFT 3
> +#define SKE_KPD_KEYMAP_SIZE (8 * 8)
> +
> +/**
> + * struct ske_keypad - data structure used by keypad driver
> + * @irq: irq no
> + * @reg_base: ske regsiters base address
> + * @input: pointer to input device object
> + * @board: keypad platform device
> + * @keymap: matrix scan code table for keycodes
> + * @clk: clock structure pointer
> + */
> +struct ske_keypad {
> + int irq;
> + void __iomem *reg_base;
> + struct input_dev *input;
> + const struct ske_keypad_platform_data *board;
> + unsigned short keymap[SKE_KPD_KEYMAP_SIZE];
> + struct clk *clk;
> + spinlock_t ske_keypad_lock;
> + struct timer_list timer;
> +};
> +
> +
> +
> +static void ske_keypad_timer(unsigned long data)
> +{
> + struct platform_device *pdev = (struct platform_device *) data;
> + struct ske_keypad *keypad = platform_get_drvdata(pdev);
> + int ske_kpason;
> + int timeout = keypad->board->debounce_ms;
> +
> + ske_kpason = readl(keypad->reg_base + SKE_CR) & SKE_KPASON;
> + if (ske_kpason) {
> + mod_timer(&keypad->timer, jiffies + msecs_to_jiffies(timeout));
> + return;
> + }
> +
> + /* read data registers and report event */
> + ske_keypad_read_data(keypad);
> +
> + return;
No need.
> +}
...
> +
> +
> +
> + spin_lock_init(&keypad->ske_keypad_lock);
> + setup_timer(&keypad->timer, ske_keypad_timer, (unsigned long)pdev);
> +
> + ret = request_threaded_irq(keypad->irq, NULL, ske_keypad_irq,
> + IRQF_ONESHOT, "ske-keypad", keypad);
> + if (ret) {
> + dev_err(&pdev->dev, "allocate irq %d failed\n", keypad->irq);
> + goto out_unregisterinput;
> + }
Can you please clarify why you need thread? Looking at the code, I don't think that
we have any need of creating thread. request_irq(...) should work just fine.
> +
> + device_init_wakeup(&pdev->dev, true);
Can you make this pdata?
> +
> + platform_set_drvdata(pdev, keypad);
> +
> + return 0;
> +
> +out_unregisterinput:
> + input_unregister_device(input);
> + input = NULL;
> + clk_disable(keypad->clk);
> +out_freeinput:
> + input_free_device(input);
> +out_freekeypad:
> + kfree(keypad);
> +out_freeclk:
> + clk_put(clk);
> +out_freeioremap:
> + iounmap(reg_base);
> +out_freerequest_memregions:
> + release_mem_region(res->start, resource_size(res));
> +out_ret:
> + return ret;
> +}
> +
> +
> +static const struct dev_pm_ops ske_keypad_dev_pm_ops = {
> + .suspend = ske_keypad_suspend,
> + .resume = ske_keypad_resume,
> +};
> +#endif
> +
> +struct platform_driver ske_keypad_driver = {
> + .driver = {
> + .name = "nmk-ske-keypad",
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &ske_keypad_dev_pm_ops,
> +#endif
> + },
> + .probe = ske_keypad_probe,
> + .remove = __devexit_p(ske_keypad_remove),
> +};
> +
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Naveen Kumar G/Sundar Iyer");
Care to add e-mail address?
> +MODULE_DESCRIPTION("Nomadik Scroll-Key-Encoder Keypad Driver");
> +MODULE_ALIAS("platform:nomadik-ske-keypad");
---Trilok Soni
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-07 14:01 ` Trilok Soni
@ 2010-09-08 12:55 ` Sundar R IYER
2010-09-08 12:58 ` Trilok Soni
0 siblings, 1 reply; 14+ messages in thread
From: Sundar R IYER @ 2010-09-08 12:55 UTC (permalink / raw)
To: Trilok Soni
Cc: dmitry.torokhov, linux-input, STEricsson_nomadik_linux, Linus WALLEIJ
Hi Trilok,
>-----Original Message-----
>From: Trilok Soni [mailto:tsoni@codeaurora.org]
>
>More description about the keypad controller please.
>
Okay.
>Is it Onchip on the NomaDik or over PMIC?
>
As stated in the Kconfig option, it is on the Nomadik and U8500 platforms.
Anyways, I will update the commit message for more description.
>> +/* register definitions for SKE peripheral */
>
>Why they need to be in the header? These registers #define can be moved to .c file.
>
These registers are for the SKE Keypad - Scroll Key Encoder and Keypad modules. The
scroll key encoder too shares them and hence in the header file.
>> + return;
>
>No need.
>
Okay.
>Can you please clarify why you need thread? Looking at the code, I don't think that
>we have any need of creating thread. request_irq(...) should work just fine.
Yes it can. I am seeing increasing tendency to migrate to threaded_irq from the legacy irq recently
and hence the threaded request.
>> + device_init_wakeup(&pdev->dev, true);
>
>Can you make this pdata?
>
Okay.
>> +MODULE_AUTHOR("Naveen Kumar G/Sundar Iyer");
>
>Care to add e-mail address?
>
Okay.
Dmitry, apart from Trilok's comments above, is there anything else you want me to re-spin?
Regards,
Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-08 12:55 ` Sundar R IYER
@ 2010-09-08 12:58 ` Trilok Soni
2010-09-08 14:19 ` Linus Walleij
0 siblings, 1 reply; 14+ messages in thread
From: Trilok Soni @ 2010-09-08 12:58 UTC (permalink / raw)
To: Sundar R IYER
Cc: dmitry.torokhov, linux-input, STEricsson_nomadik_linux, Linus WALLEIJ
Hi Sundar,
>
> These registers are for the SKE Keypad - Scroll Key Encoder and Keypad modules. The
> scroll key encoder too shares them and hence in the header file.
Let's keep the only ones which are shared and not all. I am not sure about the scroll key
encoder driver location.
>
> Okay.
>
>> Can you please clarify why you need thread? Looking at the code, I don't think that
>> we have any need of creating thread. request_irq(...) should work just fine.
>
> Yes it can. I am seeing increasing tendency to migrate to threaded_irq from the legacy irq recently
> and hence the threaded request.
>
Use threaded_irq only when needed with different flags and not everywhere. In this case we don't
need threaded irq at all.
---Trilok Soni
--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-08 12:58 ` Trilok Soni
@ 2010-09-08 14:19 ` Linus Walleij
2010-09-08 15:46 ` Dmitry Torokhov
0 siblings, 1 reply; 14+ messages in thread
From: Linus Walleij @ 2010-09-08 14:19 UTC (permalink / raw)
To: Trilok Soni
Cc: Sundar R IYER, dmitry.torokhov, linux-input, STEricsson_nomadik_linux
2010/9/8 Trilok Soni <tsoni@codeaurora.org>:
> [Sundar]
>> Yes it can. I am seeing increasing tendency to migrate to threaded_irq from the legacy irq recently
>> and hence the threaded request.
>
> Use threaded_irq only when needed with different flags and not everywhere. In this case we don't
> need threaded irq at all.
Look closer at the IRQ handler:
+static irqreturn_t ske_keypad_irq(int irq, void *dev_id)
(...)
+ /*
+ * if KPASON is not ready, we cannot read data registers
+ * so wait for a debounce period; if still not settled,
+ * we fire a timer and exit the IRQ
+ */
+ while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--)
+ cpu_relax();
Active polling. Surely you want this to be preemptible? Thus ->
threaded handler.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-08 14:19 ` Linus Walleij
@ 2010-09-08 15:46 ` Dmitry Torokhov
2010-09-09 4:22 ` Sundar R IYER
0 siblings, 1 reply; 14+ messages in thread
From: Dmitry Torokhov @ 2010-09-08 15:46 UTC (permalink / raw)
To: Linus Walleij
Cc: Trilok Soni, Sundar R IYER, linux-input, STEricsson_nomadik_linux
On Wed, Sep 08, 2010 at 04:19:45PM +0200, Linus Walleij wrote:
> 2010/9/8 Trilok Soni <tsoni@codeaurora.org>:
> > [Sundar]
> >> Yes it can. I am seeing increasing tendency to migrate to threaded_irq from the legacy irq recently
> >> and hence the threaded request.
> >
> > Use threaded_irq only when needed with different flags and not everywhere. In this case we don't
> > need threaded irq at all.
>
> Look closer at the IRQ handler:
>
> +static irqreturn_t ske_keypad_irq(int irq, void *dev_id)
> (...)
> + /*
> + * if KPASON is not ready, we cannot read data registers
> + * so wait for a debounce period; if still not settled,
> + * we fire a timer and exit the IRQ
> + */
> + while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--)
> + cpu_relax();
>
> Active polling. Surely you want this to be preemptible? Thus ->
> threaded handler.
>
Actually I'd like that form of active polling not be there at all.
--
Dmitry
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-08 15:46 ` Dmitry Torokhov
@ 2010-09-09 4:22 ` Sundar R IYER
2010-09-13 7:31 ` Sundar R IYER
0 siblings, 1 reply; 14+ messages in thread
From: Sundar R IYER @ 2010-09-09 4:22 UTC (permalink / raw)
To: Dmitry Torokhov, Linus Walleij
Cc: Trilok Soni, linux-input, STEricsson_nomadik_linux
Hi Dmitry,
>-----Original Message-----
>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>>
>> +static irqreturn_t ske_keypad_irq(int irq, void *dev_id)
>> (...)
>> + /*
>> + * if KPASON is not ready, we cannot read data registers
>> + * so wait for a debounce period; if still not settled,
>> + * we fire a timer and exit the IRQ
>> + */
>> + while ((readl(keypad->reg_base + SKE_CR) & SKE_KPASON) && timeout--)
>> + cpu_relax();
>>
>> Active polling. Surely you want this to be preemptible? Thus ->
>> threaded handler.
>>
>
>Actually I'd like that form of active polling not be there at all.
>
Hmm. Could you please help me out on how I can proceed with the
following two HW constraints:
1. KPASON polling needs to be waited in case of a multi key press so that
the keypad registers can stabilize. This is what the TRM states.
2. If I move enabling the interrupts to the timer snippet, I end up breaking multi-key
detection.
Regards,
Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v3 1/1] input: add support for Nomadik SKE keypad controller
2010-09-09 4:22 ` Sundar R IYER
@ 2010-09-13 7:31 ` Sundar R IYER
0 siblings, 0 replies; 14+ messages in thread
From: Sundar R IYER @ 2010-09-13 7:31 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Trilok Soni, linux-input, STEricsson_nomadik_linux
Hello Dmitry,
Could you suggest some ideas to move ahead on this? I see myself
getting stucked :(
Regards,
Sundar
>-----Original Message-----
>From: Sundar R IYER
>
>Hmm. Could you please help me out on how I can proceed with the
>following two HW constraints:
>
>1. KPASON polling needs to be waited in case of a multi key press so that
>the keypad registers can stabilize. This is what the TRM states.
>
>2. If I move enabling the interrupts to the timer snippet, I end up breaking multi-key
>detection.
>
>Regards,
>Sundar
^ permalink raw reply [flat|nested] 14+ messages in thread