On Tue, Jun 16, 2020 at 09:02:54PM +0300, Andi Shyti wrote: > Hi Sultan, > > > > > > 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. > > > > > > > > > + /* 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; > > > > > > > > AFAIR SMBus specification tells about 256. Why 255? > > > > > > > > Andi, am I correct? > > > > > > Actually the SMBUS 3.0 protocol from 2015[*] says 255: > > > > > > " > > > D.6 255 Bytes in Process Call > > > > > > The maximum number of bytes allowed in the Block Write-Block Read > > > Process Call (Section 6.5.8) was increased from 32 to 255. > > > " > > > > > > But why does it matter... I see the patch is detatching itself > > > from smbus. > > > > > > And, actually, I wonder if this is the right way to fix it, isn't > > > it better to fix smbus instead? > > > > I think the best solution would be to modify the i2c api to allow passing in a > > function pointer and a payload size length, to specify how to interpret the size > > of the incoming payload, so the adapter could handle both the HID over i2c > > transfer spec and SMBus block reads without needing to read more bytes than > > needed. > > Can't you do that by specifying the xfer function? > > When you use smbus_read/write in block or byte or whatever, smbus > always checks if there is an xfer function specified and uses > that. > > If it's not specified it uses the default smbus functions with > the limitations that come with it. The xfer functions are specified on a per-adapter basis. In the case of i2c-hid, we need to tell the adapter to interpret the payload size in a specific way, which I *think* is only specific to HID over i2c (i.e., using 16 bits to store the length and then checking it for device quirks). > > For example, for an SMBus block read, the payload size is specified in the first > > byte and it is limited to 32 bytes. However, for HID over i2c, the payload size > > is specified in the first two bytes, and there are also some device quirks > > involved to reinterpret the reported size. > > which is wrong. The 32 bytes limitation is outdated: in the link > that I gave before (i.e. this one [*]), the new SMBUS specifies > 255 maximum for read/write block. Oops. But still, for SMBus block reads, the size is limited to 8 bits. For HID over i2c, it can be 16 bits. I don't see how we can handle this without some api cooperation to tell the adapter what the caller is expecting to see. > > A nice solution would be to pass in how many bytes the i2c payload size can > > contain, as well as a function pointer to evaluate the reported payload size in > > a way that the caller wants. This would require modifying every i2c adapter > > driver to add this functionality, but it would fix the efficiency problem faced > > by i2c-hid and perhaps others. > > > > > I have a patch ready that fixes the smbus transfer size, perhaps > > > I should rebase, test and send it. > > > > For the i2c-hid driver? > > No, sorry, for smbus. > > Now... here you are replacing "i2c_master_recv" with > "i2c_transfer_buffer_flags". I do not really like this change, > although I understand it's necessary, because we are bypassing > the real issue that is that the smbus implementation is outdated. > > I have a patch for that that for a matter of time I never sent. Can it handle block reads (8 bit size) and HID over i2c (16 bit size)? Sultan
