input: keyboard: Add sprd-keypad driver

Add matrix keypad driver, support matrix keypad function.

Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
---
 drivers/input/keyboard/Kconfig       |  10 +
 drivers/input/keyboard/Makefile      |   1 +
 drivers/input/keyboard/sprd_keypad.c | 418 +++++++++++++++++++++++++++
 3 files changed, 429 insertions(+)
 create mode 100644 drivers/input/keyboard/sprd_keypad.c

On Tue, 8 Aug 2023 at 15:25, Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:
>
> Add matrix keypad driver, support matrix keypad function.

Add Unisoc keypad driver...

>
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
>  drivers/input/keyboard/Kconfig       |  10 +
>  drivers/input/keyboard/Makefile      |   1 +
>  drivers/input/keyboard/sprd_keypad.c | 418 +++++++++++++++++++++++++++
>  3 files changed, 429 insertions(+)
>  create mode 100644 drivers/input/keyboard/sprd_keypad.c
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 1d0c5f4c0f99..f35d9bf05f72 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -809,4 +809,14 @@ config KEYBOARD_CYPRESS_SF
>           To compile this driver as a module, choose M here: the
>           module will be called cypress-sf.
>
> +config KEYBOARD_SPRD
> +       tristate "Spreadtrum keyboard support"

%s/Spreadtrum/Unisoc

> +       depends on ARCH_SPRD || COMPILE_TEST
> +       select INPUT_MATRIXKMAP
> +       help
> +         Keypad controller is used to interface a SoC with a matrix-keypad device,
> +         The keypad controller supports multiple row and column lines.
> +         Say Y if you want to use the SPRD keyboard.
> +         Say M if you want to use the SPRD keyboard on SoC as module.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index aecef00c5d09..b747112461b1 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)               += stowaway.o
>  obj-$(CONFIG_KEYBOARD_ST_KEYSCAN)      += st-keyscan.o
>  obj-$(CONFIG_KEYBOARD_SUN4I_LRADC)     += sun4i-lradc-keys.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)          += sunkbd.o
> +obj-$(CONFIG_KEYBOARD_SPRD)            += sprd_keypad.o
>  obj-$(CONFIG_KEYBOARD_TC3589X)         += tc3589x-keypad.o
>  obj-$(CONFIG_KEYBOARD_TEGRA)           += tegra-kbc.o
>  obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)    += tm2-touchkey.o
> diff --git a/drivers/input/keyboard/sprd_keypad.c b/drivers/input/keyboard/sprd_keypad.c
> new file mode 100644
> index 000000000000..5b88072831e8
> --- /dev/null
> +++ b/drivers/input/keyboard/sprd_keypad.c
> @@ -0,0 +1,418 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Spreadtrum Communications Inc.

%s/Spreadtrum/Unisoc

> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/input.h>

Please sort the includes in alphabetical order.


