Hi Bryan,

On Mon, Feb 04, 2008 at 05:52:38PM +0800, Bryan Wu wrote:
 +

+	spi_message_add_tail(&req->xfer[0], &req->msg);
+	spi_message_add_tail(&req->xfer[1], &req->msg);
+
+	status = spi_sync(spi, &req->msg);
+
+	if (req->msg.status)
+		status = req->msg.status;
+

I think the "new way" for spi is to just check return value of
spi_sync and not bother with req->msg.status, but even using
old style spi_sycn I'd expect something like:

	status = spi_sync(spi, &req->msg);
	if (status == 0)
		status = req->msg.status;

+
+static ssize_t ad7877_gpio3_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct ad7877 *ts = dev_get_drvdata(dev);
+	char *endp;
+	int i;
+
+	i = simple_strtoul(buf, &endp, 10);
+
+	if (i)
+		ts->gpio3=1;
+	else
+		ts->gpio3=0;
+

I am tempted to change the above to ts->gpio3 = !!i;

+
+	if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
+		/* compute touch pressure resistance using equation #2 */
+		Rt = z2;
+		Rt -= z1;
+		Rt *= x;
+		Rt *= ts->x_plate_ohms;
+		Rt /= z1;
+		Rt = (Rt + 2047) >> 12;
+	} else
+		Rt = 0;
+
+	if (Rt) {
+		input_report_abs(input_dev, ABS_X, x);
+		input_report_abs(input_dev, ABS_Y, y);
+		input_report_abs(input_dev, ABS_PRESSURE, Rt);
+		input_sync(input_dev);
+	}
+
+#ifdef	VERBOSE
+	if (Rt)
+		pr_debug("%s: %d/%d/%d%s\n", ts->spi->dev.bus_id,
+			x, y, Rt, Rt ? "" : " UP");

We check the same condition 3 times in a row. The compiler might
combine them but why not help it?

+
+static int ad7877_suspend(struct spi_device *spi, pm_message_t message)
+{
+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
+
+	spi->dev.power.power_state = message;

I think they are trying to get rid of power_state...

+	ad7877_disable(ts);
+
+	return 0;
+
+}
+
+static int ad7877_resume(struct spi_device *spi)
+{
+	struct ad7877 *ts = dev_get_drvdata(&spi->dev);
+
+	spi->dev.power.power_state = PMSG_ON;

Same here.

+
+	err = input_register_device(input_dev);
+	if (err)
+		goto err_idev;
+
+	ts->intr_flag = 0;
+
+	return 0;
+
+err_idev:
+	input_dev = NULL; /* so we don't try to free it later */

But we _do_ need to free it if input_register_device() fails.

Thanks.

-- 
Dmitry