Hi Dmitry, Thanks for yours remarks. Best regards Gabriel On 03/05/2014 07:46 AM, Dmitry Torokhov wrote: > Hi Gabriel, > > On Wed, Mar 05, 2014 at 04:39:28AM +0100, Gabriel FERNANDEZ wrote: >> This patch adds ST Keyscan driver to use the keypad hw a subset >> of ST boards provide. Specific board setup will be put in the >> given dt. >> >> Signed-off-by: Giuseppe Condorelli >> Signed-off-by: Gabriel Fernandez >> --- >> .../devicetree/bindings/input/st-keypad.txt | 50 ++++ >> drivers/input/keyboard/Kconfig | 12 + >> drivers/input/keyboard/Makefile | 1 + >> drivers/input/keyboard/st-keyscan.c | 323 +++++++++++++++++++++ >> 4 files changed, 386 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/input/st-keypad.txt >> create mode 100644 drivers/input/keyboard/st-keyscan.c >> >> diff --git a/Documentation/devicetree/bindings/input/st-keypad.txt b/Documentation/devicetree/bindings/input/st-keypad.txt >> new file mode 100644 >> index 0000000..63eb07a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/input/st-keypad.txt >> @@ -0,0 +1,50 @@ >> +* ST Keypad controller device tree bindings >> + >> +The ST keypad controller device tree binding is based on the >> +matrix-keymap. >> + >> +Required properties: >> + >> +- compatible: "st,keypad" >> + >> +- reg: Register base address of st-keypad controler. >> + >> +- interrupts: Interrupt numberof st-keypad controler. >> + >> +- clocks: Must contain one entry, for the module clock. >> + See ../clocks/clock-bindings.txt for details. >> + >> +- pinctrl-0: Should specify pin control groups used for this controller. >> + >> +- pinctrl-names: Should contain only one value - "default". >> + >> +- linux,keymap: The keymap for keys as described in the binding document >> + devicetree/bindings/input/matrix-keymap.txt. >> + >> +- keypad,num-rows: Number of row lines connected to the keypad controller. >> + >> +- keypad,num-columns: Number of column lines connected to the keypad >> + controller. >> + >> +- st,debounce_us: Debouncing interval time in microseconds >> + >> + >> +Example: >> + >> +keyscan: keyscan@fe4b0000 { >> + compatible = "st,keypad"; >> + reg = <0xfe4b0000 0x2000>; >> + interrupts = ; >> + clocks = <&CLK_SYSIN>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_keyscan>; >> + >> + keypad,num-rows = <4>; >> + keypad,num-columns = <4>; >> + st,debounce_us = <5000>; >> + linux,keymap = < /* in0 in1 in2 in3 */ >> + KEY_F13 KEY_F9 KEY_F5 KEY_F1 /* out0 */ >> + KEY_F14 KEY_F10 KEY_F6 KEY_F2 /* out1 */ >> + KEY_F15 KEY_F11 KEY_F7 KEY_F3 /* out2 */ >> + KEY_F16 KEY_F12 KEY_F8 KEY_F4 >; /* out3 */ >> +}; >> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig >> index a673c9f..9e29125 100644 >> --- a/drivers/input/keyboard/Kconfig >> +++ b/drivers/input/keyboard/Kconfig >> @@ -512,6 +512,18 @@ config KEYBOARD_STOWAWAY >> To compile this driver as a module, choose M here: the >> module will be called stowaway. >> >> +config KEYBOARD_ST_KEYSCAN >> + tristate "STMicroelectronics keyscan support" >> + depends on ARCH_STI >> + select INPUT_EVDEV >> + select INPUT_MATRIXKMAP >> + help >> + Say Y here if you want to use a keypad attached to the keyscan block >> + on some STMicroelectronics SoC devices. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called stm-keyscan. >> + >> config KEYBOARD_SUNKBD >> tristate "Sun Type 4 and Type 5 keyboard" >> select SERIO >> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile >> index a699b61..5fd020a 100644 >> --- a/drivers/input/keyboard/Makefile >> +++ b/drivers/input/keyboard/Makefile >> @@ -50,6 +50,7 @@ obj-$(CONFIG_KEYBOARD_SH_KEYSC) += sh_keysc.o >> obj-$(CONFIG_KEYBOARD_SPEAR) += spear-keyboard.o >> obj-$(CONFIG_KEYBOARD_STMPE) += stmpe-keypad.o >> obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o >> +obj-$(CONFIG_KEYBOARD_ST_KEYSCAN) += st-keyscan.o >> obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o >> obj-$(CONFIG_KEYBOARD_TC3589X) += tc3589x-keypad.o >> obj-$(CONFIG_KEYBOARD_TEGRA) += tegra-kbc.o >> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c >> new file mode 100644 >> index 0000000..606cc19 >> --- /dev/null >> +++ b/drivers/input/keyboard/st-keyscan.c >> @@ -0,0 +1,323 @@ >> +/* >> + * STMicroelectronics Key Scanning driver >> + * >> + * Copyright (c) 2014 STMicroelectonics Ltd. >> + * Author: Stuart Menefy >> + * >> + * Based on sh_keysc.c, copyright 2008 Magnus Damm >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define ST_KEYSCAN_MAXKEYS 16 >> + >> +#define KEYSCAN_CONFIG_OFF 0x000 >> +#define KEYSCAN_CONFIG_ENABLE 1 >> +#define KEYSCAN_DEBOUNCE_TIME_OFF 0x004 >> +#define KEYSCAN_MATRIX_STATE_OFF 0x008 >> +#define KEYSCAN_MATRIX_DIM_OFF 0x00c >> +#define KEYSCAN_MATRIX_DIM_X_SHIFT 0 >> +#define KEYSCAN_MATRIX_DIM_Y_SHIFT 2 >> + >> +struct keypad_platform_data { >> + const struct matrix_keymap_data *keymap_data; >> + unsigned int num_out_pads; >> + unsigned int num_in_pads; >> + unsigned int debounce_us; >> +}; >> + >> +struct keyscan_priv { >> + void __iomem *base; >> + int irq; >> + struct clk *clk; >> + struct input_dev *input_dev; >> + struct keypad_platform_data *config; >> + unsigned int last_state; >> + u32 keycodes[ST_KEYSCAN_MAXKEYS]; >> +}; >> + >> +static irqreturn_t keyscan_isr(int irq, void *dev_id) >> +{ >> + struct platform_device *pdev = dev_id; >> + struct keyscan_priv *priv = platform_get_drvdata(pdev); >> + int state; >> + int change; >> + >> + state = readl(priv->base + KEYSCAN_MATRIX_STATE_OFF) & 0xffff; >> + change = priv->last_state ^ state; >> + >> + while (change) { >> + int scancode = __ffs(change); >> + int down = state & (1 << scancode); >> + >> + input_report_key(priv->input_dev, priv->keycodes[scancode], >> + down); >> + change ^= 1 << scancode; >> + }; >> + >> + input_sync(priv->input_dev); >> + >> + priv->last_state = state; >> + >> + return IRQ_HANDLED; >> +} >> + >> +static void keyscan_start(struct keyscan_priv *priv) >> +{ >> + clk_enable(priv->clk); >> + >> + writel(priv->config->debounce_us * (clk_get_rate(priv->clk) / 1000000), >> + priv->base + KEYSCAN_DEBOUNCE_TIME_OFF); >> + >> + writel(((priv->config->num_in_pads - 1) << KEYSCAN_MATRIX_DIM_X_SHIFT) | >> + ((priv->config->num_out_pads - 1) << KEYSCAN_MATRIX_DIM_Y_SHIFT), >> + priv->base + KEYSCAN_MATRIX_DIM_OFF); >> + >> + writel(KEYSCAN_CONFIG_ENABLE, priv->base + KEYSCAN_CONFIG_OFF); >> +} >> + >> +static void keyscan_stop(struct keyscan_priv *priv) >> +{ >> + writel(0, priv->base + KEYSCAN_CONFIG_OFF); >> + >> + clk_disable(priv->clk); >> +} >> + >> +static int keypad_matrix_key_parse_dt(struct keyscan_priv *st_kp) >> +{ >> + struct device *dev = st_kp->input_dev->dev.parent; >> + struct device_node *np = dev->of_node; >> + struct keypad_platform_data *pdata; >> + int error; >> + >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) { >> + dev_err(dev, "failed to allocate driver pdata\n"); >> + return -ENOMEM; >> + } >> + >> + error = matrix_keypad_parse_of_params(dev, &pdata->num_out_pads, >> + &pdata->num_in_pads); >> + if (error) { >> + dev_err(dev, "failed to parse keypad params\n"); >> + return error; >> + } >> + >> + of_property_read_u32(np, "st,debounce_us", &pdata->debounce_us); >> + >> + st_kp->config = pdata; >> + >> + dev_info(dev, "rows=%d col=%d debounce=%d\n", >> + pdata->num_out_pads, >> + pdata->num_in_pads, >> + pdata->debounce_us); >> + >> + error = of_property_read_u32_array(np, "linux,keymap", >> + st_kp->keycodes, ST_KEYSCAN_MAXKEYS); >> + if (error) { >> + dev_err(dev, "failed to parse keymap\n"); >> + return error; >> + } > Please use standard format for matrix keymap so that you can use > matrix_keypad_build_keymap(). ok > >> + >> + return 0; >> +} >> + >> +static int __init keyscan_probe(struct platform_device *pdev) >> +{ >> + struct keypad_platform_data *pdata = >> + dev_get_platdata(&pdev->dev); > const struct ... ok > >> + struct keyscan_priv *st_kp; >> + struct input_dev *input_dev; >> + struct device *dev = &pdev->dev; >> + struct resource *res; >> + int len; >> + int error; >> + int i; >> + >> + if (!pdata && !pdev->dev.of_node) { >> + dev_err(&pdev->dev, "no keymap defined\n"); >> + return -EINVAL; >> + } >> + >> + st_kp = devm_kzalloc(dev, sizeof(*st_kp), GFP_KERNEL); >> + if (!st_kp) { >> + dev_err(dev, "failed to allocate driver data\n"); >> + return -ENOMEM; >> + } >> + st_kp->config = pdata; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_err(dev, "no I/O memory specified\n"); >> + return -ENXIO; >> + } >> + >> + len = resource_size(res); >> + if (!devm_request_mem_region(dev, res->start, len, pdev->name)) { >> + dev_err(dev, "failed to reserve I/O memory\n"); >> + return -EBUSY; >> + } >> + >> + st_kp->base = devm_ioremap_nocache(dev, res->start, len); >> + if (st_kp->base == NULL) { >> + dev_err(dev, "failed to remap I/O memory\n"); >> + return -ENXIO; >> + } >> + >> + st_kp->irq = platform_get_irq(pdev, 0); >> + if (st_kp->irq < 0) { >> + dev_err(dev, "no IRQ specified\n"); >> + return -ENXIO; >> + } >> + >> + error = devm_request_irq(dev, st_kp->irq, keyscan_isr, 0, >> + pdev->name, pdev); >> + if (error) { >> + dev_err(dev, "failed to request IRQ\n"); >> + return error; >> + } >> + >> + input_dev = devm_input_allocate_device(&pdev->dev); >> + if (!input_dev) { >> + dev_err(&pdev->dev, "failed to allocate the input device\n"); >> + return -ENOMEM; >> + } >> + >> + st_kp->clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(st_kp->clk)) { >> + dev_err(dev, "cannot get clock"); >> + return PTR_ERR(st_kp->clk); >> + } >> + >> + input_dev = input_allocate_device(); >> + if (!input_dev) { >> + dev_err(dev, "failed to allocate input device\n"); >> + return -ENOMEM; >> + } > You already allocated it once a few lines above. right ** > >> + st_kp->input_dev = input_dev; >> + >> + input_dev->name = pdev->name; >> + input_dev->phys = "keyscan-keys/input0"; >> + input_dev->dev.parent = dev; >> + >> + input_dev->id.bustype = BUS_HOST; >> + input_dev->id.vendor = 0x0001; >> + input_dev->id.product = 0x0001; >> + input_dev->id.version = 0x0100; >> + >> + if (!pdata) { >> + error = keypad_matrix_key_parse_dt(st_kp); >> + if (error) >> + return error; >> + pdata = st_kp->config; >> + } >> + >> + input_dev->keycode = st_kp->keycodes; >> + input_dev->keycodesize = sizeof(st_kp->keycodes[0]); >> + input_dev->keycodemax = ARRAY_SIZE(st_kp->keycodes); >> + >> + for (i = 0; i < ST_KEYSCAN_MAXKEYS; i++) >> + input_set_capability(input_dev, EV_KEY, st_kp->keycodes[i]); >> + __clear_bit(KEY_RESERVED, input_dev->keybit); >> + >> + error = input_register_device(input_dev); >> + if (error) { >> + dev_err(dev, "failed to register input device\n"); >> + input_free_device(input_dev); >> + platform_set_drvdata(pdev, NULL); >> + return error; >> + } >> + >> + platform_set_drvdata(pdev, st_kp); >> + >> + keyscan_start(st_kp); >> + >> + device_set_wakeup_capable(&pdev->dev, 1); >> + >> + return 0; >> +} >> + >> +static int __exit keyscan_remove(struct platform_device *pdev) > Why is this marked as __exit? I can unbind device from driver without > unloading module (via sysfs). yes you are right >> +{ >> + struct keyscan_priv *priv = platform_get_drvdata(pdev); >> + >> + keyscan_stop(priv); > Call to keyscan_stop() shoudl go into keyscan_close() implementation. ok > >> + >> + input_unregister_device(priv->input_dev); > Not needed since you are trying to use devres-managed input device. ok > >> + platform_set_drvdata(pdev, NULL); > Not needed. In fact, if you move keyscan_stop() into keyscan_close() you > should be able to get rid of keyscan_remove(). ok > >> + >> + return 0; >> +} >> + >> +static int keyscan_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct keyscan_priv *priv = platform_get_drvdata(pdev); >> + >> + if (device_may_wakeup(dev)) >> + enable_irq_wake(priv->irq); >> + else >> + keyscan_stop(priv); >> + >> + return 0; >> +} >> + >> +static int keyscan_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct keyscan_priv *priv = platform_get_drvdata(pdev); >> + >> + if (device_may_wakeup(dev)) >> + disable_irq_wake(priv->irq); >> + else >> + keyscan_start(priv); >> + >> + return 0; >> +} > Guard suspend/resume with CONFIG_PM_SLEEP? i will check this point. > >> + >> +static const struct dev_pm_ops keyscan_dev_pm_ops = { >> + .suspend = keyscan_suspend, >> + .resume = keyscan_resume, >> +}; > Make it SIMPLE_DEV_PM_OPS(). ok > >> + >> +static const struct of_device_id keyscan_of_match[] = { >> + { .compatible = "st,keypad" }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, keyscan_of_match); >> + >> +__refdata struct platform_driver keyscan_device_driver = { > What is this refdata business? sorry, forgot this line... > >> + .probe = keyscan_probe, >> + .remove = __exit_p(keyscan_remove), >> + .driver = { >> + .name = "st-keyscan", >> + .pm = &keyscan_dev_pm_ops, >> + .of_match_table = of_match_ptr(keyscan_of_match), >> + } >> +}; >> + >> +static int __init keyscan_init(void) >> +{ >> + return platform_driver_register(&keyscan_device_driver); >> +} >> + >> +static void __exit keyscan_exit(void) >> +{ >> + platform_driver_unregister(&keyscan_device_driver); >> +} >> + >> +module_init(keyscan_init); >> +module_exit(keyscan_exit); > I think we have module_platform_drriver() for this. ok > >> + >> +MODULE_AUTHOR("Stuart Menefy "); >> +MODULE_DESCRIPTION("STMicroelectronics keyscan device driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 1.9.0 >> > Thanks. >