Re: [PATCH] intel_mid: Keypad Driver

There should be only one keymap per device. You can:

1. Add "direct" keys as an additional row to the main keymap
2. Set up a separate input device for direct keys.

Ok thats already fixed - the direct keys now use gpio_keys.

quoted

+config KEYBOARD_INTEL_MID
+	tristate "Intel MID keypad support"
+	depends on GPIO_LANGWELL
+	help
+	  Say Y if you want support for Intel MID keypad devices
+	  depends on GPIO_LANGWELL
+

To compile this driver as a module...

Added and put into alpha order (although one or two other entries are
not ?)

quoted

 obj-$(CONFIG_KEYBOARD_EP93XX)		+= ep93xx_keypad.o
+obj-$(CONFIG_KEYBOARD_INTEL_MID)	+= intel_mid_keypad.o

Alphabetic order please.

Fixed

quoted

+int __init mrst_keypad_set_pdata(const struct mrst_keypad_platform_data *data)
+{
+	if (mrst_keypad_pdata)
+		return -EBUSY;
+	if (!data)
+		return -EINVAL;
+
+	mrst_keypad_pdata = kmemdup(data, sizeof(*data), GFP_KERNEL);
+	if (!mrst_keypad_pdata)
+		return -ENOMEM;
+

Surely there is a better way...

The only thing that knows the keypad data for the platform is the
firmware it's not in the PCI configuration.

quoted

+			code = MATRIX_SCAN_CODE(row, col, MATRIX_ROW_SHIFT);

Please add reporting of MSC_SCAN as well.

Already done in the updates by Kristen - I'll send you the 'latest and
greatest' with the fixes from this after I've got feedback on the
firmware interface question further down.

quoted

+	err = mrst_keypad_gpio_init(keypad);

Setting up gpios should be part of probe, not open. Just rename
mrst_keypad_config() to mrst_keypad_open.

Done, I'm not aware of anyone muxing those GPIO pins so it should be no
problem.

Should be done in remove(). close() should only shut off the device.

Done

quoted

+}
+
+static int __devinit mrst_keypad_probe(struct pci_dev *pdev,
+			const struct pci_device_id *ent)
+{
+	struct mrst_keypad *keypad;
+	struct input_dev *input_dev;
+	int error;
+
+	/* Disable flag will turn off keypad support */
+	if (mrst_keypad_pdata && mrst_keypad_pdata->disable)

Why would you have a flag in platform data? If you do not want to use
the driver do not compile/load it.

We need to compile it in for such devices normally, distributions
require they can build a single image, and only the platform firmware
knows if it should be enabled (the information isn't in the PCI space)

I could turn the logic around a bit which might be cleaner - have the
firmware code export

	mrst_keypad_enabled();

and
	mrst_keypad_map();

?

quoted

+		return 0;
+
+	error = pci_enable_device(pdev);
+	if (error || (pdev->irq < 0)) {

Extra parenthesis.

Already fixed - along with the fact 0 isn't valid.

If you move pci_set_drvdata() here you won't need to clear it in error
path.

Fixed

quoted

+	if (input_dev)

No need to check.

quoted

+		input_free_device(input_dev);
+	kfree(keypad);

Ok didn't realise input_free_device(NULL) was guaranteed valid

Begs for a function as is called in several places.

Fixed

quoted

+	.remove		= __devexit_p(mrst_keypad_remove),
+#ifdef CONFIG_PM
+	.suspend	= NULL,
+	.resume	= NULL,

Why?

PM is added in the newest version so that goes sane