* Re: [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one)
2007-09-20 3:49 [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one) Kristoffer Ericson
@ 2007-09-19 19:19 ` Dmitry Torokhov
2007-09-20 5:31 ` Kristoffer Ericson
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2007-09-19 19:19 UTC (permalink / raw)
To: Kristoffer Ericson; +Cc: linux-input
Hi Kristoffer,
On 9/19/07, Kristoffer Ericson <kristoffer.ericson@gmail.com> wrote:
> Greetings,
>
> Dmitry, I had some issues with the last patch I sent you. Something with the init sequence of devices, it basicly made it kernel panic. I've re-coded the driver to instead work as a platform driver, that way I have more control of which gets started first.
Yes, that's better. I guess the issue with the other driver was that
you did not setup driver->bus and the kernel blew up in
driver_register().
> +
> + /* start ssp and do a spinlock */
> + jornada_ssp_start();
> +
> + /* request x & y data */
> + if(jornada_ssp_inout(GETTOUCHSAMPLES) != TXDUMMY) {
> + jornada_ssp_end();
> + return IRQ_HANDLED;
> + }
> +
...
> + /* end ssp communication and release spinlock */
> + jornada_ssp_end();
I really prefer acquire/release type of functions to be in pairs. I.e.
I'd prefer something like:
+ /* start ssp and do a spinlock */
+ jornada_ssp_start();
+
+ /* request x & y data */
+ if (jornada_ssp_inout(GETTOUCHSAMPLES) != TXDUMMY) {
+ jornada_parse_data(...);
+ jornada_post_data(...)
+ }
+ jornada_ssp_end();
+ return IRQ_HANDLED;
> + /* report pen down */
> + input_report_key(jornada_ts->dev, BTN_TOUCH, 1);
> + input_report_abs(jornada_ts->dev, ABS_X, jornada_ts->x);
> + input_report_abs(jornada_ts->dev, ABS_Y, jornada_ts->y);
> + input_report_abs(jornada_ts->dev, ABS_PRESSURE, 1);
The device does not really report pressure value, so I'd not report
ABS_PRESSURE. BTN_TOUCH should be enogh.
> +
> +static int __init jornada720_ts_probe(struct platform_device *pdev)
__devinit.
> +{
> + struct jornada_ts *jornada_ts;
> + struct input_dev *input_dev;
> + int ret;
> +
> + input_dev = input_allocate_device();
> + if (!input_dev)
> + return -ENODEV;
> +
> + jornada_ts = kzalloc(sizeof(struct jornada_ts), GFP_KERNEL);
> + if (!jornada_ts)
> + goto failed1;
> +
> + platform_set_drvdata(pdev, jornada_ts);
> +
> + jornada_ts->dev = input_dev;
> +
> + input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
> + input_dev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
> + input_dev->keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
> +
> + input_dev->absmin[ABS_X] = 270;
> + input_dev->absmin[ABS_Y] = 180;
> + input_dev->absmax[ABS_X] = 3900;
> + input_dev->absmax[ABS_Y] = 3700;
I like using input_set_abs_params(input_dev, ABS_X, 270, 3900, 0, 0); ...
> +
> + input_dev->name = "HP Jornada 710/720/728 Touchscreen driver";
> +
> + ret = request_irq(IRQ_GPIO9,
> + jornada720_ts_interrupt,
> + IRQF_DISABLED | IRQF_TRIGGER_RISING,
> + "HP7XX Touchscreen driver", jornada_ts);
> + if (ret) {
> + printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
> + goto failed2;
> + }
> +
> + input_register_device(input_dev);
> + return 0;
Error handling please.
> +
> +failed1:
> + input_free_device(input_dev);
> + return -ENOMEM;
> +failed2:
> + kfree(jornada_ts);
> + input_free_device(input_dev);
This leaves invalid pointer in platform data structure. I'd recommend
calling platform_set_drvdata(pdev, jornada_ts); ony after making sure
that input_register_device() was successfull.
> + return -ENODEV;
If you use out-of-line error handling path please make it have single
return point. Btw, if you allocate all memity that is needed first and
then check for success you can fold the erro handling path somewhat.
> +}
> +
> +static int jornada720_ts_remove(struct platform_device *pdev)
__devexit.
> +{
> + struct jornada_ts *jornada_ts = platform_get_drvdata(pdev);
> +
> + free_irq(IRQ_GPIO9, pdev);
> + input_unregister_device(jornada_ts->dev);
> + kfree(jornada_ts);
> + return 0;
> +}
> +
> +static struct platform_driver jornada720_ts_driver = {
> + .probe = jornada720_ts_probe,
> + .remove = jornada720_ts_remove,
__devexit_p(jornada720_ts_remove)
> + .driver = {
> + .name = "jornada_ts_driver",
I don't think platform code will match on this name. I'd expect simply
"jornada_ts" but it really depends on how you create the platform
device.
> + },
> +};
> +
> +static int __devinit jornada720_ts_init(void)
This one should be simply __init.
> +{
> + return platform_driver_register(&jornada720_ts_driver);
> +}
> +
> +static void __exit jornada720_ts_exit(void)
> +{
> + platform_driver_unregister(&jornada720_ts_driver);
> +}
> +
> +module_init(jornada720_ts_init);
> +module_exit(jornada720_ts_exit);
>
>
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one)
2007-09-20 5:31 ` Kristoffer Ericson
@ 2007-09-19 20:46 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2007-09-19 20:46 UTC (permalink / raw)
To: Kristoffer Ericson; +Cc: linux-input
On 9/20/07, Kristoffer Ericson <kristoffer.ericson@gmail.com> wrote:
> + if (jornada_ssp_inout(GETTOUCHSAMPLES == TXDUMMY)) {
Are you sure? Looks like paren is pisplaced.
> +
> + ret = request_irq(IRQ_GPIO9,
> + jornada720_ts_interrupt,
> + IRQF_DISABLED | IRQF_TRIGGER_RISING,
> + "HP7XX Touchscreen driver", jornada_ts);
> + if (ret) {
> + printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
> + goto failed2;
> + }
> +
> + if (input_register_device(input_dev))
> + goto failed2;
You will leak IRQ_GPIO9 if input_register_device fails.
Please do not forget adding Signed-off-by: to all of your patches so I
don;t have to hunt for them through old e-mails. Thanks!
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one)
@ 2007-09-20 3:49 Kristoffer Ericson
2007-09-19 19:19 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Kristoffer Ericson @ 2007-09-20 3:49 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input
[-- Attachment #1: Type: text/plain, Size: 5479 bytes --]
Greetings,
Dmitry, I had some issues with the last patch I sent you. Something with the init sequence of devices, it basicly made it kernel panic. I've re-coded the driver to instead work as a platform driver, that way I have more control of which gets started first.
diff --git a/drivers/input/touchscreen/jornada720_ts.c b/drivers/input/touchscreen/jornada720_ts.c
new file mode 100644
index 0000000..fb0f665
--- /dev/null
+++ b/drivers/input/touchscreen/jornada720_ts.c
@@ -0,0 +1,182 @@
+/*
+ * drivers/input/touchscreen/jornada720_ts.c
+ *
+ * Copyright (C) 2007 Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
+ *
+ * Copyright (C) 2006 Filip Zyzniewski <filip.zyzniewski@tefnet.pl>
+ * based on HP Jornada 56x touchscreen driver by Alex Lange <chicken@handhelds.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * HP Jornada 710/720/729 Touchscreen Driver
+ */
+
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+
+#include <asm/hardware.h>
+#include <asm/arch/jornada720.h>
+
+MODULE_AUTHOR("Kristoffer Ericson <kristoffer.ericson@gmail.com>");
+MODULE_DESCRIPTION("HP Jornada 710/720/728 touchscreen driver");
+MODULE_LICENSE("GPLv2");
+
+struct jornada_ts {
+ struct input_dev *dev;
+ int X[3];
+ int Y[3];
+ int high_x, high_y;
+ int x, y;
+};
+
+static irqreturn_t jornada720_ts_interrupt(int irq, void *dev_id)
+{
+ struct jornada_ts *jornada_ts = dev_id;
+
+ /* If GPIO_GPIO9 is set to high */
+ if(GPLR & GPIO_GPIO(9)) {
+ /* report pen up */
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 0);
+ input_report_abs(jornada_ts->dev, ABS_PRESSURE, 0);
+ input_sync(jornada_ts->dev);
+
+ return IRQ_HANDLED;
+ }
+
+ /* start ssp and do a spinlock */
+ jornada_ssp_start();
+
+ /* request x & y data */
+ if(jornada_ssp_inout(GETTOUCHSAMPLES) != TXDUMMY) {
+ jornada_ssp_end();
+ return IRQ_HANDLED;
+ }
+
+ /* 3 low word X samples */
+ jornada_ts->X[0] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->X[1] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->X[2] = jornada_ssp_byte(TXDUMMY);
+
+ /* 3 low word Y samples */
+ jornada_ts->Y[0] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->Y[1] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->Y[2] = jornada_ssp_byte(TXDUMMY);
+
+ /* combined X samples bits */
+ jornada_ts->high_x = jornada_ssp_byte(TXDUMMY);
+
+ /* combined Y sample bits */
+ jornada_ts->high_y = jornada_ssp_byte(TXDUMMY);
+
+ /* end ssp communication and release spinlock */
+ jornada_ssp_end();
+
+ /* calculating actual values */
+ jornada_ts->X[0] |= (jornada_ts->high_x & 3) << 8;
+ jornada_ts->X[1] |= (jornada_ts->high_x & 0xc) << 6;
+ jornada_ts->X[2] |= (jornada_ts->high_x & 0x30) << 4;
+
+ jornada_ts->Y[0] |= (jornada_ts->high_y & 3) << 8;
+ jornada_ts->Y[1] |= (jornada_ts->high_y & 0xc) << 6;
+ jornada_ts->Y[2] |= (jornada_ts->high_y & 0x30) << 4;
+
+ /* simple averaging filter */
+ jornada_ts->x = (jornada_ts->X[0] + jornada_ts->X[1] + jornada_ts->X[2])/3;
+ jornada_ts->y = (jornada_ts->Y[0] + jornada_ts->Y[1] + jornada_ts->Y[2])/3;
+
+ /* report pen down */
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 1);
+ input_report_abs(jornada_ts->dev, ABS_X, jornada_ts->x);
+ input_report_abs(jornada_ts->dev, ABS_Y, jornada_ts->y);
+ input_report_abs(jornada_ts->dev, ABS_PRESSURE, 1);
+ input_sync(jornada_ts->dev);
+
+ return IRQ_HANDLED;
+}
+
+
+static int __init jornada720_ts_probe(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts;
+ struct input_dev *input_dev;
+ int ret;
+
+ input_dev = input_allocate_device();
+ if (!input_dev)
+ return -ENODEV;
+
+ jornada_ts = kzalloc(sizeof(struct jornada_ts), GFP_KERNEL);
+ if (!jornada_ts)
+ goto failed1;
+
+ platform_set_drvdata(pdev, jornada_ts);
+
+ jornada_ts->dev = input_dev;
+
+ input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
+ input_dev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
+ input_dev->keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
+
+ input_dev->absmin[ABS_X] = 270;
+ input_dev->absmin[ABS_Y] = 180;
+ input_dev->absmax[ABS_X] = 3900;
+ input_dev->absmax[ABS_Y] = 3700;
+
+ input_dev->name = "HP Jornada 710/720/728 Touchscreen driver";
+
+ ret = request_irq(IRQ_GPIO9,
+ jornada720_ts_interrupt,
+ IRQF_DISABLED | IRQF_TRIGGER_RISING,
+ "HP7XX Touchscreen driver", jornada_ts);
+ if (ret) {
+ printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
+ goto failed2;
+ }
+
+ input_register_device(input_dev);
+ return 0;
+
+failed1:
+ input_free_device(input_dev);
+ return -ENOMEM;
+failed2:
+ kfree(jornada_ts);
+ input_free_device(input_dev);
+ return -ENODEV;
+}
+
+static int jornada720_ts_remove(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts = platform_get_drvdata(pdev);
+
+ free_irq(IRQ_GPIO9, pdev);
+ input_unregister_device(jornada_ts->dev);
+ kfree(jornada_ts);
+ return 0;
+}
+
+static struct platform_driver jornada720_ts_driver = {
+ .probe = jornada720_ts_probe,
+ .remove = jornada720_ts_remove,
+ .driver = {
+ .name = "jornada_ts_driver",
+ },
+};
+
+static int __devinit jornada720_ts_init(void)
+{
+ return platform_driver_register(&jornada720_ts_driver);
+}
+
+static void __exit jornada720_ts_exit(void)
+{
+ platform_driver_unregister(&jornada720_ts_driver);
+}
+
+module_init(jornada720_ts_init);
+module_exit(jornada720_ts_exit);
[-- Attachment #2: hp7xx-touchscreen-support.patch --]
[-- Type: application/octet-stream, Size: 5207 bytes --]
diff --git a/drivers/input/touchscreen/jornada720_ts.c b/drivers/input/touchscreen/jornada720_ts.c
new file mode 100644
index 0000000..fb0f665
--- /dev/null
+++ b/drivers/input/touchscreen/jornada720_ts.c
@@ -0,0 +1,182 @@
+/*
+ * drivers/input/touchscreen/jornada720_ts.c
+ *
+ * Copyright (C) 2007 Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
+ *
+ * Copyright (C) 2006 Filip Zyzniewski <filip.zyzniewski@tefnet.pl>
+ * based on HP Jornada 56x touchscreen driver by Alex Lange <chicken@handhelds.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * HP Jornada 710/720/729 Touchscreen Driver
+ */
+
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+
+#include <asm/hardware.h>
+#include <asm/arch/jornada720.h>
+
+MODULE_AUTHOR("Kristoffer Ericson <kristoffer.ericson@gmail.com>");
+MODULE_DESCRIPTION("HP Jornada 710/720/728 touchscreen driver");
+MODULE_LICENSE("GPLv2");
+
+struct jornada_ts {
+ struct input_dev *dev;
+ int X[3];
+ int Y[3];
+ int high_x, high_y;
+ int x, y;
+};
+
+static irqreturn_t jornada720_ts_interrupt(int irq, void *dev_id)
+{
+ struct jornada_ts *jornada_ts = dev_id;
+
+ /* If GPIO_GPIO9 is set to high */
+ if(GPLR & GPIO_GPIO(9)) {
+ /* report pen up */
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 0);
+ input_report_abs(jornada_ts->dev, ABS_PRESSURE, 0);
+ input_sync(jornada_ts->dev);
+
+ return IRQ_HANDLED;
+ }
+
+ /* start ssp and do a spinlock */
+ jornada_ssp_start();
+
+ /* request x & y data */
+ if(jornada_ssp_inout(GETTOUCHSAMPLES) != TXDUMMY) {
+ jornada_ssp_end();
+ return IRQ_HANDLED;
+ }
+
+ /* 3 low word X samples */
+ jornada_ts->X[0] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->X[1] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->X[2] = jornada_ssp_byte(TXDUMMY);
+
+ /* 3 low word Y samples */
+ jornada_ts->Y[0] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->Y[1] = jornada_ssp_byte(TXDUMMY);
+ jornada_ts->Y[2] = jornada_ssp_byte(TXDUMMY);
+
+ /* combined X samples bits */
+ jornada_ts->high_x = jornada_ssp_byte(TXDUMMY);
+
+ /* combined Y sample bits */
+ jornada_ts->high_y = jornada_ssp_byte(TXDUMMY);
+
+ /* end ssp communication and release spinlock */
+ jornada_ssp_end();
+
+ /* calculating actual values */
+ jornada_ts->X[0] |= (jornada_ts->high_x & 3) << 8;
+ jornada_ts->X[1] |= (jornada_ts->high_x & 0xc) << 6;
+ jornada_ts->X[2] |= (jornada_ts->high_x & 0x30) << 4;
+
+ jornada_ts->Y[0] |= (jornada_ts->high_y & 3) << 8;
+ jornada_ts->Y[1] |= (jornada_ts->high_y & 0xc) << 6;
+ jornada_ts->Y[2] |= (jornada_ts->high_y & 0x30) << 4;
+
+ /* simple averaging filter */
+ jornada_ts->x = (jornada_ts->X[0] + jornada_ts->X[1] + jornada_ts->X[2])/3;
+ jornada_ts->y = (jornada_ts->Y[0] + jornada_ts->Y[1] + jornada_ts->Y[2])/3;
+
+ /* report pen down */
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 1);
+ input_report_abs(jornada_ts->dev, ABS_X, jornada_ts->x);
+ input_report_abs(jornada_ts->dev, ABS_Y, jornada_ts->y);
+ input_report_abs(jornada_ts->dev, ABS_PRESSURE, 1);
+ input_sync(jornada_ts->dev);
+
+ return IRQ_HANDLED;
+}
+
+
+static int __init jornada720_ts_probe(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts;
+ struct input_dev *input_dev;
+ int ret;
+
+ input_dev = input_allocate_device();
+ if (!input_dev)
+ return -ENODEV;
+
+ jornada_ts = kzalloc(sizeof(struct jornada_ts), GFP_KERNEL);
+ if (!jornada_ts)
+ goto failed1;
+
+ platform_set_drvdata(pdev, jornada_ts);
+
+ jornada_ts->dev = input_dev;
+
+ input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
+ input_dev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
+ input_dev->keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
+
+ input_dev->absmin[ABS_X] = 270;
+ input_dev->absmin[ABS_Y] = 180;
+ input_dev->absmax[ABS_X] = 3900;
+ input_dev->absmax[ABS_Y] = 3700;
+
+ input_dev->name = "HP Jornada 710/720/728 Touchscreen driver";
+
+ ret = request_irq(IRQ_GPIO9,
+ jornada720_ts_interrupt,
+ IRQF_DISABLED | IRQF_TRIGGER_RISING,
+ "HP7XX Touchscreen driver", jornada_ts);
+ if (ret) {
+ printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
+ goto failed2;
+ }
+
+ input_register_device(input_dev);
+ return 0;
+
+failed1:
+ input_free_device(input_dev);
+ return -ENOMEM;
+failed2:
+ kfree(jornada_ts);
+ input_free_device(input_dev);
+ return -ENODEV;
+}
+
+static int jornada720_ts_remove(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts = platform_get_drvdata(pdev);
+
+ free_irq(IRQ_GPIO9, pdev);
+ input_unregister_device(jornada_ts->dev);
+ kfree(jornada_ts);
+ return 0;
+}
+
+static struct platform_driver jornada720_ts_driver = {
+ .probe = jornada720_ts_probe,
+ .remove = jornada720_ts_remove,
+ .driver = {
+ .name = "jornada_ts_driver",
+ },
+};
+
+static int __devinit jornada720_ts_init(void)
+{
+ return platform_driver_register(&jornada720_ts_driver);
+}
+
+static void __exit jornada720_ts_exit(void)
+{
+ platform_driver_unregister(&jornada720_ts_driver);
+}
+
+module_init(jornada720_ts_init);
+module_exit(jornada720_ts_exit);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one)
2007-09-19 19:19 ` Dmitry Torokhov
@ 2007-09-20 5:31 ` Kristoffer Ericson
2007-09-19 20:46 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Kristoffer Ericson @ 2007-09-20 5:31 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Kristoffer Ericson, linux-input
[-- Attachment #1: Type: text/plain, Size: 10497 bytes --]
Oki, believe Ive adressed all your suggestions.
diff --git a/drivers/input/touchscreen/jornada720_ts.c b/drivers/input/touchscreen/jornada720_ts.c
new file mode 100644
index 0000000..124da74
--- /dev/null
+++ b/drivers/input/touchscreen/jornada720_ts.c
@@ -0,0 +1,172 @@
+/*
+ * drivers/input/touchscreen/jornada720_ts.c
+ *
+ * Copyright (C) 2007 Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
+ *
+ * Copyright (C) 2006 Filip Zyzniewski <filip.zyzniewski@tefnet.pl>
+ * based on HP Jornada 56x touchscreen driver by Alex Lange <chicken@handhelds.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * HP Jornada 710/720/729 Touchscreen Driver
+ */
+
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+
+#include <asm/hardware.h>
+#include <asm/arch/jornada720.h>
+
+MODULE_AUTHOR("Kristoffer Ericson <kristoffer.ericson@gmail.com>");
+MODULE_DESCRIPTION("HP Jornada 710/720/728 touchscreen driver");
+MODULE_LICENSE("GPLv2");
+
+struct jornada_ts {
+ struct input_dev *dev;
+ int X[3]; /* X sample values */
+ int Y[3]; /* Y sample values */
+ int high_x, high_y;
+ int x, y; /* calculated value */
+};
+
+static void jornada720_ts_calculate(struct jornada_ts *jornada_ts)
+{
+ jornada_ts->X[0] |= (jornada_ts->high_x & 3) << 8;
+ jornada_ts->X[1] |= (jornada_ts->high_x & 0xc) << 6;
+ jornada_ts->X[2] |= (jornada_ts->high_x & 0x30) << 4;
+
+ jornada_ts->Y[0] |= (jornada_ts->high_y & 3) << 8;
+ jornada_ts->Y[1] |= (jornada_ts->high_y & 0xc) << 6;
+ jornada_ts->Y[2] |= (jornada_ts->high_y & 0x30) << 4;
+
+ jornada_ts->x = ((jornada_ts->X[0] + jornada_ts->X[1] + jornada_ts->X[2]) / 3);
+ jornada_ts->y = ((jornada_ts->Y[0] + jornada_ts->Y[1] + jornada_ts->X[2]) / 3);
+};
+
+static void jornada720_ts_collectdata(struct jornada_ts *jornada_ts)
+{
+ int i;
+
+ /* 3 low word X samples */
+ for (i = 0; i < 3; i++)
+ jornada_ts->X[i] = jornada_ssp_byte(TXDUMMY);
+ /* 3 low word Y samples */
+ for (i = 0; i < 3; i++)
+ jornada_ts->Y[i] = jornada_ssp_byte(TXDUMMY);
+
+ /* combined x samples bits */
+ jornada_ts->high_x = jornada_ssp_byte(TXDUMMY);
+ /* combined y samples bits */
+ jornada_ts->high_y = jornada_ssp_byte(TXDUMMY);
+}
+
+static irqreturn_t jornada720_ts_interrupt(int irq, void *dev_id)
+{
+ struct jornada_ts *jornada_ts = dev_id;
+
+ /* If GPIO_GPIO9 is set to high then report pen up */
+ if(GPLR & GPIO_GPIO(9)) {
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 0);
+ input_sync(jornada_ts->dev);
+ return IRQ_HANDLED;
+ }
+
+ jornada_ssp_start();
+
+ /* proper reply to request is always TXDUMMY */
+ if (jornada_ssp_inout(GETTOUCHSAMPLES == TXDUMMY)) {
+ jornada720_ts_collectdata(jornada_ts);
+ jornada720_ts_calculate(jornada_ts);
+
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 1);
+ input_report_abs(jornada_ts->dev, ABS_X, jornada_ts->x);
+ input_report_abs(jornada_ts->dev, ABS_Y, jornada_ts->y);
+ }
+
+ jornada_ssp_end();
+ return IRQ_HANDLED;
+}
+
+static int __devinit jornada720_ts_probe(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts;
+ struct input_dev *input_dev;
+ int ret;
+
+ jornada_ts = kzalloc(sizeof(struct jornada_ts), GFP_KERNEL);
+ if (!jornada_ts)
+ return -ENOMEM;
+
+ input_dev = input_allocate_device();
+ if (!input_dev)
+ goto failed1;
+
+ jornada_ts->dev = input_dev;
+
+ input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
+ input_dev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
+ input_dev->keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
+
+ input_set_abs_params(input_dev, ABS_X, 270, 3900, 0, 0);
+ input_set_abs_params(input_dev, ABS_Y, 180, 3700, 0, 0);
+
+ input_dev->name = "HP Jornada 710/720/728 Touchscreen driver";
+
+ ret = request_irq(IRQ_GPIO9,
+ jornada720_ts_interrupt,
+ IRQF_DISABLED | IRQF_TRIGGER_RISING,
+ "HP7XX Touchscreen driver", jornada_ts);
+ if (ret) {
+ printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
+ goto failed2;
+ }
+
+ if (input_register_device(input_dev))
+ goto failed2;
+
+ platform_set_drvdata(pdev, jornada_ts);
+
+ return 0;
+
+failed2:
+ input_free_device(input_dev);
+failed1:
+ kfree(jornada_ts);
+ return -ENODEV;
+}
+
+static int __devexit jornada720_ts_remove(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts = platform_get_drvdata(pdev);
+
+ free_irq(IRQ_GPIO9, pdev);
+ input_unregister_device(jornada_ts->dev);
+ kfree(jornada_ts);
+ return 0;
+}
+
+static struct platform_driver jornada720_ts_driver = {
+ .probe = jornada720_ts_probe,
+ .remove = __devexit_p(jornada720_ts_remove),
+ .driver = {
+ .name = "jornada_ts",
+ },
+};
+
+static int __init jornada720_ts_init(void)
+{
+ return platform_driver_register(&jornada720_ts_driver);
+}
+
+static void __exit jornada720_ts_exit(void)
+{
+ platform_driver_unregister(&jornada720_ts_driver);
+}
+
+module_init(jornada720_ts_init);
+module_exit(jornada720_ts_exit);
On Wed, 19 Sep 2007 15:19:36 -0400
"Dmitry Torokhov" <dmitry.torokhov@gmail.com> wrote:
> Hi Kristoffer,
>
> On 9/19/07, Kristoffer Ericson <kristoffer.ericson@gmail.com> wrote:
> > Greetings,
> >
> > Dmitry, I had some issues with the last patch I sent you. Something with the init sequence of devices, it basicly made it kernel panic. I've re-coded the driver to instead work as a platform driver, that way I have more control of which gets started first.
>
> Yes, that's better. I guess the issue with the other driver was that
> you did not setup driver->bus and the kernel blew up in
> driver_register().
>
> > +
> > + /* start ssp and do a spinlock */
> > + jornada_ssp_start();
> > +
> > + /* request x & y data */
> > + if(jornada_ssp_inout(GETTOUCHSAMPLES) != TXDUMMY) {
> > + jornada_ssp_end();
> > + return IRQ_HANDLED;
> > + }
> > +
> ...
> > + /* end ssp communication and release spinlock */
> > + jornada_ssp_end();
>
> I really prefer acquire/release type of functions to be in pairs. I.e.
> I'd prefer something like:
>
> + /* start ssp and do a spinlock */
> + jornada_ssp_start();
> +
> + /* request x & y data */
> + if (jornada_ssp_inout(GETTOUCHSAMPLES) != TXDUMMY) {
> + jornada_parse_data(...);
> + jornada_post_data(...)
> + }
> + jornada_ssp_end();
> + return IRQ_HANDLED;
>
> > + /* report pen down */
> > + input_report_key(jornada_ts->dev, BTN_TOUCH, 1);
> > + input_report_abs(jornada_ts->dev, ABS_X, jornada_ts->x);
> > + input_report_abs(jornada_ts->dev, ABS_Y, jornada_ts->y);
> > + input_report_abs(jornada_ts->dev, ABS_PRESSURE, 1);
>
> The device does not really report pressure value, so I'd not report
> ABS_PRESSURE. BTN_TOUCH should be enogh.
>
> > +
> > +static int __init jornada720_ts_probe(struct platform_device *pdev)
>
> __devinit.
>
> > +{
> > + struct jornada_ts *jornada_ts;
> > + struct input_dev *input_dev;
> > + int ret;
> > +
> > + input_dev = input_allocate_device();
> > + if (!input_dev)
> > + return -ENODEV;
> > +
> > + jornada_ts = kzalloc(sizeof(struct jornada_ts), GFP_KERNEL);
> > + if (!jornada_ts)
> > + goto failed1;
> > +
> > + platform_set_drvdata(pdev, jornada_ts);
> > +
> > + jornada_ts->dev = input_dev;
> > +
> > + input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
> > + input_dev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
> > + input_dev->keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
> > +
> > + input_dev->absmin[ABS_X] = 270;
> > + input_dev->absmin[ABS_Y] = 180;
> > + input_dev->absmax[ABS_X] = 3900;
> > + input_dev->absmax[ABS_Y] = 3700;
>
> I like using input_set_abs_params(input_dev, ABS_X, 270, 3900, 0, 0); ...
>
> > +
> > + input_dev->name = "HP Jornada 710/720/728 Touchscreen driver";
> > +
> > + ret = request_irq(IRQ_GPIO9,
> > + jornada720_ts_interrupt,
> > + IRQF_DISABLED | IRQF_TRIGGER_RISING,
> > + "HP7XX Touchscreen driver", jornada_ts);
> > + if (ret) {
> > + printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
> > + goto failed2;
> > + }
> > +
> > + input_register_device(input_dev);
> > + return 0;
>
> Error handling please.
>
> > +
> > +failed1:
> > + input_free_device(input_dev);
> > + return -ENOMEM;
> > +failed2:
> > + kfree(jornada_ts);
> > + input_free_device(input_dev);
>
> This leaves invalid pointer in platform data structure. I'd recommend
> calling platform_set_drvdata(pdev, jornada_ts); ony after making sure
> that input_register_device() was successfull.
>
> > + return -ENODEV;
>
> If you use out-of-line error handling path please make it have single
> return point. Btw, if you allocate all memity that is needed first and
> then check for success you can fold the erro handling path somewhat.
>
> > +}
> > +
> > +static int jornada720_ts_remove(struct platform_device *pdev)
>
> __devexit.
>
> > +{
> > + struct jornada_ts *jornada_ts = platform_get_drvdata(pdev);
> > +
> > + free_irq(IRQ_GPIO9, pdev);
> > + input_unregister_device(jornada_ts->dev);
> > + kfree(jornada_ts);
> > + return 0;
> > +}
> > +
> > +static struct platform_driver jornada720_ts_driver = {
> > + .probe = jornada720_ts_probe,
> > + .remove = jornada720_ts_remove,
>
> __devexit_p(jornada720_ts_remove)
>
> > + .driver = {
> > + .name = "jornada_ts_driver",
>
> I don't think platform code will match on this name. I'd expect simply
> "jornada_ts" but it really depends on how you create the platform
> device.
>
> > + },
> > +};
> > +
> > +static int __devinit jornada720_ts_init(void)
>
> This one should be simply __init.
>
> > +{
> > + return platform_driver_register(&jornada720_ts_driver);
> > +}
> > +
> > +static void __exit jornada720_ts_exit(void)
> > +{
> > + platform_driver_unregister(&jornada720_ts_driver);
> > +}
> > +
> > +module_init(jornada720_ts_init);
> > +module_exit(jornada720_ts_exit);
> >
> >
>
>
> --
> Dmitry
[-- Attachment #2: hp7xx-touchscreen-support.patch --]
[-- Type: application/octet-stream, Size: 5108 bytes --]
diff --git a/drivers/input/touchscreen/jornada720_ts.c b/drivers/input/touchscreen/jornada720_ts.c
new file mode 100644
index 0000000..124da74
--- /dev/null
+++ b/drivers/input/touchscreen/jornada720_ts.c
@@ -0,0 +1,172 @@
+/*
+ * drivers/input/touchscreen/jornada720_ts.c
+ *
+ * Copyright (C) 2007 Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
+ *
+ * Copyright (C) 2006 Filip Zyzniewski <filip.zyzniewski@tefnet.pl>
+ * based on HP Jornada 56x touchscreen driver by Alex Lange <chicken@handhelds.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * HP Jornada 710/720/729 Touchscreen Driver
+ */
+
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+
+#include <asm/hardware.h>
+#include <asm/arch/jornada720.h>
+
+MODULE_AUTHOR("Kristoffer Ericson <kristoffer.ericson@gmail.com>");
+MODULE_DESCRIPTION("HP Jornada 710/720/728 touchscreen driver");
+MODULE_LICENSE("GPLv2");
+
+struct jornada_ts {
+ struct input_dev *dev;
+ int X[3]; /* X sample values */
+ int Y[3]; /* Y sample values */
+ int high_x, high_y;
+ int x, y; /* calculated value */
+};
+
+static void jornada720_ts_calculate(struct jornada_ts *jornada_ts)
+{
+ jornada_ts->X[0] |= (jornada_ts->high_x & 3) << 8;
+ jornada_ts->X[1] |= (jornada_ts->high_x & 0xc) << 6;
+ jornada_ts->X[2] |= (jornada_ts->high_x & 0x30) << 4;
+
+ jornada_ts->Y[0] |= (jornada_ts->high_y & 3) << 8;
+ jornada_ts->Y[1] |= (jornada_ts->high_y & 0xc) << 6;
+ jornada_ts->Y[2] |= (jornada_ts->high_y & 0x30) << 4;
+
+ jornada_ts->x = ((jornada_ts->X[0] + jornada_ts->X[1] + jornada_ts->X[2]) / 3);
+ jornada_ts->y = ((jornada_ts->Y[0] + jornada_ts->Y[1] + jornada_ts->X[2]) / 3);
+};
+
+static void jornada720_ts_collectdata(struct jornada_ts *jornada_ts)
+{
+ int i;
+
+ /* 3 low word X samples */
+ for (i = 0; i < 3; i++)
+ jornada_ts->X[i] = jornada_ssp_byte(TXDUMMY);
+ /* 3 low word Y samples */
+ for (i = 0; i < 3; i++)
+ jornada_ts->Y[i] = jornada_ssp_byte(TXDUMMY);
+
+ /* combined x samples bits */
+ jornada_ts->high_x = jornada_ssp_byte(TXDUMMY);
+ /* combined y samples bits */
+ jornada_ts->high_y = jornada_ssp_byte(TXDUMMY);
+}
+
+static irqreturn_t jornada720_ts_interrupt(int irq, void *dev_id)
+{
+ struct jornada_ts *jornada_ts = dev_id;
+
+ /* If GPIO_GPIO9 is set to high then report pen up */
+ if(GPLR & GPIO_GPIO(9)) {
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 0);
+ input_sync(jornada_ts->dev);
+ return IRQ_HANDLED;
+ }
+
+ jornada_ssp_start();
+
+ /* proper reply to request is always TXDUMMY */
+ if (jornada_ssp_inout(GETTOUCHSAMPLES == TXDUMMY)) {
+ jornada720_ts_collectdata(jornada_ts);
+ jornada720_ts_calculate(jornada_ts);
+
+ input_report_key(jornada_ts->dev, BTN_TOUCH, 1);
+ input_report_abs(jornada_ts->dev, ABS_X, jornada_ts->x);
+ input_report_abs(jornada_ts->dev, ABS_Y, jornada_ts->y);
+ }
+
+ jornada_ssp_end();
+ return IRQ_HANDLED;
+}
+
+static int __devinit jornada720_ts_probe(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts;
+ struct input_dev *input_dev;
+ int ret;
+
+ jornada_ts = kzalloc(sizeof(struct jornada_ts), GFP_KERNEL);
+ if (!jornada_ts)
+ return -ENOMEM;
+
+ input_dev = input_allocate_device();
+ if (!input_dev)
+ goto failed1;
+
+ jornada_ts->dev = input_dev;
+
+ input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);
+ input_dev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
+ input_dev->keybit[LONG(BTN_TOUCH)] = BIT(BTN_TOUCH);
+
+ input_set_abs_params(input_dev, ABS_X, 270, 3900, 0, 0);
+ input_set_abs_params(input_dev, ABS_Y, 180, 3700, 0, 0);
+
+ input_dev->name = "HP Jornada 710/720/728 Touchscreen driver";
+
+ ret = request_irq(IRQ_GPIO9,
+ jornada720_ts_interrupt,
+ IRQF_DISABLED | IRQF_TRIGGER_RISING,
+ "HP7XX Touchscreen driver", jornada_ts);
+ if (ret) {
+ printk(KERN_INFO "HP7XX TS : Unable to aquire irq!\n");
+ goto failed2;
+ }
+
+ if (input_register_device(input_dev))
+ goto failed2;
+
+ platform_set_drvdata(pdev, jornada_ts);
+
+ return 0;
+
+failed2:
+ input_free_device(input_dev);
+failed1:
+ kfree(jornada_ts);
+ return -ENODEV;
+}
+
+static int __devexit jornada720_ts_remove(struct platform_device *pdev)
+{
+ struct jornada_ts *jornada_ts = platform_get_drvdata(pdev);
+
+ free_irq(IRQ_GPIO9, pdev);
+ input_unregister_device(jornada_ts->dev);
+ kfree(jornada_ts);
+ return 0;
+}
+
+static struct platform_driver jornada720_ts_driver = {
+ .probe = jornada720_ts_probe,
+ .remove = __devexit_p(jornada720_ts_remove),
+ .driver = {
+ .name = "jornada_ts",
+ },
+};
+
+static int __init jornada720_ts_init(void)
+{
+ return platform_driver_register(&jornada720_ts_driver);
+}
+
+static void __exit jornada720_ts_exit(void)
+{
+ platform_driver_unregister(&jornada720_ts_driver);
+}
+
+module_init(jornada720_ts_init);
+module_exit(jornada720_ts_exit);
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-09-20 5:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-09-20 3:49 [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one) Kristoffer Ericson
2007-09-19 19:19 ` Dmitry Torokhov
2007-09-20 5:31 ` Kristoffer Ericson
2007-09-19 20:46 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).