4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Hi!

Quoted 3 lines

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Ok, I did some more testing, and found out rather bogus values in evtest:

Input device name: "TSC2005 touchscreen"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 2514

 Min        0
 Max        0
 Fuzz       4

Which made me go through the git logs, and these patches looked suspicious. After a revert... yes, touchscreen works as well as it worked before.

0a363a380954e10fece7cd9931b66056eeb07d56
3eea8b5d68c801fec788b411582b803463834752

(It is impossible to revert just 3eea..)

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -0

index 4b641c7..09089a6 100644

Hi,

Quoted 15 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -2

index 4b641c7..09089a6 100644
@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 2 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -0

+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Quoted 21 lines · Pavel Machek

Hi!

Quoted 3 lines

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Ok, I did some more testing, and found out rather bogus values in evtest:

Input device name: "TSC2005 touchscreen"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 2514

 Min        0
 Max        0
 Fuzz       4

Quoted 8 lines

Which made me go through the git logs, and these patches looked suspicious. After a revert... yes, touchscreen works as well as it worked before.

0a363a380954e10fece7cd9931b66056eeb07d56
3eea8b5d68c801fec788b411582b803463834752

(It is impossible to revert just 3eea..)

Hmm, I see:

                touchscreen-max-x = <4096>;
                touchscreen-max-y = <4096>;

...that's n900 dts.. this should be size-x/size-y... so we have a bug in dts.

But the 3eea8b5d68c801fec788b411582b803463834752 is buggy, it should not overwrite ->maximum for axis it has no devicetree data for.

Maybe replacing

+ if (maximum || fuzz)

in 3eea to (maximum && fuzz)... would help, but it is late in the cycle now, so I'd suggest just reverting 3eea8b.

Hi,

Quoted 10 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -0

index 4b641c7..09089a6 100644

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -2

@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;
+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

$ git describe a38cfebb56898633687ab337fd53710e63a0aedd
v3.15-rc5-72-ga38cfebb5689

So, because this has been wrongly documented for so long, we should support both bindings. Sure, deprecate touchscreen-max-[xy], but they must still be supported, IMO.

Quoted 18 lines · Felipe Balbi

Hi,

Quoted 15 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -2

index 4b641c7..09089a6 100644
@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;

Quoted 8 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 2 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -0

+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Agreed. In parent email, I have list of two commits that should be reverted.

Quoted 26 lines · Felipe Balbi

Hi,

Quoted 10 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -0

index 4b641c7..09089a6 100644

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -2

@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;
+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Quoted 6 lines

$ git describe a38cfebb56898633687ab337fd53710e63a0aedd
v3.15-rc5-72-ga38cfebb5689

So, because this has been wrongly documented for so long, we should support both bindings. Sure, deprecate touchscreen-max-[xy], but they must still be supported, IMO.

This property has never been anything but a typo in a documentation of a single driver.

Feel free to fix that in that driver, but I don't see why the core code should handle that isolated typo.

Quoted 28 lines · Pavel Machek

Quoted 26 lines · Felipe Balbi

Hi,

Quoted 10 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -0

index 4b641c7..09089a6 100644

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -2

@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;
+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Quoted 2 lines

Agreed. In parent email, I have list of two commits that should be reverted.

So, if we sums things up. You introduce in some documentation example some property, that you never document, that you still use in one

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

Quoted 18 lines · Felipe Balbi

Hi,

Quoted 15 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -2

index 4b641c7..09089a6 100644
@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;

Quoted 8 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 2 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -0

+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Agreed. In parent email, I have list of two commits that should be reverted.

Quoted 29 lines · Maxime Ripard

Quoted 28 lines · Pavel Machek

Quoted 26 lines · Felipe Balbi

Hi,

Quoted 10 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -0

index 4b641c7..09089a6 100644

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -2

@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;
+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Quoted 5 lines

Quoted 2 lines

Agreed. In parent email, I have list of two commits that should be reverted.

So, if we sums things up. You introduce in some documentation example some property, that you never document, that you still use in one

it was Documented in DT bindings document for this particular driver. What are you talking about ?

Quoted 3 lines

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

not Pali, Sebastian.

Quoted 2 lines

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

