| Message ID | 20191020202856.20287-2-andi@etezian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use smbus functions to communicate through i2c | expand |
Hi Andi, Thanks for working on these patches! 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. > Thus it's not needed to make use of custom read/write functions > for accessing the i2c. Replace, whenever possible, the use of > custom functions with the more standard smbus ones. > > It's not possible only in one case, in the mms114_set_active() > function where the 'cache_mode_control' variable is updated > according to the value in the register 'MMS114_MODE_CONTROL' > register. > > Signed-off-by: Andi Shyti <andi@etezian.org> > Tested-by: Seung-Woo Kim <sw0312.kim@samsung.com> > --- > drivers/input/touchscreen/mms114.c | 32 +++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 12 deletions(-) > > 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) So we still need the custom functions here, or maybe avoid the problem by using regmap instead. > if (error < 0) > goto out; > > @@ -251,7 +252,8 @@ static int mms114_get_version(struct mms114_data *data) > > switch (data->type) { > case TYPE_MMS152: > - error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf); > + 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) > return error; > > @@ -265,7 +267,8 @@ static int mms114_get_version(struct mms114_data *data) > break; > > case TYPE_MMS114: > - error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf); > + error = i2c_smbus_read_i2c_block_data(data->client, > + MMS114_TSP_REV, 6, buf); > if (error) As above. > return error; > > @@ -297,30 +300,35 @@ static int mms114_setup_regs(struct mms114_data *data) > > val = (props->max_x >> 8) & 0xf; > val |= ((props->max_y >> 8) & 0xf) << 4; > - error = mms114_write_reg(data, MMS114_XY_RESOLUTION_H, val); > + error = i2c_smbus_write_byte_data(data->client, > + MMS114_XY_RESOLUTION_H, val); > if (error < 0) > return error; > > val = props->max_x & 0xff; > - error = mms114_write_reg(data, MMS114_X_RESOLUTION, val); > + error = i2c_smbus_write_byte_data(data->client, > + MMS114_X_RESOLUTION, val); > if (error < 0) > return error; > > val = props->max_x & 0xff; > - error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val); > + error = i2c_smbus_write_byte_data(data->client, > + MMS114_Y_RESOLUTION, val); > if (error < 0) > return error; > > if (data->contact_threshold) { > - error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD, > - data->contact_threshold); > + error = i2c_smbus_write_byte_data(data->client, > + MMS114_CONTACT_THRESHOLD, > + data->contact_threshold); > if (error < 0) > return error; > } > > if (data->moving_threshold) { > - error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD, > - data->moving_threshold); > + error = i2c_smbus_write_byte_data(data->client, > + MMS114_MOVING_THRESHOLD, > + data->moving_threshold); > if (error < 0) > return error; > } > -- > 2.24.0.rc0 >
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/
On Mon, Oct 21, 2019 at 06:41:05PM +0300, Andi Shyti wrote: > 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. You are right. It seems like that part of the specification was changed with SMBus version 3.0 [1]: 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. [1]: http://www.smbus.org/specs/SMBus_3_0_20141220.pdf
> > > 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. > > You are right. It seems like that part of the specification was changed > with SMBus version 3.0 [1]: > > 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. > > [1]: http://www.smbus.org/specs/SMBus_3_0_20141220.pdf yes :) OK, then I would ask Dmitry to hold on this patch I will try to update the smbus properly. Thanks a lot for the review, Stephan! Andi
On Mon, Oct 21, 2019 at 07:39:56PM +0300, Andi Shyti wrote: > > > > 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. > > > > You are right. It seems like that part of the specification was changed > > with SMBus version 3.0 [1]: > > > > 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. > > > > [1]: http://www.smbus.org/specs/SMBus_3_0_20141220.pdf > > yes :) > > OK, then I would ask Dmitry to hold on this patch I will try to > update the smbus properly. 3.0 is from 2014, so we can't simply update the limits. And we need to handle the case where device connected to a controller that does not implement 3.0 standard. If regmap is too much work then as a stop gap we could maybe only convert write functions and mention why read needs to be custom. Thanks.
Hi Dmitry, > > > > > 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. > > > > > > You are right. It seems like that part of the specification was changed > > > with SMBus version 3.0 [1]: > > > > > > 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. > > > > > > [1]: http://www.smbus.org/specs/SMBus_3_0_20141220.pdf > > > > yes :) > > > > OK, then I would ask Dmitry to hold on this patch I will try to > > update the smbus properly. > > 3.0 is from 2014, so we can't simply update the limits. And we need to > handle the case where device connected to a controller that does not > implement 3.0 standard. actually I don't see why, given that devices that were sending 32bytes will keep sending 32bytes and in any case I still haven't seen a controller that is strictly compliant to SMBUS 2. The mms114 device is a good example (and I think most of the touchscreens don't really care of the 32byte limit). In any case, I agree that I can't simply update the I2C_SMBUS_BLOCK_MAX because for sure I might have forgotten some cases and I'm currently looking how to do it. I have a few ideas but no one is good. I planned to send an RFC sometimes soon in order to kickstart some discussion. > If regmap is too much work then as a stop gap we could maybe only > convert write functions and mention why read needs to be custom. regmap is not too much work but I don't have the device with me, I might get one at some point, but can't do anything right now. Perhaps, for now you can take Stephan's patches and I would update everything once I get the device. They are not mutually exclusive, anyway. Thanks, Andi
Hi Dmitry, On Tue, Oct 22, 2019 at 02:18:39PM +0300, Andi Shyti wrote: > Hi Dmitry, > > > > > > > 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. > > > > > > > > You are right. It seems like that part of the specification was changed > > > > with SMBus version 3.0 [1]: > > > > > > > > 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. > > > > > > > > [1]: http://www.smbus.org/specs/SMBus_3_0_20141220.pdf > > > > > > yes :) > > > > > > OK, then I would ask Dmitry to hold on this patch I will try to > > > update the smbus properly. > > > > 3.0 is from 2014, so we can't simply update the limits. And we need to > > handle the case where device connected to a controller that does not > > implement 3.0 standard. > > actually I don't see why, given that devices that were sending > 32bytes will keep sending 32bytes and in any case I still haven't > seen a controller that is strictly compliant to SMBUS 2. The > mms114 device is a good example (and I think most of the > touchscreens don't really care of the 32byte limit). > > In any case, I agree that I can't simply update the > I2C_SMBUS_BLOCK_MAX because for sure I might have forgotten some > cases and I'm currently looking how to do it. I have a few ideas > but no one is good. I planned to send an RFC sometimes soon in > order to kickstart some discussion. > > > If regmap is too much work then as a stop gap we could maybe only > > convert write functions and mention why read needs to be custom. > > regmap is not too much work but I don't have the device with me, > I might get one at some point, but can't do anything right now. > > Perhaps, for now you can take Stephan's patches and I would > update everything once I get the device. They are not mutually > exclusive, anyway. What do you think about this? My patches for MMS345L keep behavior for MMS114 and MMS152 as-is, so we can be sure that there are no regressions for them. Cleaning up the driver to use SMBUS and/or regmap instead of the custom read/write methods is definitely something we should attempt to do at some point, but only when we can properly test them on MMS114. Until we have a MMS114 test device available, I would say that applying my patches is the least intrusive way to make MMS345L work. Thanks, Stephan
Hi Dmitry, On Mon, Nov 18, 2019 at 02:32:36PM +0100, Stephan Gerhold wrote: > Hi Dmitry, > > On Tue, Oct 22, 2019 at 02:18:39PM +0300, Andi Shyti wrote: > > Hi Dmitry, > > > > > If regmap is too much work then as a stop gap we could maybe only > > > convert write functions and mention why read needs to be custom. > > > > regmap is not too much work but I don't have the device with me, > > I might get one at some point, but can't do anything right now. > > > > Perhaps, for now you can take Stephan's patches and I would > > update everything once I get the device. They are not mutually > > exclusive, anyway. > > What do you think about this? > > My patches for MMS345L keep behavior for MMS114 and MMS152 as-is, > so we can be sure that there are no regressions for them. > > Cleaning up the driver to use SMBUS and/or regmap instead of the custom > read/write methods is definitely something we should attempt to do > at some point, but only when we can properly test them on MMS114. > > Until we have a MMS114 test device available, I would say that > applying my patches is the least intrusive way to make MMS345L work. > I would really like to find a solution for this. If it helps, I can re-send my patches for MMS345L (although they still apply cleanly...). Let me know what you would prefer! Thanks, Stephan
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); if (error < 0) goto out; @@ -251,7 +252,8 @@ static int mms114_get_version(struct mms114_data *data) switch (data->type) { case TYPE_MMS152: - error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf); + error = i2c_smbus_read_i2c_block_data(data->client, + MMS152_FW_REV, 3, buf); if (error) return error; @@ -265,7 +267,8 @@ static int mms114_get_version(struct mms114_data *data) break; case TYPE_MMS114: - error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf); + error = i2c_smbus_read_i2c_block_data(data->client, + MMS114_TSP_REV, 6, buf); if (error) return error; @@ -297,30 +300,35 @@ static int mms114_setup_regs(struct mms114_data *data) val = (props->max_x >> 8) & 0xf; val |= ((props->max_y >> 8) & 0xf) << 4; - error = mms114_write_reg(data, MMS114_XY_RESOLUTION_H, val); + error = i2c_smbus_write_byte_data(data->client, + MMS114_XY_RESOLUTION_H, val); if (error < 0) return error; val = props->max_x & 0xff; - error = mms114_write_reg(data, MMS114_X_RESOLUTION, val); + error = i2c_smbus_write_byte_data(data->client, + MMS114_X_RESOLUTION, val); if (error < 0) return error; val = props->max_x & 0xff; - error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val); + error = i2c_smbus_write_byte_data(data->client, + MMS114_Y_RESOLUTION, val); if (error < 0) return error; if (data->contact_threshold) { - error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD, - data->contact_threshold); + error = i2c_smbus_write_byte_data(data->client, + MMS114_CONTACT_THRESHOLD, + data->contact_threshold); if (error < 0) return error; } if (data->moving_threshold) { - error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD, - data->moving_threshold); + error = i2c_smbus_write_byte_data(data->client, + MMS114_MOVING_THRESHOLD, + data->moving_threshold); if (error < 0) return error; }