[PATCH V3] add mtk matrix keypad driver for keypad on MTK SoC

[PATCH V3] add mtk matrix keypad driver for keypad on MTK SoC

[fengping.yu]

Change since V2:
- add wakeup source optional property in dts node
- update debounce time as microsecond
- remove origin hw-map-num and hw-init-map properties
- use matrix-map to define keypad rows, columns and keymap
- remove tasklet and use thread irq to deal with keypad interrupt
- use devm_XXX API to get device resources
- modify dts parser using matrix-map API
- modify irq handlers using for_each_set_bit to detect which key status changed
- remove surplus log 
- remove surpluse header file
- replace readw with memcpy_fromio to get keypad status registers value
- fixed other issue according reviewer comments

fengping.yu (2):
  dt-bindings: add matrix keypad documentation
  drivers: input: keyboard: add mtk keypad driver

 .../devicetree/bindings/input/mtk-kpd.txt     |  55 ++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/input/keyboard/Kconfig                |   8 +
 drivers/input/keyboard/Makefile               |   1 +
 drivers/input/keyboard/mtk-kpd.c              | 280 ++++++++++++++++++
 5 files changed, 345 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.txt
 create mode 100644 drivers/input/keyboard/mtk-kpd.c
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V3 1/2] dt-bindings: add matrix keypad documentation

[fengping.yu]

Signed-off-by: fengping.yu <fengping.yu@mediatek.com>

Add matrix keypad binding documentation for keypad on MTK SoC.

---
 .../devicetree/bindings/input/mtk-kpd.txt     | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.txt

diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.txt b/Documentation/devicetree/bindings/input/mtk-kpd.txt
new file mode 100644
index 000000000000..fda744decbb2
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/mtk-kpd.txt
@@ -0,0 +1,55 @@
+* Mediatek's Keypad Controller device tree binding
+
+Mediatek's Keypad controller is used to interface a SoC with a matrix-type
+keypad device. The keypad controller supports multiple row and column lines.
+A key can be placed at each intersection of a unique row and a unique column.
+The keypad controller can sense a key-press and key-release and report the
+event using a interrupt to the cpu.
+
+Required properties:
+- compatible should contain:
+	* "mediatek,kp" for common keypad
+
+- reg: The base address of the Keypad register bank.
+
+- interrupts: A single interrupt specifier.
+
+- mediatek,debounce-us: Debounce interval in microseconds, maximum value
+  is 256000ms
+
+- keypad,num-rows: Number of row lines connected to the keypad controller
+
+- keypad,num-columns: Number of column lines connected to the keypad controller
+
+- linux,keymap: The keymap for keys as described in the binding document
+  devicetree/bindings/input/matrix-keymap.txt.
+
+- pinctrl: Should specify pin control groups used for this controller.
+  See ../pinctrl/pinctrl-bindings.txt for details.
+
+- clocks: Must contain one entry, for the module clock.
+  See ../clocks/clock-bindings.txt for details.
+
+Optional Properties:
+- wakeup-source: use any event on keypad as wakeup event.
+
+Example:
+
+	keypad: kp@10010000 {
+		compatible = "mediatek,kp";
+		reg = <0 0x10010000 0 0x1000>;
+		wakeup-source;
+		interrupts = <GIC_SPI 75 IRQ_TYPE_EDGE_FALLING>;
+		clocks = <&clk26m>;
+		clock-names = "kpd";
+	};
+
+	&keypad {
+		mediatek,debounce-us = <32000>;
+		keypad,num-rows = <8>;
+		keypad,num-columns = <9>;
+		linux,keymap = < MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN) >;
+		status = "okay";
+		pinctrl-names = "default";
+		pinctrl-0 = <&kpd_gpios_def_cfg>;
+	};
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V3 2/2] drivers: input: keyboard: add mtk keypad driver

[fengping.yu]

Signed-off-by: fengping.yu <fengping.yu@mediatek.com>

Add matrix keypad driver for MTK SoC.

---
 arch/arm64/configs/defconfig     |   1 +
 drivers/input/keyboard/Kconfig   |   8 +
 drivers/input/keyboard/Makefile  |   1 +
 drivers/input/keyboard/mtk-kpd.c | 280 +++++++++++++++++++++++++++++++
 4 files changed, 290 insertions(+)
 create mode 100644 drivers/input/keyboard/mtk-kpd.c

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 8e05c39eab08..62bed77ec127 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -315,6 +315,7 @@ CONFIG_KEYBOARD_ADC=m
 CONFIG_KEYBOARD_GPIO=y
 CONFIG_KEYBOARD_SNVS_PWRKEY=m
 CONFIG_KEYBOARD_CROS_EC=y
+CONFIG_KEYBOARD_MTK=y
 CONFIG_INPUT_TOUCHSCREEN=y
 CONFIG_TOUCHSCREEN_ATMEL_MXT=m
 CONFIG_INPUT_MISC=y
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 8911bc2ec42a..01747a97536b 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -775,4 +775,12 @@ config KEYBOARD_MTK_PMIC
 	  To compile this driver as a module, choose M here: the
 	  module will be called pmic-keys.
 
+config KEYBOARD_MTK
+	tristate "MediaTek Keypad Support"
+	help
+	  Say Y here if you want to use the keypad.
+	  If unuse, say N.
+	  To compile this driver as a module, choose M here: the
+	  module will be called mtk-kpd.
+
 endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 9510325c0c5d..daa654bcce6e 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
 obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
 obj-$(CONFIG_KEYBOARD_MCS)		+= mcs_touchkey.o
 obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121_touchkey.o
+obj-$(CONFIG_KEYBOARD_MTK) 		+= mtk-kpd.o
 obj-$(CONFIG_KEYBOARD_MTK_PMIC) 	+= mtk-pmic-keys.o
 obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
 obj-$(CONFIG_KEYBOARD_NOMADIK)		+= nomadik-ske-keypad.o