We shouldn't revert, that I agree. But both properties should be parsed.

Quoted 33 lines · Pavel Machek

Quoted 21 lines · Pavel Machek

Hi!

Quoted 3 lines

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Ok, I did some more testing, and found out rather bogus values in evtest:

Input device name: "TSC2005 touchscreen"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 2514

 Min        0
 Max        0
 Fuzz       4

Quoted 8 lines

Which made me go through the git logs, and these patches looked suspicious. After a revert... yes, touchscreen works as well as it worked before.

0a363a380954e10fece7cd9931b66056eeb07d56
3eea8b5d68c801fec788b411582b803463834752

(It is impossible to revert just 3eea..)

Hmm, I see:

Quoted 7 lines

                touchscreen-max-x = <4096>;
                touchscreen-max-y = <4096>;

...that's n900 dts.. this should be size-x/size-y... so we have a bug in dts.

But the 3eea8b5d68c801fec788b411582b803463834752 is buggy, it should not overwrite ->maximum for axis it has no devicetree data for.

What do you mean? touchscreen-max-* _is_ device tree data for an axis.

Quoted 2 lines

Maybe replacing

Quoted 4 lines

+ if (maximum || fuzz)

in 3eea to (maximum && fuzz)... would help, but it is late in the cycle now, so I'd suggest just reverting 3eea8b.

No, both maximum and fuzz are optional. You can perfectly have one without another.

Quoted 5 lines

Quoted 4 lines

So, because this has been wrongly documented for so long, we should support both bindings. Sure, deprecate touchscreen-max-[xy], but they must still be supported, IMO.

Quoted 5 lines

This property has never been anything but a typo in a documentation of a single driver.

Feel free to fix that in that driver, but I don't see why the core code should handle that isolated typo.

OK thanks for confirming. Will apply this into omap-for-v4.1/fixes.

Quoted 5 lines

Quoted 2 lines

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

not Pali, Sebastian.

Quoted 4 lines

Quoted 2 lines

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

We shouldn't revert, that I agree. But both properties should be parsed.

No. If the property is wrong, and nobody parsed it, I do not see any reason to start now.

Hi!

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

not Pali, Sebastian.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

We shouldn't revert, that I agree. But both properties should be parsed.

No. If the property is wrong, and nobody parsed it, I do not see any reason to start now.

Agreed.

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.

Plus it introduces new failure "if (!test_bit(axis, dev->absbit))".

Plus it fails to distinguish between "value not specified in the dt" and "zero is specified in the dt".

Quoted 15 lines · Pavel Machek

Hi!

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

not Pali, Sebastian.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

We shouldn't revert, that I agree. But both properties should be parsed.

No. If the property is wrong, and nobody parsed it, I do not see any reason to start now.

Quoted 8 lines

Agreed.

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.

Yes.

Quoted 2 lines

Plus it introduces new failure "if (!test_bit(axis, dev->absbit))".

That is not a new failure. It actually warns users that they trying to specify in DT something that will be ignored by the kernel (because without that absbit kernel will ignore all requests to that event code).

Quoted 2 lines

Plus it fails to distinguish between "value not specified in the dt" and "zero is specified in the dt".

Yes. I am not sure if we should care and support all permutations (ah, I pre-setup fuzz in the driver, but override max on X, and I pre-setup max on X, but take fuzz from DT). Maybe we should simply document that specifying one parameter for an axis will change the rest of them to be
0. Not sure though...

Quoted 33 lines · Pavel Machek

Quoted 21 lines · Pavel Machek

Hi!

Quoted 3 lines

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Ok, I did some more testing, and found out rather bogus values in evtest:

Input device name: "TSC2005 touchscreen"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 2514

 Min        0
 Max        0
 Fuzz       4

Quoted 8 lines

Which made me go through the git logs, and these patches looked suspicious. After a revert... yes, touchscreen works as well as it worked before.

0a363a380954e10fece7cd9931b66056eeb07d56
3eea8b5d68c801fec788b411582b803463834752

(It is impossible to revert just 3eea..)

Hmm, I see:

Quoted 7 lines

                touchscreen-max-x = <4096>;
                touchscreen-max-y = <4096>;

