Messages in this thread | | | Date | Thu, 14 May 2020 13:27:01 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v7 2/3] drivers: input: keyboard: Add mtk keypad driver |
| |
On Thu, May 14, 2020 at 02:17:48PM +0800, Fengping Yu wrote: > From: "fengping.yu" <fengping.yu@mediatek.com> > > 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
| |