[PATCH 0/3] input: ft5x06: Fix userspace reported maximum value

[PATCH 0/3] input: ft5x06: Fix userspace reported maximum value

[Maxime Ripard]

Hi,

The current ft5x06 reports to the user-space that its maximum
coordinates are, on both X and Y, way higher than what could be
actually usable on the screen (in my case, 5759x1151 instead of
480x800).

This causes trouble on some userspace stacks that then try to re-scale
these coordinates back to the framebuffer resolution, like QT does.

Use the of_touchscreen code to find the real touchscreen limits in the
DT case, and report that to the userspace.

Thanks,
Maxime

Maxime Ripard (3):
  input: touchscreen: of: Use input_set_abs_params
  input: touchscreen: of: Register multitouch axes
  input: ft5x06: Allow to set the maximum axes value through the DT

 drivers/input/touchscreen/edt-ft5x06.c     |  5 ++++
 drivers/input/touchscreen/of_touchscreen.c | 39 ++++++++++++++++++++++--------
 2 files changed, 34 insertions(+), 10 deletions(-)

-- 
2.1.1

[PATCH 1/3] input: touchscreen: of: Use input_set_abs_params

[Maxime Ripard]

Drivers are still required to call input_set_abs_params for their axes, as if
they only use the touchscreen_parse_of_params function, the axis bit in absbit
won't be set.

Switch to using input_set_abs_params to fully setup each and every available
axis so that drivers will be able to solely use this function.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/input/touchscreen/of_touchscreen.c | 33 +++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index f8f9b84230b1..74d6b0eb50ac 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -13,6 +13,16 @@
 #include <linux/input.h>
 #include <linux/input/touchscreen.h>
 
