On Thu, May 14, 2020 at 02:17:48PM +0800, Fengping Yu wrote:
> From: "fengping.yu" <[email protected]>
>
> 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
