* [PATCH v2 0/2] Use smbus functions to communicate through i2c @ 2019-10-20 20:28 Andi Shyti 2019-10-20 20:28 ` [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible Andi Shyti 2019-10-20 20:28 ` [PATCH v2 2/2] Input: mms114 - get read of custm i2c read/write functions Andi Shyti 0 siblings, 2 replies; 11+ messages in thread From: Andi Shyti @ 2019-10-20 20:28 UTC (permalink / raw) To: Linux Input Cc: Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim, Stephan Gerhold, Andi Shyti Hi, this is the second version of [*] sent some times ago, but Dmitry didn't feel confident enough in taking it in :P Few changes in this second version. Thanks a lot Seung-Woo Kim for his support! Andi Andi Shyti (2): Input: mms114 - use smbus functions whenever possible Input: mms114 - get read of custm i2c read/write functions drivers/input/touchscreen/mms114.c | 119 ++++++++--------------------- 1 file changed, 30 insertions(+), 89 deletions(-) -- 2.24.0.rc0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-20 20:28 [PATCH v2 0/2] Use smbus functions to communicate through i2c Andi Shyti @ 2019-10-20 20:28 ` Andi Shyti 2019-10-21 9:34 ` Stephan Gerhold 2019-10-20 20:28 ` [PATCH v2 2/2] Input: mms114 - get read of custm i2c read/write functions Andi Shyti 1 sibling, 1 reply; 11+ messages in thread From: Andi Shyti @ 2019-10-20 20:28 UTC (permalink / raw) To: Linux Input Cc: Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim, Stephan Gerhold, Andi Shyti 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. 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); 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; } -- 2.24.0.rc0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-20 20:28 ` [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible Andi Shyti @ 2019-10-21 9:34 ` Stephan Gerhold 2019-10-21 15:41 ` Andi Shyti 0 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2019-10-21 9:34 UTC (permalink / raw) To: Andi Shyti; +Cc: Linux Input, Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-21 9:34 ` Stephan Gerhold @ 2019-10-21 15:41 ` Andi Shyti 2019-10-21 16:26 ` Stephan Gerhold 0 siblings, 1 reply; 11+ messages in thread From: Andi Shyti @ 2019-10-21 15:41 UTC (permalink / raw) To: Stephan Gerhold Cc: Andi Shyti, Linux Input, Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim 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/ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-21 15:41 ` Andi Shyti @ 2019-10-21 16:26 ` Stephan Gerhold 2019-10-21 16:39 ` Andi Shyti 0 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2019-10-21 16:26 UTC (permalink / raw) To: Andi Shyti; +Cc: Linux Input, Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-21 16:26 ` Stephan Gerhold @ 2019-10-21 16:39 ` Andi Shyti 2019-10-22 3:21 ` Dmitry Torokhov 0 siblings, 1 reply; 11+ messages in thread From: Andi Shyti @ 2019-10-21 16:39 UTC (permalink / raw) To: Stephan Gerhold Cc: Andi Shyti, Linux Input, Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-21 16:39 ` Andi Shyti @ 2019-10-22 3:21 ` Dmitry Torokhov 2019-10-22 11:18 ` Andi Shyti 0 siblings, 1 reply; 11+ messages in thread From: Dmitry Torokhov @ 2019-10-22 3:21 UTC (permalink / raw) To: Andi Shyti; +Cc: Stephan Gerhold, Linux Input, Seung-Woo Kim, Joonyoung Shim 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. -- Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-22 3:21 ` Dmitry Torokhov @ 2019-10-22 11:18 ` Andi Shyti 2019-11-18 13:32 ` Stephan Gerhold 0 siblings, 1 reply; 11+ messages in thread From: Andi Shyti @ 2019-10-22 11:18 UTC (permalink / raw) To: Dmitry Torokhov Cc: Andi Shyti, Stephan Gerhold, Linux Input, Seung-Woo Kim, Joonyoung Shim 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-10-22 11:18 ` Andi Shyti @ 2019-11-18 13:32 ` Stephan Gerhold 2019-12-04 16:47 ` Stephan Gerhold 0 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2019-11-18 13:32 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Andi Shyti, Linux Input, Seung-Woo Kim, Joonyoung Shim 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible 2019-11-18 13:32 ` Stephan Gerhold @ 2019-12-04 16:47 ` Stephan Gerhold 0 siblings, 0 replies; 11+ messages in thread From: Stephan Gerhold @ 2019-12-04 16:47 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Andi Shyti, Linux Input, Seung-Woo Kim, Joonyoung Shim 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] Input: mms114 - get read of custm i2c read/write functions 2019-10-20 20:28 [PATCH v2 0/2] Use smbus functions to communicate through i2c Andi Shyti 2019-10-20 20:28 ` [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible Andi Shyti @ 2019-10-20 20:28 ` Andi Shyti 1 sibling, 0 replies; 11+ messages in thread From: Andi Shyti @ 2019-10-20 20:28 UTC (permalink / raw) To: Linux Input Cc: Seung-Woo Kim, Dmitry Torokhov, Joonyoung Shim, Stephan Gerhold, Andi Shyti The 'mms114_read_reg' and 'mms114_write_reg' are used when reading or writing to the 'MMS114_MODE_CONTROL' register for updating the 'cache_mode_control' variable. Update the 'cache_mode_control' variable in the calling mms114_set_active() function and get rid of all the custom i2c read/write functions. With this remove also the redundant sleep of MMS114_I2C_DELAY (50us) between i2c operations. The waiting should to be handled by the i2c driver. Signed-off-by: Andi Shyti <andi@etezian.org> Tested-by: Seung-Woo Kim <sw0312.kim@samsung.com> --- drivers/input/touchscreen/mms114.c | 87 ++++-------------------------- 1 file changed, 10 insertions(+), 77 deletions(-) diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c index 170dcb5312b9..4131fe94e661 100644 --- a/drivers/input/touchscreen/mms114.c +++ b/drivers/input/touchscreen/mms114.c @@ -34,9 +34,6 @@ #define MMS152_FW_REV 0xE1 #define MMS152_COMPAT_GROUP 0xF2 -/* Minimum delay time is 50us between stop and start signal of i2c */ -#define MMS114_I2C_DELAY 50 - /* 200ms needs after power on */ #define MMS114_POWERON_DELAY 200 @@ -80,76 +77,6 @@ struct mms114_touch { u8 reserved[2]; } __packed; -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg, - unsigned int len, u8 *val) -{ - struct i2c_client *client = data->client; - struct i2c_msg xfer[2]; - u8 buf = reg & 0xff; - int error; - - if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL) - BUG(); - - /* Write register: use repeated start */ - xfer[0].addr = client->addr; - xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART; - xfer[0].len = 1; - xfer[0].buf = &buf; - - /* Read data */ - xfer[1].addr = client->addr; - xfer[1].flags = I2C_M_RD; - xfer[1].len = len; - xfer[1].buf = val; - - error = i2c_transfer(client->adapter, xfer, 2); - if (error != 2) { - dev_err(&client->dev, - "%s: i2c transfer failed (%d)\n", __func__, error); - return error < 0 ? error : -EIO; - } - udelay(MMS114_I2C_DELAY); - - return 0; -} - -static int mms114_read_reg(struct mms114_data *data, unsigned int reg) -{ - u8 val; - int error; - - if (reg == MMS114_MODE_CONTROL) - return data->cache_mode_control; - - error = __mms114_read_reg(data, reg, 1, &val); - return error < 0 ? error : val; -} - -static int mms114_write_reg(struct mms114_data *data, unsigned int reg, - unsigned int val) -{ - struct i2c_client *client = data->client; - u8 buf[2]; - int error; - - buf[0] = reg & 0xff; - buf[1] = val & 0xff; - - error = i2c_master_send(client, buf, 2); - if (error != 2) { - dev_err(&client->dev, - "%s: i2c send failed (%d)\n", __func__, error); - return error < 0 ? error : -EIO; - } - udelay(MMS114_I2C_DELAY); - - if (reg == MMS114_MODE_CONTROL) - data->cache_mode_control = val; - - return 0; -} - static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch) { struct i2c_client *client = data->client; @@ -228,19 +155,25 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id) static int mms114_set_active(struct mms114_data *data, bool active) { - int val; + int val, err; - val = mms114_read_reg(data, MMS114_MODE_CONTROL); + val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL); if (val < 0) return val; - val &= ~MMS114_OPERATION_MODE_MASK; + val = data->cache_mode_control &= ~MMS114_OPERATION_MODE_MASK; /* If active is false, sleep mode */ if (active) val |= MMS114_ACTIVE; - return mms114_write_reg(data, MMS114_MODE_CONTROL, val); + err = i2c_smbus_write_byte_data(data->client, MMS114_MODE_CONTROL, val); + if (err < 0) + return err; + + data->cache_mode_control = val; + + return 0; } static int mms114_get_version(struct mms114_data *data) -- 2.24.0.rc0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-04 16:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-20 20:28 [PATCH v2 0/2] Use smbus functions to communicate through i2c Andi Shyti 2019-10-20 20:28 ` [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible Andi Shyti 2019-10-21 9:34 ` Stephan Gerhold 2019-10-21 15:41 ` Andi Shyti 2019-10-21 16:26 ` Stephan Gerhold 2019-10-21 16:39 ` Andi Shyti 2019-10-22 3:21 ` Dmitry Torokhov 2019-10-22 11:18 ` Andi Shyti 2019-11-18 13:32 ` Stephan Gerhold 2019-12-04 16:47 ` Stephan Gerhold 2019-10-20 20:28 ` [PATCH v2 2/2] Input: mms114 - get read of custm i2c read/write functions Andi Shyti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).