diff mbox series

[PATCHv2,5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr

Message ID 8dd22e21-4933-8e9c-a696-d281872c8de7@xs4all.nl (mailing list archive)
State Accepted
Commit 3f19fed743c70b06d9c8fb1400baaea584491042
Headers show
Series None | expand

Commit Message

Hans Verkuil Nov. 23, 2019, 4:27 p.m. UTC
This increment of rmi_smbus in rmi_smb_read/write_block() causes
garbage to be read/written.

The first read of SMB_MAX_COUNT bytes is fine, but after that
it is nonsense. Trial-and-error showed that by dropping the
increment of rmiaddr everything is fine and the F54 function
properly works.

I tried a hack with rmi_smb_write_block() as well (writing to the
same F54 touchpad data area, then reading it back), and that
suggests that there too the rmiaddr increment has to be dropped.
It makes sense that if it has to be dropped for read, then it has
to be dropped for write as well.

It looks like the initial work with F54 was done using i2c, not smbus,
and it seems nobody ever tested F54 with smbus. The other functions
all read/write less than SMB_MAX_COUNT as far as I can tell, so this
issue was never noticed with non-F54 functions.

With this change I can read out the touchpad data correctly on my
Lenovo X1 Carbon 6th Gen laptop.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/input/rmi4/rmi_smbus.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Dmitry Torokhov Dec. 2, 2019, 6:09 p.m. UTC | #1
On Sat, Nov 23, 2019 at 05:27:41PM +0100, Hans Verkuil wrote:
> This increment of rmi_smbus in rmi_smb_read/write_block() causes
> garbage to be read/written.
> 
> The first read of SMB_MAX_COUNT bytes is fine, but after that
> it is nonsense. Trial-and-error showed that by dropping the
> increment of rmiaddr everything is fine and the F54 function
> properly works.
> 
> I tried a hack with rmi_smb_write_block() as well (writing to the
> same F54 touchpad data area, then reading it back), and that
> suggests that there too the rmiaddr increment has to be dropped.
> It makes sense that if it has to be dropped for read, then it has
> to be dropped for write as well.
> 
> It looks like the initial work with F54 was done using i2c, not smbus,
> and it seems nobody ever tested F54 with smbus. The other functions
> all read/write less than SMB_MAX_COUNT as far as I can tell, so this
> issue was never noticed with non-F54 functions.
> 
> With this change I can read out the touchpad data correctly on my
> Lenovo X1 Carbon 6th Gen laptop.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Applied, thank you.

> ---
>  drivers/input/rmi4/rmi_smbus.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 2407ea43de59..b313c579914f 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -163,7 +163,6 @@ static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to write next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> -		rmiaddr += SMB_MAX_COUNT;
>  	}
>  exit:
>  	mutex_unlock(&rmi_smb->page_mutex);
> @@ -215,7 +214,6 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to read next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> -		rmiaddr += SMB_MAX_COUNT;
>  	}
> 
>  	retval = 0;
> -- 
> 2.24.0
> 
>
diff mbox series

Patch

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 2407ea43de59..b313c579914f 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -163,7 +163,6 @@  static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to write next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
-		rmiaddr += SMB_MAX_COUNT;
 	}
 exit:
 	mutex_unlock(&rmi_smb->page_mutex);
@@ -215,7 +214,6 @@  static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to read next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
-		rmiaddr += SMB_MAX_COUNT;
 	}

 	retval = 0;