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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