...that's n900 dts.. this should be size-x/size-y... so we have a bug in dts.

But the 3eea8b5d68c801fec788b411582b803463834752 is buggy, it should not overwrite ->maximum for axis it has no devicetree data for.

What do you mean? touchscreen-max-* _is_ device tree data for an axis.

Quoted 2 lines

Maybe replacing

Quoted 4 lines

+ if (maximum || fuzz)

in 3eea to (maximum && fuzz)... would help, but it is late in the cycle now, so I'd suggest just reverting 3eea8b.

No, both maximum and fuzz are optional. You can perfectly have one without another.

Quoted 46 lines · Maxime Ripard

Quoted 33 lines · Pavel Machek

Quoted 21 lines · Pavel Machek

Hi!

Quoted 3 lines

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Ok, I did some more testing, and found out rather bogus values in evtest:

Input device name: "TSC2005 touchscreen"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 2514

 Min        0
 Max        0
 Fuzz       4

Quoted 8 lines

Which made me go through the git logs, and these patches looked suspicious. After a revert... yes, touchscreen works as well as it worked before.

0a363a380954e10fece7cd9931b66056eeb07d56
3eea8b5d68c801fec788b411582b803463834752

(It is impossible to revert just 3eea..)

Hmm, I see:

Quoted 7 lines

                touchscreen-max-x = <4096>;
                touchscreen-max-y = <4096>;

...that's n900 dts.. this should be size-x/size-y... so we have a bug in dts.

But the 3eea8b5d68c801fec788b411582b803463834752 is buggy, it should not overwrite ->maximum for axis it has no devicetree data for.

What do you mean? touchscreen-max-* _is_ device tree data for an axis.

Quoted 2 lines

Maybe replacing

Quoted 7 lines

Quoted 4 lines

+ if (maximum || fuzz)

in 3eea to (maximum && fuzz)... would help, but it is late in the cycle now, so I'd suggest just reverting 3eea8b.

No, both maximum and fuzz are optional. You can perfectly have one without another.

Yes, which is something you got wrong.

Apply this on top of 3eea8b5d68c801fec788b411582b803463834752, and you'll get simpler code, that works like it did before.

Maxime, since you caused the regression, can I ask you to put the other patches on top of it, test it still works for you and submit it properly?

[If you want to return in !test_bit case, and are sure it does not break anything, feel free to do that.]

Thanks,
Pavel

Signed-off-by Pavel Machek <[email protected]>

drivers/input/touchscreen/of_touchscreen.c +2 -12

index b82b520..e626c8a 100644
@@ -11,39 +11,23 @@
 
 #include <linux/of.h>
 #include <linux/input.h>
