Re: [PATCH] intel_mid: Keypad Driver

On Tue, Feb 08, 2011 at 12:59:02PM +0000, Alan Cox wrote:

quoted

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.

Ah, great.

BTW, what about also using matrix_keypad and rotary_encoder (which may
need a bit of adjustments).

quoted

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 ?)

I am trying to keep them arranged but sometimes stuff slips in ;)

quoted

quoted

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

Alphabetic order please.

Fixed

[ ...regarding device tructure and it's 'enabled' fields... ]

BTW, I forgot to mention, using of 'bool' for boolean data is
encouraged.

quoted

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.

Your platform code could still set pci_dev->platform_data, like most of
other drivers that need to pass configuration bits to drivers. I do not
think PCI core touches platform_data...

quoted

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.

OK.

quoted

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.

quoted

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

Done

quoted

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();

?

If you tie it to platform data then I do not think you even need to have
mrst_keypad_enabled() - platforms that do not want to have driver
enabled won't be using it.

The question is - why would distributions not want to enable the driver
if device is there? Is there other intended users for teh underlying
device? And if so maybe implementing "naked" keypad driver is not the
proper solution and you should have this device as generic GPIO provider
and have board code create platfrom devices to which approprite consumer
drivers (such as matrix_keypad) would bind.

Thanks.

-- 
Dmitry