On Wed, Feb 23, 2011 at 09:23:16AM +0800, jcbian wrote:

quoted

quoted

+
+static int pixcir_i2c_ts_remove(struct i2c_client *client)
+{
+	struct pixcir_i2c_ts_data *tsdata = dev_get_drvdata(&client->dev);

i2c_get_clientdata(). Also empty line between variable definitions and
code.

Sorry,what's the meaning here?

1. Use i2c_get_clientdata() instead of dev_get_drvdata() to access
driver-private data

2. Add an empty line between variable definitions and the rest of the
code.

quoted

quoted

+
+	pixcir_wq = create_singlethread_workqueue("pixcir_wq");

Not needed if you use threaded IRQ.

You mean the threaded IRQ is better than workqueue? If using the threaded IRQ should remove the request_irq()?   

You request your IRQ to be serviced by a special thread by doing
request_threaded_irq(). Since interrupt processing will happen in a
thread you will be able to use sleeping functions directly in your
interrupt handler and won't need to rely on a separate workqueue
(and do not need to concern yourself with canceling/shutting down the
workqeue upon driver unload - which you did not do anyway ;) ).

Thanks.

-- 
Dmitry