diff --git a/drivers/input/keyboard/mtk-kpd.c b/drivers/input/keyboard/mtk-kpd.c
new file mode 100644
index 000000000000..e36461c9dd89
--- /dev/null
+++ b/drivers/input/keyboard/mtk-kpd.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 MediaTek Inc.
+ * Author Terry Chang <terry.chang@mediatek.com>
+ */
+#include <linux/clk.h>
+#include <linux/fs.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define KPD_NAME	"mtk-kpd"
+
+#define KP_STA				0x0000
+#define KP_MEM1				0x0004
+#define KP_MEM2				0x0008
+#define KP_MEM3				0x000c
+#define KP_MEM4				0x0010
+#define KP_MEM5				0x0014
+#define KP_DEBOUNCE			0x0018
+#define KP_SEL				0x0020
+#define KP_EN				0x0024
+
+#define KP_COL0_SEL		BIT(10)
+#define KP_COL1_SEL		BIT(11)
+#define KP_COL2_SEL		BIT(12)
+
+#define KPD_DEBOUNCE_MASK	GENMASK_ULL(13, 0)
+
+#define KPD_NUM_MEMS	5
+#define KPD_MEM5_BITS	8
+#define KPD_NUM_KEYS	72	/* 4 * 16 + KPD_MEM5_BITS */
+
+struct mtk_keypad {
+	struct input_dev *input_dev;
+	struct clk *clk;
+	void __iomem *base;
+	unsigned int irqnr;
+	bool wakeup;
+	u32 key_debounce;
+	u32 n_rows;
+	u32 n_cols;
+	u32 keymap_state[KPD_NUM_MEMS];
+};
+
+static void kpd_get_keymap_state(void __iomem *kp_base, u32 state[])
+{
+	memset_io(state, 0xff, KPD_NUM_MEMS);
+	memcpy_fromio(state, kp_base, KPD_NUM_MEMS);
+}
+
+static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
+{
+	/* use _nosync to avoid deadlock */
+	struct mtk_keypad *keypad = dev_id;
+	unsigned short *keycode = keypad->input_dev->keycode;
+	u32 new_state[KPD_NUM_MEMS], mask;
+	unsigned long change = 0;
+	int bit_nr;
+	int pressed;
+	u32 row_shift = get_count_order(keypad->n_cols);
+	unsigned short code;
+	int i;
+
+	disable_irq_nosync(keypad->irqnr);
+
+	kpd_get_keymap_state(keypad->base, new_state);
+
+	for (i = 0; i < KPD_NUM_MEMS; i++) {
+		change = new_state[i] ^ keypad->keymap_state[i];
+		if (!change)
+			continue;
+
+		for_each_set_bit(bit_nr, &change, 32) {
+			mask = 1 << bit_nr;
+			pressed = (new_state[i] & mask) == 0U;
+			dev_dbg(&keypad->input_dev->dev,
+				"%s", pressed ? "pressed" : "released");
+
+			code = keycode[MATRIX_SCAN_CODE(i, bit_nr, row_shift)];
+
+			input_report_key(keypad->input_dev, code, pressed);
+			input_sync(keypad->input_dev);
+
+			dev_dbg(&keypad->input_dev->dev,
+				"report Linux keycode = %d\n", code);
+		}
+	}
+
+	memcpy(keypad->keymap_state, new_state, sizeof(new_state));
+
+	enable_irq(keypad->irqnr);
+
+	return IRQ_HANDLED;
+}
+
+static int kpd_get_dts_info(struct mtk_keypad *keypad)
+{
+	int ret;
+	struct device *dev = keypad->input_dev->dev.parent;
+	struct device_node *node = dev->of_node;
+
+	ret = matrix_keypad_parse_properties(dev, &keypad->n_rows,
+					     &keypad->n_cols);
+
+	if (ret) {
+		dev_err(dev, "failed to parse keypad params.\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(node, "mediatek,debounce-us",
+				   &keypad->key_debounce);
+	if (ret) {
+		pr_debug("read mediatek,debounce-us error.\n");
+		return ret;
+	}
+
+	keypad->wakeup = of_property_read_bool(node, "wakeup-source");
+
+	dev_dbg(dev, "n_row=%d n_col=%d debounce=%d.\n",
+		keypad->n_rows, keypad->n_cols,
+		keypad->key_debounce);
+
+	return 0;
+}
+
+static int kpd_gpio_init(struct device *dev)
+{
+	struct pinctrl *keypad_pinctrl;
+	struct pinctrl_state *kpd_default;
+
+	keypad_pinctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(keypad_pinctrl)) {
+		dev_err(dev, "Cannot find keypad_pinctrl!\n");
+
+		return PTR_ERR(keypad_pinctrl);
+	}
+
+	kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
+	if (IS_ERR(kpd_default)) {
+		dev_err(dev, "Cannot find ecall_state!\n");
+
+		return PTR_ERR(kpd_default);
+	}
+
+	return pinctrl_select_state(keypad_pinctrl, kpd_default);
+}
+
+static int kpd_pdrv_probe(struct platform_device *pdev)
+{
+	struct mtk_keypad *keypad;
+	struct resource *res;
+	int err;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "No device tree data present.\n");
+		return -EINVAL;
+	}
+
+	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
+	if (!keypad)
+		return -ENOMEM;
+
+	memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));
+
+	keypad->input_dev = devm_input_allocate_device(&pdev->dev);
+	if (!keypad->input_dev) {
+		dev_err(&pdev->dev, "input allocate device fail.\n");
+		return -ENOMEM;
+	}
+
+	keypad->input_dev->name = KPD_NAME;
+	keypad->input_dev->id.bustype = BUS_HOST;
+	keypad->input_dev->dev.parent = &pdev->dev;
+
+	err = kpd_get_dts_info(keypad);
+	if (err) {
+		dev_err(&pdev->dev, "get dts info failed.\n");
+		return err;
+	}
+
+	err = matrix_keypad_build_keymap(NULL, NULL,
+					keypad->n_rows,
+					keypad->n_cols,
+					NULL,
+					keypad->input_dev);
+
+	if (err) {
+		dev_err(&pdev->dev, "build keymap failed.\n");
+		return err;
+	}
+
+	input_set_drvdata(keypad->input_dev, keypad);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "get IO memory resource failed.\n");
+		return -ENODEV;
+	}
+
+	keypad->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(keypad->base)) {
+		dev_err(&pdev->dev, "KP iomap failed\n");
+		return PTR_ERR(keypad->base);
+	}
+
+	writew((keypad->key_debounce * 32 / 1000 & KPD_DEBOUNCE_MASK),
+		keypad->base + KP_DEBOUNCE);
+
+	keypad->clk = devm_clk_get(&pdev->dev, "kpd");
+	if (IS_ERR(keypad->clk)) {
+		dev_err(&pdev->dev, "get kpd-clk fail.\n");
+		return PTR_ERR(keypad->clk);
+	}
+
+	err = clk_prepare_enable(keypad->clk);
+	if (err) {
+		dev_err(&pdev->dev, "kpd-clk prepare enable failed.\n");
+		return err;
+	}
+
+	err = kpd_gpio_init(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "gpio init failed\n");
+		return err;
+	}
+
+	keypad->irqnr = platform_get_irq(pdev, 0);
+	if (!keypad->irqnr) {
+		dev_err(&pdev->dev, "KP get irqnr failed\n");
+		return -EINVAL;
+	}
+
+	err = devm_request_irq(&pdev->dev, keypad->irqnr,
+				kpd_irq_handler, 0,
+				KPD_NAME, keypad);
+	if (err) {
+		dev_err(&pdev->dev, "register IRQ failed.\n");
+		return err;
+	}
+
+	err = input_register_device(keypad->input_dev);
+	if (err) {
+		dev_err(&pdev->dev, "register input device failed.\n");
+		return err;
+	}
+
+	device_init_wakeup(&pdev->dev, keypad->wakeup);
+
+	platform_set_drvdata(pdev, keypad);
+
+	return 0;
+}
+
+static const struct of_device_id kpd_of_match[] = {
+	{.compatible = "mediatek,kp"},
+	{}
+};
+
+static struct platform_driver kpd_pdrv = {
+	.probe = kpd_pdrv_probe,
+	.driver = {
+		   .name = KPD_NAME,
+		   .of_match_table = kpd_of_match,
+	},
+};
+
+module_platform_driver(kpd_pdrv);
+
+MODULE_AUTHOR("Mediatek Corporation");
+MODULE_DESCRIPTION("MTK Keypad (KPD) Driver");
+MODULE_LICENSE("GPL");
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] drivers: input: keyboard: add mtk keypad driver

