Messages in this thread | | | Date | Fri, 3 Apr 2020 16:59:40 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 2/2] add MediaTek keypad driver |
| |
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
| |