[PATCH 0/3] Add support for Melfas MMS345L touchscreen

[PATCH 0/3] Add support for Melfas MMS345L touchscreen

[Stephan Gerhold]

This patch series adds support for the Melfas MMS345L touchscreen
to the mms114 driver.

MMS345L is used in the Samsung Galaxy A5 (2015) that has recently
gained mainline support [1].

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1329c1ab0730b521e6cd3051c56a2ff3d55f21e6

Stephan Gerhold (3):
  Input: mms114 - use device_get_match_data
  dt-bindings: mms114: document melfas,mms345l binding
  Input: mms114 - add support for mms345l

 .../bindings/input/touchscreen/mms114.txt     |  3 +-
 drivers/input/touchscreen/mms114.c            | 44 +++++++++++++------
 2 files changed, 33 insertions(+), 14 deletions(-)

-- 
2.23.0

[PATCH 1/3] Input: mms114 - use device_get_match_data

[Stephan Gerhold]

device_get_match_data is available now, so we can replace the call
to of_device_get_match_data and remove the FIXME comment.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/input/touchscreen/mms114.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index a5ab774da4cc..69c6d559eeb0 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -446,8 +446,7 @@ static int mms114_probe(struct i2c_client *client,
 	data->client = client;
 	data->input_dev = input_dev;
 
-	/* FIXME: switch to device_get_match_data() when available */
-	match_data = of_device_get_match_data(&client->dev);
+	match_data = device_get_match_data(&client->dev);
 	if (!match_data)
 		return -EINVAL;
 
-- 
2.23.0

Re: [PATCH 1/3] Input: mms114 - use device_get_match_data

[Andi Shyti]

Hi Stephan,

> device_get_match_data is available now, so we can replace the call
> to of_device_get_match_data and remove the FIXME comment.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Andi Shyti <andi@etezian.org>

Thanks,
Andi

Re: [PATCH 1/3] Input: mms114 - use device_get_match_data

[Dmitry Torokhov]

On Tue, Oct 08, 2019 at 02:44:26PM +0300, Andi Shyti wrote:
> Hi Stephan,
> 
> > device_get_match_data is available now, so we can replace the call
> > to of_device_get_match_data and remove the FIXME comment.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Reviewed-by: Andi Shyti <andi@etezian.org>

Applied, thank you.

-- 
Dmitry

[PATCH 2/3] dt-bindings: mms114: document melfas,mms345l binding

[Stephan Gerhold]

The mms114 driver now supports MMS345L; document the
melfas,mms345l binding that is used for it.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 Documentation/devicetree/bindings/input/touchscreen/mms114.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
index 2cd954051d29..707234cfd7e6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/mms114.txt
@@ -1,9 +1,10 @@
-* MELFAS MMS114/MMS152 touchscreen controller
+* MELFAS MMS114/MMS152/MMS345L touchscreen controller
 
 Required properties:
 - compatible: should be one of:
 	- "melfas,mms114"
 	- "melfas,mms152"
+	- "melfas,mms345l"
 - reg: I2C address of the chip
 - interrupts: interrupt to which the chip is connected
 - touchscreen-size-x: See [1]
-- 
2.23.0

Re: [PATCH 2/3] dt-bindings: mms114: document melfas,mms345l binding

[Andi Shyti]

Hi Stephan,

On Mon, Oct 07, 2019 at 10:33:42PM +0200, Stephan Gerhold wrote:
> The mms114 driver now supports MMS345L; document the
> melfas,mms345l binding that is used for it.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Andi Shyti <andi@etezian.org>

Thanks,
Andi

Re: [PATCH 2/3] dt-bindings: mms114: document melfas,mms345l binding

[Rob Herring]

On Mon,  7 Oct 2019 22:33:42 +0200, Stephan Gerhold wrote:
> The mms114 driver now supports MMS345L; document the
> melfas,mms345l binding that is used for it.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  Documentation/devicetree/bindings/input/touchscreen/mms114.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 3/3] Input: mms114 - add support for mms345l

[Stephan Gerhold]

