Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
Hi! By the way, the previous version was busted, this one should compile. Input: improve parsing OF parameters for touchscreens From: Dmitry Torokhov dmitry.torok...@gmail.com When applying touchscreen parameters specified in device tree let's make sure we keep whatever setup was done by the driver and not reset the missing values to zero. Reported-by: Pavel Machek pa...@ucw.cz Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com Tested-by: Pavel Machek pa...@ucw.cz Thanks! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon 2015-06-01 14:32:13, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
Hi Dmitry,
On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or
suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.
Thanks.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 67
++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client
*client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
-const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+ const char *property,
+ unsigned int default_value,
+ unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
This looks good.
However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.
I know but it is not documented anywhere (as far as I know) so I'd
rather not rely on the implementation detail that might change in the
future. This is not a hot path so extra assignment should not hurt.
Seriously? Submit a patch documenting it, instead of writing strange
code. I bet everyone and their dog relies on it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon, Jun 01, 2015 at 02:32:13PM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
Hi Dmitry,
On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or
suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.
Thanks.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 67
++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client
*client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
-const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+ const char *property,
+ unsigned int default_value,
+ unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
This looks good.
However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.
I know but it is not documented anywhere (as far as I know) so I'd
rather not rely on the implementation detail that might change in the
future. This is not a hot path so extra assignment should not hurt.
It is actually: http://lxr.free-electrons.com/source/drivers/of/base.c#L1231
But you're right that it's mostly a cosmetic change.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Tue, Jun 02, 2015 at 10:58:45AM -0700, Dmitry Torokhov wrote:
On Tue, Jun 02, 2015 at 11:44:47AM +0200, Maxime Ripard wrote:
On Mon, Jun 01, 2015 at 02:32:13PM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
Hi Dmitry,
On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
-static u32 of_get_optional_u32(struct device_node *np,
-const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+ const char *property,
+ unsigned int default_value,
+ unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
This looks good.
However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.
I know but it is not documented anywhere (as far as I know) so I'd
rather not rely on the implementation detail that might change in the
future. This is not a hot path so extra assignment should not hurt.
It is actually: http://lxr.free-electrons.com/source/drivers/of/base.c#L1231
OK, fair enough. But not for ACPI properties (and I think we should
convert the parser to device_property_read_xxx() so it is usable
everywhere).
By the way, the previous version was busted, this one should compile.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 69 ++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 49 insertions(+), 29 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..806cd0a 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
- const char *property)
+static bool touchscreen_get_prop_u32(struct device_node *np,
+const char *property,
+unsigned int default_value,
+unsigned int *value)
{
- u32 val = 0;
+ u32 val;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
}
static void touchscreen_set_params(struct input_dev *dev,
@@ -54,34 +62,45 @@ static void touchscreen_set_params(struct input_dev *dev,
* input device accordingly. The function keeps previously setuped default
* values if no value is specified via DT.
*/
-void touchscreen_parse_of_params(struct input_dev *dev)
+void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
{
struct device_node *np = dev-dev.parent-of_node;
- u32 maximum, fuzz;
+ unsigned int axis;
+ unsigned int maximum, fuzz;
+ bool data_present;
input_alloc_absinfo(dev);
if (!dev-absinfo)
return;
- maximum = of_get_optional_u32(np, touchscreen-size-x);
- fuzz = of_get_optional_u32(np, touchscreen-fuzz-x);
- if (maximum || fuzz) {
- touchscreen_set_params(dev, ABS_X, maximum, fuzz);
- touchscreen_set_params(dev, ABS_MT_POSITION_X,
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Tue, Jun 02, 2015 at 11:44:47AM +0200, Maxime Ripard wrote:
On Mon, Jun 01, 2015 at 02:32:13PM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
Hi Dmitry,
On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or
suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it
in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.
Thanks.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 67
++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client
*client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS,
INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
- const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+const char *property,
+unsigned int default_value,
+unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
This looks good.
However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.
I know but it is not documented anywhere (as far as I know) so I'd
rather not rely on the implementation detail that might change in the
future. This is not a hot path so extra assignment should not hurt.
It is actually: http://lxr.free-electrons.com/source/drivers/of/base.c#L1231
OK, fair enough. But not for ACPI properties (and I think we should
convert the parser to device_property_read_xxx() so it is usable
everywhere).
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
...
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b5207c78b..7e98c2e443ab 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,10 +14,10 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
+static int of_get_optional_u32(struct device_node *np,
const char *property)
{
- u32 val = 0;
+ int val = -1;
of_property_read_u32(np, property, val);
@@ -42,8 +42,12 @@ static void touchscreen_set_params(struct input_dev *dev,
}
absinfo = dev-absinfo[axis];
- absinfo-maximum = max;
- absinfo-fuzz = fuzz;
+
+ if (max = 0)
+ absinfo-maximum = max;
+
+ if (fuzz = 0)
+ absinfo-fuzz = fuzz;
}
/**
@@ -57,7 +61,7 @@ static void touchscreen_set_params(struct input_dev *dev,
void touchscreen_parse_of_params(struct input_dev *dev)
{
struct device_node *np = dev-dev.parent-of_node;
- u32 maximum, fuzz;
+ int maximum, fuzz;
input_alloc_absinfo(dev);
if (!dev-absinfo)
-- 8 --
That reduces the max size of the screens, but I don't really expect the screen
size to reach that order of magnitude before a few years...
Umm. Won't you have to update
- if (maximum || fuzz)
+ if (maximum = 0 || fuzz = 0)
? Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon, Jun 01, 2015 at 04:06:06PM +0200, Pavel Machek wrote:
Hi!
But that's not what I'm asking. See a changelog of
3eea8b5d68c801fec788b411582b803463834752 and compare it with what it
actually does.
It is buggy. If fuzz is specified but maximum is not, it overwites
maximum with zero.
If maximum is not set, you'll have other issues anyway. But it really
boils down on what the default behaviour should be.
It was not broken before commit
3eea8b5d68c801fec788b411582b803463834752. Maximum was set, but after
your patch, it is overwritten with zero.
Plus it introduces new failure if (!test_bit(axis, dev-absbit)).
It's not a new failure, it's testing against stupid code.
Yes. In a commit marked cleanup. We call this undocumented
feature.
It has never been said that it was a cleanup (whatever the double
quotes mean) but a rework. And it's not a feature either.
If an axis is setup in the DT but not registered in the driver,
something is wrong, most probably the DT.
Yes, we have fixed the DT, so that bug you introduced will not happen
on n900 with updated device tree.
s/updated/fixed/
Plus it fails to distinguish between value not specified in the dt
and zero is specified in the dt.
Again, default behaviour.
Again, regression from 4.0 kernel, you are not willing to fix.
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
It's not really a very good way to move forward and start a productive
discussion.
I'm sorry if this cause you any issues, but reverting this patch will
break users (and this time at the kernel level only, the DT has
nothing to do with that) just like you are right now.
What about:
-- 8 --
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b5207c78b..7e98c2e443ab 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,10 +14,10 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
+static int of_get_optional_u32(struct device_node *np,
const char *property)
{
- u32 val = 0;
+ int val = -1;
of_property_read_u32(np, property, val);
@@ -42,8 +42,12 @@ static void touchscreen_set_params(struct input_dev *dev,
}
absinfo = dev-absinfo[axis];
- absinfo-maximum = max;
- absinfo-fuzz = fuzz;
+
+ if (max = 0)
+ absinfo-maximum = max;
+
+ if (fuzz = 0)
+ absinfo-fuzz = fuzz;
}
/**
@@ -57,7 +61,7 @@ static void touchscreen_set_params(struct input_dev *dev,
void touchscreen_parse_of_params(struct input_dev *dev)
{
struct device_node *np = dev-dev.parent-of_node;
- u32 maximum, fuzz;
+ int maximum, fuzz;
input_alloc_absinfo(dev);
if (!dev-absinfo)
-- 8 --
That reduces the max size of the screens, but I don't really expect the screen
size to reach that order of magnitude before a few years...
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.
Thanks.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 67 ++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
- const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+const char *property,
+unsigned int default_value,
+unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
}
static void touchscreen_set_params(struct input_dev *dev,
@@ -54,34 +62,45 @@ static void touchscreen_set_params(struct input_dev *dev,
* input device accordingly. The function keeps previously setuped default
* values if no value is specified via DT.
*/
-void touchscreen_parse_of_params(struct input_dev *dev)
+void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
{
struct device_node *np = dev-dev.parent-of_node;
- u32 maximum, fuzz;
+ unsigned int maximum, fuzz;
+ int axis;
+ bool present;
input_alloc_absinfo(dev);
if (!dev-absinfo)
return;
- maximum = of_get_optional_u32(np, touchscreen-size-x);
- fuzz = of_get_optional_u32(np, touchscreen-fuzz-x);
- if (maximum || fuzz) {
- touchscreen_set_params(dev, ABS_X, maximum, fuzz);
- touchscreen_set_params(dev, ABS_MT_POSITION_X, maximum, fuzz);
- }
+ axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
+ data_present = touchscreen_get_prop_u32(np, touchscreen-size-x,
+ input_abs_get_maximum(axis),
+ maximum) |
+ touchscreen_get_prop_u32(np, touchscreen-fuzz-x,
+ input_abs_get_fuzz(axis),
+ fuzz);
+ if (data_present)
+ touchscreen_set_params(dev, axis, maximum, fuzz);
- maximum = of_get_optional_u32(np, touchscreen-size-y);
- fuzz = of_get_optional_u32(np, touchscreen-fuzz-y);
- if (maximum || fuzz) {
- touchscreen_set_params(dev, ABS_Y, maximum, fuzz);
- touchscreen_set_params(dev, ABS_MT_POSITION_Y, maximum, fuzz);
- }
+ axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
+ data_present = touchscreen_get_prop_u32(np,
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon, Jun 01, 2015 at 10:27:40PM +0200, Pavel Machek wrote: On Mon 2015-06-01 10:47:30, Dmitry Torokhov wrote: On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote: The 3eea8b5d68c801fec788b411582b803463834752 is just bad. You were very welcome to review this patch at the time and/or suggest a fix that pleases everyone. You should be the one that should suggest fixes, as you broke it in the first place. But clearly you don't understand that. You actually never asked for a fix, and went head first calling this patch bad and asking for nothing but reverting it. Date: Fri, 29 May 2015 21:08:16 +0200 Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 ... Maxime, can you suggest a fix? How about we do something like below (it needs a small edt-ft5x06 fixup that I'll send separately). Not tested. + data_present = touchscreen_get_prop_u32(np, touchscreen-size-x, + input_abs_get_maximum(axis), + maximum) | + touchscreen_get_prop_u32(np, touchscreen-fuzz-x, + input_abs_get_fuzz(axis), + fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); Umm. So you are changing behaviour from whatever was there to input_abs_get_maximum... in n900 case.o That _is_ whatever was there. Is that a good idea for a regression fix this late in release cycle? As Maxime mentioned the new behavior is needed for other touchscreens. Given that fixed DTS is going into 4.1 (as far as I understand) we do not need to rush this change into 4.1. Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon 2015-06-01 10:47:30, Dmitry Torokhov wrote: On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote: The 3eea8b5d68c801fec788b411582b803463834752 is just bad. You were very welcome to review this patch at the time and/or suggest a fix that pleases everyone. You should be the one that should suggest fixes, as you broke it in the first place. But clearly you don't understand that. You actually never asked for a fix, and went head first calling this patch bad and asking for nothing but reverting it. Date: Fri, 29 May 2015 21:08:16 +0200 Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 ... Maxime, can you suggest a fix? How about we do something like below (it needs a small edt-ft5x06 fixup that I'll send separately). Not tested. + data_present = touchscreen_get_prop_u32(np, touchscreen-size-x, + input_abs_get_maximum(axis), + maximum) | + touchscreen_get_prop_u32(np, touchscreen-fuzz-x, + input_abs_get_fuzz(axis), + fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); Umm. So you are changing behaviour from whatever was there to input_abs_get_maximum... in n900 case. Is that a good idea for a regression fix this late in release cycle? Maxime's patch should be easy to fix... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
* Dmitry Torokhov dmitry.torok...@gmail.com [150601 13:47]: On Mon, Jun 01, 2015 at 10:27:40PM +0200, Pavel Machek wrote: On Mon 2015-06-01 10:47:30, Dmitry Torokhov wrote: On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote: The 3eea8b5d68c801fec788b411582b803463834752 is just bad. You were very welcome to review this patch at the time and/or suggest a fix that pleases everyone. You should be the one that should suggest fixes, as you broke it in the first place. But clearly you don't understand that. You actually never asked for a fix, and went head first calling this patch bad and asking for nothing but reverting it. Date: Fri, 29 May 2015 21:08:16 +0200 Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 ... Maxime, can you suggest a fix? How about we do something like below (it needs a small edt-ft5x06 fixup that I'll send separately). Not tested. + data_present = touchscreen_get_prop_u32(np, touchscreen-size-x, + input_abs_get_maximum(axis), + maximum) | + touchscreen_get_prop_u32(np, touchscreen-fuzz-x, + input_abs_get_fuzz(axis), + fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); Umm. So you are changing behaviour from whatever was there to input_abs_get_maximum... in n900 case.o That _is_ whatever was there. Is that a good idea for a regression fix this late in release cycle? As Maxime mentioned the new behavior is needed for other touchscreens. Given that fixed DTS is going into 4.1 (as far as I understand) we do not need to rush this change into 4.1. OK great. I'm planning to send a pull request to arm-soc for Pavel's DTS fix today along with few other fixes. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
Hi Dmitry,
On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or
suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.
Thanks.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 67
++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client
*client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
- const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+const char *property,
+unsigned int default_value,
+unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
This looks good.
However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.
I know but it is not documented anywhere (as far as I know) so I'd
rather not rely on the implementation detail that might change in the
future. This is not a hot path so extra assignment should not hurt.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
Hi Dmitry,
On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
You were very welcome to review this patch at the time and/or suggest
a fix that pleases everyone.
You should be the one that should suggest fixes, as you broke it in
the first place. But clearly you don't understand that.
You actually never asked for a fix, and went head first calling this
patch bad and asking for nothing but reverting it.
Date: Fri, 29 May 2015 21:08:16 +0200
Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
linux-n900
...
Maxime, can you suggest a fix?
How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.
Thanks.
--
Dmitry
Input: improve parsing OF parameters for touchscreens
From: Dmitry Torokhov dmitry.torok...@gmail.com
When applying touchscreen parameters specified in device tree let's make
sure we keep whatever setup was done by the driver and not reset the
missing values to zero.
Reported-by: Pavel Machek pa...@ucw.cz
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/touchscreen/edt-ft5x06.c |2 -
drivers/input/touchscreen/of_touchscreen.c | 67
++--
drivers/input/touchscreen/tsc2005.c|2 -
include/linux/input/touchscreen.h |5 +-
4 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client
*client,
0, tsdata-num_y * 64 - 1, 0, 0);
if (!pdata)
- touchscreen_parse_of_params(input);
+ touchscreen_parse_of_params(input, true);
error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c
b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@
#include linux/input/mt.h
#include linux/input/touchscreen.h
-static u32 of_get_optional_u32(struct device_node *np,
-const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+ const char *property,
+ unsigned int default_value,
+ unsigned int *value)
{
u32 val = 0;
+ int error;
- of_property_read_u32(np, property, val);
+ error = of_property_read_u32(np, property, val);
+ if (error) {
+ *value = default_value;
+ return false;
+ }
- return val;
+ *value = val;
+ return true;
This looks good.
However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.
So I guess you could just do:
*value = default_value;
return of_property_read_u32(np, property, value) ? true : false;
It looks good otherwise.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
