Hi Dmitry, Thank you for your review. Will the existing patches get into the current merge? Can I revisit this discussion as part of our plan to provide a Protocol B driver? Thanks and best regards, Kevin > -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > Sent: Wednesday, January 12, 2011 10:46 AM > To: Kevin McNeely > Cc: David Brown; Trilok Soni; Samuel Ortiz; Eric Miao; Mike Frysinger; > Henrik Rydberg; Alan Cox; linux-input@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx > Subject: Re: [v4 3/3] 3/3 spi: Cypress TTSP G3 SPI Device Driver > > Hi Kevin, > > On Tue, Jan 04, 2011 at 04:54:06PM -0800, Kevin McNeely wrote: > > + > > +static int spi_sync_tmo(struct cyttsp_spi *ts, struct spi_message > *message) > > +{ > > + DECLARE_COMPLETION_ONSTACK(done); > > + int status; > > + > > + message->complete = spi_complete; > > + message->context = &done; > > + status = spi_async(ts->spi_client, message); > > + if (status == 0) { > > + int ret = wait_for_completion_interruptible_timeout(&done, > HZ); > > + if (!ret) { > > + dev_dbg(ts->ops.dev, "%s: timeout\n", __func__); > > + status = -EIO; > > I do not believe we can do this and simply abort outstanding SPI > transfer. What if we unload the driver and destroy the device and then > SPI master will come around? And it does not look like SPI subsystem > allows to cancel outstanding requests... > > Have you actually seen timeouts firing? Why isn't the same needed for > I2C interface? > > > + > > +static s32 ttsp_spi_read_block_data(void *handle, u8 addr, > > + u8 length, void *data) > > +{ > > + struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, > ops); > > + int retval; > > + > > + retval = cyttsp_spi_xfer(CY_SPI_RD_OP, ts, addr, data, length); > > + if (retval < 0) > > + dev_dbg(ts->ops.dev, "%s: ttsp_spi_read_block_data > failed\n", > > + __func__); > > + > > + /* > > + * Do not print the above error if the data sync bytes were not > found. > > + * This is a normal condition for the bootloader loader startup > and need > > + * to retry until data sync bytes are found. > > + */ > > + if (retval > 0) > > + retval = -1; /* now signal fail; so retry can be done > */ > > I am a bit confused here. First of all we do retries in > cyttsp_spi_xfer(). Then cyttsp_core does retry transfer as well (but > i2c > methods do not retry). Can we settle on retrying in one place only? > > > + > > +static s32 ttsp_spi_tch_ext(void *handle, void *values) > > +{ > > + struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, > ops); > > + int retval = 0; > > + > > + /* > > + * TODO: Add custom touch extension handling code here > > + * set: retval < 0 for any returned system errors, > > + * retval = 0 if normal touch handling is required, > > + * retval > 0 if normal touch handling is *not* required > > + */ > > + > > + if (!ts || !values) > > + retval = -EINVAL; > > + > > + return retval; > > I question the utility of "per bus" extended touch handling. I can see > it being supplied from platform data but niot by interface code. > > BTW, no need to resubmit patches not, it is just a discussion... > > Thanks. > > -- > Dmitry --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- -- 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
