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
