On Tue, Feb 22, 2011 at 11:17:01AM +0800, jcbian wrote:
> +config TOUCHSCREEN_PIXCIR
> + tristate "PIXCIR touchscreen panel support"
> + depends on I2C
> + help
> + Say Y here if you have a PIXCIR based touchscreen.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called pixcir_i2c_ts.
It looks like your mail client messes with the indentation of your
files, it looks like it turned all the tabs into spaces.
> +#define DEBUG 0
Best just to remove this.
> +static struct workqueue_struct *pixcir_wq;
If you can have a single global workqueue could you use the system one
and avoid having to create one at all?
> + int posx1, posy1, posx2, posy2;
> + u_int8_t Rdbuf[10], Wrbuf[1];
Use u8. The upper case at the start of teh bariable names is also a bit
odd for Linux.
> + Wrbuf[0] = 0;
> + ret = i2c_master_send(tsdata->client, Wrbuf, 1);
> + if (ret != 1) {
> + dev_err(&tsdata->client->dev, "Unable to write to i2c touchscreen!\n");
> + goto out;
It'd be good to print the error code.
> +static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> +{
> + struct pixcir_i2c_ts_data *tsdata = dev_id;
> + disable_irq_nosync(irq);
> +
> + queue_work(pixcir_wq, &tsdata->work.work);
> +
> + return IRQ_HANDLED;
> +}
Use a threaded IRQ handler - genirq can now implement this pattern for
you.
> +static int pixcir_ts_open(struct input_dev *dev)
> +{
> + return 0;
> +}
You should have code to start the controller here, and matching shutdown
code in close().
> + #ifdef DEBUG
> + printk(KERN_EMERG "pixcir_i2c_ts probe!\n");
> + #endif
dev_dbg() or just remove this.
> + tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
> + if (!tsdata) {
> + dev_err(&client->dev, "Failed to allocate driver data!\n");
> + error = -ENOMEM;
> + dev_set_drvdata(&client->dev, NULL);
Don't need to clear the driver data if you're exiting.
> + if (request_irq(tsdata->irq, pixcir_ts_isr,
> + IRQF_TRIGGER_LOW, client->name, tsdata)) {
> + dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
> + input_unregister_device(input);
> + input = NULL;
> + }
Your error handling doesn't aways clean everything up - it's often best
to use gotos to jump to cleanup code that unwinds everything to make
sure you don't miss some things.
> +
> + dev_err(&tsdata->client->dev, "insmod successfully!\n");
No need for logs like this in production code, and the priority is all
wrong.
> +#ifdef CONFIG_PM
> +static int pixcir_i2c_ts_suspend(struct i2c_client *client,
> pm_message_t mesg)
> +{
> + struct pixcir_i2c_ts_data *tsdata = dev_get_drvdata(&client->dev);
> +
> + if (device_may_wakeup(&client->dev))
> + enable_irq_wake(tsdata->irq);
> +
> + return 0;
> +}
Please use dev_pm_ops instead of bus-specific suspend/resume callbacks -
there is a general move to remove the bus specific ones to allow better
development of the PM core.
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE(DRIVER_LICENSE);
Just put the definitions of these in directly, they're only used in one
place.
--
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
