[v2,1/2] Input: mms114 - use smbus functions whenever possible
diff mbox series

Message ID 20191020202856.20287-2-andi@etezian.org
State New
Headers show
Series
  • Use smbus functions to communicate through i2c
Related show

Commit Message

Andi Shyti Oct. 20, 2019, 8:28 p.m. UTC
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(-)

Comments

Stephan Gerhold Oct. 21, 2019, 9:34 a.m. UTC | #1
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
>
Andi Shyti Oct. 21, 2019, 3:41 p.m. UTC | #2
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/
Stephan Gerhold Oct. 21, 2019, 4:26 p.m. UTC | #3
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
Andi Shyti Oct. 21, 2019, 4:39 p.m. UTC | #4
> > > 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
Dmitry Torokhov Oct. 22, 2019, 3:21 a.m. UTC | #5
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.
Andi Shyti Oct. 22, 2019, 11:18 a.m. UTC | #6
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

Patch
diff mbox series

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;
 	}