> +
> +#define SPRD_KPD_CTRL                  0x0
> +#define SPRD_KPD_INT_EN                        0x4
> +#define SPRD_KPD_INT_RAW_STATUS                0x8
> +#define SPRD_KPD_INT_MASK_STATUS       0xc
> +#define SPRD_KPD_INT_CLR               0x10
> +#define SPRD_KPD_POLARITY              0x18
> +#define SPRD_KPD_DEBOUNCE_CNT          0x1c
> +#define SPRD_KPD_LONG_KEY_CNT          0x20
> +#define SPRD_KPD_SLEEP_CNT             0x24
> +#define SPRD_KPD_CLK_DIV_CNT           0x28
> +#define SPRD_KPD_KEY_STATUS            0x2c
> +#define SPRD_KPD_SLEEP_STATUS          0x30
> +#define SPRD_KPD_DEBUG_STATUS1         0x34
> +#define SPRD_KPD_DEBUG_STATUS2         0x38
> +
> +#define SPRD_KPD_EN                    BIT(0)
> +#define SPRD_KPD_SLEEP_EN              BIT(1)
> +#define SPRD_KPD_LONG_KEY_EN           BIT(2)
> +
> +#define SPRD_KPD_ROWS_MSK              GENMASK(23, 16)
> +#define SPRD_KPD_COLS_MSK              GENMASK(15, 8)
> +
> +#define SPRD_KPD_INT_ALL               GENMASK(11, 0)
> +#define SPRD_KPD_INT_DOWNUP            GENMASK(7, 0)
> +#define SPRD_KPD_INT_LONG              GENMASK(11, 8)
> +
> +#define SPRD_KPD_ROW_POLARITY          GENMASK(7, 0)
> +#define SPRD_KPD_COL_POLARITY          GENMASK(15, 8)
> +
> +#define SPRD_KPD_PRESS_INTX(X, V) \
> +       (((V) >> (X)) & GENMASK(0, 0))
> +#define SPRD_KPD_RELEASE_INTX(X, V) \
> +       (((V) >> ((X) + 4)) & GENMASK(0, 0))
> +#define SPRD_KPD_INTX_COL(X, V) \
> +       (((V) >> ((X) << 3)) & GENMASK(2, 0))
> +#define SPRD_KPD_INTX_ROW(X, V) \
> +       (((V) >> (((X) << 3) + 4)) & GENMASK(2, 0))
> +#define SPRD_KPD_INTX_DOWN(X, V) \
> +       (((V) >> (((X) << 3) + 7)) & GENMASK(0, 0))
> +
> +#define SPRD_KPD_RTC_HZ                        32768
> +#define SPRD_DEF_LONG_KEY_MS           1000
> +#define SPRD_DEF_DIV_CNT               1
> +#define SPRD_KPD_INT_CNT               4
> +#define SPRD_KPD_ROWS_MAX              8
> +#define SPRD_KPD_COLS_MAX              8
> +#define SPRD_KPD_ROWS_SHIFT            16
> +#define SPRD_KPD_COLS_SHIFT            8
> +
> +#define SPRD_CAP_WAKEUP                        BIT(0)
> +#define SPRD_CAP_LONG_KEY              BIT(1)
> +#define SPRD_CAP_REPEAT                        BIT(2)
> +
> +struct sprd_keypad_data {
> +       u32 rows_en; /* enabled rows bits */
> +       u32 cols_en; /* enabled cols bits */
> +       u32 num_rows;
> +       u32 num_cols;
> +       u32 capabilities;
> +       u32 debounce_ms;
> +       void __iomem *base;
> +       struct input_dev *input_dev;
> +       struct clk *enable;
> +       struct clk *rtc;
> +};
> +
> +static int sprd_keypad_enable(struct sprd_keypad_data *data)
> +{
> +       struct device *dev = data->input_dev->dev.parent;
> +       int ret;
> +
> +       ret = clk_prepare_enable(data->rtc);
> +       if (ret) {
> +               dev_err(dev, "enable rtc failed.\n");
> +               return ret;
> +       }
> +
> +       ret = clk_prepare_enable(data->enable);
> +       if (ret) {
> +               dev_err(dev, "enable keypad failed.\n");
> +               clk_disable_unprepare(data->rtc);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static void sprd_keypad_disable(struct sprd_keypad_data *data)
> +{
> +       clk_disable_unprepare(data->enable);
> +       clk_disable_unprepare(data->rtc);
> +}
> +
> +static irqreturn_t sprd_keypad_handler(int irq, void *id)
> +{
> +       struct platform_device *pdev = id;
> +       struct device *dev = &pdev->dev;
> +       struct sprd_keypad_data *data = platform_get_drvdata(pdev);
> +       u32 int_status = readl_relaxed(data->base + SPRD_KPD_INT_MASK_STATUS);
> +       u32 key_status = readl_relaxed(data->base + SPRD_KPD_KEY_STATUS);
> +       unsigned short *keycodes = data->input_dev->keycode;
> +       u32 row_shift = get_count_order(data->num_cols);
> +       unsigned short key;
> +       int col, row;
> +       u32 i;
> +
> +       writel_relaxed(SPRD_KPD_INT_ALL, data->base + SPRD_KPD_INT_CLR);
> +
> +       for (i = 0; i < SPRD_KPD_INT_CNT; i++) {
> +               if (SPRD_KPD_PRESS_INTX(i, int_status)) {
> +                       col = SPRD_KPD_INTX_COL(i, key_status);
> +                       row = SPRD_KPD_INTX_ROW(i, key_status);
> +                       key = keycodes[MATRIX_SCAN_CODE(row, col, row_shift)];
> +                       input_report_key(data->input_dev, key, 1);
> +                       input_sync(data->input_dev);
> +                       dev_dbg(dev, "%dD\n", key);
> +               }
> +               if (SPRD_KPD_RELEASE_INTX(i, int_status)) {
> +                       col = SPRD_KPD_INTX_COL(i, key_status);
> +                       row = SPRD_KPD_INTX_ROW(i, key_status);
> +                       key = keycodes[MATRIX_SCAN_CODE(row, col, row_shift)];
> +                       input_report_key(data->input_dev, key, 0);
> +                       input_sync(data->input_dev);
> +                       dev_dbg(dev, "%dU\n", key);
> +               }
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static u32 sprd_keypad_time_to_counter(u32 array_size, u32 time_ms)
> +{
> +       u32 value;
> +
> +       /*
> +        * y(ms) = (x + 1) * array_size
> +        *              / (32.768 / (clk_div_num + 1))
> +        * y means time in ms
> +        * x means counter
> +        * array_size equal to rows * columns
> +        * clk_div_num is devider to keypad source clock
> +        **/
> +       value = SPRD_KPD_RTC_HZ * time_ms;
> +       value = value / (1000 * array_size *
> +                       (SPRD_DEF_DIV_CNT + 1));
> +       if (value >= 1)
> +               value -= 1;
> +
> +       return value;
> +}
> +
> +static int sprd_keypad_hw_init(struct sprd_keypad_data *data)
> +{
> +       u32 value;
> +
> +       writel_relaxed(SPRD_KPD_INT_ALL, data->base + SPRD_KPD_INT_CLR);
> +       writel_relaxed(SPRD_KPD_ROW_POLARITY | SPRD_KPD_COL_POLARITY,
> +                       data->base + SPRD_KPD_POLARITY);
> +       writel_relaxed(SPRD_DEF_DIV_CNT, data->base + SPRD_KPD_CLK_DIV_CNT);
> +
> +       value = sprd_keypad_time_to_counter(data->num_rows * data->num_cols,
> +                                               SPRD_DEF_LONG_KEY_MS);
> +       writel_relaxed(value, data->base + SPRD_KPD_LONG_KEY_CNT);
> +
> +       value = sprd_keypad_time_to_counter(data->num_rows * data->num_cols,
> +                                               data->debounce_ms);
> +       writel_relaxed(value, data->base + SPRD_KPD_DEBOUNCE_CNT);
> +
> +       value = SPRD_KPD_INT_DOWNUP;
> +       if (data->capabilities & SPRD_CAP_LONG_KEY)
> +               value |= SPRD_KPD_INT_LONG;
> +       writel_relaxed(value, data->base + SPRD_KPD_INT_EN);
> +
> +       value = SPRD_KPD_RTC_HZ - 1;
> +       writel_relaxed(value, data->base + SPRD_KPD_SLEEP_CNT);
> +
> +       /* set enabled rows and columns */
> +       value = (((data->rows_en << SPRD_KPD_ROWS_SHIFT)
> +               | (data->cols_en << SPRD_KPD_COLS_SHIFT))
> +               & (SPRD_KPD_ROWS_MSK | SPRD_KPD_COLS_MSK))
> +               | SPRD_KPD_EN | SPRD_KPD_SLEEP_EN;
> +       if (data->capabilities & SPRD_CAP_LONG_KEY)
> +               value |= SPRD_KPD_LONG_KEY_EN;
> +       writel_relaxed(value, data->base + SPRD_KPD_CTRL);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused sprd_keypad_suspend(struct device *dev)
> +{
> +       struct sprd_keypad_data *data = dev_get_drvdata(dev);
> +
> +       if (!device_may_wakeup(dev))
> +               sprd_keypad_disable(data);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused sprd_keypad_resume(struct device *dev)
> +{
> +       struct sprd_keypad_data *data = dev_get_drvdata(dev);
> +       int ret = 0;
> +
> +       if (!device_may_wakeup(dev)) {
> +               ret = sprd_keypad_enable(data);
> +               if (ret)
> +                       return ret;
> +               ret = sprd_keypad_hw_init(data);
> +       }
> +
> +       return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(sprd_keypad_pm_ops,
> +                       sprd_keypad_suspend, sprd_keypad_resume);
> +
> +static int sprd_keypad_parse_dt(struct device *dev)
> +{
> +       struct sprd_keypad_data *data = dev_get_drvdata(dev);
> +       struct device_node *np = dev->of_node;
> +       int ret;
> +
> +       ret = matrix_keypad_parse_properties(dev, &data->num_rows, &data->num_cols);
> +       if (ret)
> +               return ret;
> +
> +       if (data->num_rows > SPRD_KPD_ROWS_MAX
> +               || data->num_cols > SPRD_KPD_COLS_MAX) {
> +               dev_err(dev, "invalid num_rows or num_cols\n");
> +               return -EINVAL;
> +       }
> +
> +       ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms);
> +       if (ret) {
> +               data->debounce_ms = 5;
> +               dev_warn(dev, "parse debounce-interval failed.\n");
> +       }
> +
> +       if (of_get_property(np, "linux,repeat", NULL))
> +               data->capabilities |= SPRD_CAP_REPEAT;
> +       if (of_get_property(np, "sprd,support_long_key", NULL))
> +               data->capabilities |= SPRD_CAP_LONG_KEY;
> +       if (of_get_property(np, "wakeup-source", NULL))
> +               data->capabilities |= SPRD_CAP_WAKEUP;
> +
> +       data->enable = devm_clk_get(dev, "enable");
> +       if (IS_ERR(data->enable)) {
> +               if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> +                       dev_err(dev, "get enable clk failed.\n");
> +               return PTR_ERR(data->enable);
> +       }
> +
> +       data->rtc = devm_clk_get(dev, "rtc");
> +       if (IS_ERR(data->rtc)) {
> +               if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> +                       dev_err(dev, "get rtc clk failed.\n");
> +               return PTR_ERR(data->rtc);
> +       }
> +
> +       return 0;
> +}
> +
> +static int sprd_keypad_probe(struct platform_device *pdev)
> +{
> +       struct sprd_keypad_data *data;
> +       struct resource *res;
> +       int ret, irq, i, j, row_shift;
> +       unsigned long rows, cols;
> +       unsigned short *keycodes;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       data->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(data->base)) {
> +               dev_err(&pdev->dev, "ioremap resource failed.\n");
> +               ret =  PTR_ERR(data->base);
> +               goto err_free;
> +       }
> +
> +       platform_set_drvdata(pdev, data);
> +       ret = sprd_keypad_parse_dt(&pdev->dev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "keypad parse dts failed.\n");
> +               goto err_free;
> +       }
> +
> +       data->input_dev = devm_input_allocate_device(&pdev->dev);
> +       if (IS_ERR(data->input_dev)) {
> +               dev_err(&pdev->dev, "alloc input dev failed.\n");
> +               ret =  PTR_ERR(data->input_dev);
> +               goto err_free;
> +       }
> +
> +       data->input_dev->name = "sprd-keypad";
> +       data->input_dev->phys = "sprd-key/input0";
> +
> +       ret = matrix_keypad_build_keymap(NULL, NULL, data->num_rows,
> +                                        data->num_cols, NULL, data->input_dev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "keypad build keymap failed.\n");
> +               goto err_free;
> +       }
> +
> +       rows = cols = 0;
> +       row_shift = get_count_order(data->num_cols);
> +       keycodes = data->input_dev->keycode;
> +       for (i = 0; i < data->num_rows; i++) {
> +               for (j = 0; j < data->num_cols; j++) {
> +                       if (!!keycodes[MATRIX_SCAN_CODE(i, j, row_shift)]) {
> +                               set_bit(i, &rows);
> +                               set_bit(j, &cols);
> +                       }
> +               }
> +       }
> +       data->rows_en = rows;
> +       data->cols_en = cols;
> +
> +       if (data->capabilities & SPRD_CAP_REPEAT)
> +               set_bit(EV_REP, data->input_dev->evbit);
> +
> +       input_set_drvdata(data->input_dev, data);
> +
> +       ret = sprd_keypad_enable(data);
> +       if (ret) {
> +               dev_err(&pdev->dev, "keypad enable failed.\n");
> +               goto err_free;
> +       }
> +
> +       ret = sprd_keypad_hw_init(data);
> +       if (ret) {
> +               dev_err(&pdev->dev, "keypad hw init failed.\n");
> +               goto clk_free;
> +       }
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               dev_err(&pdev->dev, "platform get irq failed.\n");
> +               goto clk_free;
> +       }
> +
> +       ret = devm_request_irq(&pdev->dev, irq, sprd_keypad_handler,
> +                               IRQF_NO_SUSPEND, dev_name(&pdev->dev), pdev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "request irq failed.\n");
> +               goto clk_free;
> +       }
> +
> +       ret = input_register_device(data->input_dev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "register input dev failed\n");
> +               goto clk_free;
> +       }
> +
> +       if (data->capabilities & SPRD_CAP_WAKEUP)
> +               device_init_wakeup(&pdev->dev, true);
> +
> +       return 0;
> +
> +clk_free:
> +       sprd_keypad_disable(data);
> +err_free:
> +       devm_kfree(&pdev->dev, data);
> +       return ret;
> +}
> +
> +static int sprd_keypad_remove(struct platform_device *pdev)
> +{
> +       struct sprd_keypad_data *data = platform_get_drvdata(pdev);
> +       int irq = platform_get_irq(pdev, 0);
> +
> +       if (data->capabilities & SPRD_CAP_WAKEUP)
> +               device_init_wakeup(&pdev->dev, false);
> +
> +       input_unregister_device(data->input_dev);
> +       devm_free_irq(&pdev->dev, irq, pdev);
> +       sprd_keypad_disable(data);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id sprd_keypad_match[] = {
> +       { .compatible = "sprd,sc9860-keypad", },
> +       {},
> +};
> +
> +static struct platform_driver sprd_keypad_driver = {
> +       .driver = {
> +               .name = "sprd-keypad",
> +               .owner = THIS_MODULE,
> +               .of_match_table = sprd_keypad_match,
> +               .pm = &sprd_keypad_pm_ops,
> +       },
> +       .probe = sprd_keypad_probe,
> +       .remove = sprd_keypad_remove,
> +};
> +
> +module_platform_driver(sprd_keypad_driver);
> +
> +MODULE_DESCRIPTION("Spreadtrum KPD Driver");

"Unisoc Keypad driver" would be better.

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Neo Hou <neo.hou@unisoc.com>");

You can remove this since this driver's author seems not him.

Thanks,
Chunyan

On Tue, Aug 08, 2023 at 03:25:01PM +0800, Wenhua Lin wrote:
> Add matrix keypad driver, support matrix keypad function.

Bindings should be prerequisite to this, meaning this has to be a series of
two patches.

...

> +	help
> +	  Keypad controller is used to interface a SoC with a matrix-keypad device,
> +	  The keypad controller supports multiple row and column lines.
> +	  Say Y if you want to use the SPRD keyboard.
> +	  Say M if you want to use the SPRD keyboard on SoC as module.

What will be the module name?

...

>  obj-$(CONFIG_KEYBOARD_ST_KEYSCAN)	+= st-keyscan.o
>  obj-$(CONFIG_KEYBOARD_SUN4I_LRADC)	+= sun4i-lradc-keys.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
> +obj-$(CONFIG_KEYBOARD_SPRD)		+= sprd_keypad.o

Are you sure it's ordered correctly?

>  obj-$(CONFIG_KEYBOARD_TC3589X)		+= tc3589x-keypad.o
>  obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
>  obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)	+= tm2-touchkey.o

