* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-05 3:39 ` Gabriel FERNANDEZ
0 siblings, 0 replies; 51+ messages in thread
From: Gabriel FERNANDEZ @ 2014-03-05 3:39 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely
Cc: devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Lee Jones, Gabriel Fernandez,
Giuseppe Condorelli
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;
+ }
+
+ 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");
--
1.9.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-05 3:39 ` Gabriel FERNANDEZ
0 siblings, 0 replies; 51+ messages in thread
From: Gabriel FERNANDEZ @ 2014-03-05 3:39 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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");
--
1.9.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-05 3:39 ` Gabriel FERNANDEZ
@ 2014-03-05 6:46 ` Dmitry Torokhov
-1 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-05 6:46 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Rob Landley, Russell King, Grant Likely, devicetree, linux-doc,
linux-kernel, linux-arm-kernel, linux-input, kernel, Lee Jones,
Giuseppe Condorelli
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.
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-05 6:46 ` Dmitry Torokhov
0 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-05 6:46 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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.
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-05 6:46 ` Dmitry Torokhov
(?)
@ 2014-03-07 4:51 ` Gabriel Fernandez
-1 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-07 4:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Rob Landley, Russell King, Grant Likely, devicetree, linux-doc,
linux-kernel, linux-arm-kernel, linux-input, kernel, Lee Jones,
Giuseppe Condorelli
[-- Attachment #1: Type: text/plain, Size: 14359 bytes --]
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 <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().
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 <stuart.menefy@st.com>");
>> +MODULE_DESCRIPTION("STMicroelectronics keyscan device driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 1.9.0
>>
> Thanks.
>
[-- Attachment #2: Type: text/html, Size: 17173 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-05 3:39 ` Gabriel FERNANDEZ
@ 2014-03-10 11:48 ` Lee Jones
-1 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 11:48 UTC (permalink / raw)
To: Gabriel FERNANDEZ
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
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>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-10 11:48 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 11:48 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-10 11:48 ` Lee Jones
@ 2014-03-10 15:28 ` Dmitry Torokhov
-1 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-10 15:28 UTC (permalink / raw)
To: Lee Jones
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
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.
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-10 15:28 ` Dmitry Torokhov
0 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-10 15:28 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-10 15:28 ` Dmitry Torokhov
@ 2014-03-10 15:38 ` Lee Jones
-1 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 15:38 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
> > > 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.
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-10 15:38 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 15:38 UTC (permalink / raw)
To: linux-arm-kernel
> > > 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.
[...]
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-10 15:38 ` Lee Jones
@ 2014-03-10 15:58 ` Dmitry Torokhov
-1 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-10 15:58 UTC (permalink / raw)
To: Lee Jones
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
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.
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-10 15:58 ` Dmitry Torokhov
0 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-10 15:58 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-10 15:58 ` Dmitry Torokhov
(?)
@ 2014-03-10 16:35 ` Lee Jones
-1 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 16:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
> > > > > 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.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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] 51+ messages in thread
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-10 16:35 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 16:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Gabriel FERNANDEZ, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
> > > > > 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.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-10 16:35 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-10 16:35 UTC (permalink / raw)
To: linux-arm-kernel
> > > > > 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 at vger.kernel.org with me in CC for more clarification if
required.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-10 11:48 ` Lee Jones
(?)
@ 2014-03-14 10:13 ` Gabriel Fernandez
-1 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-14 10:13 UTC (permalink / raw)
To: Lee Jones
Cc: Mark Rutland, devicetree, Russell King, kernel, Pawel Moll,
Ian Campbell, Dmitry Torokhov, linux-doc, linux-kernel,
Rob Herring, Giuseppe Condorelli, Rob Landley, Kumar Gala,
Grant Likely, linux-input, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-14 10:13 ` Gabriel Fernandez
0 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-14 10:13 UTC (permalink / raw)
To: Lee Jones
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
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
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-14 10:13 ` Gabriel Fernandez
0 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-14 10:13 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-14 10:13 ` Gabriel Fernandez
(?)
@ 2014-03-14 10:42 ` Lee Jones
-1 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-14 10:42 UTC (permalink / raw)
To: Gabriel Fernandez
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
> >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)?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-14 10:42 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-14 10:42 UTC (permalink / raw)
To: Gabriel Fernandez
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
> >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)?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-14 10:42 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-14 10:42 UTC (permalink / raw)
To: linux-arm-kernel
> >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 at 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)?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-14 10:42 ` Lee Jones
(?)
@ 2014-03-18 10:25 ` Gabriel Fernandez
-1 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-18 10:25 UTC (permalink / raw)
To: Lee Jones
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-18 10:25 ` Gabriel Fernandez
0 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-18 10:25 UTC (permalink / raw)
To: Lee Jones
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
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
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-18 10:25 ` Gabriel Fernandez
0 siblings, 0 replies; 51+ messages in thread
From: Gabriel Fernandez @ 2014-03-18 10:25 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-18 10:25 ` Gabriel Fernandez
@ 2014-03-18 11:01 ` Lee Jones
-1 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-18 11:01 UTC (permalink / raw)
To: Gabriel Fernandez
Cc: Dmitry Torokhov, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Rob Landley, Russell King, Grant Likely,
devicetree, linux-doc, linux-kernel, linux-arm-kernel,
linux-input, kernel, Giuseppe Condorelli
> >>>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>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-18 11:01 ` Lee Jones
0 siblings, 0 replies; 51+ messages in thread
From: Lee Jones @ 2014-03-18 11:01 UTC (permalink / raw)
To: linux-arm-kernel
> >>>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 at 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>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
2014-03-14 10:13 ` Gabriel Fernandez
@ 2014-03-14 15:26 ` Dmitry Torokhov
-1 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-14 15:26 UTC (permalink / raw)
To: Gabriel Fernandez
Cc: Lee Jones, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, Rob Landley, Russell King, Grant Likely, devicetree,
linux-doc, linux-kernel, linux-arm-kernel, linux-input, kernel,
Giuseppe Condorelli
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]$
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread* [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
@ 2014-03-14 15:26 ` Dmitry Torokhov
0 siblings, 0 replies; 51+ messages in thread
From: Dmitry Torokhov @ 2014-03-14 15:26 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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 at dtor-d630 work]$ git log --oneline --
drivers/input/keyboard/sh_keysc.c | wc -l
31
[dtor at dtor-d630 work]$
--
Dmitry
^ permalink raw reply [flat|nested] 51+ messages in thread