Hi Cyril,

Thank you for making changes. Some more comments below.

On Thu, Sep 16, 2010 at 03:54:40PM -0400, Cyril Chemparathy wrote:

No need to cast.

+	int i, bit, val, row, col, code;
+	u32 curr_keys[3];
+	u32 change;
+
+	memset(curr_keys, 0, sizeof(curr_keys));
+	if (irq == kp->irq_press)
+		for (i = 0; i < 3; i++)
+			curr_keys[i] = keypad_read(kp, pres[i]);
+
+	for (i = 0; i < 3; i++) {
+		change = curr_keys[i] ^ kp->prev_keys[i];
+
+		while (change) {
+			bit     = fls(change) - 1;
+			change ^= BIT(bit);
+			val     = curr_keys[i] & BIT(bit);
+			bit    += i * 32;
+			row     = bit / KEYPAD_COLS;
+			col     = bit % KEYPAD_COLS;
+
+			code = MATRIX_SCAN_CODE(row, col, kp->row_shift);
+			input_event(kp->input_dev, EV_MSC, MSC_SCAN, code);
+			input_report_key(kp->input_dev, kp->keycodes[code],
+					 val);
+		}
+	}
+	input_sync(kp->input_dev);
+	memcpy(kp->prev_keys, curr_keys, sizeof(curr_keys));
+
+	if (irq == kp->irq_press)
+		keypad_write(kp, lock, 0); /* Allow hardware updates */
+
+	return IRQ_HANDLED;
+}
+
+static int keypad_start(struct input_dev *dev)
+{
+	struct keypad_data *kp = input_get_drvdata(dev);
+	unsigned long mask, debounce, clk_rate_khz;
+	int error;
+
+	clk_enable(kp->clk);
+	clk_rate_khz = clk_get_rate(kp->clk) / 1000;
+
+	/* Initialize device registers */
+	keypad_write(kp, mode, 0);
+
+	mask  = BITS(kp->rows) << KEYPAD_COLS;
+	mask |= BITS(kp->cols);
+	keypad_write(kp, mask, ~mask);
+
+	keypad_write(kp, pol, kp->active_low ? 0 : 0x3ffff);
+	debounce = kp->debounce_ms * clk_rate_khz;
+	debounce = min(debounce, DEBOUNCE_MAX);
+	debounce = max(debounce, DEBOUNCE_MIN);

	debounce = clamp(debounce, DEBOUNCE_MIN, DEBOUNCE_MAX);

Also I think defining DEBOUNCE_MIN and DEBOUNCE_MAX via BIT() is
confusing (I was expecting to see some bitwise ops); just use a
constant.

+	keypad_write(kp, dclock, debounce);
+	keypad_write(kp, rclock, 4 * debounce);
+	keypad_write(kp, stable_cnt, 3);
+	keypad_write(kp, in_en, 1);
+
+	error = request_irq(kp->irq_press, keypad_irq, 0, "key-press", kp);
+	if (error < 0) {
+		dev_err(kp->dev, "Could not allocate keypad press key irq\n");
+		clk_disable(kp->clk);
+		return error;
+	}
+
+	error = request_irq(kp->irq_release, keypad_irq, 0, "key-release", kp);
+	if (error < 0) {
+		dev_err(kp->dev, "Could not allocate keypad release key irq\n");
+		free_irq(kp->irq_press, kp);
+		clk_disable(kp->clk);
+		return error;
+	}
+

Request IRQ should go into keypad_probe(). The split is as follows - if
possible probe() should leave the device fully ready (all needed
resources acquired) but inactive, and open() should just activate the
device.

+	return 0;
+}
+
+static void keypad_stop(struct input_dev *dev)
+{
+	struct keypad_data *kp = input_get_drvdata(dev);
+
+	free_irq(kp->irq_press, kp);
+	free_irq(kp->irq_release, kp);
+	clk_disable(kp->clk);
+}
+
+static int __devinit keypad_probe(struct platform_device *pdev)
+{
+	const struct matrix_keypad_platform_data *pdata;
+	const struct matrix_keymap_data *keymap_data;
+	struct device *dev = &pdev->dev;
+	struct keypad_data *kp;
+	int error = 0, sz;
+	u32 rev = 0;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(dev, "cannot find device data\n");
+		return -EINVAL;
+	}
+
+	keymap_data = pdata->keymap_data;
+	if (!keymap_data) {
+		dev_err(dev, "cannot find keymap data\n");
+		return -EINVAL;
+	}
+
+	kp = kzalloc(sizeof(struct keypad_data), GFP_KERNEL);
+	if (!kp) {
+		dev_err(dev, "cannot allocate device info\n");
+		return -ENOMEM;
+	}
+
+	kp->dev  = dev;
+	kp->rows = pdata->num_row_gpios;
+	kp->cols = pdata->num_col_gpios;
+	kp->row_shift = get_count_order(kp->cols);
+	platform_set_drvdata(pdev, kp);
+
+	sz = (kp->rows << kp->row_shift) * sizeof(*kp->keycodes);
+	kp->keycodes = kzalloc(sz, GFP_KERNEL);
+	if (!kp->keycodes) {
+		dev_err(dev, "cannot allocate keymap info\n");
+		error = -ENOMEM;
+		goto error;
+	}