...

> +/*
> + * Copyright (C) 2018 Spreadtrum Communications Inc.
> + */

A single line

...

Missing bits.h at least.
Revisit you header inclusion block to add exactly what you are using.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>

> +#include <linux/of.h>

Why?

> +#include <linux/input.h>

Order please?

...

> +#define SPRD_KPD_CTRL			0x0
> +#define SPRD_KPD_INT_EN			0x4
> +#define SPRD_KPD_INT_RAW_STATUS		0x8
> +#define SPRD_KPD_INT_MASK_STATUS	0xc

Use fixed width for register offset, like 0x00.

...

> +#define SPRD_DEF_LONG_KEY_MS		1000

	(1 * MSEC_PER_SEC)

?

...

> +struct sprd_keypad_data {
> +	u32 rows_en; /* enabled rows bits */
> +	u32 cols_en; /* enabled cols bits */

Why not bitmaps?

> +	u32 num_rows;
> +	u32 num_cols;
> +	u32 capabilities;
> +	u32 debounce_ms;
> +	void __iomem *base;
> +	struct input_dev *input_dev;
> +	struct clk *enable;
> +	struct clk *rtc;
> +};

...

> +	/*

> +	 * y(ms) = (x + 1) * array_size
> +	 *		/ (32.768 / (clk_div_num + 1))

One line.

+	 * Where:

> +	 * y means time in ms
> +	 * x means counter
> +	 * array_size equal to rows * columns
> +	 * clk_div_num is devider to keypad source clock
> +	 **/

