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