Re: [PATCH] HP Jornada 7xx keyboard support

Hi Kristoffer,

On 7/18/07, Kristoffer Ericson [off-list ref] wrote:

Greetings,

Thank you for your patch, I have a couple of comments.

Couldnt find any mailinglist, but please CC this there if there is one.

CCing linus-input...

+       /* Lets drag them out one at a time */
+       while (count-- > 0) {
+               /* Exchange TxDummy for a real key */
+               key = kbd_data = jornada_ssp_inout(TxDummy);
+
+               if (key < 0) { /* whoops, no more waiting */

key is declared as "unsigned char", how can this condition ever trigger?

+                   jornada_ssp_end(); /* End transmission */
+                   return IRQ_HANDLED;
+               }
+
+               if (key > 128) /* Key released */
+                       key = key - 128;
+
+               keycode = jornada_normal_keymap[key];
+
+               /* A key pressed down */
+               if (kbd_data < 128) {
+                       input_report_key(input_dev,keycode, 1);
+                       input_sync(input_dev);
+               }
+               else { /* Key released */
+                       input_report_key(input_dev, keycode, 0);
+                       input_sync(input_dev);
+               }

All of this can be probably written as:
        input_report_key(input_dev,
                jornada_normal_keymap[kbd_data & 0x7f],
                !(kbd_data & 0x80));
       input_sync(input_dev);

+       }
+       jornada_ssp_end(); /* End Transmission */
+       return IRQ_HANDLED;
+};
+
+static int jornada720_kbd_probe(struct platform_device *pdev)

__devinit?

+{
+       int i, ret;
+
+       input_dev = input_allocate_device();
+       if (!input_dev) {
+               return -ENOMEM;
+       };
+
+       input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP);
+       input_dev->keybit[LONG(KEY_SUSPEND)] |= BIT(KEY_SUSPEND);

Why special treatment of KEY_SUSPEND?

+
+       input_dev->name = "HP Jornada 720 keyboard";
+       input_dev->phys = "jornadakbd/input0";

please add "input_dev->dev.parent = &pdev->dev;" to place input device
into proper place in sysfs.

+
+       for (i=0 ; i<=128 ; i++) {
+               if (jornada_normal_keymap[i])
+                       set_bit(jornada_normal_keymap[i], input_dev->keybit);
+       }

Please allocate a copy of keymap and assign keycode, keycodemax and
keycodesize in input_dev so that keymap can be adjusted at runtime.

+
+       ret = request_irq(IRQ_GPIO0,
+                       jornada720_kbd_interrupt,
+                       IRQF_DISABLED | IRQF_TRIGGER_FALLING,
+                       "jornadakbd", input_dev);
+       if (ret) {
+               printk(KERN_INFO "Unable to grab IRQ for %s: %d\n", "jornadakbd", ret);
+               input_free_device(input_dev);
+               return ret;
+       }
+
+       input_register_device(input_dev);

Error hanling please.

+
+       return 0;
+};
+
+static int jornada720_kbd_remove(struct platform_device *pdev)

__devexit?

+{
+       free_irq(IRQ_GPIO0, input_dev);
+       input_unregister_device(input_dev);
+       return 0;
+}
+
+static struct platform_driver jornada720_kbd_driver = {
+        .driver  = {
+                  .name    = "jornada720_kbd",
+                },
+       .probe   = jornada720_kbd_probe,
+       .remove  = jornada720_kbd_remove,
+};
+
+static int __devinit jornada720_kbd_init(void)
+{
+        printk(KERN_INFO "HP Jornada 720 keyboard driver initialized\n");

I'd lose this printk - input core will print appropriate message when
input device is registered.

+       return platform_driver_register(&jornada720_kbd_driver);
+}
+
+static void __exit jornada720_kbd_exit(void)
+{
+       platform_driver_unregister(&jornada720_kbd_driver);
+}
+
+module_init(jornada720_kbd_init);
+module_exit(jornada720_kbd_exit);

Kconfig and Makefile changes seem to be missing..?

-- 
Dmitry