Why don't you allocate keymap together with the keypad structure:

struct keypad_data {
	...
	u32				prev_keys[3];
	unsigned short			keycodes[];
};

	keymap_size = (pdata->num_row_gpios <<
			get_order(pdata->num_col_gpios)) *
			sizeof(unsigned short);
	kp = kzalloc(sizeof(struct keypad_data) + keymap_size, GFP_KERNEL);
	...

+
+	kp->irq_press   = platform_get_irq_byname(pdev, "press");
+	kp->irq_release = platform_get_irq_byname(pdev, "release");
+	if (kp->irq_press < 0 || kp->irq_release < 0) {
+		dev_err(dev, "cannot determine device interrupts\n");
+		error = -ENODEV;
+		goto error;
+	}
+
+	kp->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!kp->res) {
+		dev_err(dev, "cannot determine register area\n");
+		error = -ENODEV;
+		goto error;
+	}
+
+	if (!request_mem_region(kp->res->start, resource_size(kp->res),
+				pdev->name)) {
+		dev_err(dev, "cannot claim register memory\n");
+		kp->res = NULL;
+		error = -EINVAL;
+		goto error;
+	}
+
+	kp->regs = ioremap(kp->res->start, resource_size(kp->res));
+	if (!kp->regs) {
+		dev_err(dev, "cannot map register memory\n");
+		error = -ENOMEM;
+		goto error;
+	}
+
+	kp->clk = clk_get(dev, NULL);
+	if (!kp->clk) {
+		dev_err(dev, "cannot claim device clock\n");
+		error = -EINVAL;
+		goto error;
+	}
+
+	kp->input_dev = input_allocate_device();
+	if (!kp->input_dev) {
+		dev_err(dev, "cannot allocate input device\n");
+		error = -ENOMEM;
+		goto error;
+	}
+	input_set_drvdata(kp->input_dev, kp);
+
+	kp->input_dev->name	  = pdev->name;
+	kp->input_dev->dev.parent = &pdev->dev;
+	kp->input_dev->open	  = keypad_start;
+	kp->input_dev->close	  = keypad_stop;
+	kp->input_dev->evbit[0]	  = BIT_MASK(EV_KEY);
+	if (!pdata->no_autorepeat)
+		kp->input_dev->evbit[0] |= BIT_MASK(EV_REP);
+
+	clk_enable(kp->clk);
+	rev = keypad_read(kp, rev);
+	kp->input_dev->id.bustype = BUS_HOST;
+	kp->input_dev->id.product = ((rev >>  8) & 0x07);
+	kp->input_dev->id.version = ((rev >> 16) & 0xfff);
+	clk_disable(kp->clk);
+
+	kp->input_dev->keycode     = kp->keycodes;
+	kp->input_dev->keycodesize = sizeof(*kp->keycodes);
+	kp->input_dev->keycodemax  = kp->rows << kp->row_shift;
+
+	matrix_keypad_build_keymap(keymap_data, kp->row_shift, kp->keycodes,
+				   kp->input_dev->keybit);
+
+	input_set_capability(kp->input_dev, EV_MSC, MSC_SCAN);
+
+	error = input_register_device(kp->input_dev);
+	if (error < 0) {
+		dev_err(dev, "Could not register input device\n");
+		goto error;
+	}
+
+	return 0;
+
+error:
+	if (kp->input_dev)
+		input_free_device(kp->input_dev);
+	if (kp->clk)
+		clk_put(kp->clk);
+	if (kp->regs)
+		iounmap(kp->regs);
+	if (kp->res)
+		release_mem_region(kp->res->start, resource_size(kp->res));

I generally prefer having multiple error path labels instead of checking
the state.

+	platform_set_drvdata(pdev, NULL);
+	kfree(kp->keycodes);
+	kfree(kp);
+	return error;
+}
+
+static int __devexit keypad_remove(struct platform_device *pdev)
+{
+	struct keypad_data *kp = platform_get_drvdata(pdev);
+
+	input_unregister_device(kp->input_dev);
+	clk_put(kp->clk);
+	iounmap(kp->regs);
+	release_mem_region(kp->res->start, resource_size(kp->res));
+	platform_set_drvdata(pdev, NULL);
+	kfree(kp->keycodes);
+	kfree(kp);
+
+	return 0;
+}
+
+static struct platform_driver keypad_driver = {
+	.probe		= keypad_probe,
+	.remove		= keypad_remove,
+	.driver.name	= "tnetv107x-keypad",
+	.driver.owner	= THIS_MODULE,
+};
+
+static int __init keypad_init(void)
+{
+	return platform_driver_register(&keypad_driver);
+}
+
+static void __exit keypad_exit(void)
+{
+	platform_driver_unregister(&keypad_driver);
+}
+
+module_init(keypad_init);
+module_exit(keypad_exit);
+
+MODULE_AUTHOR("Cyril Chemparathy");
+MODULE_DESCRIPTION("TNETV107X Keypad Driver");
+MODULE_ALIAS("platform: tnetv107x-keypad");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Thanks.

-- 
Dmitry