HTC Dream for staging: small cleanups [was Re: Support for synaptic touchscreen in HTC dream]

[Pavel Machek <pavel@ucw.cz>]

Hi!

> > This adds support for synaptic touchscreen, used in HTC dream
> > cellphone.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> This is pretty large body of code, could we get a sign-off from Arve as
> well since he seems to be the author?

Yes; it is now in staging with his sign-offs.

> > +config TOUCHSCREEN_SYNAPTICS_I2C_RMI
> > +	tristate "Synaptics i2c touchscreen"
> > +	depends on I2C
> > +	help
> > +	  This enables support for Synaptics RMI over I2C based touchscreens.
> > +	  Such touchscreen is used in HTC Dream.
> > +
> 
> To compile this driver as a module...

Ok.

> > +static struct workqueue_struct *synaptics_wq;
> 
> Do we need a separate workqueue? Is reading the device that slow that we
> can use keventd?

Added to the TODO.

> > +	int snap_up[2];
> > +	u32 flags;
> > +	int (*power)(int on);
> 
> 'bool on'?

Added to the TODO.

> > +static int i2c_read(struct synaptics_ts_data *ts, u8 reg, char *msg)
> > +{
> > +	int ret = i2c_smbus_read_byte_data(ts->client, 0xf0);
> 
> We are not using 'reg'?

Fixed, thanks.

> Some more info as to what you are trying to do here would be nice.

Done, Arve provided a comment.

> > +	input_report_abs(ts->input_dev, ABS_PRESSURE, z);
> > +	input_report_abs(ts->input_dev, ABS_TOOL_WIDTH, w);
> > +	input_report_key(ts->input_dev, BTN_TOUCH, finger);
> > +	finger2_pressed = finger > 1 && finger != 7;
> > +	input_report_key(ts->input_dev, BTN_2, finger2_pressed);
> > +	if (finger2_pressed) {
> > +		input_report_abs(ts->input_dev, ABS_HAT0X, pos[1][0]);
> > +		input_report_abs(ts->input_dev, ABS_HAT0Y, pos[1][1]);
> 
> ABS_HAT is pretty unusual for a touchscreen...

Added to the TODO list. It should be cleaned up before merge.

> > +		if ((buf[14] & 0xc0) != 0x40) {
> > +			printk(KERN_WARNING "synaptics_ts_work_func:"
> > +			       " bad read %x %x %x %x %x %x %x %x %x"
> > +			       " %x %x %x %x %x %x, ret %d\n",
> > +			       buf[0], buf[1], buf[2], buf[3],
> > +			       buf[4], buf[5], buf[6], buf[7],
> > +			       buf[8], buf[9], buf[10], buf[11],
> > +			       buf[12], buf[13], buf[14], ret);
> 
> Umm... for ()?

I removed debugging instead.

> > +static int synaptics_ts_probe(
> 
> __devinit()

Done.

> > +	ts->input_dev = input_allocate_device();
> > +	if (ts->input_dev == NULL) {
> > +		ret = -ENOMEM;
> > +		pr_err("synaptics: Failed to allocate input device\n");
> > +		goto err_input_dev_alloc_failed;
> > +	}
> > +	ts->input_dev->name = "synaptics-rmi-touchscreen";
> 
> BUS_I2C, etc.

I'm doing this:

	ts->input_dev->name = "synaptics-rmi-touchscreen";
	ts->input_dev->phys = "msm/input0";
	ts->input_dev->id.bustype = BUS_I2C;

I don't know if it is right; if you can suggest better strings...

> > +	set_bit(EV_SYN, ts->input_dev->evbit);
> 
> __set_bit(), no need to lock the bus.

Done.

> > +	if (client->irq) {
> > +		ret = request_irq(client->irq, synaptics_ts_irq_handler,
> > +				  0, client->name, ts);
> 
> I think threaded IRQ will fit the bill and will take care of
> IRQ/workqueue shutdown races. Of course you still need to use workqueue
> if polling.

Yep, but that's TODO for now.

> > +	pr_info("synaptics: Start touchscreen %s in %s mode\n", 
> 
> Probably "synaptics_ts" throughout so nobody is confused?

Yep.

> > +static int synaptics_ts_remove(struct i2c_client *client)
> 
> __devexit()

Ok.

> > +MODULE_DESCRIPTION("Synaptics Touchscreen Driver");
> > +MODULE_LICENSE("GPL");
> 
> MODULE_AUTHOR()?

Fixed; this, and some others were fixed before. Here is just the new
stuff.

---

Small fixes to synaptics touchscreen driver, and add a TODO list of
stuff that should be fixed between merge into kernel proper.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/dream/Kconfig b/drivers/staging/dream/Kconfig
index 1fdbd48..e19d403 100644
--- a/drivers/staging/dream/Kconfig
+++ b/drivers/staging/dream/Kconfig
@@ -8,4 +8,8 @@ config TOUCHSCREEN_SYNAPTICS_I2C_RMI
 	help
 	  This enables support for Synaptics RMI over I2C based touchscreens.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called synaptics_i2c_rmi.
+
+
 
diff --git a/drivers/staging/dream/synaptics_i2c_rmi.c b/drivers/staging/dream/synaptics_i2c_rmi.c
index 6e23276..d786a55 100644
--- a/drivers/staging/dream/synaptics_i2c_rmi.c
+++ b/drivers/staging/dream/synaptics_i2c_rmi.c
@@ -189,13 +189,7 @@ static void synaptics_ts_work_func(struct work_struct *work)
 			continue;
 		}
 		if ((buf[14] & 0xc0) != 0x40) {
-			pr_warning("synaptics_ts_work_func:"
-			       " bad read %x %x %x %x %x %x %x %x %x"
-			       " %x %x %x %x %x %x, ret %d\n",
-			       buf[0], buf[1], buf[2], buf[3],
-			       buf[4], buf[5], buf[6], buf[7],
-			       buf[8], buf[9], buf[10], buf[11],
-			       buf[12], buf[13], buf[14], ret);
+			pr_warning("synaptics_ts_work_func: bad data\n");
 			if (bad_data)
 				synaptics_init_panel(ts);
 			bad_data = 1;
@@ -517,7 +511,7 @@ static int __devinit synaptics_ts_probe(
 	register_early_suspend(&ts->early_suspend);
 #endif
 
-	pr_info("synaptics: Start touchscreen %s in %s mode\n", 
+	pr_info("synaptics_ts: Start touchscreen %s in %s mode\n", 
 		ts->input_dev->name, ts->use_irq ? "interrupt" : "polling");
 
 	return 0;
@@ -534,7 +528,7 @@ err_check_functionality_failed:
 	return ret;
 }
 
-static int synaptics_ts_remove(struct i2c_client *client)
+static int __devexit synaptics_ts_remove(struct i2c_client *client)
 {
 	struct synaptics_ts_data *ts = i2c_get_clientdata(client);
 #ifdef CONFIG_HAS_EARLYSUSPEND


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html