Single asterisk is enough.

...

> +	value = value / (1000 * array_size *

value /= ... ?

MSEC_PER_SEC ?

> +			(SPRD_DEF_DIV_CNT + 1));

One line.

...

> +	if (value >= 1)
> +		value -= 1;

	if (value)
		value--;

...

> +	value = (((data->rows_en << SPRD_KPD_ROWS_SHIFT)
> +		| (data->cols_en << SPRD_KPD_COLS_SHIFT))
> +		& (SPRD_KPD_ROWS_MSK | SPRD_KPD_COLS_MSK))
> +		| SPRD_KPD_EN | SPRD_KPD_SLEEP_EN;

Move | & etc on previous lines respectively.

...

> +static int __maybe_unused sprd_keypad_suspend(struct device *dev)

No __maybe_unused.

...

> +static int __maybe_unused sprd_keypad_resume(struct device *dev)

Ditto.

...

> +static SIMPLE_DEV_PM_OPS(sprd_keypad_pm_ops,
> +			sprd_keypad_suspend, sprd_keypad_resume);

Use new DEFINE_*() PM macro.

...

> +static int sprd_keypad_parse_dt(struct device *dev)

dt -> fw

...

> +	if (data->num_rows > SPRD_KPD_ROWS_MAX
> +		|| data->num_cols > SPRD_KPD_COLS_MAX) {

Move the || to the previous line.

> +		dev_err(dev, "invalid num_rows or num_cols\n");
> +		return -EINVAL;
> +	}

