Re: [PATCH] input: vt8500: Add power button keypad driver

quoted

+Example:
+	powerkey: pwrkey@0 {
+		compatible = "wm,power-keypad";
+		interrupts = <22>;
+		keymap = <116>; /* KEY_POWER */

Do we really need this in DT? I'd say just having it manageable from
userspace is enough.

Just seemed easier this way. Will be changed.

quoted

+config KEYBOARD_WMT_POWER_KEYPAD
+	tristate "Wondermedia Power keypad support"
+	depends on ARCH_VT8500

I'd feel safer if we added "depends on OF" as well.

ARCH_VT8500 always selects USE_OF, but I can add it as well if you think
its necessary.

quoted

+
+static void __iomem *pmc_base;
+static struct input_dev *kpad_power;
+static spinlock_t kpad_power_lock;
+static int power_button_pressed;
+static struct timer_list kpad_power_timer;
+static unsigned int kpad_power_code;

Maybe wrap it in a structure?

Will do.

quoted

+
+static inline void kpad_power_timeout(unsigned long fcontext)

Why inline? It can't be inlined anyway since its address is used.

Umm, no idea why this is inline. Will remove.

quoted

+{
+	u32 status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&kpad_power_lock, flags);
+
+	status = readl(pmc_base + 0x14);
+
+	if (power_button_pressed) {

This check is useless, you won't ever get here if button hasn't been
pressed.

Hmm, correct. Will fix.

quoted

+	status = readl(pmc_base + 0x14);
+	udelay(100);
+	writel(status, pmc_base + 0x14);
+
+	if (status & BIT(14)) {
+		if (!power_button_pressed) {

No need to do this check.

The hardware generates multiple interrupts when the button is held.
Without the !power_button_pressed, it will generate multiple pressed
events without releases, because the timer doesn't get to finish.

The interrupt is non-maskable.

quoted

+static int vt8500_pwr_kpad_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	u32 status;
+	int err;
+	int irq;
+
+	np = of_find_compatible_node(NULL, NULL, "via,vt8500-pmc");
+	if (!np) {
+		dev_err(&pdev->dev, "pmc node not found\n");
+		return -EINVAL;
+	}
+
+	pmc_base = of_iomap(np, 0);
+	if (!pmc_base) {
+		dev_err(&pdev->dev, "unable to map pmc memory\n");
+		return -ENOMEM;
+	}
+
+	np = pdev->dev.of_node;
+	if (!np) {
+		dev_err(&pdev->dev, "devicenode not found\n");
+		return -ENODEV;
+	}
+
+	err = of_property_read_u32(np, "keymap", &kpad_power_code);
+	if (err) {
+		dev_warn(&pdev->dev, "defaulting to KEY_POWER\n");
+		kpad_power_code = KEY_POWER;
+	}
+
+	/* set power button to soft-power */
+	status = readl(pmc_base + 0x54);
+	writel(status | 1, pmc_base + 0x54);
+
+	/* clear any pending interrupts */
+	status = readl(pmc_base + 0x14);
+	writel(status, pmc_base + 0x14);
+
+	kpad_power = input_allocate_device();
+	if (!kpad_power) {
+		dev_err(&pdev->dev, "failed to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&kpad_power_lock);
+	setup_timer(&kpad_power_timer, kpad_power_timeout,
+				       (unsigned long)kpad_power);
+
+	irq = irq_of_parse_and_map(np, 0);
+	err = request_irq(irq, &kpad_power_isr, 0, "pwrbtn", NULL);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return err;
+	}
+
+	kpad_power->evbit[0] = BIT_MASK(EV_KEY);
+	set_bit(kpad_power_code, kpad_power->keybit);
+
+	kpad_power->name = "wmt_power_keypad";
+	kpad_power->phys = "wmt_power_keypad";
+	kpad_power->keycode = &kpad_power_code;
+	kpad_power->keycodesize = sizeof(unsigned int);
+	kpad_power->keycodemax = 1;
+
+	err = input_register_device(kpad_power);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to register input device\n");

You either need to use managed resources or clean up after yourself;
leaking memory and other resources is not nice. Given that you are using
timer, which needs to be canceled, and I am not sure if your device
allows enabling/disabling generating interrupts while keeping the
interrupt handler registered, manual cleanup looks like the only option
for you.

BTW, where is your remove() method?

Eeek. Too much turkey :) I will tidy this up and resubmit.

Thanks for the quick review.

Regards
Tony P