[PATCH v2 0/2] Use smbus functions to communicate through i2c

[PATCH v2 0/2] Use smbus functions to communicate through i2c

[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

[PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[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

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Stephan Gerhold]

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
> 

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Andi Shyti]

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/

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Stephan Gerhold]

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

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Andi Shyti]

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

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Dmitry Torokhov]

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

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Andi Shyti]

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

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Stephan Gerhold]

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

Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Stephan Gerhold]

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

[PATCH v2 2/2] Input: mms114 - get read of custm i2c read/write functions

[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