[5/5] input/rmi4/rmi_smbus.c: don't increment rmiaddr in rmi_smb_read_block()
diff mbox series

Message ID 20191119105118.54285-6-hverkuil-cisco@xs4all.nl
State New
Headers show
Series
  • input: rmi4/synaptics fixes
Related show

Commit Message

Hans Verkuil Nov. 19, 2019, 10:51 a.m. UTC
This increment of rmi_smbus causes garbage to be returned.
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.

Even going back to the original code when F54 was added, I
could not make it work without this patch. So I do not understand
how this ever worked.

My guess is that the same change is needed in rmi_smb_write_block,
but I wouldn't know how to test this.

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

Comments

Lucas Stach Nov. 19, 2019, 11:48 a.m. UTC | #1
Hi Hans,

On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
> This increment of rmi_smbus causes garbage to be returned.
> 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.
> 
> Even going back to the original code when F54 was added, I
> could not make it work without this patch. So I do not understand
> how this ever worked.

My guess is that F54 has mostly been tested with touchscreens that are
connected to a regular i2c bus, not smbus. i2c has a separate transport
implementation in rmi4. Most of the other functions are probably
reading much smaller block data than F54.

Regards,
Lucas

> My guess is that the same change is needed in rmi_smb_write_block,
> but I wouldn't know how to test this.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/input/rmi4/rmi_smbus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 2407ea43de59..79ecea5edacc 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -215,7 +215,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;
Hans Verkuil Nov. 19, 2019, 12:19 p.m. UTC | #2
Hi Lucas,

On 11/19/19 12:48 PM, Lucas Stach wrote:
> Hi Hans,
> 
> On Di, 2019-11-19 at 11:51 +0100, Hans Verkuil wrote:
>> This increment of rmi_smbus causes garbage to be returned.
>> 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.
>>
>> Even going back to the original code when F54 was added, I
>> could not make it work without this patch. So I do not understand
>> how this ever worked.
> 
> My guess is that F54 has mostly been tested with touchscreens that are
> connected to a regular i2c bus, not smbus. i2c has a separate transport
> implementation in rmi4. Most of the other functions are probably
> reading much smaller block data than F54.

That's my suspicion as well. I tried to configure my kernel so that it
would be using i2c instead of smbus, but I couldn't make it work. I'll
have to try again next week.

I only have Rev E of the RMI4 spec, which does not appear to document
the mapping table entry that rmi_smb_get_command_code() uses.

Do you (or someone else) have access to a newer version? If so, can you check
how this is supposed to work for reading large blocks over smbus?

With the current code it will create a new mapping entry for every 32
byte read. I suspect that that is not how it is supposed to work, but
without a spec this is just trial-and-error.

Regards,

	Hans

> 
> Regards,
> Lucas
> 
>> My guess is that the same change is needed in rmi_smb_write_block,
>> but I wouldn't know how to test this.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/input/rmi4/rmi_smbus.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
>> index 2407ea43de59..79ecea5edacc 100644
>> --- a/drivers/input/rmi4/rmi_smbus.c
>> +++ b/drivers/input/rmi4/rmi_smbus.c
>> @@ -215,7 +215,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;
>

Patch
diff mbox series

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 2407ea43de59..79ecea5edacc 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -215,7 +215,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;