Re: [HP7XX/PATCH] - HP720 Jornada Touchscreen (Updated, please use this one)

[Kristoffer Ericson <kristoffer.ericson@gmail.com>]

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