Re: [PATCH v3 2/2] add MediaTek keypad driver

Andy Shevchenko
2020-04-03T13:59:41Z

On Fri, Apr 03, 2020 at 09:14:23PM +0800, Fengping yu wrote:
> From: "fengping.yu" <fengping.yu@mediatek.com>

You have to give a proper commit message. Emptiness is not good enough.

> Signed-off-by: fengping.yu <fengping.yu@mediatek.com>

...

> +#include <linux/fs.h>

This is for..?

> +#include <linux/gpio.h>

Wrong header.

> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>

I hardly found any user of these two.

...

> +#define BITS_TO_U32(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(u32))

I believe this is in bits.h. But see below.

> +struct mtk_keypad {
> + struct input_dev *input_dev;
> + struct clk *clk;
> + void __iomem *base;
> + unsigned int irqnr;
> + bool wakeup;
> + u32 key_debounce;
> + u32 n_rows;
> + u32 n_cols;

> + DECLARE_BITMAP(keymap_state, KPD_NUM_BITS);

And where is bitmap.h?

> +};

...

> + for_each_set_bit(bit_nr, change, KPD_NUM_BITS) {
> + pressed = test_bit(bit_nr, new_state) == 0U;
> + dev_dbg(&keypad->input_dev->dev, "%s",
> + pressed ? "pressed" : "released");
> +

> + /* per 32bit register only use low 16bit as keypad mem register */

Indentation issue.

> + code = keycode[bit_nr - 16 * (BITS_TO_U32(bit_nr) - 1)];

For example,
128 - 16 * (4 - 1) = 80
135 - 16 * (5 - 1) = 71
Is this correct?

> + input_report_key(keypad->input_dev, code, pressed);
> + input_sync(keypad->input_dev);
> +
> + dev_dbg(&keypad->input_dev->dev,
> + "report Linux keycode = %d\n", code);
> + }

...

> + ret = of_property_read_u32(node, "mediatek,debounce-us",
> + &keypad->key_debounce);

Can't you use device property API instead?

> + keypad->wakeup = of_property_read_bool(node, "wakeup-source");

Ditto.

...

> + keypad_pinctrl = devm_pinctrl_get(dev);
> + if (IS_ERR(keypad_pinctrl)) {

> + dev_err(dev, "Cannot find keypad_pinctrl!\n");

Isn't it a duplicate and pin control actually does this for you?

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

> + dev_err(dev, "Cannot find ecall_state!\n");

Ditto.

> + return PTR_ERR(kpd_default);
> + }

> + return pinctrl_select_state(keypad_pinctrl, kpd_default);

Indentation issue.

> +}

...

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

> + dev_err(&pdev->dev, "KP iomap failed\n");

Duplicate noise.

> + return PTR_ERR(keypad->base);
> + }

...

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

> + dev_err(&pdev->dev, "KP get irqnr failed\n");

Duplicate noise.

> + ret = -keypad->irqnr;

Why -?

> + goto disable_kpd_clk;
> + }

--
With Best Regards,
Andy Shevchenko