| Message ID | 1393990772-9567-2-git-send-email-gabriel.fernandez@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <giuseppe.condorelli@st.com> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > --- > .../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 = <GIC_SPI 212 IRQ_TYPE_NONE>; > + 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 <stuart.menefy@st.com> > + * > + * 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 <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/input/matrix_keypad.h> > + > +#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(). > + > + return 0; > +} > + > +static int __init keyscan_probe(struct platform_device *pdev) > +{ > + struct keypad_platform_data *pdata = > + dev_get_platdata(&pdev->dev); const struct ... > + 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. > + 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). > +{ > + struct keyscan_priv *priv = platform_get_drvdata(pdev); > + > + keyscan_stop(priv); Call to keyscan_stop() shoudl go into keyscan_close() implementation. > + > + input_unregister_device(priv->input_dev); Not needed since you are trying to use devres-managed input device. > + 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(). > + > + 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? > + > +static const struct dev_pm_ops keyscan_dev_pm_ops = { > + .suspend = keyscan_suspend, > + .resume = keyscan_resume, > +}; Make it SIMPLE_DEV_PM_OPS(). > + > +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? > + .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. > + > +MODULE_AUTHOR("Stuart Menefy <stuart.menefy@st.com>"); > +MODULE_DESCRIPTION("STMicroelectronics keyscan device driver"); > +MODULE_LICENSE("GPL"); > -- > 1.9.0 > Thanks.
Hi Gabi, Sorry for the delay. It was a hectic week last week. As promised: > 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 <giuseppe.condorelli@st.com> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> Are you sure these are in the correct order? What is the history of this commit? > --- > .../devicetree/bindings/input/st-keypad.txt | 50 ++++ This should be submitted as a seperate patch. > 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 s/device tree/Device Tree > + > +The ST keypad controller device tree binding is based on the As above. > +matrix-keymap. > + > +Required properties: > + > +- compatible: "st,keypad" I think there will be subsequent st,keypad drivers? Is there any way we can make this compatible string more specific to _this_ device? st,stih4xx-keypad? > +- reg: Register base address of st-keypad controler. s/address/address and size AND s/controler/controller > +- interrupts: Interrupt numberof st-keypad controler. s/numberof/number for the > +- clocks: Must contain one entry, for the module clock. > + See ../clocks/clock-bindings.txt for details. Great! > +- pinctrl-0: Should specify pin control groups used for this controller. > + > +- pinctrl-names: Should contain only one value - "default". Like to ../pinctrl/pinctrl-bindings.txt > +- 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 I'm sure there will be a shared binding for de-bounce. If not, there certainly should be. > + > + Superfluous new lines. > +Example: > + > +keyscan: keyscan@fe4b0000 { > + compatible = "st,keypad"; Is there any way we can make this more specific to _this_ IP? > + reg = <0xfe4b0000 0x2000>; > + interrupts = <GIC_SPI 212 IRQ_TYPE_NONE>; > + 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 */ Do these line up in the file? I know Git can be a bit funny about this sometimes. > + 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. Might be worth mentioning which ones. > + 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 <stuart.menefy@st.com> > + * > + * Based on sh_keysc.c, copyright 2008 Magnus Damm Did sh_keysc.c ever exist in Mainline? > + * 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 <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/input/matrix_keypad.h> > + > +#define ST_KEYSCAN_MAXKEYS 16 > + > +#define KEYSCAN_CONFIG_OFF 0x000 > +#define KEYSCAN_CONFIG_ENABLE 1 0x001? > +#define KEYSCAN_DEBOUNCE_TIME_OFF 0x004 > +#define KEYSCAN_MATRIX_STATE_OFF 0x008 > +#define KEYSCAN_MATRIX_DIM_OFF 0x00c Odd that these are 3 digit padded? Is there a reason for that? > +#define KEYSCAN_MATRIX_DIM_X_SHIFT 0 > +#define KEYSCAN_MATRIX_DIM_Y_SHIFT 2 For all 'ST_KEYSCAN_...'? > +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]; Seems odd to limit this. Can't the information come from DT i.e. keypad,num-rows and keypad,num-columns? > +}; > + > +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); int down = state & BIT(scancode); > + input_report_key(priv->input_dev, priv->keycodes[scancode], > + down); > + change ^= 1 << scancode; change ^= BIT(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"); If the system is OOM, this sort of error message will be pretty redundant. Just return -ENOMEM instead. > + 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; Nit: It's pretty unusual to use this for a standard error handling variable. Consider 'ret' or 'err' as a replacement. > + } > + > + of_property_read_u32(np, "st,debounce_us", &pdata->debounce_us); Isn't this a required property? Might be worth checking the return value if so. > + 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); Might be worth moving this down passed the final point of failure. > + 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; > + } > + > + return 0; > +} > + > +static int __init keyscan_probe(struct platform_device *pdev) > +{ > + struct keypad_platform_data *pdata = > + dev_get_platdata(&pdev->dev); Do we really support platform data still? > + struct keyscan_priv *st_kp; > + struct input_dev *input_dev; > + struct device *dev = &pdev->dev; dev and pdev are used randomly in probe(). Just remove the dev de-reference and use &pdev->dev exclusively. It's pretty common to de-reference 'np = pdev->dev.of_node' though. > + 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; I wouldn't print any messages on -ENOMEM personally. It's not a driver failure per se, but a system one where the user will soon be notified. > + } > + st_kp->config = pdata; You only want to do this in the !np case. > + 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; > + } Replace the two above with devm_ioremap_resource(). Make sure the IORESOURCE_CACHEABLE is set. > + st_kp->irq = platform_get_irq(pdev, 0); > + if (st_kp->irq < 0) { > + dev_err(dev, "no IRQ specified\n"); > + return -ENXIO; No such device or address, are you sure? Perhaps -EINVAL would be better, as one should be specified. > + } > + > + 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; > + } Wasn't this done already? > + 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; Any chance we can #define these? > + if (!pdata) { > + error = keypad_matrix_key_parse_dt(st_kp); > + if (error) > + return error; > + pdata = st_kp->config; Is pdata used after this? > + } > + > + 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); You don't need to do this anymore. It's taken care of elsewhere. > + 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) > +{ > + struct keyscan_priv *priv = platform_get_drvdata(pdev); > + > + keyscan_stop(priv); > + > + input_unregister_device(priv->input_dev); > + platform_set_drvdata(pdev, NULL); > + > + return 0; I already saw that Dmitry commented on the rest of the file. <snip>
Hi Lee, On Mon, Mar 10, 2014 at 11:48:19AM +0000, Lee Jones wrote: > Hi Gabi, > > Sorry for the delay. It was a hectic week last week. > > As promised: > > > 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 <giuseppe.condorelli@st.com> > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > > Are you sure these are in the correct order? > > What is the history of this commit? > > > --- > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++ > > This should be submitted as a seperate patch. Why do we have such requirement? To me it would make more sense to add binding documentation in the same commit as the code that uses these bindings. [...] > > + > > + 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; > > Nit: It's pretty unusual to use this for a standard error handling > variable. Consider 'ret' or 'err' as a replacement. I like "error", in fact there are a lot of these in input. I use "error" for data that is only returned from error path and "retval" when the same variable is returned in both success and error paths. > > + > > + input_dev->id.bustype = BUS_HOST; > > + input_dev->id.vendor = 0x0001; > > + input_dev->id.product = 0x0001; > > + input_dev->id.version = 0x0100; > > Any chance we can #define these? Even better would be not use 0x0001 as vendor as there (unfortunately) quite a few other drivers use it already. Either omit or chose something else. Does ST have PCI or USB VID assigned? Thanks.
> > > 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 <giuseppe.condorelli@st.com> > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > > > > Are you sure these are in the correct order? > > > > What is the history of this commit? > > > > > --- > > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++ > > > > This should be submitted as a seperate patch. > > Why do we have such requirement? To me it would make more sense to add > binding documentation in the same commit as the code that uses these > bindings. I'm inclined to agree with you and that's actually how we used to do it, but a decision was made by the DT guys at one of the Kernel Summits to submit Documentation as a separate patch. > [...] > > > > + > > > + 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; > > > > Nit: It's pretty unusual to use this for a standard error handling > > variable. Consider 'ret' or 'err' as a replacement. > > I like "error", in fact there are a lot of these in input. I use "error" for > data that is only returned from error path and "retval" when the same > variable is returned in both success and error paths. If that's your preference then I'm cool with it too. Scrap my comment. [...]
On Mon, Mar 10, 2014 at 03:38:15PM +0000, Lee Jones 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 <giuseppe.condorelli@st.com> > > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > > > > > > Are you sure these are in the correct order? > > > > > > What is the history of this commit? > > > > > > > --- > > > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++ > > > > > > This should be submitted as a seperate patch. > > > > Why do we have such requirement? To me it would make more sense to add > > binding documentation in the same commit as the code that uses these > > bindings. > > I'm inclined to agree with you and that's actually how we used to do > it, but a decision was made by the DT guys at one of the Kernel > Summits to submit Documentation as a separate patch. Do you have background for this decision? To me it is akin splitting header file into a separate patch.
> > > > > 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 <giuseppe.condorelli@st.com> > > > > > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > > > > > > > > Are you sure these are in the correct order? > > > > > > > > What is the history of this commit? > > > > > > > > > --- > > > > > .../devicetree/bindings/input/st-keypad.txt | 50 ++++ > > > > > > > > This should be submitted as a seperate patch. > > > > > > Why do we have such requirement? To me it would make more sense to add > > > binding documentation in the same commit as the code that uses these > > > bindings. > > > > I'm inclined to agree with you and that's actually how we used to do > > it, but a decision was made by the DT guys at one of the Kernel > > Summits to submit Documentation as a separate patch. > > Do you have background for this decision? To me it is akin splitting > header file into a separate patch. The discussion/decision was verbal unfortunately. I don't really mind either way. I'm just attempting to enforce the decisions that were made by the forces-that-be. Perhaps ping devicetree@vger.kernel.org with me in CC for more clarification if required.
Hi Lee, On 03/10/2014 12:48 PM, Lee Jones wrote: > Hi Gabi, > > Sorry for the delay. It was a hectic week last week. > > As promised: > >> 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 <giuseppe.condorelli@st.com> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > Are you sure these are in the correct order? ok i change the order >> +- 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 > I'm sure there will be a shared binding for de-bounce. > > If not, there certainly should be. you want to refer to "debounce-interval" ? > > +Example: > + > +keyscan: keyscan@fe4b0000 { > + compatible = "st,keypad"; > Is there any way we can make this more specific to _this_ IP? for my knowledge this IP is the same for stixxxx platform. > >> + 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 <stuart.menefy@st.com> >> + * >> + * Based on sh_keysc.c, copyright 2008 Magnus Damm > Did sh_keysc.c ever exist in Mainline? i think no, i 'll suppress "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 <linux/module.h> >> +#include <linux/interrupt.h> >> +#include <linux/platform_device.h> >> +#include <linux/clk.h> >> +#include <linux/io.h> >> +#include <linux/input/matrix_keypad.h> >> + >> +#define ST_KEYSCAN_MAXKEYS 16 >> + >> +#define KEYSCAN_CONFIG_OFF 0x000 >> +#define KEYSCAN_CONFIG_ENABLE 1 > 0x001? > >> +#define KEYSCAN_DEBOUNCE_TIME_OFF 0x004 >> +#define KEYSCAN_MATRIX_STATE_OFF 0x008 >> +#define KEYSCAN_MATRIX_DIM_OFF 0x00c > Odd that these are 3 digit padded? Is there a reason for that? no reason for the padded, i will change that. >> +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]; > Seems odd to limit this. Can't the information come from DT > i.e. keypad,num-rows and keypad,num-columns? > i 'll rename 'num_out_pads' into 'n_rows' and 'num_in_pads' into 'n_cols' > Nit: It's pretty unusual to use this for a standard error handling > variable. Consider 'ret' or 'err' as a replacement. > >> + } >> + >> + of_property_read_u32(np, "st,debounce_us", &pdata->debounce_us); > Isn't this a required property? Might be worth checking the return > value if so. no mandatory property, i will update the dt binding. > >> + 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); > Might be worth moving this down passed the final point of failure. > >> + 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; >> + } >> + >> + return 0; >> +} >> + >> +static int __init keyscan_probe(struct platform_device *pdev) >> +{ >> + struct keypad_platform_data *pdata = >> + dev_get_platdata(&pdev->dev); > Do we really support platform data still? no, i will remove that. > + 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; > + } > Replace the two above with devm_ioremap_resource(). > > Make sure the IORESOURCE_CACHEABLE is set. ok, we are in no cacheable use case. > >> + } >> + >> + 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; >> + } > Wasn't this done already? yes, crap these lines. >> + 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; > Any chance we can #define these? i will follow Dmitry remark (omit these information) > >> + if (!pdata) { >> + error = keypad_matrix_key_parse_dt(st_kp); >> + if (error) >> + return error; >> + pdata = st_kp->config; > Is pdata used after this? no, i will remove platform data support Thanks
> >Sorry for the delay. It was a hectic week last week. > > > >As promised: > > > >>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 <giuseppe.condorelli@st.com> > >>Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > >Are you sure these are in the correct order? > ok i change the order I'm not saying they are in the wrong order, I'm just asking. Who wrote the patch? Has it changed since? > >>+- 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 > >I'm sure there will be a shared binding for de-bounce. > > > >If not, there certainly should be. > > you want to refer to "debounce-interval" ? That sounds more generic, but if it's not documented as such, then please consider doing so. > >+Example: > >+ > >+keyscan: keyscan@fe4b0000 { > >+ compatible = "st,keypad"; > >Is there any way we can make this more specific to _this_ IP? > for my knowledge this IP is the same for stixxxx platform. So st,stix-keypad, or st,sti4x-keypad? I'm just thinking about future proofing the architecture. What if ST released stj which has a different keypad IP? > >>+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]; > >Seems odd to limit this. Can't the information come from DT > >i.e. keypad,num-rows and keypad,num-columns? > > > i 'll rename 'num_out_pads' into 'n_rows' and 'num_in_pads' into > 'n_cols' That's not quite what I meant, I mean can't ST_KEY_MAXKEYS be more dynamic and be obtained from (keypad,num-rows * keypad,num-columns)?
On Fri, Mar 14, 2014 at 11:13:17AM +0100, Gabriel Fernandez wrote: > Hi Lee, > > On 03/10/2014 12:48 PM, Lee Jones wrote: > >Hi Gabi, > > > >Sorry for the delay. It was a hectic week last week. > > > >As promised: > > > >>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 <giuseppe.condorelli@st.com> > >>Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com> > >Are you sure these are in the correct order? > ok i change the order > > >>+- 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 > >I'm sure there will be a shared binding for de-bounce. > > > >If not, there certainly should be. > > you want to refer to "debounce-interval" ? > > > > >+Example: > >+ > >+keyscan: keyscan@fe4b0000 { > >+ compatible = "st,keypad"; > >Is there any way we can make this more specific to _this_ IP? > for my knowledge this IP is the same for stixxxx platform. > > > > > >>+ 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 <stuart.menefy@st.com> > >>+ * > >>+ * Based on sh_keysc.c, copyright 2008 Magnus Damm > >Did sh_keysc.c ever exist in Mainline? > i think no, i 'll suppress "Based on sh_keysc.c, copyright 2008 Magnus Damm" It is still in here: [dtor@dtor-d630 work]$ git log --oneline -- drivers/input/keyboard/sh_keysc.c | wc -l 31 [dtor@dtor-d630 work]$
Hi Lee, On 03/14/2014 11:42 AM, Lee Jones wrote: >>> Sorry for the delay. It was a hectic week last week. >>> >>> As promised: >>> >>>> 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<giuseppe.condorelli@st.com> >>>> Signed-off-by: Gabriel Fernandez<gabriel.fernandez@st.com> >>> Are you sure these are in the correct order? >> ok i change the order > I'm not saying they are in the wrong order, I'm just asking. Who wrote > the patch? Has it changed since? Sorry... I wrote the patch, then Guiseppe has changed and tested, and I re-modifiedit >>>> +- 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 >>> I'm sure there will be a shared binding for de-bounce. >>> >>> If not, there certainly should be. >> you want to refer to "debounce-interval" ? > That sounds more generic, but if it's not documented as such, then > please consider doing so. > >>> +Example: >>> + >>> +keyscan: keyscan@fe4b0000 { >>> + compatible = "st,keypad"; >>> Is there any way we can make this more specific to _this_ IP? >> for my knowledge this IP is the same for stixxxx platform. > So st,stix-keypad, or st,sti4x-keypad? > > I'm just thinking about future proofing the architecture. What if ST > released stj which has a different keypad IP? After discussing internally with st "st,sti-keyscan" is better >>>> +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]; >>> Seems odd to limit this. Can't the information come from DT >>> i.e. keypad,num-rows and keypad,num-columns? >>> >> i 'll rename 'num_out_pads' into 'n_rows' and 'num_in_pads' into >> 'n_cols' > That's not quite what I meant, I mean can't ST_KEY_MAXKEYS be more > dynamic and be obtained from (keypad,num-rows * keypad,num-columns)? ok
> >>>Sorry for the delay. It was a hectic week last week. > >>> > >>>As promised: > >>> > >>>>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<giuseppe.condorelli@st.com> > >>>>Signed-off-by: Gabriel Fernandez<gabriel.fernandez@st.com> > >>>Are you sure these are in the correct order? > >>ok i change the order > >I'm not saying they are in the wrong order, I'm just asking. Who wrote > >the patch? Has it changed since? > Sorry... > I wrote the patch, then Guiseppe has changed and tested, and I re-modifiedit Ah, then it starts to get very complicated. :) If you wrote the patch, you need to be at the top of the list. <snip> > >>>+keyscan: keyscan@fe4b0000 { > >>>+ compatible = "st,keypad"; > >>>Is there any way we can make this more specific to _this_ IP? > >>for my knowledge this IP is the same for stixxxx platform. > >So st,stix-keypad, or st,sti4x-keypad? > > > >I'm just thinking about future proofing the architecture. What if ST > >released stj which has a different keypad IP? > After discussing internally with st "st,sti-keyscan" is better Perfect. <snip>
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 = <GIC_SPI 212 IRQ_TYPE_NONE>; + 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 <stuart.menefy@st.com> + * + * 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 <linux/module.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/input/matrix_keypad.h> + +#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; + } + + return 0; +} + +static int __init keyscan_probe(struct platform_device *pdev) +{ + struct keypad_platform_data *pdata = + dev_get_platdata(&pdev->dev); + 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; + } + 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) +{ + struct keyscan_priv *priv = platform_get_drvdata(pdev); + + keyscan_stop(priv); + + input_unregister_device(priv->input_dev); + platform_set_drvdata(pdev, NULL); + + 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; +} + +static const struct dev_pm_ops keyscan_dev_pm_ops = { + .suspend = keyscan_suspend, + .resume = keyscan_resume, +}; + +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 = { + .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); + +MODULE_AUTHOR("Stuart Menefy <stuart.menefy@st.com>"); +MODULE_DESCRIPTION("STMicroelectronics keyscan device driver"); +MODULE_LICENSE("GPL");