Re: [PATCH 2/7] input: add driver for tnetv107x on-chip keypad controller

On Mon, Sep 13, 2010 at 12:29:43PM -0400, Cyril Chemparathy wrote:

Please keep makefile and Kconfig sorted alphabetically please.

This is called xor.

Also, instead of printing key names just send EV_MSC/MSC_SCAN - much
more useful.

+	}
+
+	/* Update shadow copy */
+	for (i = 0; i < 3; i++)
+		kp->prev_keys[i] = kp->curr_keys[i];

memcpy(kp->prev_keys, kp->curr_keys, sizeof(kp->prev_keys));

+
+	input_sync(kp->input_dev);
+}
+
+static irqreturn_t keypad_irq_press(int irq, void *data)
+{
+	struct keypad_data *kp = (struct keypad_data *)data;
+	int i;
+
+	for (i = 0; i < 3; i++)
+		kp->curr_keys[i] = keypad_read(kp, pres[i]);
+	handle_change(kp);
+	keypad_write(kp, lock, 0); /* Allow hardware updates */
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t keypad_irq_release(int irq, void *data)
+{
+	struct keypad_data *kp = (struct keypad_data *)data;
+	int i;
+
+	/* Hardware says all keys have been released */
+	for (i = 0; i < 3; i++)
+		kp->curr_keys[i] = 0;

	memset(kp->curr_keys, 0, sizeof(kp->curr_keys));

+	handle_change(kp);
+	return IRQ_HANDLED;
+}
+
+static int tnetv107x_keypad_probe(struct platform_device *pdev)
+{
+	struct tnetv107x_keypad_data *pdata = pdev->dev.platform_data;

This should be const.

+	struct device *dev = &pdev->dev;
+	struct keypad_data *kp;
+	int i, ret = 0;
+	u32 rev = 0;
+
+	ret = -EINVAL;
+	if (!pdata) {
+		dev_err(dev, "cannot find device data\n");
+		return ret;
+	}

Please assign error code in the error branch. I also prefer variable
be called error if possible.

+
+	ret = -ENOMEM;
+	kp = kzalloc(sizeof(struct keypad_data), GFP_KERNEL);
+	if (!kp) {
+		dev_err(dev, "cannot allocate device info\n");
+		return ret;
+	}
+
+	dev_set_drvdata(dev, kp);
+	kp->data = *pdata;
+	kp->dev = dev;
+
+	ret = -ENOMEM;
+	kp->input_dev = input_allocate_device();
+	if (!kp->input_dev) {
+		dev_err(dev, "cannot allocate input device\n");
+		goto error0;
+	}
+
+	ret = -ENODEV;
+	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");
+		goto error1;
+	}
+
+	ret = -ENODEV;
+	kp->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!kp->res) {
+		dev_err(dev, "cannot determine register area\n");
+		goto error1;
+	}
+
+	ret = -EINVAL;
+	if (!request_mem_region(kp->res->start, resource_size(kp->res),
+				pdev->name)) {
+		dev_err(dev, "cannot claim register memory\n");
+		goto error1;
+	}
+
+	ret = -ENOMEM;
+	kp->regs = ioremap(kp->res->start, resource_size(kp->res));
+	if (!kp->regs) {
+		dev_err(dev, "cannot map register memory\n");
+		goto error2;
+	}
+
+	ret = -EINVAL;
+	kp->clk = clk_get(dev, NULL);
+	if (!kp->clk) {
+		dev_err(dev, "cannot claim device clock\n");
+		goto error3;
+	}
+	clk_enable(kp->clk);
+
+	/* Initialize device registers */
+	keypad_write(kp, mode, 0);
+	keypad_write(kp, mask, ~((((1 << kp->data.rows)-1) << 9) |
+			   (((1 << kp->data.cols)-1))));
+	keypad_write(kp, pol, 0x3ffff);
+	keypad_write(kp, dclock, kp->data.debounce);
+	keypad_write(kp, rclock, 4*kp->data.debounce);
+	keypad_write(kp, stable_cnt, kp->data.stable);
+
+	/* Enable Input */
+	keypad_write(kp, in_en, 0);
+	mdelay(1);
+	keypad_write(kp, in_en, 1);
+
+	ret = request_irq(kp->irq_press, keypad_irq_press, 0,
+			  "keypad-press", kp);
+	if (ret < 0) {
+		dev_err(dev, "Could not allocate keypad press key irq\n");
+		goto error4;
+	}
+
+	ret = request_irq(kp->irq_release, keypad_irq_release, 0,
+			  "keypad-release", kp);
+	if (ret < 0) {
+		dev_err(dev, "Could not allocate keypad release key irq\n");
+		goto error5;
+	}
+
+	set_bit(EV_KEY, kp->input_dev->evbit);
+	for (i = 0; i < kp->data.keymap_size; i++)
+		set_bit(kp->data.keymap[i] & KEY_MAX, kp->input_dev->keybit);
+
+	kp->input_dev->name = "tnetv107x-keypad";
+	kp->input_dev->phys = "tnetv107x-keypad/input0";
+	kp->input_dev->dev.parent = &pdev->dev;
+	kp->input_dev->id.bustype = BUS_HOST;
+	kp->input_dev->id.vendor = 0x0001;
+
+	rev = keypad_read(kp, rev);
+	kp->input_dev->id.product = ((rev >>  8) & 0x07);
+	kp->input_dev->id.version = ((rev >> 16) & 0xfff);
+
+	kp->input_dev->keycode = kp->data.keymap;
+	kp->input_dev->keycodesize = sizeof(int);
+	kp->input_dev->keycodemax = kp->data.keymap_size;

