On Tue, Jan 14, 2020 at 08:27:41PM +0800, fengping.yu wrote:

Signed-off-by: fengping.yu <redacted>

Add matrix keypad driver for MTK SoC.

Something wrong with ordering.

...

+obj-$(CONFIG_KEYBOARD_MTK) 		+= mtk-kpd.o

TABs / space mix. I think you ignored my comment.

...

+#include <linux/clk.h>
+#include <linux/fs.h>
+#include <linux/gpio.h>
+#include <linux/init.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>

...

+#define KPD_DEBOUNCE_MASK	GENMASK_ULL(13, 0)

ULL? Why?

...

+static void kpd_get_keymap_state(void __iomem *kp_base, u32 state[])

u32 *state

+{

+	memset_io(state, 0xff, KPD_NUM_MEMS);

This is strange. What is this for and why is it needed?

+	memcpy_fromio(state, kp_base, KPD_NUM_MEMS);
+}

...

+static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
+{
+	/* use _nosync to avoid deadlock */
+	struct mtk_keypad *keypad = dev_id;
+	unsigned short *keycode = keypad->input_dev->keycode;
+	u32 new_state[KPD_NUM_MEMS], mask;
+	unsigned long change = 0;
+	int bit_nr;
+	int pressed;
+	u32 row_shift = get_count_order(keypad->n_cols);
+	unsigned short code;
+	int i;
+
+	disable_irq_nosync(keypad->irqnr);
+
+	kpd_get_keymap_state(keypad->base, new_state);
+
+	for (i = 0; i < KPD_NUM_MEMS; i++) {
+		change = new_state[i] ^ keypad->keymap_state[i];
+		if (!change)
+			continue;
+
+		for_each_set_bit(bit_nr, &change, 32) {

+			mask = 1 << bit_nr;
+			pressed = (new_state[i] & mask) == 0U;

test_bit() ?

Can't new_state be unsigned long? In this case you may get rid of outer loop I
suppose.

+			dev_dbg(&keypad->input_dev->dev,
+				"%s", pressed ? "pressed" : "released");
+
+			code = keycode[MATRIX_SCAN_CODE(i, bit_nr, row_shift)];
+
+			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);
+		}
+	}
+
+	memcpy(keypad->keymap_state, new_state, sizeof(new_state));
+
+	enable_irq(keypad->irqnr);
+
+	return IRQ_HANDLED;
+}

...

+static int kpd_get_dts_info(struct mtk_keypad *keypad)
+{
+	int ret;
+	struct device *dev = keypad->input_dev->dev.parent;
+	struct device_node *node = dev->of_node;
+
+	ret = matrix_keypad_parse_properties(dev, &keypad->n_rows,
+					     &keypad->n_cols);

+

Redundant line.

+	if (ret) {
+		dev_err(dev, "failed to parse keypad params.\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(node, "mediatek,debounce-us",
+				   &keypad->key_debounce);
+	if (ret) {

+		pr_debug("read mediatek,debounce-us error.\n");

dev_dbg() ?

+		return ret;
+	}
+
+	keypad->wakeup = of_property_read_bool(node, "wakeup-source");
+
+	dev_dbg(dev, "n_row=%d n_col=%d debounce=%d.\n",
+		keypad->n_rows, keypad->n_cols,
+		keypad->key_debounce);
+
+	return 0;
+}

...

+static int kpd_gpio_init(struct device *dev)
+{
+	struct pinctrl *keypad_pinctrl;
+	struct pinctrl_state *kpd_default;
+
+	keypad_pinctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(keypad_pinctrl)) {
+		dev_err(dev, "Cannot find keypad_pinctrl!\n");

+

Redundant line

+		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);
+}

...

+	int err;

err or ret? Be consistent with the name of variable that keeps returned code.

+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "No device tree data present.\n");
+		return -EINVAL;
+	}

I think this is redundant. below calls will fail when property is not found.

+	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
+	if (!keypad)
+		return -ENOMEM;
+

+	memset(keypad->keymap_state, 0xff, sizeof(keypad->keymap_state));

bitmap_fill() ?

+	err = matrix_keypad_build_keymap(NULL, NULL,
+					keypad->n_rows,
+					keypad->n_cols,
+					NULL,
+					keypad->input_dev);

+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "get IO memory resource failed.\n");
+		return -ENODEV;
+	}
+
+	keypad->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

+	if (IS_ERR(keypad->base)) {
+		dev_err(&pdev->dev, "KP iomap failed\n");
+		return PTR_ERR(keypad->base);
+	}
+
+	writew((keypad->key_debounce * 32 / 1000 & KPD_DEBOUNCE_MASK),

Useless parentheses. It's not lisp!

+		keypad->base + KP_DEBOUNCE);

+	err = clk_prepare_enable(keypad->clk);
+	if (err) {
+		dev_err(&pdev->dev, "kpd-clk prepare enable failed.\n");
+		return err;
+	}
+
+	err = kpd_gpio_init(&pdev->dev);
+	if (err) {
+		dev_err(&pdev->dev, "gpio init failed\n");

+		return err;

Starting from here down the clocks left in a wrong state.

+	}
+
+	keypad->irqnr = platform_get_irq(pdev, 0);

+	if (!keypad->irqnr) {

This is wrong...

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

+		return -EINVAL;

...and so does this.

+	}

-- 
With Best Regards,
Andy Shevchenko