Hi Roland,

On Wed, May 16, 2012 at 10:25:54PM +0200, Roland Stigge wrote:

Hmm, this is not usedul if it is hidden in this driver. You need to
decide if you want to split it into a separate header file and place it
in include/linux/input/... or include/linux/platform_data/... so that it
can be used by the platform code, or make your driver pure DT driver and
have it depend on DT support and require all board carry DT bindings.

+
+struct lpc32xx_kscan_drv {
+	struct input_dev *input;
+	struct lpc32xx_kscan_cfg *kscancfg;
+	struct clk *clk;
+	void __iomem *kscan_base;
+	int irq;
+	u8 lastkeystates[8];
+};
+
+static void lpc32xx_mod_states(struct lpc32xx_kscan_drv *kscandat, int off)
+{
+	u8 key;
+	int j;
+	unsigned changed, scancode, keycode;
+
+	key = readl(LPC32XX_KS_DATA(kscandat->kscan_base, off));
+	changed = key ^ kscandat->lastkeystates[off];
+	if (changed) {
+		for (j = 0; j < kscandat->kscancfg->matrix_sz; j++) {
+			if (changed & (1 << j)) {
+				/* Key state changed, signal an event */
+				scancode =
+				    (j * kscandat->kscancfg->matrix_sz) + off;

Please use:

	scancode = MATRIX_SCAN_CODE(j, off, kscandat->row_shift);

BTW, instead of "j" and "off" maybe switch to "row" and "col" naming?

+				keycode = kscandat->kscancfg->keymap[scancode];
+				input_report_key(kscandat->input, keycode,
+						 key & (1 << j));

You might consider emitting MSC_SCAN as well.

+			}
+		}
+
+		kscandat->lastkeystates[off] = key;
+	}
+}
+
+static irqreturn_t lpc32xx_kscan_irq(int irq, void *dev_id)
+{
+	int i;
+	struct lpc32xx_kscan_drv *kscandat = dev_id;
+
+	for (i = 0; i < kscandat->kscancfg->matrix_sz; i++)
+		lpc32xx_mod_states(kscandat, i);
+
+	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
+
+	input_sync(kscandat->input);
+
+	return IRQ_HANDLED;
+}
+
+static int lpc32xx_kscan_open(struct input_dev *dev)
+{
+	struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev);
+
+	return clk_prepare_enable(kscandat->clk);
+}
+
+static void lpc32xx_kscan_close(struct input_dev *dev)
+{
+	struct lpc32xx_kscan_drv *kscandat = input_get_drvdata(dev);
+
+	clk_disable_unprepare(kscandat->clk);
+}
+
+#ifdef CONFIG_OF
+static struct lpc32xx_kscan_cfg *lpc32xx_parse_dt(struct device *dev)
+{
+	struct lpc32xx_kscan_cfg *pdata;
+	struct device_node *np = dev->of_node;
+	int key_count;
+	int i;
+	u32 rows, columns;
+	const __be32 *prop;
+	int proplen;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "could not allocate memory for platform data\n");
+		return NULL;
+	}
+
+	of_property_read_u32(np, "keypad,num-rows", &rows);
+	of_property_read_u32(np, "keypad,num-columns", &columns);
+	if (!rows || rows != columns) {
+		dev_err(dev, "rows and columns must be specified and be equal!\n");

Why?

+		return NULL;
+	}
+
+	pdata->matrix_sz = rows;
+	key_count = rows * columns;
+
+	of_property_read_u32(np, "nxp,debounce-delay-ms", &pdata->deb_clks);
+	of_property_read_u32(np, "nxp,scan-delay-ms", &pdata->scan_delay);
+	if (!pdata->deb_clks || !pdata->scan_delay) {
+		dev_err(dev, "debounce or scan delay not specified\n");
+		return NULL;
+	}
+
+	pdata->keymap = devm_kzalloc(dev, sizeof(int) * key_count, GFP_KERNEL);
+	if (!pdata->keymap) {
+		dev_err(dev, "could not allocate memory for keymap\n");
+		return NULL;
+	}
+
+	prop = of_get_property(np, "linux,keymap", &proplen);
+	if (!prop) {
+		dev_err(dev, "linux,keymap not specified\n");
+		return NULL;
+	}
+
+	if (proplen % sizeof(u32)) {
+		dev_err(dev, "bad linux,keymap property size: %d\n", proplen);
+		return NULL;
+	}
+	proplen /= sizeof(u32);
+
+	for (i = 0; i < proplen; i++) {
+		u32 tmp = be32_to_cpup(prop + i);
+		u32 row, column, key_code;
+
+		row = (tmp >> 24) & 0xff;
+		column = (tmp >> 16) & 0xff;
+		key_code = tmp & 0xffff;
+		pdata->keymap[row * columns + column] = key_code;
+	}

If you use matrix_keypad_build_keymap() most of this code will go away.

+
+	return pdata;
+}
+#else
+static struct lpc32xx_kscan_cfg *lpc32xx_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
+static int __devinit lpc32xx_kscan_probe(struct platform_device *pdev)
+{
+	struct lpc32xx_kscan_drv *kscandat;
+	struct resource *res;
+	int retval, i, keynum;
+
+	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");
+		retval = -EBUSY;
+		goto out1;
+	}
+
+	kscandat->kscan_base = devm_request_and_ioremap(&pdev->dev, res);

I would prefer you did not mix devm_* and nonmanaged resources in the
driver; stick with nonmanaged ones.

