Re: [PATCH v2] HID: i2c-hid: Use block reads when possible to save power

[Sultan Alsawaf]

On Wed, Jun 17, 2020 at 02:17:19PM +0300, Jarkko Nikula wrote:
> On 6/16/20 6:49 PM, Sultan Alsawaf wrote:
> > From: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > Jarkko, could you try this?
> >   drivers/hid/i2c-hid/i2c-hid-core.c | 5 ++++-
> >   1 file changed, 4 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..739dccfc57e1 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -472,11 +472,14 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
> >   	int ret;
> >   	u32 ret_size;
> >   	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
> > +	u16 flags;
> >   	if (size > ihid->bufsize)
> >   		size = ihid->bufsize;
> > -	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> > +	/* Try to do a block read if the size fits in one byte */
> > +	flags = size > 255 ? I2C_M_RD : I2C_M_RD | I2C_M_RECV_LEN;
> > +	ret = i2c_transfer_buffer_flags(ihid->client, ihid->inbuf, size, flags);
> >   	if (ret != size) {
> >   		if (ret < 0)
> >   			return;
> 
> This still causes a regression for me.

Hmm, for some reason in 5.8 I get the same problem, but 5.7 is fine. Could you
try this on 5.7 and see if it works?

In the meantime I'll bisect 5.8 to see why it's causing problems for me...

Sultan