HI Roland,

On Sun, Jun 24, 2012 at 04:44:38PM +0200, Roland Stigge wrote:

This patch adds a driver for the key scan interface of the LPC32xx SoC

Signed-off-by: Roland Stigge <redacted>

This looks _very_ good, I just have a couple more comments:

+
+static int lpc32xx_kscan_open(struct input_dev *dev)
+{
+	struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev);
+
+	return clk_prepare_enable(kscandat->clk);

Don't you need to clear IRQ here, the same way you do it in resume?

+}
+
+static void lpc32xx_kscan_close(struct input_dev *dev)
+{
+	struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev);
+
+	clk_disable_unprepare(kscandat->clk);

And same here.

+}
+
+int lpc32xx_parse_dt(struct platform_device *pdev)

Static? Also why don't you pass kscandat instead of platform device?

+{
+	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	u32 rows, columns;
+
+	of_property_read_u32(np, "keypad,num-rows", &rows);
+	of_property_read_u32(np, "keypad,num-columns", &columns);
+	if (!rows || rows != columns) {
+		dev_err(&pdev->dev,
+			"rows and columns must be specified and be equal!\n");
+		return -EINVAL;
+	}
+
+	kscandat->matrix_sz = rows;
+	kscandat->row_shift = get_count_order(columns);
+
+	of_property_read_u32(np, "nxp,debounce-delay-ms", &kscandat->deb_clks);
+	of_property_read_u32(np, "nxp,scan-delay-ms", &kscandat->scan_delay);
+	if (!kscandat->deb_clks || !kscandat->scan_delay) {
+		dev_err(&pdev->dev, "debounce or scan delay not specified\n");
+		return -ENXIO;
+	}
+
+	kscandat->keymap = kzalloc(sizeof(kscandat->keymap[0]) *
+				(rows << kscandat->row_shift), GFP_KERNEL);
+	if (!kscandat->keymap) {
+		dev_err(&pdev->dev, "could not allocate memory for keymap\n");
+		return -ENOMEM;
+	}

I think this allocation belongs to the probe().

+
+	return 0;
+}
+
+static int __devinit lpc32xx_kscan_probe(struct platform_device *pdev)
+{
+	struct lpc32xx_kscan_drv *kscandat;
+	struct resource *res;
+	int error;
+	int irq;
+
+	kscandat = kzalloc(sizeof(struct lpc32xx_kscan_drv), GFP_KERNEL);
+	if (!kscandat) {
+		dev_err(&pdev->dev, "failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get platform I/O memory\n");
+		error = -EBUSY;
+		goto out1;
+	}
+
+	res = request_mem_region(res->start, resource_size(res), pdev->name);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to request I/O memory\n");
+		error = -EBUSY;
+		goto out1;
+	}
+	kscandat->io_p_start = res->start;
+	kscandat->io_p_size = resource_size(res);
+
+	kscandat->kscan_base = ioremap(res->start, resource_size(res));
+	if (!kscandat->kscan_base) {
+		dev_err(&pdev->dev, "failed to remap I/O memory\n");
+		error = -EBUSY;
+		goto out2;
+	}
+
+	/* Get the key scanner clock */
+	kscandat->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(kscandat->clk)) {
+		dev_err(&pdev->dev, "failed to get clock\n");
+		error = PTR_ERR(kscandat->clk);
+		goto out3;
+	}
+	clk_enable(kscandat->clk);

Why not clk_prepare_enable()? Also I think you need to move it down, to
where you actually write to the device's registers.

+
+	irq = platform_get_irq(pdev, 0);
+	if ((irq < 0) || (irq >= NR_IRQS)) {
+		dev_err(&pdev->dev, "failed to get platform irq\n");
+		error = -EINVAL;
+		goto out4;
+	}
+	error = request_irq(irq, lpc32xx_kscan_irq, 0, pdev->name, kscandat);
+	if (error) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		goto out4;
+	}
+
+	kscandat->input = input_allocate_device();
+	if (kscandat->input == NULL) {
+		dev_err(&pdev->dev, "failed to allocate device\n");
+		error = -ENOMEM;
+		goto out5;
+	}