+	if (kscandat->kscan_base == NULL) {
+		dev_err(&pdev->dev, "failed to request and remap I/O memory\n");
+		retval = -EBUSY;
+		goto out1;
+	}
+
+	/* 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");
+		retval = -ENODEV;

Do not clobber retval:

		retval = PTR_ERR(kscandat->clk);

BTW, can we call it "error" pleasei since you return "0" explicitly on
success?

+		goto out1;
+	}
+
+	kscandat->irq = platform_get_irq(pdev, 0);
+	if ((kscandat->irq < 0) || (kscandat->irq >= NR_IRQS)) {
+		dev_err(&pdev->dev, "failed to get platform irq\n");
+		retval = -EINVAL;
+		goto out2;
+	}
+	retval = request_irq(kscandat->irq, lpc32xx_kscan_irq,
+			     0, pdev->name, kscandat);
+	if (retval) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		goto out2;
+	}
+
+	kscandat->input = input_allocate_device();
+	if (kscandat->input == NULL) {
+		dev_err(&pdev->dev, "failed to allocate device\n");
+		retval = -ENOMEM;
+		goto out3;
+	}
+
+	if (pdev->dev.of_node)
+		kscandat->kscancfg = lpc32xx_parse_dt(&pdev->dev);
+	else
+		kscandat->kscancfg = pdev->dev.platform_data;
+	if (!kscandat->kscancfg) {
+		dev_err(&pdev->dev, "failed to get platform data\n");
+		retval = -EINVAL;
+		goto out4;
+	}
+
+	platform_set_drvdata(pdev, kscandat);
+
+	/* Setup key input */
+	kscandat->input->evbit[0]	= BIT_MASK(EV_KEY);
+	kscandat->input->name		= pdev->name;
+	kscandat->input->phys		= "matrix-keys/input0";
+	kscandat->input->dev.parent	=  &pdev->dev;
+	kscandat->input->id.vendor	= 0x0001;
+	kscandat->input->id.product	= 0x0001;
+	kscandat->input->id.version	= 0x0100;
+	kscandat->input->open		= &lpc32xx_kscan_open;

No need for '&'.

+	kscandat->input->close		= &lpc32xx_kscan_close;

+	keynum = kscandat->kscancfg->matrix_sz * kscandat->kscancfg->matrix_sz;
+	for (i = 0; i < keynum; i++)
+		__set_bit(kscandat->kscancfg->keymap[i],
+			kscandat->input->keybit);

This should use matrix_keypad_build_keymap() with support for OF maps
(see 'next' branch in my tree).

+
+	input_set_drvdata(kscandat->input, kscandat);
+	input_set_capability(kscandat->input, EV_MSC, MSC_SCAN);
+
+	retval = input_register_device(kscandat->input);
+	if (retval) {
+		dev_err(&pdev->dev, "failed to register input device\n");
+		goto out4;
+	}
+
+	/* Configure the key scanner */
+	writel(kscandat->kscancfg->deb_clks,
+		LPC32XX_KS_DEB(kscandat->kscan_base));
+	writel(kscandat->kscancfg->scan_delay,
+		LPC32XX_KS_SCAN_CTL(kscandat->kscan_base));
+	writel(LPC32XX_KSCAN_FTST_USE32K_CLK,
+		LPC32XX_KS_FAST_TST(kscandat->kscan_base));
+	writel(kscandat->kscancfg->matrix_sz,
+		LPC32XX_KS_MATRIX_DIM(kscandat->kscan_base));
+	writel(1, LPC32XX_KS_IRQ(kscandat->kscan_base));
+
+	return 0;
+
+out4:
+	input_free_device(kscandat->input);
+out3:
+	free_irq(kscandat->irq, pdev);
+out2:
+	clk_put(kscandat->clk);
+out1:
+	kfree(kscandat);
+
+	return retval;
+}
+
+static int __devexit lpc32xx_kscan_remove(struct platform_device *pdev)
+{
+	struct lpc32xx_kscan_drv *kscandat = platform_get_drvdata(pdev);
+
+	free_irq(kscandat->irq, pdev);
+	input_unregister_device(kscandat->input);
+	clk_put(kscandat->clk);
+	kfree(kscandat);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM

Please use CONFIG_PM_SLEEP since you do not use runtime PM.

+static int lpc32xx_kscan_suspend(struct platform_device *pdev,
+	pm_message_t state)

Please convert to dev_pm_ops structure instead of legacy PM methods.

+{
+	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);
+
+	return 0;
+}
+
+static int lpc32xx_kscan_resume(struct platform_device *pdev)
+{
+	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));
+
+	return 0;
+}
+#else
+#define lpc32xx_kscan_suspend	NULL
+#define lpc32xx_kscan_resume	NULL
+#endif

Use "static SIMPLE_DEV_PM_OPS()" outside of #ifdef block. You won't need
defining lpc32xx_kscan_suspend/lpc32xx_kscan_resume to NULL.

+
+#ifdef CONFIG_OF
+static const struct of_device_id lpc32xx_kscan_match[] = {
+	{ .compatible = "nxp,lpc3220-key" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, lpc32xx_kscan_match);
+#endif
+
+static struct platform_driver lpc32xx_kscan_driver = {
+	.probe		= lpc32xx_kscan_probe,
+	.remove		= __devexit_p(lpc32xx_kscan_remove),
+	.suspend	= lpc32xx_kscan_suspend,
+	.resume		= lpc32xx_kscan_resume,

These 2 should go away.

+	.driver		= {
+		.name	= "lpc32xx_keys",
+		.of_match_table = of_match_ptr(lpc32xx_kscan_match),

		.pm	= lpc32xx_kscan_pm_ops,
		.owner	= THIS_MODULE,

+	}
+};
+
+module_platform_driver(lpc32xx_kscan_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Kevin Wells [off-list ref]");
+MODULE_AUTHOR("Roland Stigge [off-list ref]");
+MODULE_DESCRIPTION("Key scanner driver for LPC32XX devices");

Thanks.

-- 
Dmitry