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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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.