Re: [PATCH] input: keyboard: Add sprd-keypad driver

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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);

-- 
With Best Regards,
Andy Shevchenko