This might be too late - IRQ is alreday registered and clock is enabled
- what is to stop interrupts to be delivered? I'd allocate the input
device together with kscandat structure.

+
+	platform_set_drvdata(pdev, kscandat);
+
+	error = lpc32xx_parse_dt(pdev);
+	if (error) {
+		dev_err(&pdev->dev, "failed to parse device tree\n");
+		goto out6;
+	}
+
+	/* Setup key input */
+	kscandat->input->evbit[0]	= BIT_MASK(EV_KEY);
+	kscandat->input->name		= pdev->name;
+	kscandat->input->phys		= "matrix-keys/input0";
+	kscandat->input->id.vendor	= 0x0001;
+	kscandat->input->id.product	= 0x0001;
+	kscandat->input->id.version	= 0x0100;
+	kscandat->input->open		= lpc32xx_kscan_open;
+	kscandat->input->close		= lpc32xx_kscan_close;
+	kscandat->input->dev.parent	= &pdev->dev;
+
+	error = matrix_keypad_build_keymap(NULL, NULL, kscandat->matrix_sz,
+					   kscandat->matrix_sz,
+					   kscandat->keymap, kscandat->input);
+	if (error) {
+		dev_err(&pdev->dev, "failed to build keymap\n");
+		goto out6;
+	}
+
+	input_set_drvdata(kscandat->input, kscandat);
+	input_set_capability(kscandat->input, EV_MSC, MSC_SCAN);
+
+	error = input_register_device(kscandat->input);
+	if (error) {
+		dev_err(&pdev->dev, "failed to register input device\n");
+		goto out6;
+	}
+

I think this needs to be done before you register input device as as
soon as it is registered it may be used.

+	/* Configure the key scanner */
+	writel(kscandat->deb_clks, LPC32XX_KS_DEB(kscandat->kscan_base));
+	writel(kscandat->scan_delay, LPC32XX_KS_SCAN_CTL(kscandat->kscan_base));
+	writel(LPC32XX_KSCAN_FTST_USE32K_CLK,
+	       LPC32XX_KS_FAST_TST(kscandat->kscan_base));
+	writel(kscandat->matrix_sz,
+	       LPC32XX_KS_MATRIX_DIM(kscandat->kscan_base));
+	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
+	clk_disable(kscandat->clk);

clk_disable_unprepare()?

+	return 0;
+
+out6:
+	input_free_device(kscandat->input);
+out5:
+	free_irq(irq, pdev);
+out4:
+	clk_disable(kscandat->clk);
+	clk_put(kscandat->clk);
+out3:
+	iounmap(kscandat->kscan_base);
+out2:
+	release_mem_region(kscandat->io_p_start, kscandat->io_p_size);
+out1:
+	kfree(kscandat);
+
+	return error;
+}
+
+static int __devexit lpc32xx_kscan_remove(struct platform_device *pdev)
+{
+	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+
+	input_unregister_device(kscandat->input);

Are we certain it is safe to free input device before freeing IRQ? I see
we have close(), still I'd feel safer if you swapper unregister and
free_irq().

+	free_irq(platform_get_irq(pdev, 0), pdev);
+	clk_put(kscandat->clk);
+	iounmap(kscandat->kscan_base);
+	release_mem_region(kscandat->io_p_start, kscandat->io_p_size);
+	kfree(kscandat);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int lpc32xx_kscan_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+
+	/* Clear IRQ and disable clock */
+	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
+	clk_disable_unprepare(kscandat->clk);
+

This may race with open()/close(). You need to take input_dev->mutex and
avoid touching the device if it sis not opened (users == 0).

+	return 0;
+}
+
+static int lpc32xx_kscan_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+
+	/* Enable clock and clear IRQ */
+	clk_prepare_enable(kscandat->clk);
+	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
+

Same here.

Thanks!

-- 
Dmitry