Hi Stephan, > Not sure if you saw my comment regarding your patch [1], > so I'll just repeat it properly inline here: > > [1]: https://patchwork.kernel.org/patch/11178515/#22927311 > > On Sun, Oct 20, 2019 at 11:28:55PM +0300, Andi Shyti wrote: > > The exchange of data to and from the mms114 touchscreen never > > exceeds 256 bytes. In the worst case it goes up to 80 bytes in > > the interrupt handler while reading the events. > > > > i2c_smbus_read_i2c_block_data() is actually limited to > I2C_SMBUS_BLOCK_MAX = 32. oh sorry, I don't know how I slipped on this. But this means that the i2c in the kernel is wrong (or outdated), smbus specifies 256 bytes of data[*]. I might have relied on the specification more than the code. I guess SMBUS needs some update. > > diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c > > index a5ab774da4cc..170dcb5312b9 100644 > > --- a/drivers/input/touchscreen/mms114.c > > +++ b/drivers/input/touchscreen/mms114.c > > @@ -204,14 +204,15 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id) > > } > > mutex_unlock(&input_dev->mutex); > > > > - packet_size = mms114_read_reg(data, MMS114_PACKET_SIZE); > > + packet_size = i2c_smbus_read_byte_data(data->client, > > + MMS114_PACKET_SIZE); > > if (packet_size <= 0) > > goto out; > > > > touch_size = packet_size / MMS114_PACKET_NUM; > > > > - error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, > > - (u8 *)touch); > > + error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFORMATION, > > + packet_size, (u8 *)touch); > > ... and here we try to read up to 80 bytes, as you mentioned. > > i2c_smbus_read_i2c_block_data() will silently fall back to reading only > 32 bytes. Therefore, if we try to read more than 32 bytes here we will > later read uninitialized data. > > With this change, if you use more than 4 fingers you can easily trigger > a situation where one of the fingers gets "stuck", together with: > mms114 4-0048: Wrong touch type (0) yes, it can be that for this there might be some delays or miss reads. > So we still need the custom functions here, or maybe avoid the problem > by using regmap instead. This was what Dmitry was proposing almost a couple of years ago, but then I stopped working on this. > > + error = i2c_smbus_read_i2c_block_data(data->client, > > + MMS152_FW_REV, 3, buf); > > if (error) > > i2c_smbus_read_i2c_block_data() returns the number of read bytes, > therefore this check will always fail. > > It should be: if (error < 0) > > + error = i2c_smbus_read_i2c_block_data(data->client, > > + MMS114_TSP_REV, 6, buf); > > if (error) > > As above. Yes, an oversight in these two cases. Thanks! Well, I guess some more rework needs to be done in this patch... at the end Dmitry was right :) Thanks, Andi [*] http://www.smbus.org/specs/