+static u32 of_get_optional_u32(struct device_node *np,
+			       const char *property)
+{
+	u32 val = 0;
+
+	of_property_read_u32(np, property, &val);
+
+	return val;
+}
+
 /**
  * touchscreen_parse_of_params - parse common touchscreen DT properties
  * @dev: device that should be parsed
@@ -24,22 +34,25 @@
 void touchscreen_parse_of_params(struct input_dev *dev)
 {
 	struct device_node *np = dev->dev.parent->of_node;
-	struct input_absinfo *absinfo;
+	u32 maximum, fuzz;
 
 	input_alloc_absinfo(dev);
 	if (!dev->absinfo)
 		return;
 
-	absinfo = &dev->absinfo[ABS_X];
-	of_property_read_u32(np, "touchscreen-size-x", &absinfo->maximum);
-	of_property_read_u32(np, "touchscreen-fuzz-x", &absinfo->fuzz);
+	maximum = of_get_optional_u32(np, "touchscreen-size-x");
+	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x");
+	if (maximum || fuzz)
+		input_set_abs_params(dev, ABS_X, 0, maximum, fuzz, 0);
 
-	absinfo = &dev->absinfo[ABS_Y];
-	of_property_read_u32(np, "touchscreen-size-y", &absinfo->maximum);
-	of_property_read_u32(np, "touchscreen-fuzz-y", &absinfo->fuzz);
+	maximum = of_get_optional_u32(np, "touchscreen-size-y");
+	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-y");
+	if (maximum || fuzz)
+		input_set_abs_params(dev, ABS_Y, 0, maximum, fuzz, 0);
 
-	absinfo = &dev->absinfo[ABS_PRESSURE];
-	of_property_read_u32(np, "touchscreen-max-pressure", &absinfo->maximum);
-	of_property_read_u32(np, "touchscreen-fuzz-pressure", &absinfo->fuzz);
+	maximum = of_get_optional_u32(np, "touchscreen-max-pressure");
+	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure");
+	if (maximum || fuzz)
+		input_set_abs_params(dev, ABS_PRESSURE, 0, maximum, fuzz, 0);
 }
 EXPORT_SYMBOL(touchscreen_parse_of_params);
-- 
2.1.1

[PATCH 2/3] input: touchscreen: of: Register multitouch axes

[Maxime Ripard]

So far, the DT parsing code was only setting up the regular input axes,
completely ignoring their multitouch counter parts.

Fill them with the same parameters than the regular axes.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/input/touchscreen/of_touchscreen.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index 74d6b0eb50ac..cf2a753edd96 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -42,17 +42,23 @@ void touchscreen_parse_of_params(struct input_dev *dev)
 
 	maximum = of_get_optional_u32(np, "touchscreen-size-x");
 	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x");
-	if (maximum || fuzz)
+	if (maximum || fuzz) {
 		input_set_abs_params(dev, ABS_X, 0, maximum, fuzz, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_X, 0, maximum, fuzz, 0);
+	}
 
 	maximum = of_get_optional_u32(np, "touchscreen-size-y");
 	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-y");
-	if (maximum || fuzz)
+	if (maximum || fuzz) {
 		input_set_abs_params(dev, ABS_Y, 0, maximum, fuzz, 0);
+		input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, maximum, fuzz, 0);
+	}
 
 	maximum = of_get_optional_u32(np, "touchscreen-max-pressure");
 	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure");
-	if (maximum || fuzz)
+	if (maximum || fuzz) {
 		input_set_abs_params(dev, ABS_PRESSURE, 0, maximum, fuzz, 0);
+		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, maximum, fuzz, 0);
+	}
 }
 EXPORT_SYMBOL(touchscreen_parse_of_params);
-- 
2.1.1

Re: [PATCH 2/3] input: touchscreen: of: Register multitouch axes

[Dmitry Torokhov]

On Wed, Nov 05, 2014 at 04:07:49PM +0100, Maxime Ripard wrote:
> So far, the DT parsing code was only setting up the regular input axes,
> completely ignoring their multitouch counter parts.
> 
> Fill them with the same parameters than the regular axes.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/input/touchscreen/of_touchscreen.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> index 74d6b0eb50ac..cf2a753edd96 100644
> --- a/drivers/input/touchscreen/of_touchscreen.c
> +++ b/drivers/input/touchscreen/of_touchscreen.c
> @@ -42,17 +42,23 @@ void touchscreen_parse_of_params(struct input_dev *dev)
>  
>  	maximum = of_get_optional_u32(np, "touchscreen-size-x");
>  	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x");
> -	if (maximum || fuzz)
> +	if (maximum || fuzz) {
>  		input_set_abs_params(dev, ABS_X, 0, maximum, fuzz, 0);
> +		input_set_abs_params(dev, ABS_MT_POSITION_X, 0, maximum, fuzz, 0);

Not all devices are multitouch so you shoudl not be setting multitouch
bits unconditionally.

In I think we should rely on driver to set capability bits properly and
then here test them and apply the readings. Probably also issue a
warning if we see max/fuzz setting but neither ABS_<N>/ABS_MT_<N>
capabilities.

Thanks.

> +	}
>  
>  	maximum = of_get_optional_u32(np, "touchscreen-size-y");
>  	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-y");
> -	if (maximum || fuzz)
> +	if (maximum || fuzz) {
>  		input_set_abs_params(dev, ABS_Y, 0, maximum, fuzz, 0);
> +		input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, maximum, fuzz, 0);
> +	}
>  
>  	maximum = of_get_optional_u32(np, "touchscreen-max-pressure");
>  	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure");
> -	if (maximum || fuzz)
> +	if (maximum || fuzz) {
>  		input_set_abs_params(dev, ABS_PRESSURE, 0, maximum, fuzz, 0);
> +		input_set_abs_params(dev, ABS_MT_PRESSURE, 0, maximum, fuzz, 0);
> +	}
>  }
>  EXPORT_SYMBOL(touchscreen_parse_of_params);
> -- 
> 2.1.1
> 

-- 
Dmitry

Re: [PATCH 2/3] input: touchscreen: of: Register multitouch axes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

Hi,

On Wed, Nov 05, 2014 at 09:56:35AM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 05, 2014 at 04:07:49PM +0100, Maxime Ripard wrote:
> > So far, the DT parsing code was only setting up the regular input axes,
> > completely ignoring their multitouch counter parts.
> > 
> > Fill them with the same parameters than the regular axes.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/input/touchscreen/of_touchscreen.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> > index 74d6b0eb50ac..cf2a753edd96 100644
> > --- a/drivers/input/touchscreen/of_touchscreen.c
> > +++ b/drivers/input/touchscreen/of_touchscreen.c
> > @@ -42,17 +42,23 @@ void touchscreen_parse_of_params(struct input_dev *dev)
> >  
> >  	maximum = of_get_optional_u32(np, "touchscreen-size-x");
> >  	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x");
> > -	if (maximum || fuzz)
> > +	if (maximum || fuzz) {
> >  		input_set_abs_params(dev, ABS_X, 0, maximum, fuzz, 0);
> > +		input_set_abs_params(dev, ABS_MT_POSITION_X, 0, maximum, fuzz, 0);
> 
> Not all devices are multitouch so you shoudl not be setting multitouch
> bits unconditionally.

Hmmm, right.

> In I think we should rely on driver to set capability bits properly and
> then here test them and apply the readings. Probably also issue a
> warning if we see max/fuzz setting but neither ABS_<N>/ABS_MT_<N>
> capabilities.

What I was aiming at was to avoid DT parsing duplication for the
!multitouch and multitouch axis. I don't think there's a way to copy
the parameters.

The mt_init_slots might be a solution, but it does the copy the other
way around: from the multitouch to the !multitouch axis, and without
enabling it, which renders using both input_mt_init_slots and the
of_touchscreen code together impossible.

Is there a way to just enable an axis without calling
input_set_abs_params? Is __set_bit enough?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] input: touchscreen: of: Register multitouch axes

[Dmitry Torokhov]

On Thu, Nov 06, 2014 at 05:06:21PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Nov 05, 2014 at 09:56:35AM -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 05, 2014 at 04:07:49PM +0100, Maxime Ripard wrote:
> > > So far, the DT parsing code was only setting up the regular input axes,
> > > completely ignoring their multitouch counter parts.
> > > 
> > > Fill them with the same parameters than the regular axes.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > ---
> > >  drivers/input/touchscreen/of_touchscreen.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> > > index 74d6b0eb50ac..cf2a753edd96 100644
> > > --- a/drivers/input/touchscreen/of_touchscreen.c
> > > +++ b/drivers/input/touchscreen/of_touchscreen.c
> > > @@ -42,17 +42,23 @@ void touchscreen_parse_of_params(struct input_dev *dev)
> > >  
> > >  	maximum = of_get_optional_u32(np, "touchscreen-size-x");
> > >  	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x");
> > > -	if (maximum || fuzz)
> > > +	if (maximum || fuzz) {
> > >  		input_set_abs_params(dev, ABS_X, 0, maximum, fuzz, 0);
> > > +		input_set_abs_params(dev, ABS_MT_POSITION_X, 0, maximum, fuzz, 0);
> > 
> > Not all devices are multitouch so you shoudl not be setting multitouch
> > bits unconditionally.
> 
> Hmmm, right.
> 
> > In I think we should rely on driver to set capability bits properly and
> > then here test them and apply the readings. Probably also issue a
> > warning if we see max/fuzz setting but neither ABS_<N>/ABS_MT_<N>
> > capabilities.
> 
> What I was aiming at was to avoid DT parsing duplication for the
> !multitouch and multitouch axis. I don't think there's a way to copy
> the parameters.
> 
> The mt_init_slots might be a solution, but it does the copy the other
> way around: from the multitouch to the !multitouch axis, and without
> enabling it, which renders using both input_mt_init_slots and the
> of_touchscreen code together impossible.
> 
> Is there a way to just enable an axis without calling
> input_set_abs_params? Is __set_bit enough?

Hmm, not anymore I'm afraid as you still need absinfo being allocated,
but there is input_set_capability() that is basically a fancy
__set_bit(). If you set ABS_* with it that would mean that the device
has unspecified min/max/fuzz.

And then, in DT parsing code, I'd do:

	...read max and fuzz for an axis...
	if (max || fuzz) {
		if (!test_bit(ABS_N, dev->absbit) &&
		    !test_bit(ABS_MT_N, dev->absbit)) {
			dev_warn("... have parameters but axis not set up...");
		} else {
			if (test_bit(ABS_N...)
				input_set_abs_params(dev, ABS_N...);
			if (test_bit(ABS_MT_N...)
				input_set_abs_params(dev, ABS_MT_N...);
	}

Thanks.

-- 
Dmitry

[PATCH 3/3] input: ft5x06: Allow to set the maximum axes value through the DT

[Maxime Ripard]

Currently the driver relies on some obscure and undocumented register to set
the maximum axis value.

The reported value is way too high to be meaningful, which confuses some
userspace tools like QT's evdevtouch plugin which try to scale the reported
events to the maximum values.

Use the values from the DT to set meaningful values.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index ee3434f1e949..3ebc1a7e46c5 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -37,6 +37,7 @@
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
 #include <linux/input/edt-ft5x06.h>
 
 #define MAX_SUPPORT_POINTS		5
@@ -1042,6 +1043,10 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 			     0, tsdata->num_x * 64 - 1, 0, 0);
 	input_set_abs_params(input, ABS_MT_POSITION_Y,
 			     0, tsdata->num_y * 64 - 1, 0, 0);
+
+	if (!pdata)
+		touchscreen_parse_of_params(input);
+
 	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, 0);
 	if (error) {
 		dev_err(&client->dev, "Unable to init MT slots.\n");
-- 
2.1.1