[PATCH v2 1/2] Input: mms114 - add support for mms345l

[PATCH v2 1/2] 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.

Reviewed-by: Andi Shyti <andi@etezian.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2:
  - Pick up review tags
  - Clarify intention

I sent more or less the same patch an half year(!) ago:
  https://patchwork.kernel.org/patch/11178515/

Back then we discussed removing the I2C_M_NOSTART quirk for all
devices in the mms114 driver, and using the chance to convert it to
use smbus/regmap functions instead of the custom i2c read/write functions.

However, this changes the behavior for all existing devices,
(eventually causing a regression if the quirk is really needed for them).
The problem with the smbus patch was that i2c_smbus_read_i2c_block_data()
is currently limited to 32 bytes (as specified in older versions of the SMBus
standard), which broke larger touchscreen packets (i.e. multi-touch).

Until today, I'm still not quite sure what needs to be changed to finally
gain support for MMS345L in mainline, despite several emails asking for
clarification. MMS345L support is seemingly stuck forever :(

I have only my device with MMS345L for testing, and everyone with one
of the other devices seems to be rather busy. IMO it would be much safer
to take this patch, which keeps behavior for all existing devices as-is.

Can we reconsider applying this patch and do the refactoring
separately later?

Thanks,
Stephan
---
 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.26.0

[PATCH v2 2/2] 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.

Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andi Shyti <andi@etezian.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2:
  - Pick up review tags

v1: https://patchwork.kernel.org/patch/11178503/
---
 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.26.0

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

[Andi Shyti]

Hi Stephan,

On Sun, Apr 05, 2020 at 07:09:04PM +0200, Stephan Gerhold wrote:
> The mms114 driver now supports MMS345L; document the
> melfas,mms345l binding that is used for it.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Andi Shyti <andi@etezian.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

just one nitpick, the signing should be sorted in chronological
order, I see that you reverted it.

You first signed it as the author, then I reviewed it and the Rob
acked it, so that it should be:

  Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
  Reviewed-by: Andi Shyti <andi@etezian.org>
  Acked-by: Rob Herring <robh@kernel.org>

you reversed it.

Other than that, I'm sorry I couldn't help you but I haven't
received answers from Samsung about getting the prototypes.
However I don't see reason for not applying the patch.

Andi

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

[Stephan Gerhold]

On Wed, Apr 08, 2020 at 12:51:43AM +0300, Andi Shyti wrote:
> Hi Stephan,
> 
> On Sun, Apr 05, 2020 at 07:09:04PM +0200, Stephan Gerhold wrote:
> > The mms114 driver now supports MMS345L; document the
> > melfas,mms345l binding that is used for it.
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> > Reviewed-by: Andi Shyti <andi@etezian.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> just one nitpick, the signing should be sorted in chronological
> order, I see that you reverted it.
> 
> You first signed it as the author, then I reviewed it and the Rob
> acked it, so that it should be:
> 
>   Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>   Reviewed-by: Andi Shyti <andi@etezian.org>
>   Acked-by: Rob Herring <robh@kernel.org>
> 
> you reversed it.
> 

I didn't really "reverse" it, I didn't know the order of these tags is
important at all. :) Is this documented somewhere?

Since I have added the tags and re-sent the patch,
I thought it would make sense have my Signed-off-by last.

I suppose this can be easily fixed up if Dmitry wants to apply the
patch. But if I should re-send with the order changed in some way,
just let me know. :)

> Other than that, I'm sorry I couldn't help you but I haven't
> received answers from Samsung about getting the prototypes.
> However I don't see reason for not applying the patch.

It's fine, don't worry :)

I just think at this point we should either take my patch
(and preserve existing behavior for all currently supported devices),
or accept the potential breakage when refactoring the driver
(because seemingly no-one is able to test it on the other versions
at the moment).

Thanks,
Stephan

Re: [PATCH v2 1/2] Input: mms114 - add support for mms345l

[Dmitry Torokhov]

Hi Stephan,

On Sun, Apr 05, 2020 at 07:09:03PM +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.
> 
> Reviewed-by: Andi Shyti <andi@etezian.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Sorry for sitting on this for so long. I looked around, and I think that
instead of adding separate handling for 345L we shoudl simply drop the
"no start" bit for everyone. I am not sure what the original version
tried to do here, as far as I can see in various public Android trees the
driver for these devices does not use I2C_M_NOSTART.

Actually, I wonder if the difference is not in the touch controller that
is being used, but rather in the I2C controller the device in connected
to.

I would like to apply the version of the patch below.

Thanks.

-- 
Dmitry


Input: mms114 - fix handling of mms345l

From: Stephan Gerhold <stephan@gerhold.net>

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.

Reviewed-by: Andi Shyti <andi@etezian.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Link: https://lore.kernel.org/r/20200405170904.61512-1-stephan@gerhold.net
Patchwork-Id: 11474771
[dtor: removed separate mms345l handling, made everyone use standard
transfer mode, propagated the 10bit addressing flag to the read part of the
transfer as well.]
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/mms114.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 69c6d559eeb0..2ef1adaed9af 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -91,15 +91,15 @@ 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;
+	xfer[0].flags = client->flags & I2C_M_TEN;
 	xfer[0].len = 1;
 	xfer[0].buf = &buf;
 
 	/* Read data */
 	xfer[1].addr = client->addr;
-	xfer[1].flags = I2C_M_RD;
+	xfer[1].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
 	xfer[1].len = len;
 	xfer[1].buf = val;
 