Since you allow changing data keymap should be moved into per-device
instance (platform data should be kept constant).

+	ret = input_register_device(kp->input_dev);
+	if (ret < 0) {
+		dev_err(dev, "Could not register input device\n");
+		goto error6;
+	}
+
+	dev_info(dev, "registered keypad device\n");

Input core already emits message when new device is registered, should
be enough.

+	return 0;
+
+error6:
+	free_irq(kp->irq_release, kp);
+error5:
+	free_irq(kp->irq_press, kp);
+error4:
+	clk_disable(kp->clk);
+	clk_put(kp->clk);
+error3:
+	iounmap(kp->regs);
+error2:
+	release_mem_region(kp->res->start, resource_size(kp->res));
+error1:
+	input_free_device(kp->input_dev);
+error0:
+	platform_set_drvdata(pdev, NULL);
+	kfree(kp);
+	return ret;
+}
+
+static int tnetv107x_keypad_remove(struct platform_device *pdev)
+{
+	struct keypad_data *kp = dev_get_drvdata(&pdev->dev);

platform_get_drvdata();

+
+	if (kp) {

How kp can be NULL here?

+		input_unregister_device(kp->input_dev);
+		free_irq(kp->irq_release, kp);
+		free_irq(kp->irq_press, kp);
+		clk_disable(kp->clk);
+		clk_put(kp->clk);
+		iounmap(kp->regs);
+		release_mem_region(kp->res->start, resource_size(kp->res));
+		input_free_device(kp->input_dev);

No free after unregister.

+		platform_set_drvdata(pdev, NULL);
+		kfree(kp);
+	}
+	return 0;
+}
+
+static int tnetv107x_keypad_suspend(struct platform_device *pdev,
+		pm_message_t state)
+{
+	/* Nothing yet */
+	return 0;
+}
+
+static int tnetv107x_keypad_resume(struct platform_device *pdev)
+{
+	/* Nothing yet */
+	return 0;
+}

If there is nothing yet - drop them.

+
+static struct platform_driver tnetv107x_keypad_driver = {
+	.probe		= tnetv107x_keypad_probe,
+	.remove		= tnetv107x_keypad_remove,
+	.suspend	= tnetv107x_keypad_suspend,
+	.resume		= tnetv107x_keypad_resume,
+	.driver.name	= "tnetv107x-keypad",

Please assign .owner = THIS_MODULE as well.

+};
+
+static int __init tnetv107x_keypad_init(void)
+{
+	return platform_driver_register(&tnetv107x_keypad_driver);
+}
+
+static void __exit tnetv107x_keypad_exit(void)
+{
+	platform_driver_unregister(&tnetv107x_keypad_driver);
+}
+
+module_init(tnetv107x_keypad_init);
+module_exit(tnetv107x_keypad_exit);
+
+MODULE_AUTHOR("Cyril Chemparathy");
+MODULE_DESCRIPTION("TNETV107X Keypad Driver");
+MODULE_LICENSE("GPL");

MODULE_ALIAS("platform: tnetv107x-keypad"); in case driver gets renamed.

Thanks.

-- 
Dmitry