MMS345L is another first generation touch screen from Melfas,
which uses the same registers as MMS152.

However, using I2C_M_NOSTART for it causes errors when reading:

	i2c i2c-0: sendbytes: NAK bailout.
	mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5)

The driver works fine as soon as I2C_M_NOSTART is removed.

Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART
only for MMS114 and MMS152.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Note: I was not able to find a datasheet for any of the models,
so this change is merely based on testing and comparison with
the downstream driver [1].

There was a related patch [2] that removes I2C_M_NOSTART for all models,
but it seems abandoned and I do not have any other model for testing.
Therefore, this patch implements the least instrusive solution
and only removes I2C_M_NOSTART for MMS345L.

[1]: https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/SM-A500FU/drivers/input/touchscreen/mms300_a.c
[2]: https://patchwork.kernel.org/patch/10189541/
---
 drivers/input/touchscreen/mms114.c | 43 +++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 69c6d559eeb0..d9f45755d073 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -54,6 +54,7 @@
 enum mms_type {
 	TYPE_MMS114	= 114,
 	TYPE_MMS152	= 152,
+	TYPE_MMS345L	= 345,
 };
 
 struct mms114_data {
@@ -91,9 +92,14 @@ static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
 	if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
 		BUG();
 
-	/* Write register: use repeated start */
+	/* Write register */
 	xfer[0].addr = client->addr;
-	xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
+	if (data->type != TYPE_MMS345L)
+		/* use repeated start */
+		xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
+	else
+		xfer[0].flags = client->flags & I2C_M_TEN;
+
 	xfer[0].len = 1;
 	xfer[0].buf = &buf;
 
@@ -250,6 +256,15 @@ static int mms114_get_version(struct mms114_data *data)
 	int error;
 
 	switch (data->type) {
+	case TYPE_MMS345L:
+		error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+		if (error)
+			return error;
+
+		dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x\n",
+			 buf[0], buf[1], buf[2]);
+		break;
+
 	case TYPE_MMS152:
 		error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
 		if (error)
@@ -287,8 +302,8 @@ static int mms114_setup_regs(struct mms114_data *data)
 	if (error < 0)
 		return error;
 
-	/* MMS152 has no configuration or power on registers */
-	if (data->type == TYPE_MMS152)
+	/* Only MMS114 has configuration and power on registers */
+	if (data->type != TYPE_MMS114)
 		return 0;
 
 	error = mms114_set_active(data, true);
@@ -425,11 +440,16 @@ static int mms114_probe(struct i2c_client *client,
 {
 	struct mms114_data *data;
 	struct input_dev *input_dev;
-	const void *match_data;
+	enum mms_type type;
 	int error;
 
-	if (!i2c_check_functionality(client->adapter,
-				I2C_FUNC_PROTOCOL_MANGLING)) {
+	type = (enum mms_type)device_get_match_data(&client->dev);
+	if (!type)
+		return -EINVAL;
+
+	if (type != TYPE_MMS345L &&
+	    !i2c_check_functionality(client->adapter,
+				     I2C_FUNC_PROTOCOL_MANGLING)) {
 		dev_err(&client->dev,
 			"Need i2c bus that supports protocol mangling\n");
 		return -ENODEV;
@@ -446,11 +466,7 @@ static int mms114_probe(struct i2c_client *client,
 	data->client = client;
 	data->input_dev = input_dev;
 
-	match_data = device_get_match_data(&client->dev);
-	if (!match_data)
-		return -EINVAL;
-
-	data->type = (enum mms_type)match_data;
+	data->type = type;
 
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
@@ -599,6 +615,9 @@ static const struct of_device_id mms114_dt_match[] = {
 	}, {
 		.compatible = "melfas,mms152",
 		.data = (void *)TYPE_MMS152,
+	}, {
+		.compatible = "melfas,mms345l",
+		.data = (void *)TYPE_MMS345L,
 	},
 	{ }
 };
-- 
2.23.0

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Stephan,

On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote:
> MMS345L is another first generation touch screen from Melfas,
> which uses the same registers as MMS152.
> 
> However, using I2C_M_NOSTART for it causes errors when reading:
> 
> 	i2c i2c-0: sendbytes: NAK bailout.
> 	mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5)
> 
> The driver works fine as soon as I2C_M_NOSTART is removed.
> 
> Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART
> only for MMS114 and MMS152.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Andi Shyti <andi@etezian.org>

just a nitpick in case you will resend it (but you don't need
to).

> -	if (!i2c_check_functionality(client->adapter,
> -				I2C_FUNC_PROTOCOL_MANGLING)) {
> +	type = (enum mms_type)device_get_match_data(&client->dev);

you don't need any cast here.

Thanks,
Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

> > -	if (!i2c_check_functionality(client->adapter,
> > -				I2C_FUNC_PROTOCOL_MANGLING)) {
> > +	type = (enum mms_type)device_get_match_data(&client->dev);
> 
> you don't need any cast here.

sorry, please ignore :)

Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Dmitry Torokhov]

On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote:
> MMS345L is another first generation touch screen from Melfas,
> which uses the same registers as MMS152.
> 
> However, using I2C_M_NOSTART for it causes errors when reading:
> 
> 	i2c i2c-0: sendbytes: NAK bailout.
> 	mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5)
> 
> The driver works fine as soon as I2C_M_NOSTART is removed.
> 
> Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART
> only for MMS114 and MMS152.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Note: I was not able to find a datasheet for any of the models,
> so this change is merely based on testing and comparison with
> the downstream driver [1].
> 
> There was a related patch [2] that removes I2C_M_NOSTART for all models,
> but it seems abandoned and I do not have any other model for testing.
> Therefore, this patch implements the least instrusive solution
> and only removes I2C_M_NOSTART for MMS345L.

Hmm,  at this point I am inclined to pick up Andi's patch since it seems
to work for you and him and it looks like Android drivers are not using
I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
it was originally developed.

Any objections?

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Dmitry,

> > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > but it seems abandoned and I do not have any other model for testing.
> > Therefore, this patch implements the least instrusive solution
> > and only removes I2C_M_NOSTART for MMS345L.
> 
> Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> to work for you and him and it looks like Android drivers are not using
> I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> it was originally developed.

I completely forgot about that patch :)

I should refresh some old work on that device which was left
undone.

> Any objections?

It's OK for me. If you can just update my e-mail, please, when
applying the patch to "andi@etezian.org". Thanks!

Thank you,
Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Dmitry Torokhov]

On Wed, Oct 09, 2019 at 10:26:12AM +0300, Andi Shyti wrote:
> Hi Dmitry,
> 
> > > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > > but it seems abandoned and I do not have any other model for testing.
> > > Therefore, this patch implements the least instrusive solution
> > > and only removes I2C_M_NOSTART for MMS345L.
> > 
> > Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> > to work for you and him and it looks like Android drivers are not using
> > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> > it was originally developed.
> 
> I completely forgot about that patch :)
> 
> I should refresh some old work on that device which was left
> undone.
> 
> > Any objections?
> 
> It's OK for me. If you can just update my e-mail, please, when
> applying the patch to "andi@etezian.org". Thanks!

