[2/2] HID: i2c-hid: Use block reads when possible to save power
diff mbox series

Message ID 20200614210255.4641-3-sultan@kerneltoast.com
State New
Delegated to: Jiri Kosina
Headers show
Series
  • i2c-hid: Save power by reducing i2c xfers with block reads
Related show

Commit Message

Sultan Alsawaf June 14, 2020, 9:02 p.m. UTC
From: Sultan Alsawaf <sultan@kerneltoast.com>

We have no way of knowing how large an incoming payload is going to be,
so the only strategy available up until now has been to always retrieve
the maximum possible report length over i2c, which can be quite
inefficient. For devices that send reports in block read format, the i2c
controller driver can read the payload length on the fly and terminate
the i2c transaction early, resulting in considerable power savings.

On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the
touchpad causes psys power readings to go up by about 4W and hover there
until I remove my finger. With this patch, my psys readings go from 4.7W
down to 3.1W, yielding about 1.6W in savings. This is because my
touchpad's max report length is 60 bytes, but all of the regular reports
it sends for touch events are only 32 bytes, so the i2c transfer is
roughly halved for the common case.

Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jarkko Nikula June 16, 2020, 1:43 p.m. UTC | #1
On 6/15/20 12:02 AM, Sultan Alsawaf wrote:
> From: Sultan Alsawaf <sultan@kerneltoast.com>
> 
> We have no way of knowing how large an incoming payload is going to be,
> so the only strategy available up until now has been to always retrieve
> the maximum possible report length over i2c, which can be quite
> inefficient. For devices that send reports in block read format, the i2c
> controller driver can read the payload length on the fly and terminate
> the i2c transaction early, resulting in considerable power savings.
> 
> On a Dell Precision 15 5540 with an i9-9880H, resting my finger on the
> touchpad causes psys power readings to go up by about 4W and hover there
> until I remove my finger. With this patch, my psys readings go from 4.7W
> down to 3.1W, yielding about 1.6W in savings. This is because my
> touchpad's max report length is 60 bytes, but all of the regular reports
> it sends for touch events are only 32 bytes, so the i2c transfer is
> roughly halved for the common case.
> 
> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
> ---
>   drivers/hid/i2c-hid/i2c-hid-core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 294c84e136d7..4b507de48d70 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -476,7 +476,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
>   	if (size > ihid->bufsize)
>   		size = ihid->bufsize;
>   
> -	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> +	ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size,
> +					I2C_M_RD | I2C_M_RECV_LEN);

This causes i2c-hid compatible touchscreen to stop working for me.

Ok (with patch 1/2)

[    9.346134] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[    9.346141] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[    9.362082] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[    9.385897] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[    9.386547] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[    9.386598] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[    9.386616] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[    9.391595] i2c_hid i2c-ELAN221D:00: resetting...
[    9.408864] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[    9.410162] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[    9.418223] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[    9.418231] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[    9.418236] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[    9.418531] i2c_hid i2c-ELAN221D:00: asking HID report descriptor
[    9.418537] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=02 00
[    9.440093] i2c_hid i2c-ELAN221D:00: Report Descriptor: 05 0d 09 04 
a1 01 85 01 09 22 a1 02 09 42 15 00 25 01 75 01 95 01 81 02 75 01 81 03 
75 06 09 51 25 3f 81 02 26 ff 00 75 08 09 48 81 02 09 49 81 02 95 01 05 
01 a4 26 c0 0c 75 10 55 0f 65 11 09

Not ok (with patches 1-2/2)

[    9.428690] i2c_hid i2c-ELAN221D:00: Fetching the HID descriptor
[    9.428698] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=01 00
[    9.430017] i2c_hid i2c-ELAN221D:00: HID Descriptor: 1e 00 00 01 31 
02 02 00 03 00 43 00 04 00 ff 00 05 00 06 00 f3 04 1d 22 10 56 00 00 00 00
[    9.430836] i2c_hid i2c-ELAN221D:00: entering i2c_hid_parse
[    9.431205] i2c_hid i2c-ELAN221D:00: i2c_hid_hwreset
[    9.431294] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[    9.431314] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 08
[    9.435937] i2c_hid i2c-ELAN221D:00: resetting...
[    9.435944] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 00 01
[    9.436150] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: waiting...
[   10.461304] i2c_designware i2c_designware.3: controller timed out
[   10.498312] i2c_designware i2c_designware.3: timeout in disabling adapter
[   14.525115] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: finished.
[   14.525130] i2c_hid i2c-ELAN221D:00: failed to reset device.
[   14.532507] i2c_hid i2c-ELAN221D:00: i2c_hid_set_power
[   14.532520] i2c_hid i2c-ELAN221D:00: __i2c_hid_command: cmd=05 00 01 08
[   14.553027] i2c_designware i2c_designware.3: timeout waiting for bus 
ready
...

I don't know what causes the breakage but according to HID Over I2C 
Protocol Specification the descriptor length is 16 bits. Maybe the code 
misses the last byte and/or the data is off by one byte by taking the 
2nd length byte as 1st data byte?

Patch
diff mbox series

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 294c84e136d7..4b507de48d70 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -476,7 +476,8 @@  static void i2c_hid_get_input(struct i2c_hid *ihid)
 	if (size > ihid->bufsize)
 		size = ihid->bufsize;
 
-	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+	ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size,
+					I2C_M_RD | I2C_M_RECV_LEN);
 	if (ret != size) {
 		if (ret < 0)
 			return;