Re: [PATCH 3/3] Input: cyttsp - add support for Cypress TTSP touchscreen SPI bus interface

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

Hi Javier,

On Sat, Aug 20, 2011 at 12:01:52AM -0400, Javier Martinez Canillas wrote:
> +
> +static int cyttsp_spi_xfer_(u8 op, struct cyttsp_spi *ts,
> +			    u8 reg, u8 *buf, int length)
> +{
> +	struct spi_message msg;
> +	struct spi_transfer xfer[2];
> +	u8 *wr_buf = ts->wr_buf;
> +	u8 *rd_buf = ts->rd_buf;
> +	int retval;
> +
> +	if (length > CY_SPI_DATA_SIZE) {
> +		dev_dbg(ts->ops.dev, "%s: length %d is too big.\n",
> +			__func__, length);
> +		return -EINVAL;
> +	}
> +
> +	memset(wr_buf, 0, CY_SPI_DATA_BUF_SIZE);
> +	memset(rd_buf, 0, CY_SPI_DATA_BUF_SIZE);
> +
> +	wr_buf[0] = 0x00; /* header byte 0 */
> +	wr_buf[1] = 0xFF; /* header byte 1 */
> +	wr_buf[2] = reg;  /* reg index */
> +	wr_buf[3] = op;   /* r/~w */
> +	if (op == CY_SPI_WR_OP)
> +		memcpy(wr_buf + CY_SPI_CMD_BYTES, buf, length);
> +
> +	memset((void *)xfer, 0, sizeof(xfer));
> +	spi_message_init(&msg);
> +	xfer[0].tx_buf = wr_buf;
> +	xfer[0].rx_buf = rd_buf;

Since we are setting both TX and RX buffer here does this mean that the
device will not work with controllers in half-duplex mode? What is the
amount of data that will be placed in rd_buf?

> +	if (op == CY_SPI_WR_OP) {
> +		xfer[0].len = length + CY_SPI_CMD_BYTES;
> +		spi_message_add_tail(&xfer[0], &msg);
> +	} else if (op == CY_SPI_RD_OP) {
> +		xfer[0].len = CY_SPI_CMD_BYTES;
> +		spi_message_add_tail(&xfer[0], &msg);
> +
> +		xfer[1].rx_buf = buf;
> +		xfer[1].len = length;
> +		spi_message_add_tail(&xfer[1], &msg);
> +	}
> +
> +	retval = spi_sync_tmo(ts, &msg);
> +	if (retval < 0) {
> +		dev_dbg(ts->ops.dev, "%s: spi sync error %d, len=%d, op=%d\n",
> +			__func__, retval, xfer[1].len, op);
> +		retval = 0;

Why do we ignore the error?

> +	}
> +
> +	if (op == CY_SPI_RD_OP) {
> +		if ((rd_buf[CY_SPI_SYNC_BYTE] == CY_SPI_SYNC_ACK1) &&
> +			(rd_buf[CY_SPI_SYNC_BYTE+1] == CY_SPI_SYNC_ACK2))
> +			retval = 0;

What about write operation? Does the device send anything in rd_buf for
write command?

> +		else {
> +			int i;
> +			for (i = 0; i < (CY_SPI_CMD_BYTES); i++)
> +				dev_dbg(ts->ops.dev,
> +					"%s: test rd_buf[%d]:0x%02x\n",
> +					__func__, i, rd_buf[i]);
> +			for (i = 0; i < (length); i++)
> +				dev_dbg(ts->ops.dev,
> +					"%s: test buf[%d]:0x%02x\n",
> +					__func__, i, buf[i]);
> +			retval = 1;
> +		}
> +	}
> +	return retval;
> +}
> +
> +static int cyttsp_spi_xfer(u8 op, struct cyttsp_spi *ts,
> +			    u8 reg, u8 *buf, int length)
> +{
> +	int tries;
> +	int retval;
> +
> +	if (op == CY_SPI_RD_OP) {
> +		for (tries = CY_NUM_RETRY; tries; tries--) {
> +			retval = cyttsp_spi_xfer_(op, ts, reg, buf, length);
> +			if (retval == 0)
> +				break;
> +			else
> +				msleep(20);

Why do we need retry in SPI case but do not need it when device is
connected via I2C? And not on writes?

> +		}
> +	} else {
> +		retval = cyttsp_spi_xfer_(op, ts, reg, buf, length);
> +	}
> +
> +	return retval;
> +}
> +
> +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 */
> +
> +	return retval;
> +}
> +
> +static s32 ttsp_spi_write_block_data(void *handle, u8 addr,
> +				     u8 length, const void *data)
> +{
> +	struct cyttsp_spi *ts = container_of(handle, struct cyttsp_spi, ops);
> +	int retval;
> +
> +	retval = cyttsp_spi_xfer(CY_SPI_WR_OP, ts, addr, (void *)data, length);
> +	if (retval < 0)
> +		dev_dbg(ts->ops.dev, "%s: ttsp_spi_write_block_data failed\n",
> +			__func__);
> +
> +	if (retval == -EIO)
> +		return 0;

Why is -EIO special?

> +	else
> +		return retval;
> +}
> +

Thanks.

-- 
Dmitry