Andi, any chance you could resend it with the new email? One thing that
I try to avoid modifying whenever I can is S-O-B strings...

Thanks.

-- 
Dmitry

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Dmitry,

> > > > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > > > but it seems abandoned and I do not have any other model for testing.
> > > > Therefore, this patch implements the least instrusive solution
> > > > and only removes I2C_M_NOSTART for MMS345L.
> > > 
> > > Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> > > to work for you and him and it looks like Android drivers are not using
> > > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> > > it was originally developed.
> > 
> > I completely forgot about that patch :)
> > 
> > I should refresh some old work on that device which was left
> > undone.
> > 
> > > Any objections?
> > 
> > It's OK for me. If you can just update my e-mail, please, when
> > applying the patch to "andi@etezian.org". Thanks!
> 
> Andi, any chance you could resend it with the new email? One thing that
> I try to avoid modifying whenever I can is S-O-B strings...

sure, I will resend the patches, but it might take some time (a
few days I hope) because I don't have the devices with me right
now for testing (and I added some small fixes, as well)

Thanks,
Andi

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Stephan Gerhold]

On Tue, Oct 08, 2019 at 03:00:14PM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote:
> > MMS345L is another first generation touch screen from Melfas,
> > which uses the same registers as MMS152.
> > 
> > However, using I2C_M_NOSTART for it causes errors when reading:
> > 
> > 	i2c i2c-0: sendbytes: NAK bailout.
> > 	mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5)
> > 
> > The driver works fine as soon as I2C_M_NOSTART is removed.
> > 
> > Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART
> > only for MMS114 and MMS152.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > Note: I was not able to find a datasheet for any of the models,
> > so this change is merely based on testing and comparison with
> > the downstream driver [1].
> > 
> > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > but it seems abandoned and I do not have any other model for testing.
> > Therefore, this patch implements the least instrusive solution
> > and only removes I2C_M_NOSTART for MMS345L.
> 
> Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> to work for you and him and it looks like Android drivers are not using
> I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> it was originally developed.
> 
> Any objections?