[Andy Shevchenko]

On Tue, Jan 14, 2020 at 08:27:41PM +0800, fengping.yu wrote:
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> 
> Add matrix keypad driver for MTK SoC.

Something wrong with ordering.

...

> +obj-$(CONFIG_KEYBOARD_MTK) 		+= mtk-kpd.o

TABs / space mix. I think you ignored my comment.

...

> +#include <linux/clk.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

...

> +#define KPD_DEBOUNCE_MASK	GENMASK_ULL(13, 0)

ULL? Why?

...

> +static void kpd_get_keymap_state(void __iomem *kp_base, u32 state[])

u32 *state

> +{

> +	memset_io(state, 0xff, KPD_NUM_MEMS);

This is strange. What is this for and why is it needed?

> +	memcpy_fromio(state, kp_base, KPD_NUM_MEMS);
> +}

...

> +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> +{
> +	/* use _nosync to avoid deadlock */
> +	struct mtk_keypad *keypad = dev_id;
> +	unsigned short *keycode = keypad->input_dev->keycode;
> +	u32 new_state[KPD_NUM_MEMS], mask;
> +	unsigned long change = 0;
> +	int bit_nr;
> +	int pressed;
> +	u32 row_shift = get_count_order(keypad->n_cols);
> +	unsigned short code;
> +	int i;
> +
> +	disable_irq_nosync(keypad->irqnr);
> +
> +	kpd_get_keymap_state(keypad->base, new_state);
> +
> +	for (i = 0; i < KPD_NUM_MEMS; i++) {
> +		change = new_state[i] ^ keypad->keymap_state[i];
> +		if (!change)
> +			continue;
> +
> +		for_each_set_bit(bit_nr, &change, 32) {

> +			mask = 1 << bit_nr;
> +			pressed = (new_state[i] & mask) == 0U;

test_bit() ?

Can't new_state be unsigned long? In this case you may get rid of outer loop I
suppose.

> +			dev_dbg(&keypad->input_dev->dev,
> +				"%s", pressed ? "pressed" : "released");
> +
> +			code = keycode[MATRIX_SCAN_CODE(i, bit_nr, row_shift)];
> +
> +			input_report_key(keypad->input_dev, code, pressed);
> +			input_sync(keypad->input_dev);
> +
> +			dev_dbg(&keypad->input_dev->dev,
> +				"report Linux keycode = %d\n", code);
> +		}
> +	}
> +
> +	memcpy(keypad->keymap_state, new_state, sizeof(new_state));
> +
> +	enable_irq(keypad->irqnr);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int kpd_get_dts_info(struct mtk_keypad *keypad)
> +{
> +	int ret;
> +	struct device *dev = keypad->input_dev->dev.parent;
> +	struct device_node *node = dev->of_node;
> +
> +	ret = matrix_keypad_parse_properties(dev, &keypad->n_rows,
> +					     &keypad->n_cols);

> +

Redundant line.

> +	if (ret) {
> +		dev_err(dev, "failed to parse keypad params.\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "mediatek,debounce-us",
> +				   &keypad->key_debounce);
> +	if (ret) {

> +		pr_debug("read mediatek,debounce-us error.\n");

dev_dbg() ?

> +		return ret;
> +	}
> +
> +	keypad->wakeup = of_property_read_bool(node, "wakeup-source");
> +
> +	dev_dbg(dev, "n_row=%d n_col=%d debounce=%d.\n",
> +		keypad->n_rows, keypad->n_cols,
> +		keypad->key_debounce);
> +
> +	return 0;
> +}

...

> +static int kpd_gpio_init(struct device *dev)
> +{
> +	struct pinctrl *keypad_pinctrl;
> +	struct pinctrl_state *kpd_default;
> +
> +	keypad_pinctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(keypad_pinctrl)) {
> +		dev_err(dev, "Cannot find keypad_pinctrl!\n");

> +

Redundant line

> +		return PTR_ERR(keypad_pinctrl);
> +	}
> +
> +	kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> +	if (IS_ERR(kpd_default)) {
> +		dev_err(dev, "Cannot find ecall_state!\n");

> +

Ditto.

> +		return PTR_ERR(kpd_default);
> +	}
> +
> +	return pinctrl_select_state(keypad_pinctrl, kpd_default);
> +}

...

> +	int err;

err or ret? Be consistent with the name of variable that keeps returned code.

> +	if (!pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "No device tree data present.\n");
> +		return -EINVAL;
> +	}

I think this is redundant. below calls will fail when property is not found.

> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> +	if (!keypad)
> +		return -ENOMEM;
> +

> +	memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));

bitmap_fill() ?

> +	err = matrix_keypad_build_keymap(NULL, NULL,
> +					keypad->n_rows,
> +					keypad->n_cols,
> +					NULL,
> +					keypad->input_dev);


> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "get IO memory resource failed.\n");
> +		return -ENODEV;
> +	}
> +
> +	keypad->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

> +	if (IS_ERR(keypad->base)) {
> +		dev_err(&pdev->dev, "KP iomap failed\n");
> +		return PTR_ERR(keypad->base);
> +	}
> +
> +	writew((keypad->key_debounce * 32 / 1000 & KPD_DEBOUNCE_MASK),

Useless parentheses. It's not lisp!

> +		keypad->base + KP_DEBOUNCE);

> +	err = clk_prepare_enable(keypad->clk);
> +	if (err) {
> +		dev_err(&pdev->dev, "kpd-clk prepare enable failed.\n");
> +		return err;
> +	}
> +
> +	err = kpd_gpio_init(&pdev->dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "gpio init failed\n");

> +		return err;

Starting from here down the clocks left in a wrong state.

> +	}
> +
> +	keypad->irqnr = platform_get_irq(pdev, 0);

> +	if (!keypad->irqnr) {

This is wrong...

> +		dev_err(&pdev->dev, "KP get irqnr failed\n");

> +		return -EINVAL;

...and so does this.

> +	}

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 1/2] dt-bindings: add matrix keypad documentation

[Yingjoe Chen]

Hi Fengping,

Please add all people gave you comment in past series to CC list.


