* Re: [PATCH V2 2/2] drivers: input: keyboard
[not found] ` <20200108062923.14684-3-fengping.yu@mediatek.com>
@ 2020-01-08 7:11 ` Marco Felsch
2020-01-08 7:23 ` Marco Felsch
2020-01-08 11:22 ` Andy Shevchenko
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Marco Felsch @ 2020-01-08 7:11 UTC (permalink / raw)
To: fengping yu
Cc: Matthias Brugger, Dmitry Torokhov, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Shawn Guo, Olof Johansson,
Aisheng Dong, Anson Huang, Maxime Ripard, Leonard Crestez,
Dinh Nguyen, Marcin Juszkiewicz, Valentin Schneider,
Arnd Bergmann, Andy Shevchenko, Mark Brown, Thierry Reding,
YueHaibing, Stefan Agner, Jacky Bai, linux-input,
linux-arm-kernel, linux-kernel, linux-mediatek, devicetree,
wsd_upstream
Hi,
this is just a short review I did not check all details. Since this is a
v2 you should say what you changed.
Regards,
Marco
On 20-01-08 14:29, fengping yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
>
> add mtk keypad driver
Hm.. can you explain it a bit more?
> Change-Id: I20bb26ee4112f51f60476b7ff7d6e42b43f729dc
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> ---
> .../devicetree/bindings/input/mtk-kpd.txt | 13 +-
> arch/arm64/configs/defconfig | 1 +
> drivers/input/keyboard/Kconfig | 8 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/mtk-kpd.c | 357 ++++++++++++++++++
> 5 files changed, 377 insertions(+), 3 deletions(-)
> create mode 100644 drivers/input/keyboard/mtk-kpd.c
>
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.txt b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> index 2af81e696159..e3c17513c02c 100644
> --- a/Documentation/devicetree/bindings/input/mtk-kpd.txt
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> @@ -38,8 +38,15 @@ Example:
> };
>
> &keypad {
> - mediatek,key-debounce-ms = <1024>;
> + mediatek,key-debounce-ms = <32>;
> /*HW Keycode [0~71] -> Linux Keycode*/
> - mediatek,khw-map-num = <72>;
> - mediatek,hw-init-map = <114 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 >;
> + mediatek,hw-map-num = <72>;
> + mediatek,hw-init-map = <114 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0 >;
> };
This should be part of patch 1.
> 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
This should be a extra 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"
> + 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..740a58b44dac
> --- /dev/null
> +++ b/drivers/input/keyboard/mtk-kpd.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 MediaTek Inc.
> + * Author Terry Chang <terry.chang@mediatek.com>
> + */
> +#include <linux/atomic.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioctl.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/regmap.h>
> +#include <linux/timer.h>
> +#include <linux/workqueue.h>
Please check if you need all these includes at least the workqueue isn't
required.
> +#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 (1 << 10)
> +#define KP_COL1_SEL (1 << 11)
> +#define KP_COL2_SEL (1 << 12)
> +
> +#define KPD_DEBOUNCE_MASK ((1U << 14) - 1)
> +#define KPD_DOUBLE_KEY_MASK (1U << 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 wakeup_source *suspend_lock;
> + struct tasklet_struct tasklet;
Why do you use a tasklet instead of a simple softirq?
> + struct clk *clk;
> + void __iomem *base;
> + unsigned int irqnr;
> + u32 key_debounce;
> + u32 hw_map_num;
> + u32 hw_init_map[KPD_NUM_KEYS];
> + u16 keymap_state[KPD_NUM_MEMS];
> +};
> +
> +/* for keymap handling */
> +static void kpd_keymap_handler(unsigned long data);
> +
> +static int kpd_pdrv_probe(struct platform_device *pdev);
Why do you delcare these functions here?
> +static void kpd_get_keymap_state(void __iomem *kp_base, u16 state[])
> +{
> + state[0] = readw(kp_base + KP_MEM1);
> + state[1] = readw(kp_base + KP_MEM2);
> + state[2] = readw(kp_base + KP_MEM3);
> + state[3] = readw(kp_base + KP_MEM4);
> + state[4] = readw(kp_base + KP_MEM5);
> + pr_debug("kpd register = %x %x %x %x %x\n",
> + state[0], state[1], state[2], state[3], state[4]);
> +}
> +
> +static void kpd_keymap_handler(unsigned long data)
> +{
> + int i, j;
> + int pressed;
> + u16 new_state[KPD_NUM_MEMS], change, mask;
> + u16 hw_keycode, keycode;
> + void *dest;
> + struct mtk_keypad *keypad = (struct mtk_keypad *)data;
> +
> + kpd_get_keymap_state(keypad->base, new_state);
> +
> + __pm_wakeup_event(keypad->suspend_lock, 500);
> +
> + for (i = 0; i < KPD_NUM_MEMS; i++) {
> + change = new_state[i] ^ keypad->keymap_state[i];
> + if (!change)
> + continue;
> +
> + for (j = 0; j < 16U; j++) {
> + mask = (u16)1 << j;
> + if (!(change & mask))
> + continue;
> +
> + hw_keycode = (i << 4) + j;
> +
> + if (hw_keycode >= KPD_NUM_KEYS)
> + continue;
> +
> + /* bit is 1: not pressed, 0: pressed */
> + pressed = (new_state[i] & mask) == 0U;
> + pr_debug("(%s) HW keycode = %d\n",
> + (pressed) ? "pressed" : "released",
> + hw_keycode);
> +
> + keycode = keypad->hw_init_map[hw_keycode];
> + if (!keycode)
> + continue;
> + input_report_key(keypad->input_dev, keycode, pressed);
> + input_sync(keypad->input_dev);
> + pr_debug("report Linux keycode = %d\n", keycode);
> + }
> + }
> +
> + dest = memcpy(keypad->keymap_state, new_state, sizeof(new_state));
> + enable_irq(keypad->irqnr);
> +}
> +
> +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> +{
> + /* use _nosync to avoid deadlock */
> + struct mtk_keypad *keypad = dev_id;
> +
> + disable_irq_nosync(keypad->irqnr);
> + tasklet_schedule(&keypad->tasklet);
> + return IRQ_HANDLED;
> +}
> +
> +static int kpd_get_dts_info(struct mtk_keypad *keypad,
> + struct device_node *node)
> +{
> + int ret;
> +
> + ret = of_property_read_u32(node, "mediatek,key-debounce-ms",
> + &keypad->key_debounce);
> + if (ret) {
> + pr_debug("read mediatek,key-debounce-ms error.\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(node, "mediatek,hw-map-num",
> + &keypad->hw_map_num);
> + if (ret) {
> + pr_debug("read mediatek,hw-map-num error.\n");
> + return ret;
> + }
> +
> + if (keypad->hw_map_num > KPD_NUM_KEYS) {
> + pr_debug("hw-map-num error, it cannot bigger than %d.\n",
> + KPD_NUM_KEYS);
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_array(node, "mediatek,hw-init-map",
> + keypad->hw_init_map,
> + keypad->hw_map_num);
> +
> + if (ret) {
> + pr_debug("hw-init-map was not defined in dts.\n");
> + return ret;
> + }
> +
> + pr_debug("deb= %d\n", 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)) {
> + pr_debug("Cannot find keypad_pinctrl!\n");
> +
> + return (int)PTR_ERR(keypad_pinctrl);
> + }
> +
> + kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> + if (IS_ERR(kpd_default)) {
> + pr_debug("Cannot find ecall_state!\n");
> +
> + return (int)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 i;
> + int err;
> +
> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> + if (!keypad)
> + return -ENOMEM;
> +
> + keypad->clk = devm_clk_get(&pdev->dev, "kpd");
> + if (IS_ERR(keypad->clk)) {
> + pr_notice("get kpd-clk fail: %d\n", (int)PTR_ERR(keypad->clk));
> + return (int)PTR_ERR(keypad->clk);
> + }
There is no need to cast it to (int).
> +
> + err = clk_prepare_enable(keypad->clk);
> + if (err) {
> + pr_notice("kpd-clk prepare enable failed.\n");
> + return err;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + err = -ENODEV;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->base = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!keypad->base) {
> + pr_notice("KP iomap failed\n");
> + err = -EBUSY;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->irqnr = irq_of_parse_and_map(pdev->dev.of_node, 0);
> + if (!keypad->irqnr) {
> + pr_notice("KP get irqnr failed\n");
> + err = -ENODEV;
> + goto err_unprepare_clk;
> + }
> +
> + pr_info("kp base: 0x%p, addr:0x%p, kp irq: %d\n",
> + keypad->base, &keypad->base, keypad->irqnr);
Can we move that to pr_info.
> + err = kpd_gpio_init(&pdev->dev);
This can be done here too but is jsut a nitpick.
> + if (err) {
> + pr_debug("gpio init failed\n");
> + goto err_unprepare_clk;
> + }
> +
> + err = kpd_get_dts_info(keypad, pdev->dev.of_node);
> + if (err) {
> + pr_debug("get dts info failed.\n");
> + goto err_unprepare_clk;
> + }
> +
> + memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));
> +
> + keypad->input_dev = devm_input_allocate_device(&pdev->dev);
> + if (!keypad->input_dev) {
> + pr_notice("input allocate device fail.\n");
> + err = -ENOMEM;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->input_dev->name = KPD_NAME;
> + keypad->input_dev->id.bustype = BUS_HOST;
> + keypad->input_dev->dev.parent = &pdev->dev;
> +
> + __set_bit(EV_KEY, keypad->input_dev->evbit);
> +
> + for (i = 0; i < KPD_NUM_KEYS; i++) {
> + if (keypad->hw_init_map[i])
> + __set_bit(keypad->hw_init_map[i],
> + keypad->input_dev->keybit);
> + }
> +
> + err = input_register_device(keypad->input_dev);
> + if (err) {
> + pr_notice("register input device failed (%d)\n", err);
> + goto err_unprepare_clk;
> + }
> +
> + input_set_drvdata(keypad->input_dev, keypad);
> +
> + keypad->suspend_lock = wakeup_source_register(NULL, "kpd wakelock");
> + if (!keypad->suspend_lock) {
> + pr_notice("wakeup source init failed.\n");
> + err = -ENOMEM;
> + goto err_unregister_device;
> + }
What if someone won't use this device as wakeup source?
> + tasklet_init(&keypad->tasklet, kpd_keymap_handler,
> + (unsigned long)keypad);
> +
> + writew((u16)(keypad->key_debounce & KPD_DEBOUNCE_MASK),
> + keypad->base + KP_DEBOUNCE);
> +
> + /* register IRQ */
> + err = request_irq(keypad->irqnr, kpd_irq_handler, IRQF_TRIGGER_NONE,
> + KPD_NAME, keypad);
> + if (err) {
> + pr_notice("register IRQ failed (%d)\n", err);
> + goto err_irq;
> + }
> +
> + pr_info("kpd_probe OK.\n");
> +
> + return 0;
> +
> +err_irq:
> + tasklet_kill(&keypad->tasklet);
> + wakeup_source_unregister(keypad->suspend_lock);
> +
> +err_unregister_device:
> + input_unregister_device(keypad->input_dev);
> +
> +err_unprepare_clk:
> + clk_disable_unprepare(keypad->clk);
> +
> + return err;
> +}
> +
> +static int kpd_pdrv_remove(struct platform_device *pdev)
> +{
> + struct mtk_keypad *keypad = platform_get_drvdata(pdev);
> +
> + tasklet_kill(&keypad->tasklet);
> + wakeup_source_unregister(keypad->suspend_lock);
> + input_unregister_device(keypad->input_dev);
> + clk_disable_unprepare(keypad->clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id kpd_of_match[] = {
> + {.compatible = "mediatek,mt6779-keypad"},
> + {.compatible = "mediatek,kp"},
> + {},
> +};
> +
> +static struct platform_driver kpd_pdrv = {
> + .probe = kpd_pdrv_probe,
> + .remove = kpd_pdrv_remove,
> + .driver = {
> + .name = KPD_NAME,
> + .of_match_table = kpd_of_match,
> + },
Please align this correct.
> +};
> +
> +module_platform_driver(kpd_pdrv);
> +
> +MODULE_AUTHOR("Mediatek Corporation");
> +MODULE_DESCRIPTION("MTK Keypad (KPD) Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.18.0
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V2 2/2] drivers: input: keyboard
2020-01-08 7:11 ` [PATCH V2 2/2] drivers: input: keyboard Marco Felsch
@ 2020-01-08 7:23 ` Marco Felsch
0 siblings, 0 replies; 10+ messages in thread
From: Marco Felsch @ 2020-01-08 7:23 UTC (permalink / raw)
To: fengping yu
Cc: Matthias Brugger, Dmitry Torokhov, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Shawn Guo, Olof Johansson,
Aisheng Dong, Anson Huang, Maxime Ripard, Leonard Crestez,
Dinh Nguyen, Marcin Juszkiewicz, Valentin Schneider,
Arnd Bergmann, Andy Shevchenko, Mark Brown, Thierry Reding,
YueHaibing, Stefan Agner, Jacky Bai, linux-input,
linux-arm-kernel, linux-kernel, linux-mediatek, devicetree,
wsd_upstream
Hi,
On 20-01-08 08:11, Marco Felsch wrote:
...
> > +struct mtk_keypad {
> > + struct input_dev *input_dev;
> > + struct wakeup_source *suspend_lock;
> > + struct tasklet_struct tasklet;
>
> Why do you use a tasklet instead of a simple softirq?
Sorry, what I mean was a threaded_irq.
Regards,
Marco
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 2/2] drivers: input: keyboard
[not found] ` <20200108062923.14684-3-fengping.yu@mediatek.com>
2020-01-08 7:11 ` [PATCH V2 2/2] drivers: input: keyboard Marco Felsch
@ 2020-01-08 11:22 ` Andy Shevchenko
[not found] ` <1578473162.12131.13.camel@mtksdaap41>
2020-01-10 1:32 ` Dmitry Torokhov
3 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-01-08 11:22 UTC (permalink / raw)
To: fengping yu
Cc: Matthias Brugger, Dmitry Torokhov, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Shawn Guo, Olof Johansson,
Aisheng Dong, Anson Huang, Maxime Ripard, Leonard Crestez,
Dinh Nguyen, Marcin Juszkiewicz, Valentin Schneider,
Arnd Bergmann, Mark Brown, Thierry Reding, YueHaibing,
Stefan Agner, Jacky Bai, Marco Felsch, linux-input,
linux-arm-kernel, linux-kernel, linux-mediatek, devicetree,
wsd_upstream
On Wed, Jan 08, 2020 at 02:29:23PM +0800, fengping yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
>
> add mtk keypad driver
A bit more details, perhaps?
Also, what about proper English grammar and punctuation?
> Change-Id: I20bb26ee4112f51f60476b7ff7d6e42b43f729dc
Are you sure it's ready for upstream?
...
> arch/arm64/configs/defconfig | 1 +
Why?
...
> --- a/Documentation/devicetree/bindings/input/mtk-kpd.txt
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> @@ -38,8 +38,15 @@ Example:
> };
>
> &keypad {
> - mediatek,key-debounce-ms = <1024>;
> + mediatek,key-debounce-ms = <32>;
> /*HW Keycode [0~71] -> Linux Keycode*/
> - mediatek,khw-map-num = <72>;
> - mediatek,hw-init-map = <114 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 >;
> + mediatek,hw-map-num = <72>;
> + mediatek,hw-init-map = <114 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0 >;
> };
Why above in this patch?
...
> +obj-$(CONFIG_KEYBOARD_MTK) += mtk-kpd.o
TABs/spaces mix.
...
> +#include <linux/atomic.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioctl.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/regmap.h>
> +#include <linux/timer.h>
> +#include <linux/workqueue.h>
Do you really need all of them?!
...
> +#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)
What the purpose of parentheses?
...
> +#define KP_COL0_SEL (1 << 10)
> +#define KP_COL1_SEL (1 << 11)
> +#define KP_COL2_SEL (1 << 12)
BIT()
> +#define KPD_DEBOUNCE_MASK ((1U << 14) - 1)
> +#define KPD_DOUBLE_KEY_MASK (1U << 0)
GENMASK() / BIT_MASK() / BIT()
...
> +static int kpd_pdrv_probe(struct platform_device *pdev);
Why this forward declaration is present?
...
> +static void kpd_get_keymap_state(void __iomem *kp_base, u16 state[])
> +{
> + state[0] = readw(kp_base + KP_MEM1);
> + state[1] = readw(kp_base + KP_MEM2);
> + state[2] = readw(kp_base + KP_MEM3);
> + state[3] = readw(kp_base + KP_MEM4);
> + state[4] = readw(kp_base + KP_MEM5);
> + pr_debug("kpd register = %x %x %x %x %x\n",
> + state[0], state[1], state[2], state[3], state[4]);
Consider print_hex_dump() instead. But do you need this at all?
> +}
...
> + for (j = 0; j < 16U; j++) {
> + mask = (u16)1 << j;
> + if (!(change & mask))
> + continue;
for_each_set_bit() ?
> + }
...
> +static int kpd_get_dts_info(struct mtk_keypad *keypad,
> + struct device_node *node)
> +{
> + int ret;
Consider pr_*() -> dev_*() in all below code.
> +
> + ret = of_property_read_u32(node, "mediatek,key-debounce-ms",
> + &keypad->key_debounce);
> + if (ret) {
> + pr_debug("read mediatek,key-debounce-ms error.\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(node, "mediatek,hw-map-num",
> + &keypad->hw_map_num);
> + if (ret) {
> + pr_debug("read mediatek,hw-map-num error.\n");
> + return ret;
> + }
> +
> + if (keypad->hw_map_num > KPD_NUM_KEYS) {
> + pr_debug("hw-map-num error, it cannot bigger than %d.\n",
> + KPD_NUM_KEYS);
dev_err() ?
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_array(node, "mediatek,hw-init-map",
> + keypad->hw_init_map,
> + keypad->hw_map_num);
> +
> + if (ret) {
> + pr_debug("hw-init-map was not defined in dts.\n");
> + return ret;
> + }
> +
> + pr_debug("deb= %d\n", keypad->key_debounce);
> +
> + return 0;
> +}
It's for ARM64 which might support ACPI. Perhaps you may use device property
API instead of be OF-centric.
...
> + keypad_pinctrl = devm_pinctrl_get(dev);
> + if (IS_ERR(keypad_pinctrl)) {
> + pr_debug("Cannot find keypad_pinctrl!\n");
> +
These kind of messages are not for production.
> + return (int)PTR_ERR(keypad_pinctrl);
Why explicit casting? (Same for similar places all over the driver)
Is it really a fatal error?
> + }
...
> + return pinctrl_select_state(keypad_pinctrl,
> + kpd_default);
One line. Perhaps you need to configure your text editor.
...
> + keypad->clk = devm_clk_get(&pdev->dev, "kpd");
> + if (IS_ERR(keypad->clk)) {
> + pr_notice("get kpd-clk fail: %d\n", (int)PTR_ERR(keypad->clk));
Noise as a lot of such messages.
> + return (int)PTR_ERR(keypad->clk);
Casting?!
> + }
...
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + err = -ENODEV;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->base = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!keypad->base) {
> + pr_notice("KP iomap failed\n");
> + err = -EBUSY;
> + goto err_unprepare_clk;
> + }
What's wrong with devm_platform_ioremap_resource()?
...
> + keypad->irqnr = irq_of_parse_and_map(pdev->dev.of_node, 0);
> + if (!keypad->irqnr) {
> + pr_notice("KP get irqnr failed\n");
> + err = -ENODEV;
> + goto err_unprepare_clk;
> + }
Can't you use platform_get_irq()?
Why not?
...
> + pr_info("kp base: 0x%p, addr:0x%p, kp irq: %d\n",
> + keypad->base, &keypad->base, keypad->irqnr);
Useless message. Moreover I believe it has wrong specifiers in use.
...
> + memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));
Shouldn't be bitmap_fill()? In such case the variable has to be type of
unsigned long.
...
> + writew((u16)(keypad->key_debounce & KPD_DEBOUNCE_MASK),
> + keypad->base + KP_DEBOUNCE);
Why explicit casting?
...
> + /* register IRQ */
> + err = request_irq(keypad->irqnr, kpd_irq_handler, IRQF_TRIGGER_NONE,
> + KPD_NAME, keypad);
> + if (err) {
> + pr_notice("register IRQ failed (%d)\n", err);
> + goto err_irq;
> + }
Why not threaded IRQ handler instead of ugly tasklet?
...
> + pr_info("kpd_probe OK.\n");
Noise.
...
> +static const struct of_device_id kpd_of_match[] = {
> + {.compatible = "mediatek,mt6779-keypad"},
> + {.compatible = "mediatek,kp"},
> + {},
Comma is not needed in terminator lines.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread[parent not found: <1578473162.12131.13.camel@mtksdaap41>]
* Re: [PATCH V2 2/2] drivers: input: keyboard
[not found] ` <1578473162.12131.13.camel@mtksdaap41>
@ 2020-01-08 11:26 ` Andy Shevchenko
2020-01-10 1:20 ` Dmitry Torokhov
0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-01-08 11:26 UTC (permalink / raw)
To: Yingjoe Chen
Cc: fengping yu, Matthias Brugger, Dmitry Torokhov, Rob Herring,
Mark Rutland, Catalin Marinas, Will Deacon, Shawn Guo,
Olof Johansson, Aisheng Dong, Anson Huang, Maxime Ripard,
Leonard Crestez, Dinh Nguyen, Marcin Juszkiewicz,
Valentin Schneider, Arnd Bergmann, Mark Brown, Thierry Reding,
YueHaibing, Stefan Agner, Jacky Bai, Marco Felsch, devicetree,
wsd_upstream, linux-kernel, linux-mediatek, linux-input,
linux-arm-kernel
On Wed, Jan 08, 2020 at 04:46:02PM +0800, Yingjoe Chen wrote:
> On Wed, 2020-01-08 at 14:29 +0800, fengping yu wrote:
> > + tasklet_init(&keypad->tasklet, kpd_keymap_handler,
> > + (unsigned long)keypad);
> > +
> > + writew((u16)(keypad->key_debounce & KPD_DEBOUNCE_MASK),
> > + keypad->base + KP_DEBOUNCE);
>
> You use a 13 bits mask and set it directly to KP_DEBOUNCE register. Are
> you sure the debounce unit is ms?
>
> > +
> > + /* register IRQ */
> > + err = request_irq(keypad->irqnr, kpd_irq_handler, IRQF_TRIGGER_NONE,
> > + KPD_NAME, keypad);
>
> please consider using devm_request_irq, otherwise you have to free it in
> _remove function.
No, you may not use devm_*_irq() when tasklets are in use. There is a nasty
race condition.
Actually the rule of thumb is to NOT use devm_*_irq() unless you exactly know
what you are doing.
P.S. Why simple not to switch to threaded IRQ handler and drop tasklet? In such
case devm_*_irq() is fine.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V2 2/2] drivers: input: keyboard
2020-01-08 11:26 ` Andy Shevchenko
@ 2020-01-10 1:20 ` Dmitry Torokhov
0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Torokhov @ 2020-01-10 1:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Yingjoe Chen, fengping yu, Matthias Brugger, Rob Herring,
Mark Rutland, Catalin Marinas, Will Deacon, Shawn Guo,
Olof Johansson, Aisheng Dong, Anson Huang, Maxime Ripard,
Leonard Crestez, Dinh Nguyen, Marcin Juszkiewicz,
Valentin Schneider, Arnd Bergmann, Mark Brown, Thierry Reding,
YueHaibing, Stefan Agner, Jacky Bai, Marco Felsch, devicetree,
wsd_upstream, linux-kernel, linux-mediatek, linux-input,
linux-arm-kernel
On Wed, Jan 08, 2020 at 01:26:09PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 08, 2020 at 04:46:02PM +0800, Yingjoe Chen wrote:
> > On Wed, 2020-01-08 at 14:29 +0800, fengping yu wrote:
>
> > > + tasklet_init(&keypad->tasklet, kpd_keymap_handler,
> > > + (unsigned long)keypad);
> > > +
> > > + writew((u16)(keypad->key_debounce & KPD_DEBOUNCE_MASK),
> > > + keypad->base + KP_DEBOUNCE);
> >
> > You use a 13 bits mask and set it directly to KP_DEBOUNCE register. Are
> > you sure the debounce unit is ms?
> >
> > > +
> > > + /* register IRQ */
> > > + err = request_irq(keypad->irqnr, kpd_irq_handler, IRQF_TRIGGER_NONE,
> > > + KPD_NAME, keypad);
> >
> > please consider using devm_request_irq, otherwise you have to free it in
> > _remove function.
>
> No, you may not use devm_*_irq() when tasklets are in use. There is a nasty
> race condition.
>
> Actually the rule of thumb is to NOT use devm_*_irq() unless you exactly know
> what you are doing.
>
> P.S. Why simple not to switch to threaded IRQ handler and drop tasklet? In such
> case devm_*_irq() is fine.
Actually, we are simply reading iomem data and forward it to input
subsystem, there is no need to use threaded interrupt nor the tasklet.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 2/2] drivers: input: keyboard
[not found] ` <20200108062923.14684-3-fengping.yu@mediatek.com>
` (2 preceding siblings ...)
[not found] ` <1578473162.12131.13.camel@mtksdaap41>
@ 2020-01-10 1:32 ` Dmitry Torokhov
2020-01-10 7:35 ` Andy Shevchenko
3 siblings, 1 reply; 10+ messages in thread
From: Dmitry Torokhov @ 2020-01-10 1:32 UTC (permalink / raw)
To: fengping yu
Cc: Matthias Brugger, Rob Herring, Mark Rutland, Catalin Marinas,
Will Deacon, Shawn Guo, Olof Johansson, Aisheng Dong,
Anson Huang, Maxime Ripard, Leonard Crestez, Dinh Nguyen,
Marcin Juszkiewicz, Valentin Schneider, Arnd Bergmann,
Andy Shevchenko, Mark Brown, Thierry Reding, YueHaibing,
Stefan Agner, Jacky Bai, Marco Felsch, linux-input,
linux-arm-kernel, linux-kernel, linux-mediatek, devicetree,
wsd_upstream
On Wed, Jan 08, 2020 at 02:29:23PM +0800, fengping yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>
>
> add mtk keypad driver
>
> Change-Id: I20bb26ee4112f51f60476b7ff7d6e42b43f729dc
> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>
> ---
> .../devicetree/bindings/input/mtk-kpd.txt | 13 +-
> arch/arm64/configs/defconfig | 1 +
> drivers/input/keyboard/Kconfig | 8 +
> drivers/input/keyboard/Makefile | 1 +
> drivers/input/keyboard/mtk-kpd.c | 357 ++++++++++++++++++
> 5 files changed, 377 insertions(+), 3 deletions(-)
> create mode 100644 drivers/input/keyboard/mtk-kpd.c
>
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.txt b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> index 2af81e696159..e3c17513c02c 100644
> --- a/Documentation/devicetree/bindings/input/mtk-kpd.txt
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.txt
> @@ -38,8 +38,15 @@ Example:
> };
>
> &keypad {
> - mediatek,key-debounce-ms = <1024>;
> + mediatek,key-debounce-ms = <32>;
> /*HW Keycode [0~71] -> Linux Keycode*/
> - mediatek,khw-map-num = <72>;
> - mediatek,hw-init-map = <114 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 >;
> + mediatek,hw-map-num = <72>;
> + mediatek,hw-init-map = <114 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0
> + 0 0 0 0 0 0 0 0 0 >;
> };
> 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..740a58b44dac
> --- /dev/null
> +++ b/drivers/input/keyboard/mtk-kpd.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 MediaTek Inc.
> + * Author Terry Chang <terry.chang@mediatek.com>
> + */
> +#include <linux/atomic.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioctl.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/regmap.h>
> +#include <linux/timer.h>
> +#include <linux/workqueue.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 (1 << 10)
> +#define KP_COL1_SEL (1 << 11)
> +#define KP_COL2_SEL (1 << 12)
> +
> +#define KPD_DEBOUNCE_MASK ((1U << 14) - 1)
> +#define KPD_DOUBLE_KEY_MASK (1U << 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 wakeup_source *suspend_lock;
> + struct tasklet_struct tasklet;
> + struct clk *clk;
> + void __iomem *base;
> + unsigned int irqnr;
> + u32 key_debounce;
> + u32 hw_map_num;
> + u32 hw_init_map[KPD_NUM_KEYS];
> + u16 keymap_state[KPD_NUM_MEMS];
> +};
> +
> +/* for keymap handling */
> +static void kpd_keymap_handler(unsigned long data);
> +
> +static int kpd_pdrv_probe(struct platform_device *pdev);
> +
> +static void kpd_get_keymap_state(void __iomem *kp_base, u16 state[])
> +{
> + state[0] = readw(kp_base + KP_MEM1);
> + state[1] = readw(kp_base + KP_MEM2);
> + state[2] = readw(kp_base + KP_MEM3);
> + state[3] = readw(kp_base + KP_MEM4);
> + state[4] = readw(kp_base + KP_MEM5);
Please use memcpy_formio() here.
> + pr_debug("kpd register = %x %x %x %x %x\n",
> + state[0], state[1], state[2], state[3], state[4]);
> +}
> +
> +static void kpd_keymap_handler(unsigned long data)
> +{
> + int i, j;
> + int pressed;
> + u16 new_state[KPD_NUM_MEMS], change, mask;
> + u16 hw_keycode, keycode;
> + void *dest;
> + struct mtk_keypad *keypad = (struct mtk_keypad *)data;
> +
> + kpd_get_keymap_state(keypad->base, new_state);
> +
> + __pm_wakeup_event(keypad->suspend_lock, 500);
> +
> + for (i = 0; i < KPD_NUM_MEMS; i++) {
> + change = new_state[i] ^ keypad->keymap_state[i];
> + if (!change)
> + continue;
> +
> + for (j = 0; j < 16U; j++) {
> + mask = (u16)1 << j;
> + if (!(change & mask))
> + continue;
If you define state as N longs instead of M u16s, you can do
for_each_set_bit()
as the single loop, figure row/col from offset, and get the state and
fetch the code from keymap.
> +
> + hw_keycode = (i << 4) + j;
> +
> + if (hw_keycode >= KPD_NUM_KEYS)
> + continue;
> +
> + /* bit is 1: not pressed, 0: pressed */
> + pressed = (new_state[i] & mask) == 0U;
> + pr_debug("(%s) HW keycode = %d\n",
> + (pressed) ? "pressed" : "released",
> + hw_keycode);
> +
> + keycode = keypad->hw_init_map[hw_keycode];
> + if (!keycode)
> + continue;
> + input_report_key(keypad->input_dev, keycode, pressed);
> + input_sync(keypad->input_dev);
> + pr_debug("report Linux keycode = %d\n", keycode);
> + }
> + }
> +
> + dest = memcpy(keypad->keymap_state, new_state, sizeof(new_state));
> + enable_irq(keypad->irqnr);
> +}
> +
> +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> +{
> + /* use _nosync to avoid deadlock */
> + struct mtk_keypad *keypad = dev_id;
> +
> + disable_irq_nosync(keypad->irqnr);
> + tasklet_schedule(&keypad->tasklet);
> + return IRQ_HANDLED;
> +}
> +
> +static int kpd_get_dts_info(struct mtk_keypad *keypad,
> + struct device_node *node)
> +{
> + int ret;
> +
> + ret = of_property_read_u32(node, "mediatek,key-debounce-ms",
> + &keypad->key_debounce);
> + if (ret) {
> + pr_debug("read mediatek,key-debounce-ms error.\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(node, "mediatek,hw-map-num",
> + &keypad->hw_map_num);
> + if (ret) {
> + pr_debug("read mediatek,hw-map-num error.\n");
> + return ret;
> + }
> +
> + if (keypad->hw_map_num > KPD_NUM_KEYS) {
> + pr_debug("hw-map-num error, it cannot bigger than %d.\n",
> + KPD_NUM_KEYS);
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_array(node, "mediatek,hw-init-map",
> + keypad->hw_init_map,
> + keypad->hw_map_num);
As I mentioned, please use matrix keymap API and linux,keymap property.
> +
> + if (ret) {
> + pr_debug("hw-init-map was not defined in dts.\n");
> + return ret;
> + }
> +
> + pr_debug("deb= %d\n", 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)) {
> + pr_debug("Cannot find keypad_pinctrl!\n");
> +
> + return (int)PTR_ERR(keypad_pinctrl);
> + }
> +
> + kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> + if (IS_ERR(kpd_default)) {
> + pr_debug("Cannot find ecall_state!\n");
> +
> + return (int)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 i;
> + int err;
> +
> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> + if (!keypad)
> + return -ENOMEM;
> +
> + keypad->clk = devm_clk_get(&pdev->dev, "kpd");
> + if (IS_ERR(keypad->clk)) {
> + pr_notice("get kpd-clk fail: %d\n", (int)PTR_ERR(keypad->clk));
> + return (int)PTR_ERR(keypad->clk);
> + }
> +
> + err = clk_prepare_enable(keypad->clk);
Do not mix devm and non-devm APIs. Use devm_add_action_or_reset() for
disabling clock in proper order.
> + if (err) {
> + pr_notice("kpd-clk prepare enable failed.\n");
> + return err;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + err = -ENODEV;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->base = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!keypad->base) {
> + pr_notice("KP iomap failed\n");
> + err = -EBUSY;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->irqnr = irq_of_parse_and_map(pdev->dev.of_node, 0);
Why is this done in the driver? Use platform_get_irq() instead.
> + if (!keypad->irqnr) {
> + pr_notice("KP get irqnr failed\n");
> + err = -ENODEV;
> + goto err_unprepare_clk;
> + }
> +
> + pr_info("kp base: 0x%p, addr:0x%p, kp irq: %d\n",
> + keypad->base, &keypad->base, keypad->irqnr);
> + err = kpd_gpio_init(&pdev->dev);
> + if (err) {
> + pr_debug("gpio init failed\n");
> + goto err_unprepare_clk;
> + }
> +
> + err = kpd_get_dts_info(keypad, pdev->dev.of_node);
> + if (err) {
> + pr_debug("get dts info failed.\n");
> + goto err_unprepare_clk;
> + }
> +
> + memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));
> +
> + keypad->input_dev = devm_input_allocate_device(&pdev->dev);
> + if (!keypad->input_dev) {
> + pr_notice("input allocate device fail.\n");
> + err = -ENOMEM;
> + goto err_unprepare_clk;
> + }
> +
> + keypad->input_dev->name = KPD_NAME;
> + keypad->input_dev->id.bustype = BUS_HOST;
> + keypad->input_dev->dev.parent = &pdev->dev;
No need to set this since you are using devm_input_allocate_device().
> +
> + __set_bit(EV_KEY, keypad->input_dev->evbit);
> +
> + for (i = 0; i < KPD_NUM_KEYS; i++) {
> + if (keypad->hw_init_map[i])
> + __set_bit(keypad->hw_init_map[i],
> + keypad->input_dev->keybit);
> + }
> +
> + err = input_register_device(keypad->input_dev);
> + if (err) {
> + pr_notice("register input device failed (%d)\n", err);
> + goto err_unprepare_clk;
> + }
> +
> + input_set_drvdata(keypad->input_dev, keypad);
> +
> + keypad->suspend_lock = wakeup_source_register(NULL, "kpd wakelock");
> + if (!keypad->suspend_lock) {
> + pr_notice("wakeup source init failed.\n");
> + err = -ENOMEM;
> + goto err_unregister_device;
> + }
Why are we doing it in this fashion instead of checking "wakeup-source"
property and doing device_init_wakeup() when this flag is set?
> +
> + tasklet_init(&keypad->tasklet, kpd_keymap_handler,
> + (unsigned long)keypad);
No need to use tasklet, you are not doing anything heavy in the ISR.
> +
> + writew((u16)(keypad->key_debounce & KPD_DEBOUNCE_MASK),
> + keypad->base + KP_DEBOUNCE);
> +
> + /* register IRQ */
> + err = request_irq(keypad->irqnr, kpd_irq_handler, IRQF_TRIGGER_NONE,
> + KPD_NAME, keypad);
> + if (err) {
> + pr_notice("register IRQ failed (%d)\n", err);
> + goto err_irq;
> + }
> +
> + pr_info("kpd_probe OK.\n");
> +
> + return 0;
> +
> +err_irq:
> + tasklet_kill(&keypad->tasklet);
> + wakeup_source_unregister(keypad->suspend_lock);
> +
> +err_unregister_device:
> + input_unregister_device(keypad->input_dev);
> +
> +err_unprepare_clk:
> + clk_disable_unprepare(keypad->clk);
> +
> + return err;
> +}
> +
> +static int kpd_pdrv_remove(struct platform_device *pdev)
> +{
> + struct mtk_keypad *keypad = platform_get_drvdata(pdev);
> +
> + tasklet_kill(&keypad->tasklet);
> + wakeup_source_unregister(keypad->suspend_lock);
> + input_unregister_device(keypad->input_dev);
> + clk_disable_unprepare(keypad->clk);
Please remove this all and rely on devm for cleanup.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id kpd_of_match[] = {
> + {.compatible = "mediatek,mt6779-keypad"},
> + {.compatible = "mediatek,kp"},
Do we need both compatibles? Why?
> + {},
> +};
> +
> +static struct platform_driver kpd_pdrv = {
> + .probe = kpd_pdrv_probe,
> + .remove = kpd_pdrv_remove,
> + .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
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V2 2/2] drivers: input: keyboard
2020-01-10 1:32 ` Dmitry Torokhov
@ 2020-01-10 7:35 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-01-10 7:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: fengping yu, Matthias Brugger, Rob Herring, Mark Rutland,
Catalin Marinas, Will Deacon, Shawn Guo, Olof Johansson,
Aisheng Dong, Anson Huang, Maxime Ripard, Leonard Crestez,
Dinh Nguyen, Marcin Juszkiewicz, Valentin Schneider,
Arnd Bergmann, Andy Shevchenko, Mark Brown, Thierry Reding,
YueHaibing, Stefan Agner, Jacky Bai, Marco Felsch, linux-input,
linux-arm Mailing List, Linux Kernel Mailing List,
moderated list:ARM/Mediatek SoC support, devicetree,
wsd_upstream
On Fri, Jan 10, 2020 at 3:34 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Jan 08, 2020 at 02:29:23PM +0800, fengping yu wrote:
> > From: "fengping.yu" <fengping.yu@mediatek.com>
...
> > +static void kpd_get_keymap_state(void __iomem *kp_base, u16 state[])
> > +{
> > + state[0] = readw(kp_base + KP_MEM1);
> > + state[1] = readw(kp_base + KP_MEM2);
> > + state[2] = readw(kp_base + KP_MEM3);
> > + state[3] = readw(kp_base + KP_MEM4);
> > + state[4] = readw(kp_base + KP_MEM5);
>
> Please use memcpy_formio() here.
Actually it's not an equivalent and may have side effects.
I already told somewhere that we lack of
mem*_*io{32,16,8}() implementation.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread