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

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Fengping yu <fengping.yu@mediatek.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Yingjoe Chen <yingjoe.chen@mediatek.com>,
	linux-kernel@vger.kernel.org, wsd_upstream@mediatek.com
Subject: Re: [PATCH v3 2/2] add MediaTek keypad driver
Date: Fri, 3 Apr 2020 16:59:40 +0300	[thread overview]
Message-ID: <20200403135940.GN3676135@smile.fi.intel.com> (raw)
In-Reply-To: <20200403131419.6555-3-fengping.yu@mediatek.com>

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