On Tue, 2020-01-14 at 20:27 +0800, fengping.yu wrote:
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> 
> Add matrix keypad binding documentation for keypad on MTK SoC.
> 
> ---
>  .../devicetree/bindings/input/mtk-kpd.txt     | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.txt b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> new file mode 100644
> index 000000000000..fda744decbb2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> @@ -0,0 +1,55 @@
> +* Mediatek's Keypad Controller device tree binding
> +
> +Mediatek's Keypad controller is used to interface a SoC with a matrix-type
> +keypad device. The keypad controller supports multiple row and column lines.
> +A key can be placed at each intersection of a unique row and a unique column.
> +The keypad controller can sense a key-press and key-release and report the
> +event using a interrupt to the cpu.
> +
> +Required properties:
> +- compatible should contain:
> +	* "mediatek,kp" for common keypad


Not every keypad are the same. It is OK to havethis  one for
common/basic support, but you should still add IC specific compatible.


> +
> +- reg: The base address of the Keypad register bank.
> +
> +- interrupts: A single interrupt specifier.
> +
> +- mediatek,debounce-us: Debounce interval in microseconds, maximum value
> +  is 256000ms

not ms/microseconds now.


> +
> +- keypad,num-rows: Number of row lines connected to the keypad controller
> +
> +- keypad,num-columns: Number of column lines connected to the keypad controller

These number will change how driver interpret register bits.
In this case, you should add required value for each IC.

For example, if the IC have 4 columns, but PCB only use 2, dts still
need to specify 4 columns.

> +
> +- linux,keymap: The keymap for keys as described in the binding document
> +  devicetree/bindings/input/matrix-keymap.txt.
> +
> +- pinctrl: Should specify pin control groups used for this controller.
> +  See ../pinctrl/pinctrl-bindings.txt for details.
> +
> +- clocks: Must contain one entry, for the module clock.
> +  See ../clocks/clock-bindings.txt for details.

Need to include required clock-names here.

Joe.C

> +
> +Optional Properties:
> +- wakeup-source: use any event on keypad as wakeup event.
> +
> +Example:
> +
> +	keypad: kp@10010000 {
> +		compatible = "mediatek,kp";
> +		reg = <0 0x10010000 0 0x1000>;
> +		wakeup-source;
> +		interrupts = <GIC_SPI 75 IRQ_TYPE_EDGE_FALLING>;
> +		clocks = <&clk26m>;
> +		clock-names = "kpd";
> +	};
> +
> +	&keypad {
> +		mediatek,debounce-us = <32000>;
> +		keypad,num-rows = <8>;
> +		keypad,num-columns = <9>;
> +		linux,keymap = < MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN) >;
> +		status = "okay";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&kpd_gpios_def_cfg>;
> +	};

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] drivers: input: keyboard: add mtk keypad driver

[Yingjoe Chen]

Fengpin,

Please address all comments in previous review, either change your code
or explain why you think it is better to keep it as-is.



On Tue, 2020-01-14 at 20:27 +0800, fengping.yu wrote:
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> 
> Add matrix keypad driver for MTK SoC.
> 
> ---
>  arch/arm64/configs/defconfig     |   1 +
>  drivers/input/keyboard/Kconfig   |   8 +
>  drivers/input/keyboard/Makefile  |   1 +
>  drivers/input/keyboard/mtk-kpd.c | 280 +++++++++++++++++++++++++++++++
>  4 files changed, 290 insertions(+)
>  create mode 100644 drivers/input/keyboard/mtk-kpd.c
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 8e05c39eab08..62bed77ec127 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -315,6 +315,7 @@ CONFIG_KEYBOARD_ADC=m
>  CONFIG_KEYBOARD_GPIO=y
>  CONFIG_KEYBOARD_SNVS_PWRKEY=m
>  CONFIG_KEYBOARD_CROS_EC=y
> +CONFIG_KEYBOARD_MTK=y
>  CONFIG_INPUT_TOUCHSCREEN=y
>  CONFIG_TOUCHSCREEN_ATMEL_MXT=m
>  CONFIG_INPUT_MISC=y

Please put defconfig to another patch.


> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 8911bc2ec42a..01747a97536b 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -775,4 +775,12 @@ config KEYBOARD_MTK_PMIC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pmic-keys.
>  
> +config KEYBOARD_MTK
> +	tristate "MediaTek Keypad Support"

We already have KEYBOARD_MTK_PMIC, I think it make sense to call this
KEYBOARD_MTK_KPD to avoid confusion.
Please move this before KEYBOARD_MTK_PMIC.


> +	help
> +	  Say Y here if you want to use the keypad.
> +	  If unuse, say N.
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called mtk-kpd.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 9510325c0c5d..daa654bcce6e 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
>  obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
>  obj-$(CONFIG_KEYBOARD_MCS)		+= mcs_touchkey.o
>  obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121_touchkey.o
> +obj-$(CONFIG_KEYBOARD_MTK) 		+= mtk-kpd.o
>  obj-$(CONFIG_KEYBOARD_MTK_PMIC) 	+= mtk-pmic-keys.o
>  obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
>  obj-$(CONFIG_KEYBOARD_NOMADIK)		+= nomadik-ske-keypad.o
> diff --git a/drivers/input/keyboard/mtk-kpd.c b/drivers/input/keyboard/mtk-kpd.c
> new file mode 100644
> index 000000000000..e36461c9dd89
> --- /dev/null
> +++ b/drivers/input/keyboard/mtk-kpd.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 MediaTek Inc.
> + * Author Terry Chang <terry.chang@mediatek.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

why do we need regmap.h?

> +
> +#define KPD_NAME	"mtk-kpd"
> +
> +#define KP_STA				0x0000
> +#define KP_MEM1				0x0004
> +#define KP_MEM2				0x0008
> +#define KP_MEM3				0x000c
> +#define KP_MEM4				0x0010
> +#define KP_MEM5				0x0014
> +#define KP_DEBOUNCE			0x0018
> +#define KP_SEL				0x0020
> +#define KP_EN				0x0024
> +
> +#define KP_COL0_SEL		BIT(10)
> +#define KP_COL1_SEL		BIT(11)
> +#define KP_COL2_SEL		BIT(12)

Not used in code, please remove.


> +
> +#define KPD_DEBOUNCE_MASK	GENMASK_ULL(13, 0)
> +
> +#define KPD_NUM_MEMS	5
> +#define KPD_MEM5_BITS	8
> +#define KPD_NUM_KEYS	72	/* 4 * 16 + KPD_MEM5_BITS */

these are no longer used. Please remove.