...

> +	ret = of_property_read_u32(np, "debounce-interval", &data->debounce_ms);

device_property_...()

> +	if (ret) {
> +		data->debounce_ms = 5;

> +		dev_warn(dev, "parse debounce-interval failed.\n");

Why do we need this message?

> +	}

...

> +	if (of_get_property(np, "linux,repeat", NULL))
> +		data->capabilities |= SPRD_CAP_REPEAT;
> +	if (of_get_property(np, "sprd,support_long_key", NULL))
> +		data->capabilities |= SPRD_CAP_LONG_KEY;
> +	if (of_get_property(np, "wakeup-source", NULL))
> +		data->capabilities |= SPRD_CAP_WAKEUP;

device_property_*()

And I'm wondering if input subsystem already does this for you.

...

> +	data->enable = devm_clk_get(dev, "enable");
> +	if (IS_ERR(data->enable)) {
> +		if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> +			dev_err(dev, "get enable clk failed.\n");
> +		return PTR_ERR(data->enable);
> +	}
> +
> +	data->rtc = devm_clk_get(dev, "rtc");
> +	if (IS_ERR(data->rtc)) {
> +		if (PTR_ERR(data->enable) != -EPROBE_DEFER)
> +			dev_err(dev, "get rtc clk failed.\n");
> +		return PTR_ERR(data->rtc);
> +	}