-#include <linux/input/mt.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;
-}
-
 static void touchscreen_set_params(struct input_dev *dev,
 				   unsigned long axis,
-				   int max, int fuzz)
+				   char *max, char *fuzz)
 {
 	struct input_absinfo *absinfo;
+	struct device_node *np = dev->dev.parent->of_node;
 
 	if (!test_bit(axis, dev->absbit)) {

Quoted 28 lines · Dmitry Torokhov

Quoted 15 lines · Pavel Machek

Hi!

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

not Pali, Sebastian.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

We shouldn't revert, that I agree. But both properties should be parsed.

No. If the property is wrong, and nobody parsed it, I do not see any reason to start now.

Quoted 8 lines

Agreed.

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.

Yes.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Plus it introduces new failure "if (!test_bit(axis, dev->absbit))".

That is not a new failure. It actually warns users that they trying to specify in DT something that will be ignored by the kernel (because without that absbit kernel will ignore all requests to that event code).

What if driver sets the bits after parsing device tree?

Quoted 101 lines · Pavel Machek

Quoted 46 lines · Maxime Ripard

Quoted 33 lines · Pavel Machek

Quoted 21 lines · Pavel Machek

Hi!

Quoted 3 lines

mh I remember having problems with tsc2005 before. It helped to reset the controller (should actually happen automatically when it hangs, but I'm not sure, that it actually works).

Ok, I did some more testing, and found out rather bogus values in evtest:

Input device name: "TSC2005 touchscreen"
Supported events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 330 (BTN_TOUCH)
Event type 3 (EV_ABS)
Event code 0 (ABS_X)
Value 2514

 Min        0
 Max        0
 Fuzz       4

Quoted 8 lines

Which made me go through the git logs, and these patches looked suspicious. After a revert... yes, touchscreen works as well as it worked before.

0a363a380954e10fece7cd9931b66056eeb07d56
3eea8b5d68c801fec788b411582b803463834752

(It is impossible to revert just 3eea..)

Hmm, I see:

Quoted 7 lines

                touchscreen-max-x = <4096>;
                touchscreen-max-y = <4096>;

...that's n900 dts.. this should be size-x/size-y... so we have a bug in dts.

But the 3eea8b5d68c801fec788b411582b803463834752 is buggy, it should not overwrite ->maximum for axis it has no devicetree data for.

What do you mean? touchscreen-max-* _is_ device tree data for an axis.

Quoted 2 lines

Maybe replacing

Quoted 7 lines

Quoted 4 lines

+ if (maximum || fuzz)

in 3eea to (maximum && fuzz)... would help, but it is late in the cycle now, so I'd suggest just reverting 3eea8b.

No, both maximum and fuzz are optional. You can perfectly have one without another.

Yes, which is something you got wrong.

Apply this on top of 3eea8b5d68c801fec788b411582b803463834752, and you'll get simpler code, that works like it did before.

Maxime, since you caused the regression, can I ask you to put the other patches on top of it, test it still works for you and submit it properly?

[If you want to return in !test_bit case, and are sure it does not break anything, feel free to do that.]

Thanks,
Pavel

Signed-off-by Pavel Machek <[email protected]>

drivers/input/touchscreen/of_touchscreen.c +2 -12

index b82b520..e626c8a 100644
@@ -11,39 +11,23 @@
 
 #include <linux/of.h>
 #include <linux/input.h>
-#include <linux/input/mt.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;
-}
-
 static void touchscreen_set_params(struct input_dev *dev,
 				   unsigned long axis,
-				   int max, int fuzz)
+				   char *max, char *fuzz)
 {
 	struct input_absinfo *absinfo;
+	struct device_node *np = dev->dev.parent->of_node;
 

drivers/input/touchscreen/of_touchscreen.c line 25

Quoted 11 lines

drivers/input/touchscreen/of_touchscreen.c +2 -8

 	if (!test_bit(axis, dev->absbit)) {
-		/*
-		 * Emit a warning only if the axis is not a multitouch
-		 * axis, which might not be set by the driver.
-		 */
-		if (!input_is_mt_axis(axis))
-			dev_warn(&dev->dev,
-				 "DT specifies parameters but the axis is not set up\n");
-		return;
+		dev_warn(&dev->dev,
+			 "DT specifies parameters but the axis is not set up\n");

No, this is not right either. We do not want to bitch about axis not being set up if there is nothing in DT mentioning this axis.

Plus the code structure won't work well if we decide to add more to it (such as min, or flat, or resolution).

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -2

index 4b641c7..09089a6 100644
@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;
+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;
 		touchscreen-max-pressure = <2048>;
 
 		ti,x-plate-ohms = <280>;

arch/arm/boot/dts/omap3-n900.dts +0 -0

index 5c16145..5f5e0f3 100644

Commit 3eea8b5d68c8 introduced a dependency between touchscreen-max-* and touchscreen-fuzz-*, so that either both must be specified or none of them. If only one of them is specified the other value will be reset to 0. This commit restores the previous behaviour, that the drivers default value will be used for the unspecified value.

Reported-By Pavel Machek <[email protected]>

Fixes 3eea8b5d68c8 (Input: of_touchscreen - rework the DT parsing function)

Signed-off-by Sebastian Reichel <[email protected]>

---
 drivers/input/touchscreen/of_touchscreen.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

drivers/input/touchscreen/of_touchscreen.c +0 -0

index b82b520..7d2cf59 100644

Quoted 26 lines · Felipe Balbi

Hi,

Quoted 10 lines · Pavel Machek

Fix dts to match what the Linux kernel expects. This works around touchscreen problems in 4.1 linux on Nokia n900.

Signed-off-by Pavel Machek <[email protected]>

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +0 -0

index 4b641c7..09089a6 100644

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt Example: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +2 -2

@@ -32,8 +32,8 @@ Example:
 		touchscreen-fuzz-x = <4>;
 		touchscreen-fuzz-y = <7>;
 		touchscreen-fuzz-pressure = <2>;
-		touchscreen-max-x = <4096>;
-		touchscreen-max-y = <4096>;
+		touchscreen-size-x = <4096>;
+		touchscreen-size-y = <4096>;

IMHO, the older binding needs to be supported as well. It's fine to update the DTS for the new binding, but even Documentation says touchscreen-max-[xy] and if the driver changed that, the driver should be fixed too. Besides, it seems like this has been in tree since v3.16:

Quoted 6 lines

$ git describe a38cfebb56898633687ab337fd53710e63a0aedd
v3.15-rc5-72-ga38cfebb5689

So, because this has been wrongly documented for so long, we should support both bindings. Sure, deprecate touchscreen-max-[xy], but they must still be supported, IMO.

This property has never been anything but a typo in a documentation of a single driver.

Feel free to fix that in that driver, but I don't see why the core code should handle that isolated typo.

Well... the driver was not broken... before you did "cleanup" that did two functional changes. And yes, the dts should be fixed, but that does not make your "cleanup" good.

Quoted 3 lines · Pavel Machek

Well... the driver was not broken... before you did "cleanup" that did two functional changes. And yes, the dts should be fixed, but that does not make your "cleanup" good.

Whether it's good or not is arguable, and it really boils down to what we consider the default value when properties are missing.

If it's 0, then my code does the right thing. If it's undefined, well, I'd expect undefined behaviour to change without any notice.

Hi!

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

single DT, you don't even use that property in your driver, and now that you realise you meant something else, you want the code that

not Pali, Sebastian.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

actually parse the *right* property and does the right thing, that all other DT agree (and depend on) to be reverted?

We shouldn't revert, that I agree. But both properties should be parsed.

No. If the property is wrong, and nobody parsed it, I do not see any reason to start now.

Agreed.

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.

Plus it introduces new failure "if (!test_bit(axis, dev->absbit))".

Plus it fails to distinguish between "value not specified in the dt" and "zero is specified in the dt".

The 3eea8b5d68c801fec788b411582b803463834752 is just bad.

Quoted 6 lines

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.

Quoted 1 line

Plus it introduces new failure "if (!test_bit(axis, dev->absbit))".

It's not a new failure, it's testing against stupid code.

If an axis is setup in the DT but not registered in the driver, something is wrong, most probably the DT.

Quoted 2 lines

Plus it fails to distinguish between "value not specified in the dt" and "zero is specified in the dt".

Again, default behaviour.

Quoted 1 line

The 3eea8b5d68c801fec788b411582b803463834752 is just bad.

You were very welcome to review this patch at the time and/or suggest a fix that pleases everyone.

Hi!

Quoted 9 lines

Quoted 6 lines

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.

Quoted 3 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

The 3eea8b5d68c801fec788b411582b803463834752 is just bad.

Quoted 5 lines

Quoted 2 lines

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.

drivers/input/touchscreen/of_touchscreen.c +9 -6

index b82b5207c78b..7e98c2e443ab 100644
@@ -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...

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 13 lines · Pavel Machek

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 14 lines · Dmitry Torokhov

Quoted 13 lines · Pavel Machek

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

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

(quoted diff) +2 -0

+                                               &maximum) |
+                      touchscreen_get_prop_u32(np,

"touchscreen-fuzz-x",
+
input_abs_get_fuzz(axis),

drivers/input/touchscreen/of_touchscreen.c +3 -0

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

Is that a good idea for a
regression fix this late in release cycle?

Quoted 37 lines · Pavel Machek

Quoted 14 lines · Dmitry Torokhov

Quoted 13 lines · Pavel Machek

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

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

(quoted diff) +2 -0

+                                               &maximum) |
+                      touchscreen_get_prop_u32(np,

"touchscreen-fuzz-x",
+
input_abs_get_fuzz(axis),

drivers/input/touchscreen/of_touchscreen.c +3 -0

+                                               &fuzz);
+       if (data_present)
+               touchscreen_set_params(dev, axis, maximum, fuzz);

Quoted 2 lines

Umm. So you are changing behaviour from "whatever was there" to "input_abs_get_maximum"... in n900 case.o

That _is_ "whatever was there".

Quoted 2 lines

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.

Quoted 10 lines · Pavel Machek

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Signature · 19 lines

Dmitry


Input: improve parsing OF parameters for touchscreens

From: Dmitry Torokhov <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
 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(-)

drivers/input/touchscreen/edt-ft5x06.c +1 -1

index 29d179a..394b1de 100644
@@ -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) {

drivers/input/touchscreen/of_touchscreen.c +5 -2

index b82b520..c132624 100644
@@ -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;
 

Hi Dmitry,

Quoted 84 lines · Dmitry Torokhov

Quoted 10 lines · Pavel Machek

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Signature · 19 lines

Dmitry


Input: improve parsing OF parameters for touchscreens

From: Dmitry Torokhov <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
 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(-)

drivers/input/touchscreen/edt-ft5x06.c +1 -1

index 29d179a..394b1de 100644
@@ -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) {

drivers/input/touchscreen/of_touchscreen.c +10 -3

index b82b520..c132624 100644
@@ -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;
+	}
 

drivers/input/touchscreen/of_touchscreen.c line 32

Quoted 3 lines

drivers/input/touchscreen/of_touchscreen.c +2 -1

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

Quoted 93 lines · Maxime Ripard

Hi Dmitry,

Quoted 77 lines · Dmitry Torokhov

Quoted 10 lines · Pavel Machek

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Signature · 19 lines

Dmitry


Input: improve parsing OF parameters for touchscreens

From: Dmitry Torokhov <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
 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(-)

drivers/input/touchscreen/edt-ft5x06.c +1 -1

index 29d179a..394b1de 100644
@@ -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) {

drivers/input/touchscreen/of_touchscreen.c +5 -2

index b82b520..c132624 100644
@@ -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;
 

drivers/input/touchscreen/of_touchscreen.c line 32

Quoted 10 lines

drivers/input/touchscreen/of_touchscreen.c +7 -2

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

Quoted 2 lines

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.

Quoted 93 lines · Maxime Ripard

Hi Dmitry,

Quoted 77 lines · Dmitry Torokhov

Quoted 10 lines · Pavel Machek

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Signature · 19 lines

Dmitry


Input: improve parsing OF parameters for touchscreens

From: Dmitry Torokhov <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
 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(-)

drivers/input/touchscreen/edt-ft5x06.c +1 -1

index 29d179a..394b1de 100644
@@ -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) {

drivers/input/touchscreen/of_touchscreen.c +5 -2

index b82b520..c132624 100644
@@ -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;
 

drivers/input/touchscreen/of_touchscreen.c line 32

Quoted 10 lines

drivers/input/touchscreen/of_touchscreen.c +7 -2

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

Quoted 2 lines

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.

Quoted 94 lines · Dmitry Torokhov

Quoted 93 lines · Maxime Ripard

Hi Dmitry,

Quoted 77 lines · Dmitry Torokhov

Quoted 10 lines · Pavel Machek

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines

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.

Signature · 19 lines

Dmitry


Input: improve parsing OF parameters for touchscreens

From: Dmitry Torokhov <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
 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(-)

drivers/input/touchscreen/edt-ft5x06.c +1 -1

index 29d179a..394b1de 100644
@@ -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) {

drivers/input/touchscreen/of_touchscreen.c +5 -2

index b82b520..c132624 100644
@@ -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;
 

drivers/input/touchscreen/of_touchscreen.c line 32

Quoted 10 lines

drivers/input/touchscreen/of_touchscreen.c +7 -2

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

Quoted 6 lines

Quoted 2 lines

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

Quoted 35 lines · Maxime Ripard

Quoted 33 lines · Dmitry Torokhov

Quoted 20 lines · Maxime Ripard

Hi Dmitry,

Quoted 17 lines · Dmitry Torokhov

(quoted diff) +10 -3

-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;
+	}

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

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

Quoted 1 line

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.

Input: improve parsing OF parameters for touchscreens

From: Dmitry Torokhov <[email protected]>

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 <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>