> +
> +struct mtk_keypad {
> +	struct input_dev *input_dev;
> +	struct clk *clk;
> +	void __iomem *base;
> +	unsigned int irqnr;
> +	bool wakeup;
> +	u32 key_debounce;
> +	u32 n_rows;
> +	u32 n_cols;
> +	u32 keymap_state[KPD_NUM_MEMS];
> +};
> +
> +static void kpd_get_keymap_state(void __iomem *kp_base, u32 state[])
> +{
> +	memset_io(state, 0xff, KPD_NUM_MEMS);
> +	memcpy_fromio(state, kp_base, KPD_NUM_MEMS);
> +}
> +
> +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> +{
> +	/* use _nosync to avoid deadlock */
> +	struct mtk_keypad *keypad = dev_id;
> +	unsigned short *keycode = keypad->input_dev->keycode;
> +	u32 new_state[KPD_NUM_MEMS], mask;
> +	unsigned long change = 0;
> +	int bit_nr;
> +	int pressed;
> +	u32 row_shift = get_count_order(keypad->n_cols);
> +	unsigned short code;
> +	int i;
> +
> +	disable_irq_nosync(keypad->irqnr);
> +
> +	kpd_get_keymap_state(keypad->base, new_state);
> +
> +	for (i = 0; i < KPD_NUM_MEMS; i++) {
> +		change = new_state[i] ^ keypad->keymap_state[i];
> +		if (!change)
> +			continue;
> +
> +		for_each_set_bit(bit_nr, &change, 32) {
> +			mask = 1 << bit_nr;
> +			pressed = (new_state[i] & mask) == 0U;
> +			dev_dbg(&keypad->input_dev->dev,
> +				"%s", pressed ? "pressed" : "released");
> +
> +			code = keycode[MATRIX_SCAN_CODE(i, bit_nr, row_shift)];

Although the register is 32bits wide, KP_MEM* only have 16bits data,
after changing to bit array & memcpy_fromio, you need to skip the extra
16bits in each register to get correct row/col counting.

MATRX_SCAN_CODE use row/col, using i & bit_nr doesn't looks right.

> +
> +			input_report_key(keypad->input_dev, code, pressed);
> +			input_sync(keypad->input_dev);
> +
> +			dev_dbg(&keypad->input_dev->dev,
> +				"report Linux keycode = %d\n", code);
> +		}
> +	}
> +
> +	memcpy(keypad->keymap_state, new_state, sizeof(new_state));
> +
> +	enable_irq(keypad->irqnr);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int kpd_get_dts_info(struct mtk_keypad *keypad)
> +{
> +	int ret;
> +	struct device *dev = keypad->input_dev->dev.parent;
> +	struct device_node *node = dev->of_node;
> +
> +	ret = matrix_keypad_parse_properties(dev, &keypad->n_rows,
> +					     &keypad->n_cols);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to parse keypad params.\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "mediatek,debounce-us",
> +				   &keypad->key_debounce);

according to binding, maximum is 256ms.
Please add range check here.

> +	if (ret) {
> +		pr_debug("read mediatek,debounce-us error.\n");
> +		return ret;
> +	}
> +
> +	keypad->wakeup = of_property_read_bool(node, "wakeup-source");
> +
> +	dev_dbg(dev, "n_row=%d n_col=%d debounce=%d.\n",
> +		keypad->n_rows, keypad->n_cols,
> +		keypad->key_debounce);
> +
> +	return 0;
> +}
> +
> +static int kpd_gpio_init(struct device *dev)
> +{
> +	struct pinctrl *keypad_pinctrl;
> +	struct pinctrl_state *kpd_default;
> +
> +	keypad_pinctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(keypad_pinctrl)) {
> +		dev_err(dev, "Cannot find keypad_pinctrl!\n");
> +
> +		return PTR_ERR(keypad_pinctrl);
> +	}
> +
> +	kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> +	if (IS_ERR(kpd_default)) {
> +		dev_err(dev, "Cannot find ecall_state!\n");
> +
> +		return PTR_ERR(kpd_default);
> +	}
> +
> +	return pinctrl_select_state(keypad_pinctrl, kpd_default);
> +}
> +
> +static int kpd_pdrv_probe(struct platform_device *pdev)
> +{
> +	struct mtk_keypad *keypad;
> +	struct resource *res;
> +	int err;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "No device tree data present.\n");
> +		return -EINVAL;
> +	}
> +
> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> +	if (!keypad)
> +		return -ENOMEM;
> +
> +	memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));
> +
> +	keypad->input_dev = devm_input_allocate_device(&pdev->dev);
> +	if (!keypad->input_dev) {
> +		dev_err(&pdev->dev, "input allocate device fail.\n");
> +		return -ENOMEM;
> +	}
> +
> +	keypad->input_dev->name = KPD_NAME;
> +	keypad->input_dev->id.bustype = BUS_HOST;
> +	keypad->input_dev->dev.parent = &pdev->dev;
> +
> +	err = kpd_get_dts_info(keypad);
> +	if (err) {
> +		dev_err(&pdev->dev, "get dts info failed.\n");
> +		return err;
> +	}
> +
> +	err = matrix_keypad_build_keymap(NULL, NULL,
> +					keypad->n_rows,
> +					keypad->n_cols,
> +					NULL,
> +					keypad->input_dev);
> +
> +	if (err) {
> +		dev_err(&pdev->dev, "build keymap failed.\n");
> +		return err;
> +	}
> +
> +	input_set_drvdata(keypad->input_dev, keypad);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "get IO memory resource failed.\n");
> +		return -ENODEV;
> +	}
> +
> +	keypad->base = devm_ioremap_resource(&pdev->dev, res);

please use devm_platform_ioremap_resource instead of using
platform_get_resource+devm_ioremap

Joe.C

> +	if (IS_ERR(keypad->base)) {
> +		dev_err(&pdev->dev, "KP iomap failed\n");
> +		return PTR_ERR(keypad->base);
> +	}
> +
> +	writew((keypad->key_debounce * 32 / 1000 & KPD_DEBOUNCE_MASK),
> +		keypad->base + KP_DEBOUNCE);
> +
> +	keypad->clk = devm_clk_get(&pdev->dev, "kpd");
> +	if (IS_ERR(keypad->clk)) {
> +		dev_err(&pdev->dev, "get kpd-clk fail.\n");
> +		return PTR_ERR(keypad->clk);
> +	}
> +
> +	err = clk_prepare_enable(keypad->clk);
> +	if (err) {
> +		dev_err(&pdev->dev, "kpd-clk prepare enable failed.\n");
> +		return err;
> +	}
> +
> +	err = kpd_gpio_init(&pdev->dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "gpio init failed\n");
> +		return err;
> +	}
> +
> +	keypad->irqnr = platform_get_irq(pdev, 0);
> +	if (!keypad->irqnr) {
> +		dev_err(&pdev->dev, "KP get irqnr failed\n");
> +		return -EINVAL;
> +	}
> +
> +	err = devm_request_irq(&pdev->dev, keypad->irqnr,
> +				kpd_irq_handler, 0,
> +				KPD_NAME, keypad);
> +	if (err) {
> +		dev_err(&pdev->dev, "register IRQ failed.\n");
> +		return err;
> +	}
> +
> +	err = input_register_device(keypad->input_dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "register input device failed.\n");
> +		return err;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, keypad->wakeup);
> +
> +	platform_set_drvdata(pdev, keypad);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id kpd_of_match[] = {
> +	{.compatible = "mediatek,kp"},
> +	{}
> +};
> +
> +static struct platform_driver kpd_pdrv = {
> +	.probe = kpd_pdrv_probe,
> +	.driver = {
> +		   .name = KPD_NAME,
> +		   .of_match_table = kpd_of_match,
> +	},
> +};
> +
> +module_platform_driver(kpd_pdrv);
> +
> +MODULE_AUTHOR("Mediatek Corporation");
> +MODULE_DESCRIPTION("MTK Keypad (KPD) Driver");
> +MODULE_LICENSE("GPL");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] drivers: input: keyboard: add mtk keypad driver

