Re: Support for synaptic touchscreen in HTC dream

[Pavel Machek]

On Tue 2009-07-14 10:52:12, Dmitry Torokhov wrote:
> On Tue, Jul 14, 2009 at 12:06:34PM +0200, Pavel Machek wrote:
> > From: Arve Hj?nnev?g <arve@xxxxxxxxxxx>
> > 
> > This adds support for synaptic touchscreen, used in HTC dream
> > cellphone.
> > 
> > Signed-off-by: Pavel Machek <pavel@xxxxxx>
> 
> This is pretty large body of code, could we get a sign-off from Arve as
> well since he seems to be the author?

Arve?

> > +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...

Done.

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

Arve?

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

Ok.

> > +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'?

Uff... I thought I actually tested it?!

> > +	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...

It is trying to do something useful for multitouch. People actually
use multitouch on HTC dream. I guess I can strip it from version being
merged...

> > +		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 ()?

> > +static enum hrtimer_restart synaptics_ts_timer_func(struct hrtimer *timer)
> > +{
> > +	struct synaptics_ts_data *ts =
> > +		container_of(timer, struct synaptics_ts_data, timer);
> > +
> > +	queue_work(synaptics_wq, &ts->work);
> > +
> > +	hrtimer_start(&ts->timer, ktime_set(0, 12500000), HRTIMER_MODE_REL);
> 
> I was always wondering why use high-resolution timer and then offload to
> a workqueue which scheduling we have no idea about. Why can't we simply
> reschedule work in case when we are in polling mode?


> > +	INIT_WORK(&ts->work, synaptics_ts_work_func);
> > +	ts->client = client;
> > +	i2c_set_clientdata(client, ts);
> > +	pdata = client->dev.platform_data;
> > +	if (pdata)
> > +		ts->power = pdata->power;
> > +	else
> > +		pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> 
> Where is it freed?

It is not, but this should not trigger. Converted to static fake_pdata.

> > +	ts->input_dev->name = "synaptics-rmi-touchscreen";
> 
> BUS_I2C, etc.

I figured out BUS_I2C. Anything else I need to set?

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

Ok.

> > +	set_bit(EV_KEY, ts->input_dev->evbit);
> > +	set_bit(BTN_TOUCH, ts->input_dev->keybit);
> > +	set_bit(BTN_2, ts->input_dev->keybit);
> 
> BTN_2 is kinda generic... Normally joysticks use it... 

Ok, what is the recommended protocol for multitouch? Should I strip it
out from merge version?

> > +	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.

I guess we'll just strip polling version for now?

> > +	if (!ts->use_irq) {
> > +		hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +		ts->timer.function = synaptics_ts_timer_func;
> > +		hrtimer_start(&ts->timer, ktime_set(1, 0), HRTIMER_MODE_REL);
> > +	}
> > +
> > +	pr_info("synaptics: Start touchscreen %s in %s mode\n", 
> 
> Probably "synaptics_ts" throughout so nobody is confused?

Well, there's no other hardware from synaptics in those machines :-).

> > +		ts->input_dev->name, ts->use_irq ? "interrupt" : "polling");
> > +
> > +	return 0;
> > +
> > + err_input_register_device_failed:
> > +	input_free_device(ts->input_dev);
> > +
> 
> Don't see canceling timer nor shutting off WQ here. Also, maybe
> implement open() and close() so we don't reschedule WQ while polling?

> > +{
> > +	struct synaptics_ts_data *ts = i2c_get_clientdata(client);
> > +
> > +	if (ts->use_irq)
> > +		free_irq(client->irq, ts);
> > +	else
> > +		hrtimer_cancel(&ts->timer);
> 
> What about the work?

> > +static void __exit synaptics_ts_exit(void)
> > +{
> > +	i2c_del_driver(&synaptics_ts_driver);
> > +	if (synaptics_wq)
> > +		destroy_workqueue(synaptics_wq);
> > +}
> > +
> > +module_init(synaptics_ts_init);
> > +module_exit(synaptics_ts_exit);
> > +
> > +MODULE_DESCRIPTION("Synaptics Touchscreen Driver");
> > +MODULE_LICENSE("GPL");
> 
> MODULE_AUTHOR()?

Ok.

> Thanks.

Thanks for review!
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/