Why not bulk?
Why not _enabled() variant?

> +	return 0;
> +}

...

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

> +	if (IS_ERR(data->base)) {

> +		dev_err(&pdev->dev, "ioremap resource failed.\n");

Dup message.

> +		ret =  PTR_ERR(data->base);
> +		goto err_free;
> +	}

...

> +	data->input_dev = devm_input_allocate_device(&pdev->dev);
> +	if (IS_ERR(data->input_dev)) {
> +		dev_err(&pdev->dev, "alloc input dev failed.\n");
> +		ret =  PTR_ERR(data->input_dev);

Too many spaces.

> +		goto err_free;

Here and elsewhere in ->probe() use return dev_err_probe() approach as Dmitry
nowadays is okay with that.

> +	}

...

> +	ret = matrix_keypad_build_keymap(NULL, NULL, data->num_rows,
> +					 data->num_cols, NULL, data->input_dev);

Reindent this to be split logically.

> +	if (ret) {
> +		dev_err(&pdev->dev, "keypad build keymap failed.\n");
> +		goto err_free;
> +	}

...

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {

> +		dev_err(&pdev->dev, "platform get irq failed.\n");

Dup message.

> +		goto clk_free;
> +	}

...

> +clk_free:
> +	sprd_keypad_disable(data);

