Re: [PATCH 1/5] drivers: input: keyboard: st-keyscan: add keyscan driver
From: Gabriel Fernandez
Date: Fri Mar 14 2014 - 06:52:00 EST
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@xxxxxx>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
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@xxxxxx>
+ *
+ * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/