Re: [PATCH v7 2/3] drivers: input: keyboard: Add mtk keypad driver

From: Andy Shevchenko
Date: Thu May 14 2020 - 06:27:03 EST


On Thu, May 14, 2020 at 02:17:48PM +0800, Fengping Yu wrote:
> From: "fengping.yu" <fengping.yu@xxxxxxxxxxxx>
>
> This adds matrix keypad support for Mediatek SoCs.

...

> +config KEYBOARD_MTK_KPD
> + tristate "MediaTek Keypad Support"

> + depends on OF && HAVE_CLK

What makes it OF dependent?

> + help
> + Say Y here if you want to use the keypad on MediaTek SoCs.
> + If unsure, say N.
> + To compile this driver as a module, choose M here: the
> + module will be called mtk-kpd.

...

> +#define KPD_DEBOUNCE_MAX_US 256000 /*256ms */

Comment, besides missed space, is redundant. That's how we use unit suffixes in
the definitions.

...

> +static const struct regmap_config keypad_regmap_cfg = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = sizeof(u32),

> + .max_register = 0x0024,

Can it be definition?

> +};

...

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

+ blank line here.

> + keypad->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(keypad->base))
> + return PTR_ERR(keypad->base);

...

> + if (debounce > KPD_DEBOUNCE_MAX_US) {
> + dev_err(&pdev->dev, "Debounce time exceeds the maximum allowed time 256ms\n");

...%dus\n", KPD_DEBOUNCE_MAX_US);
or
...%dms\n", KPD_DEBOUNCE_MAX_US / USEC_PER_MSEC);

> + return -EINVAL;
> + }

...

> + keypad_pinctrl = devm_pinctrl_get(&pdev->dev);

> + if (IS_ERR(keypad_pinctrl)) {
> + return PTR_ERR(keypad_pinctrl);
> + }

Extra {}.

...

> + kpd_default = pinctrl_lookup_state(keypad_pinctrl, "default");
> + if (IS_ERR(kpd_default)) {

> + dev_err(&pdev->dev, "No default pinctrl state\n");

Isn't it done by pin control core?

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

And basically entire part is duplicating device core part? (Look at dd.c)

...

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

> + dev_err(&pdev->dev, "Failed to get irq\n");

This duplicates what platform core does.

> + return -irqnr;

- ?!

> + }

--
With Best Regards,
Andy Shevchenko