I cannot really speak for any of the other models, but no objections for
removing I2C_M_NOSTART from my side. I'm actually rather confused by it
since it is used on the first partial message.

The documentation [1] says:
  If you set the I2C_M_NOSTART variable for the first partial message,
  we do not generate Addr, but we do generate the startbit S.
  ** This will probably confuse all other clients on your bus,
  so don't try this. **

Yet, someone felt like trying this here. ;)

I have tested the following two patches from Andy on MMS345L:
  - Input: mms114 - use smbus functions whenever possible [2]
  - Input: mms114 - get read of custm i2c read/write functions [3]

Indeed, with I2C_M_NOSTART removed I can actually use "melfas,mms152"
and the touchscreen appears to work without further changes.
(The only weird thing is that it displays an empty "Compat group" in
  "TSP FW Rev: bootloader 0x6 / core 0x26 / config 0x26, Compat group: ",
 I suspect that register does not exist on MMS345L...)

But there is a limitation that won't let us take these two patches as-is:
i2c_smbus_read_i2c_block_data() is limited to I2C_SMBUS_BLOCK_MAX,
which is: 32 /* As specified in SMBus standard */

According to "Input: mms114 - use smbus functions whenever possible":
> 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.

Since i2c_smbus_read_i2c_block_data() is limited to 32, larger packets
won't be read correctly. For example, 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)
(It attempts to read uninitialized data in this case...)

So we still need the custom functions for reading the touch packet,
or maybe change the driver to use regmap instead.

[1]: https://www.kernel.org/doc/html/latest/i2c/i2c-protocol.html
[2]: https://patchwork.kernel.org/patch/10189551/
[3]: https://patchwork.kernel.org/patch/10189541/

Re: [PATCH 3/3] Input: mms114 - add support for mms345l

[Andi Shyti]

Hi Stephan,

> > > There was a related patch [2] that removes I2C_M_NOSTART for all models,
> > > but it seems abandoned and I do not have any other model for testing.
> > > Therefore, this patch implements the least instrusive solution
> > > and only removes I2C_M_NOSTART for MMS345L.
> > 
> > Hmm,  at this point I am inclined to pick up Andi's patch since it seems
> > to work for you and him and it looks like Android drivers are not using
> > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where
> > it was originally developed.
> > 
> > Any objections?
> 
> I cannot really speak for any of the other models, but no objections for
> removing I2C_M_NOSTART from my side. I'm actually rather confused by it
> since it is used on the first partial message.
> 
> The documentation [1] says:
>   If you set the I2C_M_NOSTART variable for the first partial message,
>   we do not generate Addr, but we do generate the startbit S.
>   ** This will probably confuse all other clients on your bus,
>   so don't try this. **
> 
> Yet, someone felt like trying this here. ;)

still it should be specified in the i2c protocol of the device,
if it's not, then most probably it's not needed, I guess.

Andi