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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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