[Dmitry Torokhov]

Hi Fendping,

On Tue, Jan 14, 2020 at 08:27:41PM +0800, fengping.yu wrote:
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> 
> Add matrix keypad driver for MTK SoC.
> 
> ---
>  arch/arm64/configs/defconfig     |   1 +
>  drivers/input/keyboard/Kconfig   |   8 +
>  drivers/input/keyboard/Makefile  |   1 +
>  drivers/input/keyboard/mtk-kpd.c | 280 +++++++++++++++++++++++++++++++
>  4 files changed, 290 insertions(+)
>  create mode 100644 drivers/input/keyboard/mtk-kpd.c
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 8e05c39eab08..62bed77ec127 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -315,6 +315,7 @@ CONFIG_KEYBOARD_ADC=m
>  CONFIG_KEYBOARD_GPIO=y
>  CONFIG_KEYBOARD_SNVS_PWRKEY=m
>  CONFIG_KEYBOARD_CROS_EC=y
> +CONFIG_KEYBOARD_MTK=y
>  CONFIG_INPUT_TOUCHSCREEN=y
>  CONFIG_TOUCHSCREEN_ATMEL_MXT=m
>  CONFIG_INPUT_MISC=y
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 8911bc2ec42a..01747a97536b 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -775,4 +775,12 @@ config KEYBOARD_MTK_PMIC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pmic-keys.
>  
> +config KEYBOARD_MTK
> +	tristate "MediaTek Keypad Support"
> +	help
> +	  Say Y here if you want to use the keypad.
> +	  If unuse, say N.
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called mtk-kpd.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 9510325c0c5d..daa654bcce6e 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
>  obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
>  obj-$(CONFIG_KEYBOARD_MCS)		+= mcs_touchkey.o
>  obj-$(CONFIG_KEYBOARD_MPR121)		+= mpr121_touchkey.o
> +obj-$(CONFIG_KEYBOARD_MTK) 		+= mtk-kpd.o
>  obj-$(CONFIG_KEYBOARD_MTK_PMIC) 	+= mtk-pmic-keys.o
>  obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
>  obj-$(CONFIG_KEYBOARD_NOMADIK)		+= nomadik-ske-keypad.o
> diff --git a/drivers/input/keyboard/mtk-kpd.c b/drivers/input/keyboard/mtk-kpd.c
> new file mode 100644
> index 000000000000..e36461c9dd89
> --- /dev/null
> +++ b/drivers/input/keyboard/mtk-kpd.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 MediaTek Inc.
> + * Author Terry Chang <terry.chang@mediatek.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define KPD_NAME	"mtk-kpd"
> +
> +#define KP_STA				0x0000
> +#define KP_MEM1				0x0004
> +#define KP_MEM2				0x0008
> +#define KP_MEM3				0x000c
> +#define KP_MEM4				0x0010
> +#define KP_MEM5				0x0014
> +#define KP_DEBOUNCE			0x0018
> +#define KP_SEL				0x0020
> +#define KP_EN				0x0024
> +
> +#define KP_COL0_SEL		BIT(10)
> +#define KP_COL1_SEL		BIT(11)
> +#define KP_COL2_SEL		BIT(12)
> +
> +#define KPD_DEBOUNCE_MASK	GENMASK_ULL(13, 0)
> +
> +#define KPD_NUM_MEMS	5
> +#define KPD_MEM5_BITS	8
> +#define KPD_NUM_KEYS	72	/* 4 * 16 + KPD_MEM5_BITS */
> +
> +struct mtk_keypad {
> +	struct input_dev *input_dev;
> +	struct clk *clk;
> +	void __iomem *base;
> +	unsigned int irqnr;
> +	bool wakeup;
> +	u32 key_debounce;
> +	u32 n_rows;
> +	u32 n_cols;
> +	u32 keymap_state[KPD_NUM_MEMS];

	DECLARE_BITMAP(keymap_state, KPD_NUM_KEYS);

> +};
> +
> +static void kpd_get_keymap_state(void __iomem *kp_base, u32 state[])
> +{
> +	memset_io(state, 0xff, KPD_NUM_MEMS);
> +	memcpy_fromio(state, kp_base, KPD_NUM_MEMS);
> +}
> +
> +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> +{
> +	/* use _nosync to avoid deadlock */
> +	struct mtk_keypad *keypad = dev_id;
> +	unsigned short *keycode = keypad->input_dev->keycode;
> +	u32 new_state[KPD_NUM_MEMS], mask;
	DECLARE_BITMAP(new_state, KPD_NUM_KEYS);
	DECLARE_BITMAP(changed_state, KPD_NUM_KEYS);
> +	unsigned long change = 0;
> +	int bit_nr;
> +	int pressed;
> +	u32 row_shift = get_count_order(keypad->n_cols);
> +	unsigned short code;
> +	int i;
> +
> +	disable_irq_nosync(keypad->irqnr);

Not needed.


> +
> +	kpd_get_keymap_state(keypad->base, new_state);

	bitmap_xor(changed_state, keypad->keymap_state, new_state,
		   KPD_NUM_KEYS);

	for_each_set_bit(bit_nr, changed_state, KPD_NUM_KEYS) {
		pressed = test_bit(new_state, bit_nr);

		...
	}

> +
> +	for (i = 0; i < KPD_NUM_MEMS; i++) {
> +		change = new_state[i] ^ keypad->keymap_state[i];
> +		if (!change)
> +			continue;
> +
> +		for_each_set_bit(bit_nr, &change, 32) {
> +			mask = 1 << bit_nr;
> +			pressed = (new_state[i] & mask) == 0U;
> +			dev_dbg(&keypad->input_dev->dev,
> +				"%s", pressed ? "pressed" : "released");
> +
> +			code = keycode[MATRIX_SCAN_CODE(i, bit_nr, row_shift)];
> +
> +			input_report_key(keypad->input_dev, code, pressed);
> +			input_sync(keypad->input_dev);
> +
> +			dev_dbg(&keypad->input_dev->dev,
> +				"report Linux keycode = %d\n", code);
> +		}
> +	}
> +
> +	memcpy(keypad->keymap_state, new_state, sizeof(new_state));

	bitmap_copy(keypad->keymap_state, new_state, KPD_NUM_KEYS);

> +
> +	enable_irq(keypad->irqnr);

Not needed.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int kpd_get_dts_info(struct mtk_keypad *keypad)
> +{
> +	int ret;
> +	struct device *dev = keypad->input_dev->dev.parent;
> +	struct device_node *node = dev->of_node;
> +
> +	ret = matrix_keypad_parse_properties(dev, &keypad->n_rows,
> +					     &keypad->n_cols);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to parse keypad params.\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "mediatek,debounce-us",
> +				   &keypad->key_debounce);

Please use device_property_read_*() APIs instead of of_ ones.

Thanks.

-- 
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] drivers: input: keyboard: add mtk keypad driver