@@ -428,10 +428,8 @@ static int mms114_probe(struct i2c_client *client,
 	const void *match_data;
 	int error;
 
-	if (!i2c_check_functionality(client->adapter,
-				I2C_FUNC_PROTOCOL_MANGLING)) {
-		dev_err(&client->dev,
-			"Need i2c bus that supports protocol mangling\n");
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "Not supported I2C adapter\n");
 		return -ENODEV;
 	}
 

Re: [PATCH v2 1/2] Input: mms114 - add support for mms345l

[Stephan Gerhold]

Hi Dmitry,

On Thu, Apr 09, 2020 at 03:15:03PM -0700, Dmitry Torokhov wrote:
> Hi Stephan,
> 
> On Sun, Apr 05, 2020 at 07:09:03PM +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.
> > 
> > Reviewed-by: Andi Shyti <andi@etezian.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> 
> Sorry for sitting on this for so long. I looked around, and I think that
> instead of adding separate handling for 345L we shoudl simply drop the
> "no start" bit for everyone. I am not sure what the original version
> tried to do here, as far as I can see in various public Android trees the
> driver for these devices does not use I2C_M_NOSTART.
> 
> Actually, I wonder if the difference is not in the touch controller that
> is being used, but rather in the I2C controller the device in connected
> to.
> 
> I would like to apply the version of the patch below.

Thanks a lot for your reply!

Your suggested patch below works fine on my MMS345L device
(with the melfas,mms152 compatible). Applying it would allow
the touchscreen to work on my device in mainline :)

I wonder if we should still add a separate melfas,mms345l compatible:

The only remaining difference to MMS152 at the moment is the
MMS152_COMPAT_GROUP register. It seems to be used for something
different on MMS345L, so when it is printed in mms114_get_version()
it just displays some garbage:

TSP FW Rev: bootloader 0x6 / core 0x26 / config 0x26, Compat group: \x06

(It used to actually print it as some ASCII control character but
 apparently that was fixed...)

It's not really critical, but in case we find more differences I think
it's better to directly add a separate compatible string to the device
tree.

If you agree with this I can send a separate patch with the new
compatible string once you have applied the patch below.

Thanks again!
Stephan

> 
> Input: mms114 - fix handling of mms345l
> 
> From: Stephan Gerhold <stephan@gerhold.net>
> 
> 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.
> 
> Reviewed-by: Andi Shyti <andi@etezian.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> Link: https://lore.kernel.org/r/20200405170904.61512-1-stephan@gerhold.net
> Patchwork-Id: 11474771
> [dtor: removed separate mms345l handling, made everyone use standard
> transfer mode, propagated the 10bit addressing flag to the read part of the
> transfer as well.]
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/mms114.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 69c6d559eeb0..2ef1adaed9af 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -91,15 +91,15 @@ 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;
> +	xfer[0].flags = client->flags & I2C_M_TEN;
>  	xfer[0].len = 1;
>  	xfer[0].buf = &buf;
>  
>  	/* Read data */
>  	xfer[1].addr = client->addr;
> -	xfer[1].flags = I2C_M_RD;
> +	xfer[1].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
>  	xfer[1].len = len;
>  	xfer[1].buf = val;
>  
> @@ -428,10 +428,8 @@ static int mms114_probe(struct i2c_client *client,
>  	const void *match_data;
>  	int error;
>  
> -	if (!i2c_check_functionality(client->adapter,
> -				I2C_FUNC_PROTOCOL_MANGLING)) {
> -		dev_err(&client->dev,
> -			"Need i2c bus that supports protocol mangling\n");
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "Not supported I2C adapter\n");
>  		return -ENODEV;
>  	}
>  

Re: [PATCH v2 1/2] Input: mms114 - add support for mms345l

[Dmitry Torokhov]

On Fri, Apr 10, 2020 at 12:27:02PM +0200, Stephan Gerhold wrote:
> Hi Dmitry,
> 
> On Thu, Apr 09, 2020 at 03:15:03PM -0700, Dmitry Torokhov wrote:
> > Hi Stephan,
> > 
> > On Sun, Apr 05, 2020 at 07:09:03PM +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.
> > > 
> > > Reviewed-by: Andi Shyti <andi@etezian.org>
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > 
> > Sorry for sitting on this for so long. I looked around, and I think that
> > instead of adding separate handling for 345L we shoudl simply drop the
> > "no start" bit for everyone. I am not sure what the original version
> > tried to do here, as far as I can see in various public Android trees the
> > driver for these devices does not use I2C_M_NOSTART.
> > 
> > Actually, I wonder if the difference is not in the touch controller that
> > is being used, but rather in the I2C controller the device in connected
> > to.
> > 
> > I would like to apply the version of the patch below.
> 
> Thanks a lot for your reply!
> 
> Your suggested patch below works fine on my MMS345L device
> (with the melfas,mms152 compatible). Applying it would allow
> the touchscreen to work on my device in mainline :)
> 
> I wonder if we should still add a separate melfas,mms345l compatible:
> 
> The only remaining difference to MMS152 at the moment is the
> MMS152_COMPAT_GROUP register. It seems to be used for something
> different on MMS345L, so when it is printed in mms114_get_version()
> it just displays some garbage:
> 
> TSP FW Rev: bootloader 0x6 / core 0x26 / config 0x26, Compat group: \x06
> 
> (It used to actually print it as some ASCII control character but
>  apparently that was fixed...)
> 
> It's not really critical, but in case we find more differences I think
> it's better to directly add a separate compatible string to the device
> tree.
> 
> If you agree with this I can send a separate patch with the new
> compatible string once you have applied the patch below.

Yes, if there is legitimate differences between the chips that would be
a good reason to add a new compatible string.

Please send a separate patch with updated justification and I will apply
it.

Thank you.

-- 
Dmitry