Dmitry, Thanks for you feedback. Bryan is going to send out an updated patch. Best regards, Michael >-----Original Message----- >From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] >Sent: Montag, 4. Februar 2008 18:13 >To: Bryan Wu >Cc: Michael Hennerich; linux-input@xxxxxxxxxxxxxxx >Subject: Re: [PATCH] Input/Touchscreen Driver: add support >AD7877touchscreen driver (resend to linux-input mailist) > >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
