Re: [PATCH v2] input: add driver for pixcir i2c touchscreens

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, Feb 23, 2011 at 09:23:16AM +0800, jcbian wrote:
> > > +
> > > +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.

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