See above how this can be avoided.

...

> +err_free:
> +	devm_kfree(&pdev->dev, data);

Huh?!

> +	return ret;

...

> +static const struct of_device_id sprd_keypad_match[] = {
> +	{ .compatible = "sprd,sc9860-keypad", },
> +	{},

No comma for the terminator.

> +};

...

> +static struct platform_driver sprd_keypad_driver = {
> +	.driver = {
> +		.name = "sprd-keypad",

> +		.owner = THIS_MODULE,

~15 years this is not needed.
Where did you get this code from? Time machine?

> +		.of_match_table = sprd_keypad_match,
> +		.pm = &sprd_keypad_pm_ops,
> +	},
> +	.probe = sprd_keypad_probe,
> +	.remove = sprd_keypad_remove,
> +};

> +

No need to have this blank line.

> +module_platform_driver(sprd_keypad_driver);

On Tue, Aug 08, 2023 at 10:07:21AM +0100, Jonathan Cameron wrote:
> On Tue, 8 Aug 2023 15:25:01 +0800
> Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:

...

> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	data->base = devm_ioremap_resource(&pdev->dev, res);
> 
> devm_platform_get_and_ioremap_resource)

Is res used anywhere else?

> > +	if (IS_ERR(data->base)) {
> > +		dev_err(&pdev->dev, "ioremap resource failed.\n");
> > +		ret =  PTR_ERR(data->base);
> > +		goto err_free;
> 
> Read up on what devm calls do - there is no need to manually free
> things that were allocated with them in the error paths or remove.
> So this should be a direct return. Also use
> 		return dev_err_probe(&pdev->dev, PTR_ERR(data->base),
> 				     "....")

Btw, with

	struct device *dev = &pdev->dev;

all these become neater.

> It both creates neater code and for cases where deferred probing
> is possible you will get a nice message on 'why' registered for
> debug purposes.
> 
> > +	}

Hi--

On 8/8/23 00:25, Wenhua Lin wrote:
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 1d0c5f4c0f99..f35d9bf05f72 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -809,4 +809,14 @@ config KEYBOARD_CYPRESS_SF
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cypress-sf.
>  
> +config KEYBOARD_SPRD
> +	tristate "Spreadtrum keyboard support"
> +	depends on ARCH_SPRD || COMPILE_TEST
> +	select INPUT_MATRIXKMAP
> +	help
> +	  Keypad controller is used to interface a SoC with a matrix-keypad device,

	                                         an SoC                     device.

> +	  The keypad controller supports multiple row and column lines.
> +	  Say Y if you want to use the SPRD keyboard.
> +	  Say M if you want to use the SPRD keyboard on SoC as module.
> +
>  endif

On Tue, Aug 8, 2023 at 4:02 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Aug 8, 2023, at 09:25, Wenhua Lin wrote:
> > Add matrix keypad driver, support matrix keypad function.
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
>
> Looks fine to me, just one minor thing to remember:
>
> > +static int __maybe_unused sprd_keypad_resume(struct device *dev)
> > +{
> > +     struct sprd_keypad_data *data = dev_get_drvdata(dev);
> > +     int ret = 0;
> > +
> > +     if (!device_may_wakeup(dev)) {
> > +             ret = sprd_keypad_enable(data);
> > +             if (ret)
> > +                     return ret;
> > +             ret = sprd_keypad_hw_init(data);
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(sprd_keypad_pm_ops,
> > +                     sprd_keypad_suspend, sprd_keypad_resume);
> > +
>
> SIMPLE_DEV_PM_OPS() is deprecated, please use the new
> DEFINE_SIMPLE_DEV_PM_OPS() for all new drivers, and
> remove the __maybe_unused annotation that is no longer
> needed with that.
>
> With that addressed (for the driver in general, I know nothing
> about the drivers/input specifics)
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
>
>      Arnd

Hi Arnd:
   We will fix the problem of SIMPLE_DEV_PM_OPS in patch v2.

Thanks
Wenhua.Lin