Re: [PATCH 3/4] input: Add new sun4i-keypad driver

Hi Yassin,

On Wed, Sep 16, 2015 at 12:05:56AM +1000, yassinjaffer-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:

Maybe send sync only after you send all keys?

+		}
+
+		keypad->press_state[reg_nr] = edge ?  0 : key_scan[reg_nr];
+	}
+}
+
+static irqreturn_t sun4i_keypad_irq(int irq, void *dev_id)
+{
+	struct sun4i_keypad_data *keypad = dev_id;
+	u32 intr_status;
+
+	intr_status  = readl(keypad->base + KP_INT_STA);
+
+	/* release only gives a valid information for a single key release */
+	if (intr_status & KP_STA_REDGE)
+		sun4i_keypad_scan(keypad, true);
+
+	/* press does not give valid information when
+	 * multiple rows are selected
+	*/
+	if (intr_status & KP_STA_FEDGE)
+		sun4i_keypad_scan(keypad, false);
+
+	writel(intr_status, keypad->base + KP_INT_STA);
+
+	return IRQ_HANDLED;
+}
+
+static int sun4i_keypad_open(struct input_dev *dev)
+{
+	struct sun4i_keypad_data *keypad = input_get_drvdata(dev);
+	int reg_nr;
+
+	if (clk_prepare_enable(keypad->apb_clk))
+		return -EINVAL;
+
+	if (clk_prepare_enable(keypad->clk)) {
+		clk_disable_unprepare(keypad->apb_clk);
+		return -EINVAL;
+	}
+
+	for (reg_nr = 0; reg_nr < N_ROWS_REG; reg_nr++)
+		keypad->press_state[reg_nr] = ~(readl(keypad->base +
+							KP_INX_OFFSET(reg_nr)));
+
+	writel(KP_IRQ_FEDGE | KP_IRQ_REDGE, keypad->base + KP_INT_CFG);
+
+	writel(SCAN_CYCLE(keypad->scan_cycl) |
+		DBC_CYCLE(keypad->debounce_cycl), keypad->base + KP_TIMING);
+
+	writel(ENABLE(1) | ROWMASK(keypad->rows_en_mask) |
+		COLMASK(keypad->cols_en_mask), keypad->base + KP_CTL);
+
+	return 0;
+}
+
+static void sun4i_keypad_close(struct input_dev *dev)
+{
+	struct sun4i_keypad_data *keypad = input_get_drvdata(dev);
+
+	writel(ENABLE(0) | ROWMASK(keypad->rows_en_mask) |
+		COLMASK(keypad->cols_en_mask), keypad->base + KP_CTL);
+
+	clk_disable_unprepare(keypad->clk);
+	clk_disable_unprepare(keypad->apb_clk);
+}
+
+static int sun4i_keypad_parse_dt(struct device *dev,
+				 struct sun4i_keypad_data *keypad)
+{
+	struct device_node *np;
+
+	np = dev->of_node;
+	if (!np)
+		return -EINVAL;
+
+	of_property_read_u32(np, "debounce-cycle", &keypad->debounce_cycl);
+	if (keypad->debounce_cycl < MIN_CYCLE)
+		keypad->debounce_cycl = MIN_DBC_CYCLE;
+
+	of_property_read_u32(np, "scan-cycle", &keypad->scan_cycl);
+	if (keypad->scan_cycl < MIN_CYCLE)
+		keypad->scan_cycl = MIN_SCAN_CYCLE;
+
+	if (of_property_read_bool(np, "autorepeat"))
+		keypad->autorepeat = true;
+
+	return 0;
+}
+
+static int sun4i_keypad_probe(struct platform_device *pdev)
+{
+	struct sun4i_keypad_data *keypad;
+	struct device *dev = &pdev->dev;
+	int row, col, code;
+	int ret = 0;

Why do you need to initialize this variable? Also, my personal
preference to name variables that hold erorr codes "error".

+
+	keypad = devm_kzalloc(dev, sizeof(struct sun4i_keypad_data),
+			      GFP_KERNEL);
+	if (!keypad)
+		return -ENOMEM;
+
+	ret  = sun4i_keypad_parse_dt(dev, keypad);
+	if (ret)
+		return ret;
+
+	keypad->base = devm_ioremap_resource(dev,
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
+	if (IS_ERR(keypad->base))
+		return PTR_ERR(keypad->base);
+
+	keypad->dev = dev;
+	keypad->input = devm_input_allocate_device(dev);
+	if (!keypad->input)
+		return -ENOMEM;
+
+	keypad->input->name = pdev->name;
+	keypad->input->phys = "sun4i_keypad/input0";
+
+	keypad->input->id.bustype = BUS_HOST;
+	keypad->input->id.vendor  = 0x0001;
+	keypad->input->id.product = 0x0001;
+	keypad->input->id.version = 0x0100;
+
+	keypad->input->open = sun4i_keypad_open;
+	keypad->input->close = sun4i_keypad_close;
+
+	/* matrix keypad keymap as:
+	 * row << 24 | column << 16 | key-code
+	 */
+	ret = matrix_keypad_build_keymap(NULL, NULL,
+					 KP_MAX_ROWS,
+					 KP_MAX_COLS,
+					 keypad->keymap,
+					 keypad->input);
+	if (ret) {
+		dev_err(dev, "failed to build keymap\n");
+		return ret;
+	}
+
+	/* Search for enabled rows and cols */
+	for (row = 0; row < KP_MAX_ROWS; row++) {
+		for (col = 0; col < KP_MAX_COLS; col++) {
+			code = MATRIX_SCAN_CODE(row, col, KP_ROW_SHIFT);
+			if (keypad->keymap[code] != KEY_RESERVED) {
+				keypad->rows_en_mask |= 1 << row;
+				keypad->cols_en_mask |= 1 << col;
+			}
+		}
+	}
+
+	if (keypad->autorepeat)
+		__set_bit(EV_REP, keypad->input->evbit);
+
+	input_set_capability(keypad->input, EV_MSC, MSC_SCAN);
+	input_set_drvdata(keypad->input, keypad);
+
+	keypad->irq = platform_get_irq(pdev, 0);
+	if (keypad->irq < 0) {
+		dev_err(dev, "failed to get keypad IRQ\n");
+		return -ENXIO;

Why not

		return keypad->irq;

?

+	}
+
+	ret = devm_request_irq(dev, keypad->irq,
+			       sun4i_keypad_irq, 0,
+			       "sun4i-a10-keypad", keypad);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ\n");
+		return ret;
+	}
+
+	keypad->apb_clk = devm_clk_get(dev, "apb");
+	if (IS_ERR(keypad->apb_clk)) {
+		dev_err(dev, "failed to get a apb clock.\n");
+		return PTR_ERR(keypad->apb_clk);
+	}
+
+	keypad->clk = devm_clk_get(dev, "keypad");
+	if (IS_ERR(keypad->clk)) {
+		dev_err(dev, "failed to get a keypad clock.\n");
+		return PTR_ERR(keypad->clk);
+	}
+
+	ret = clk_set_rate(keypad->clk, KP_BASE_CLK);
+	if (ret) {
+		dev_err(dev, "set keypad base clock failed!\n");
+		return ret;
+	}
+
+	ret = input_register_device(keypad->input);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, keypad);
+
+	return 0;
+}
+
+static int sun4i_keypad_remove(struct platform_device *pdev)
+{
+	struct sun4i_keypad_data *keypad = platform_get_drvdata(pdev);
+
+	free_irq(keypad->irq, keypad);
+	input_unregister_device(keypad->input);
+	kfree(keypad);

You have never tried unlocking the module or unbinding the driver,
haven't you? free_irq() and kfree() can't be used with devm-alloctaed
resources. The good news is that you can simply remove
sun4i_keypad_remove() altogether, devm will take care of destroying all
resources, including input device, in right order.

+
+	return 0;
+}
+
+static const struct of_device_id sun4i_keypad_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-keypad", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sun4i_keypad_of_match);
+
+static struct platform_driver sun4i_keypad_driver = {
+	.driver = {
+		.name	= "sun4i-a10-keypad",
+		.of_match_table = of_match_ptr(sun4i_keypad_of_match),
+	},
+	.probe	= sun4i_keypad_probe,
+	.remove = sun4i_keypad_remove,
+};
+
+module_platform_driver(sun4i_keypad_driver);
+
+MODULE_DESCRIPTION("Allwinner sun4i keypad controller driver");
+MODULE_AUTHOR("Yassin Jaffer [off-list ref]");
+MODULE_LICENSE("GPL");
+
-- 
1.9.1

-- 
Dmitry