RE: [v4 3/3] 3/3 spi: Cypress TTSP G3 SPI Device Driver

[Kevin McNeely]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/