Re: [PATCH] drivers: input: Use single i2c_transfer transaction when using RM_CMD_BANK_SWITCH

[Dmitry Torokhov]

Hi Furquan,

On Tue, Aug 18, 2020 at 04:42:15PM -0700, Furquan Shaikh wrote:
> On an AMD chromebook, where the same I2C bus is shared by both Raydium
> touchscreen and a trackpad device, it is observed that interleaving of
> I2C messages when raydium_i2c_read_message() is called leads to the
> Raydium touch IC reporting incorrect information. This is the sequence
> that was observed to result in the above issue:
> 
> * I2C write to Raydium device for RM_CMD_BANK_SWITCH
> * I2C write to trackpad device
> * I2C read from trackpad device
> * I2C write to Raydium device for setting address
> * I2C read from Raydium device >>>> This provides incorrect
>   information
> 
> This change updates raydium_i2c_read_message and
> raydium_i2c_send_message to perform all the I2C transfers in the
> function as part of a single i2c_transfer transaction. This ensures
> that no transactions are initiated to any other device on the same bus
> in between and hence the information obtained from the touchscreen
> device is correct. Verified with the patch across multiple
> reboots (>100) that the information reported by the Raydium
> touchscreen device during probe is correct.
> 
> Signed-off-by: Furquan Shaikh <furquan@xxxxxxxxxx>
> ---
>  drivers/input/touchscreen/raydium_i2c_ts.c | 102 ++++++++++++++++-----
>  1 file changed, 80 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c b/drivers/input/touchscreen/raydium_i2c_ts.c
> index fe245439adee..11c00d341eb1 100644
> --- a/drivers/input/touchscreen/raydium_i2c_ts.c
> +++ b/drivers/input/touchscreen/raydium_i2c_ts.c
> @@ -111,6 +111,15 @@ struct raydium_info {
>  	u8 y_res;		/* units/mm */
>  };
>  
> +/*
> + * Header to be sent for RM_CMD_BANK_SWITCH command. This is used by
> + * raydium_i2c_{read|send}_message below.
> + */
> +struct __packed raydium_bank_switch_header {
> +	u8 cmd;
> +	__be32 be_addr;
> +};

I believe the preferred placement of __packed attribute is after the
definition:

dtor@dtor-ws:~/kernel/work $ git grep "struct __packed" | wc -l
210
dtor@dtor-ws:~/kernel/work $ git grep "} __packed" | wc -l
8718

> +
>  /* struct raydium_data - represents state of Raydium touchscreen device */
>  struct raydium_data {
>  	struct i2c_client *client;
> @@ -198,22 +207,44 @@ static int raydium_i2c_read(struct i2c_client *client,
>  static int raydium_i2c_read_message(struct i2c_client *client,
>  				    u32 addr, void *data, size_t len)
>  {
> -	__be32 be_addr;
> -	size_t xfer_len;
> -	int error;
> +	int ret;
>  
>  	while (len) {
> -		xfer_len = min_t(size_t, len, RM_MAX_READ_SIZE);
> -
> -		be_addr = cpu_to_be32(addr);
> +		u8 read_addr = addr & 0xff;
> +		struct raydium_bank_switch_header header = {
> +			.cmd = RM_CMD_BANK_SWITCH,
> +			.be_addr = cpu_to_be32(addr),
> +		};
> +		size_t xfer_len = min_t(size_t, len, RM_MAX_READ_SIZE);
> +		/*
> +		 * Perform as a single i2c_transfer transaction to ensure that
> +		 * no other I2C transactions are initiated on the bus to any
> +		 * other device in between. Initiating transacations to other
> +		 * devices after RM_CMD_BANK_SWITCH is sent is known to cause
> +		 * read issues.
> +		 */
> +		struct i2c_msg xfer[] = {
> +			{
> +				.addr = client->addr,
> +				.len = sizeof(header),
> +				.buf = (u8 *)&header,
> +			},
> +			{
> +				.addr = client->addr,
> +				.len = 1,
> +				.buf = &read_addr,
> +			},
> +			{
> +				.addr = client->addr,
> +				.flags = I2C_M_RD,
> +				.len = xfer_len,
> +				.buf = data,
> +			}
> +		};

I think this can be moved out of while loop.

I also wonder if this can be actually combined with raydium_i2c_read().
As far as I understand read/writes to register above 255 require
page select write, so we can always prepare the header and then submit
either 3 or 2 messages in the transfer depending on the register we are
dealing with. Or maybe convert to regmap?

Thanks.

-- 
Dmitry