[Fabien Parent]

On Tue, Jan 14, 2020 at 1:39 PM fengping.yu <fengping.yu@mediatek.com> wrote:
>
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
>
> Add matrix keypad driver for MTK SoC.
>
> ---
>  arch/arm64/configs/defconfig     |   1 +
>  drivers/input/keyboard/Kconfig   |   8 +
>  drivers/input/keyboard/Makefile  |   1 +
>  drivers/input/keyboard/mtk-kpd.c | 280 +++++++++++++++++++++++++++++++
>  4 files changed, 290 insertions(+)
>  create mode 100644 drivers/input/keyboard/mtk-kpd.c
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 8e05c39eab08..62bed77ec127 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -315,6 +315,7 @@ CONFIG_KEYBOARD_ADC=m
>  CONFIG_KEYBOARD_GPIO=y
>  CONFIG_KEYBOARD_SNVS_PWRKEY=m
>  CONFIG_KEYBOARD_CROS_EC=y
> +CONFIG_KEYBOARD_MTK=y
>  CONFIG_INPUT_TOUCHSCREEN=y
>  CONFIG_TOUCHSCREEN_ATMEL_MXT=m
>  CONFIG_INPUT_MISC=y
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 8911bc2ec42a..01747a97536b 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -775,4 +775,12 @@ config KEYBOARD_MTK_PMIC
>           To compile this driver as a module, choose M here: the
>           module will be called pmic-keys.
>
> +config KEYBOARD_MTK
> +       tristate "MediaTek Keypad Support"
> +       help
> +         Say Y here if you want to use the keypad.
> +         If unuse, say N.
> +         To compile this driver as a module, choose M here: the
> +         module will be called mtk-kpd.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 9510325c0c5d..daa654bcce6e 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)         += matrix_keypad.o
>  obj-$(CONFIG_KEYBOARD_MAX7359)         += max7359_keypad.o
>  obj-$(CONFIG_KEYBOARD_MCS)             += mcs_touchkey.o
>  obj-$(CONFIG_KEYBOARD_MPR121)          += mpr121_touchkey.o
> +obj-$(CONFIG_KEYBOARD_MTK)             += mtk-kpd.o
>  obj-$(CONFIG_KEYBOARD_MTK_PMIC)        += mtk-pmic-keys.o
>  obj-$(CONFIG_KEYBOARD_NEWTON)          += newtonkbd.o
>  obj-$(CONFIG_KEYBOARD_NOMADIK)         += nomadik-ske-keypad.o
> diff --git a/drivers/input/keyboard/mtk-kpd.c b/drivers/input/keyboard/mtk-kpd.c
> new file mode 100644
> index 000000000000..e36461c9dd89
> --- /dev/null
> +++ b/drivers/input/keyboard/mtk-kpd.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 MediaTek Inc.
> + * Author Terry Chang <terry.chang@mediatek.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define KPD_NAME       "mtk-kpd"
> +
> +#define KP_STA                         0x0000
> +#define KP_MEM1                                0x0004
> +#define KP_MEM2                                0x0008
> +#define KP_MEM3                                0x000c
> +#define KP_MEM4                                0x0010
> +#define KP_MEM5                                0x0014
> +#define KP_DEBOUNCE                    0x0018
> +#define KP_SEL                         0x0020
> +#define KP_EN                          0x0024
> +
> +#define KP_COL0_SEL            BIT(10)
> +#define KP_COL1_SEL            BIT(11)
> +#define KP_COL2_SEL            BIT(12)
> +
> +#define KPD_DEBOUNCE_MASK      GENMASK_ULL(13, 0)
> +
> +#define KPD_NUM_MEMS   5
> +#define KPD_MEM5_BITS  8
> +#define KPD_NUM_KEYS   72      /* 4 * 16 + KPD_MEM5_BITS */
> +
> +struct mtk_keypad {
> +       struct input_dev *input_dev;
> +       struct clk *clk;
> +       void __iomem *base;
> +       unsigned int irqnr;
> +       bool wakeup;
> +       u32 key_debounce;
> +       u32 n_rows;
> +       u32 n_cols;
> +       u32 keymap_state[KPD_NUM_MEMS];
> +};
> +
> +static void kpd_get_keymap_state(void __iomem *kp_base, u32 state[])
> +{
> +       memset_io(state, 0xff, KPD_NUM_MEMS);
> +       memcpy_fromio(state, kp_base, KPD_NUM_MEMS);
> +}
> +
> +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> +{
> +       /* use _nosync to avoid deadlock */
> +       struct mtk_keypad *keypad = dev_id;
> +       unsigned short *keycode = keypad->input_dev->keycode;
> +       u32 new_state[KPD_NUM_MEMS], mask;
> +       unsigned long change = 0;
> +       int bit_nr;
> +       int pressed;
> +       u32 row_shift = get_count_order(keypad->n_cols);
> +       unsigned short code;
> +       int i;
> +
> +       disable_irq_nosync(keypad->irqnr);
> +
> +       kpd_get_keymap_state(keypad->base, new_state);

kpd_get_keymap_state(keypad->base + KP_MEM1, new_state);

> +
> +       for (i = 0; i < KPD_NUM_MEMS; i++) {
> +               change = new_state[i] ^ keypad->keymap_state[i];
> +               if (!change)
> +                       continue;
> +
> +               for_each_set_bit(bit_nr, &change, 32) {
> +                       mask = 1 << bit_nr;
> +                       pressed = (new_state[i] & mask) == 0U;
> +                       dev_dbg(&keypad->input_dev->dev,
> +                               "%s", pressed ? "pressed" : "released");
> +
> +                       code = keycode[MATRIX_SCAN_CODE(i, bit_nr, row_shift)];
> +
> +                       input_report_key(keypad->input_dev, code, pressed);
> +                       input_sync(keypad->input_dev);
> +
> +                       dev_dbg(&keypad->input_dev->dev,
> +                               "report Linux keycode = %d\n", code);
> +               }
> +       }
> +
> +       memcpy(keypad->keymap_state, new_state, sizeof(new_state));
> +
> +       enable_irq(keypad->irqnr);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int kpd_get_dts_info(struct mtk_keypad *keypad)
> +{
> +       int ret;
> +       struct device *dev = keypad->input_dev->dev.parent;
> +       struct device_node *node = dev->of_node;
> +
> +       ret = matrix_keypad_parse_properties(dev, &keypad->n_rows,
> +                                            &keypad->n_cols);
> +
> +       if (ret) {
> +               dev_err(dev, "failed to parse keypad params.\n");
> +               return ret;
> +       }
> +
> +       ret = of_property_read_u32(node, "mediatek,debounce-us",
> +                                  &keypad->key_debounce);
> +       if (ret) {
> +               pr_debug("read mediatek,debounce-us error.\n");
> +               return ret;
> +       }
> +
> +       keypad->wakeup = of_property_read_bool(node, "wakeup-source");
> +
> +       dev_dbg(dev, "n_row=%d n_col=%d debounce=%d.\n",
> +               keypad->n_rows, keypad->n_cols,
> +               keypad->key_debounce);
> +
> +       return 0;
> +}
> +
> +static int kpd_gpio_init(struct device *dev)
> +{
> +       struct pinctrl *keypad_pinctrl;
> +       struct pinctrl_state *kpd_default;
> +
> +       keypad_pinctrl = devm_pinctrl_get(dev);
> +       if (IS_ERR(keypad_pinctrl)) {
> +               dev_err(dev, "Cannot find keypad_pinctrl!\n");
> +
> +               return PTR_ERR(keypad_pinctrl);
> +       }
> +
> +       kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> +       if (IS_ERR(kpd_default)) {
> +               dev_err(dev, "Cannot find ecall_state!\n");
> +
> +               return PTR_ERR(kpd_default);
> +       }
> +
> +       return pinctrl_select_state(keypad_pinctrl, kpd_default);
> +}
> +
> +static int kpd_pdrv_probe(struct platform_device *pdev)
> +{
> +       struct mtk_keypad *keypad;
> +       struct resource *res;
> +       int err;
> +
> +       if (!pdev->dev.of_node) {
> +               dev_err(&pdev->dev, "No device tree data present.\n");
> +               return -EINVAL;
> +       }
> +
> +       keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> +       if (!keypad)
> +               return -ENOMEM;
> +
> +       memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));
> +
> +       keypad->input_dev = devm_input_allocate_device(&pdev->dev);
> +       if (!keypad->input_dev) {
> +               dev_err(&pdev->dev, "input allocate device fail.\n");
> +               return -ENOMEM;
> +       }
> +
> +       keypad->input_dev->name = KPD_NAME;
> +       keypad->input_dev->id.bustype = BUS_HOST;
> +       keypad->input_dev->dev.parent = &pdev->dev;
> +
> +       err = kpd_get_dts_info(keypad);
> +       if (err) {
> +               dev_err(&pdev->dev, "get dts info failed.\n");
> +               return err;
> +       }
> +
> +       err = matrix_keypad_build_keymap(NULL, NULL,
> +                                       keypad->n_rows,
> +                                       keypad->n_cols,
> +                                       NULL,
> +                                       keypad->input_dev);
> +
> +       if (err) {
> +               dev_err(&pdev->dev, "build keymap failed.\n");
> +               return err;
> +       }
> +
> +       input_set_drvdata(keypad->input_dev, keypad);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res) {
> +               dev_err(&pdev->dev, "get IO memory resource failed.\n");
> +               return -ENODEV;
> +       }
> +
> +       keypad->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(keypad->base)) {
> +               dev_err(&pdev->dev, "KP iomap failed\n");
> +               return PTR_ERR(keypad->base);
> +       }
> +
> +       writew((keypad->key_debounce * 32 / 1000 & KPD_DEBOUNCE_MASK),
> +               keypad->base + KP_DEBOUNCE);
> +
> +       keypad->clk = devm_clk_get(&pdev->dev, "kpd");
> +       if (IS_ERR(keypad->clk)) {
> +               dev_err(&pdev->dev, "get kpd-clk fail.\n");
> +               return PTR_ERR(keypad->clk);
> +       }
> +
> +       err = clk_prepare_enable(keypad->clk);
> +       if (err) {
> +               dev_err(&pdev->dev, "kpd-clk prepare enable failed.\n");
> +               return err;
> +       }
> +
> +       err = kpd_gpio_init(&pdev->dev);
> +       if (err) {
> +               dev_err(&pdev->dev, "gpio init failed\n");
> +               return err;
> +       }
> +
> +       keypad->irqnr = platform_get_irq(pdev, 0);
> +       if (!keypad->irqnr) {
> +               dev_err(&pdev->dev, "KP get irqnr failed\n");
> +               return -EINVAL;
> +       }
> +
> +       err = devm_request_irq(&pdev->dev, keypad->irqnr,
> +                               kpd_irq_handler, 0,
> +                               KPD_NAME, keypad);
> +       if (err) {
> +               dev_err(&pdev->dev, "register IRQ failed.\n");
> +               return err;
> +       }
> +
> +       err = input_register_device(keypad->input_dev);
> +       if (err) {
> +               dev_err(&pdev->dev, "register input device failed.\n");
> +               return err;
> +       }
> +
> +       device_init_wakeup(&pdev->dev, keypad->wakeup);
> +
> +       platform_set_drvdata(pdev, keypad);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id kpd_of_match[] = {
> +       {.compatible = "mediatek,kp"},
> +       {}
> +};
> +
> +static struct platform_driver kpd_pdrv = {
> +       .probe = kpd_pdrv_probe,
> +       .driver = {
> +                  .name = KPD_NAME,
> +                  .of_match_table = kpd_of_match,
> +       },
> +};
> +
> +module_platform_driver(kpd_pdrv);
> +
> +MODULE_AUTHOR("Mediatek Corporation");
> +MODULE_DESCRIPTION("MTK Keypad (KPD) Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V3 2/2] drivers: input: keyboard: add mtk keypad driver

[Andy Shevchenko]

On Tue, Feb 18, 2020 at 04:14:24PM +0100, Fabien Parent wrote:
> On Tue, Jan 14, 2020 at 1:39 PM fengping.yu <fengping.yu@mediatek.com> wrote:

...

> > +#define KP_STA                         0x0000
> > +#define KP_MEM1                                0x0004
> > +#define KP_MEM2                                0x0008
> > +#define KP_MEM3                                0x000c
> > +#define KP_MEM4                                0x0010
> > +#define KP_MEM5                                0x0014
> > +#define KP_DEBOUNCE                    0x0018
> > +#define KP_SEL                         0x0020
> > +#define KP_EN                          0x0024

...

> > +#define KPD_NUM_MEMS   5
> > +#define KPD_MEM5_BITS  8
> > +#define KPD_NUM_KEYS   72      /* 4 * 16 + KPD_MEM5_BITS */

> > +       kpd_get_keymap_state(keypad->base, new_state);
> 
> kpd_get_keymap_state(keypad->base + KP_MEM1, new_state);

Why?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel