Re: [PATCH 4/4] Input: synaptics-rmi4 - switch to using i2c_transfer()

[Christopher Heiny <cheiny@synaptics.com>]

On 01/09/2014 11:44 PM, Dmitry Torokhov wrote:
> Instead of using 2 separate transactions when reading from the device let's
> use i2c_transfer. Because we now have single point of failure I had to
> change how we collect statistics. I elected to drop control data from the
> stats and only track number of bytes read/written for the device data.
>
> Also, since we are not prepared to deal with short reads and writes change
> read_block_data and write_block_data to indicate error if we detect short
> transfers.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/rmi4/rmi_i2c.c | 71 ++++++++++++++++++++++++--------------------
>   1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index c176218..51f5bc8 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -57,22 +57,17 @@ struct rmi_i2c_xport {
>    */
>   static int rmi_set_page(struct rmi_i2c_xport *rmi_i2c, u8 page)
>   {
> -	struct rmi_transport_dev *xport = &rmi_i2c->xport;
>   	struct i2c_client *client = rmi_i2c->client;
>   	u8 txbuf[2] = {RMI_PAGE_SELECT_REGISTER, page};
>   	int retval;
>
> -	dev_dbg(&client->dev, "writes 3 bytes: %02x %02x\n",
> -			txbuf[0], txbuf[1]);
> -	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += sizeof(txbuf);
>   	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
>   	if (retval != sizeof(txbuf)) {
> -		xport->stats.tx_errs++;
>   		dev_err(&client->dev,
>   			"%s: set page failed: %d.", __func__, retval);
>   		return (retval < 0) ? retval : -EIO;
>   	}
> +
>   	rmi_i2c->page = page;
>   	return 0;
>   }
> @@ -107,22 +102,27 @@ static int rmi_i2c_write_block(struct rmi_transport_dev *xport, u16 addr,
>
>   	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
>   		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> -		if (retval < 0)
> +		if (retval)
>   			goto exit;
>   	}
>
> +	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> +	if (retval == tx_size)
> +		retval = 0;
> +	else if (retval >= 0)
> +		retval = -EIO;
> +
> +exit:
>   	dev_dbg(&client->dev,
> -		"writes %zd bytes at %#06x: %*ph\n", len, addr, (int)len, buf);
> +		"write %zd bytes at %#06x: %d (%*ph)\n",
> +		len, addr, retval, (int)len, buf);
>
>   	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += tx_size;
> -	retval = i2c_master_send(client, rmi_i2c->tx_buf, tx_size);
> -	if (retval < 0)
> +	if (retval)
>   		xport->stats.tx_errs++;
>   	else
> -		retval--; /* don't count the address byte */
> +		xport->stats.tx_bytes += len;
>
> -exit:
>   	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
> @@ -133,40 +133,47 @@ static int rmi_i2c_read_block(struct rmi_transport_dev *xport, u16 addr,
>   	struct rmi_i2c_xport *rmi_i2c =
>   		container_of(xport, struct rmi_i2c_xport, xport);
>   	struct i2c_client *client = rmi_i2c->client;
> -	u8 txbuf[1] = {addr & 0xff};
> +	u8 addr_offset = addr & 0xff;
>   	int retval;
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.len	= sizeof(addr_offset),
> +			.buf	= &addr_offset,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags	= I2C_M_RD,
> +			.len	= len,
> +			.buf	= buf,
> +		},
> +	};
>
>   	mutex_lock(&rmi_i2c->page_mutex);
>
>   	if (RMI_I2C_PAGE(addr) != rmi_i2c->page) {
>   		retval = rmi_set_page(rmi_i2c, RMI_I2C_PAGE(addr));
> -		if (retval < 0)
> +		if (retval)
>   			goto exit;
>   	}
>
> -	dev_dbg(&client->dev, "writes 1 bytes: %02x\n", txbuf[0]);
> +	retval = i2c_transfer(client->adapter, msgs, sizeof(msgs));
> +	if (retval == sizeof(msgs))

I think this should be:
	retval = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
	if (retval == ARRAY_SIZE(msgs))
At least, that change resolved some random misbehaviors, including 
kernel panics.


> +		retval = 0; /* success */
> +	else if (retval >= 0)
> +		retval = -EIO;
>
> -	xport->stats.tx_count++;
> -	xport->stats.tx_bytes += sizeof(txbuf);
> -	retval = i2c_master_send(client, txbuf, sizeof(txbuf));
> -	if (retval != sizeof(txbuf)) {
> -		xport->stats.tx_errs++;
> -		retval = (retval < 0) ? retval : -EIO;
> -		goto exit;
> -	}
> +exit:
> +	dev_dbg(&client->dev,
> +		"read %zd bytes at %#06x: %d (%*ph)\n",
> +		len, addr, retval, (int)len, buf);
>
>   	xport->stats.rx_count++;
> -	xport->stats.rx_bytes += len;
> -
> -	retval = i2c_master_recv(client, buf, len);
> -	if (retval < 0)
> +	if (retval)
>   		xport->stats.rx_errs++;
>   	else
> -		dev_dbg(&client->dev,
> -			"read %zd bytes at %#06x: %*ph\n",
> -			len, addr, (int)len, buf);
> +		xport->stats.rx_bytes += len;
>
> -exit:
>   	mutex_unlock(&rmi_i2c->page_mutex);
>   	return retval;
>   }
>


-- 

Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated