* 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] [not found] ` <20150527145837.GA13223@earth> @ 2015-05-29 19:08 ` Pavel Machek 2015-05-29 19:25 ` Pavel Machek 0 siblings, 1 reply; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:08 UTC (permalink / raw) To: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov Cc: Pali Rohár, Ivaylo Dimitrov Hi! > 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..) Its funny... changelog complains about duplicated code.. then inserts 30 lines of code. Along with check that was not there before. Maxime, can you suggest a fix? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] 2015-05-29 19:08 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Pavel Machek @ 2015-05-29 19:25 ` Pavel Machek 2015-05-29 19:32 ` Pavel Machek 2015-05-29 19:57 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Maxime Ripard 0 siblings, 2 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:25 UTC (permalink / raw) To: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov Cc: Pali Rohár, Ivaylo Dimitrov On Fri 2015-05-29 21:08:16, Pavel Machek wrote: > Hi! > > > 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..) 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. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:25 ` Pavel Machek 2015-05-29 19:32 ` Pavel Machek @ 2015-05-29 19:32 ` Pavel Machek 1 sibling, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:32 UTC (permalink / raw) To: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan Cc: Pali Rohár, Ivaylo Dimitrov 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 <pavel@ucw.cz> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt index 4b641c7..09089a6 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt @@ -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>; diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 5c16145..5f5e0f3 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -832,8 +832,8 @@ 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>; -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply related [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 19:32 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:32 UTC (permalink / raw) To: linux-arm-kernel 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 <pavel@ucw.cz> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt index 4b641c7..09089a6 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt @@ -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>; diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 5c16145..5f5e0f3 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -832,8 +832,8 @@ 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>; -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply related [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 19:32 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:32 UTC (permalink / raw) To: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, patrikbachan Cc: Pali Rohár, Ivaylo Dimitrov 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 <pavel@ucw.cz> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt index 4b641c7..09089a6 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt @@ -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>; diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 5c16145..5f5e0f3 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -832,8 +832,8 @@ 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>; -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:32 ` Pavel Machek (?) @ 2015-05-29 19:49 ` Felipe Balbi -1 siblings, 0 replies; 77+ messages in thread From: Felipe Balbi @ 2015-05-29 19:49 UTC (permalink / raw) To: Pavel Machek Cc: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 1476 bytes --] Hi, On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > 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 <pavel@ucw.cz> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > index 4b641c7..09089a6 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > @@ -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. In any case, for this patch: Reviewed-by: Felipe Balbi <balbi@ti.com> -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 19:49 ` Felipe Balbi 0 siblings, 0 replies; 77+ messages in thread From: Felipe Balbi @ 2015-05-29 19:49 UTC (permalink / raw) To: linux-arm-kernel Hi, On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > 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 <pavel@ucw.cz> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > index 4b641c7..09089a6 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > @@ -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. In any case, for this patch: Reviewed-by: Felipe Balbi <balbi@ti.com> -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150529/f83a90c8/attachment-0001.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 19:49 ` Felipe Balbi 0 siblings, 0 replies; 77+ messages in thread From: Felipe Balbi @ 2015-05-29 19:49 UTC (permalink / raw) To: Pavel Machek Cc: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 1476 bytes --] Hi, On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > 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 <pavel@ucw.cz> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > index 4b641c7..09089a6 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > @@ -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. In any case, for this patch: Reviewed-by: Felipe Balbi <balbi@ti.com> -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:49 ` Felipe Balbi @ 2015-05-29 19:56 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:56 UTC (permalink / raw) To: Felipe Balbi Cc: Sebastian Reichel, kernel list, maxime.ripard, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > Hi, > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > 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 <pavel@ucw.cz> > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > index 4b641c7..09089a6 100644 > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > @@ -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: Agreed. In parent email, I have list of two commits that should be reverted. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 19:56 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 19:56 UTC (permalink / raw) To: linux-arm-kernel On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > Hi, > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > 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 <pavel@ucw.cz> > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > index 4b641c7..09089a6 100644 > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > @@ -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: Agreed. In parent email, I have list of two commits that should be reverted. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:56 ` Pavel Machek @ 2015-05-29 20:17 ` Maxime Ripard -1 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-05-29 20:17 UTC (permalink / raw) To: Pavel Machek Cc: Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 2086 bytes --] On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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: > > 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? This is a joke, right? If not, I can show you a handful of DTs that will be equally broken if you revert the commits. Who wins? Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:17 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-05-29 20:17 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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: > > 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? This is a joke, right? If not, I can show you a handful of DTs that will be equally broken if you revert the commits. Who wins? Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150529/8c07edc5/attachment.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:17 ` Maxime Ripard (?) @ 2015-05-29 20:21 ` Felipe Balbi -1 siblings, 0 replies; 77+ messages in thread From: Felipe Balbi @ 2015-05-29 20:21 UTC (permalink / raw) To: Maxime Ripard Cc: Pavel Machek, Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 2189 bytes --] On Fri, May 29, 2015 at 10:17:45PM +0200, Maxime Ripard wrote: > On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > > Hi, > > > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > > 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 <pavel@ucw.cz> > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > index 4b641c7..09089a6 100644 > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > @@ -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: > > > > 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 ? > 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. > 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. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:21 ` Felipe Balbi 0 siblings, 0 replies; 77+ messages in thread From: Felipe Balbi @ 2015-05-29 20:21 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 10:17:45PM +0200, Maxime Ripard wrote: > On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > > Hi, > > > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > > 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 <pavel@ucw.cz> > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > index 4b641c7..09089a6 100644 > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > @@ -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: > > > > 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 ? > 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. > 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. -- balbi -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150529/0fb63681/attachment.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:21 ` Felipe Balbi 0 siblings, 0 replies; 77+ messages in thread From: Felipe Balbi @ 2015-05-29 20:21 UTC (permalink / raw) To: Maxime Ripard Cc: Pavel Machek, Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 2189 bytes --] On Fri, May 29, 2015 at 10:17:45PM +0200, Maxime Ripard wrote: > On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > > Hi, > > > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > > 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 <pavel@ucw.cz> > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > index 4b641c7..09089a6 100644 > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > @@ -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: > > > > 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 ? > 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. > 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. -- balbi [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:21 ` Felipe Balbi @ 2015-05-29 20:29 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 20:29 UTC (permalink / raw) To: Felipe Balbi Cc: Maxime Ripard, Pavel Machek, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri, May 29, 2015 at 03:21:23PM -0500, Felipe Balbi wrote: > On Fri, May 29, 2015 at 10:17:45PM +0200, Maxime Ripard wrote: > > On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > > > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > > > Hi, > > > > > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > > > 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 <pavel@ucw.cz> > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > > index 4b641c7..09089a6 100644 > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > > @@ -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: > > > > > > 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 ? It was documented in "example", not in the documentation that says: - properties defined in touchscreen.txt which says nothing about touchscreen-max-x. And _noone_ has ever parsed this property, so adding support for it does not make any sense. > > > 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. > > > 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. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:29 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 20:29 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 03:21:23PM -0500, Felipe Balbi wrote: > On Fri, May 29, 2015 at 10:17:45PM +0200, Maxime Ripard wrote: > > On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > > > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > > > Hi, > > > > > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > > > 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 <pavel@ucw.cz> > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > > index 4b641c7..09089a6 100644 > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > > > @@ -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: > > > > > > 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 ? It was documented in "example", not in the documentation that says: - properties defined in touchscreen.txt which says nothing about touchscreen-max-x. And _noone_ has ever parsed this property, so adding support for it does not make any sense. > > > 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. > > > 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. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:29 ` Dmitry Torokhov @ 2015-05-29 20:34 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 20:34 UTC (permalink / raw) To: Dmitry Torokhov Cc: Felipe Balbi, Maxime Ripard, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan Hi! > > > 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. > > > > > 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. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:34 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 20:34 UTC (permalink / raw) To: linux-arm-kernel Hi! > > > 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. > > > > > 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. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:34 ` Pavel Machek @ 2015-05-29 20:48 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 20:48 UTC (permalink / raw) To: Pavel Machek Cc: Felipe Balbi, Maxime Ripard, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > Hi! > > > > > 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. > > > > > > > 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. Yes. > > 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). > > 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... Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:48 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 20:48 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > Hi! > > > > > 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. > > > > > > > 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. Yes. > > 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). > > 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... Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:48 ` Dmitry Torokhov @ 2015-05-29 21:02 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 21:02 UTC (permalink / raw) To: Dmitry Torokhov Cc: Felipe Balbi, Maxime Ripard, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri 2015-05-29 13:48:47, Dmitry Torokhov wrote: > On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > > Hi! > > > > > > > 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. > > > > > > > > > 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. > > Yes. > > > > > 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? > > 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... Well, the old code did support all permutations. "cleanups" should not change such details... -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 21:02 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 21:02 UTC (permalink / raw) To: linux-arm-kernel On Fri 2015-05-29 13:48:47, Dmitry Torokhov wrote: > On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > > Hi! > > > > > > > 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. > > > > > > > > > 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. > > Yes. > > > > > 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? > > 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... Well, the old code did support all permutations. "cleanups" should not change such details... -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 21:02 ` Pavel Machek @ 2015-05-29 21:38 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 21:38 UTC (permalink / raw) To: Pavel Machek Cc: Felipe Balbi, Maxime Ripard, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri, May 29, 2015 at 11:02:59PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 13:48:47, Dmitry Torokhov wrote: > > On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > > > Hi! > > > > > > > > > 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. > > > > > > > > > > > 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. > > > > Yes. > > > > > > > > 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? It should not. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 21:38 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 21:38 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 11:02:59PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 13:48:47, Dmitry Torokhov wrote: > > On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > > > Hi! > > > > > > > > > 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. > > > > > > > > > > > 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. > > > > Yes. > > > > > > > > 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? It should not. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:34 ` Pavel Machek @ 2015-06-01 9:55 ` Maxime Ripard -1 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 9:55 UTC (permalink / raw) To: Pavel Machek Cc: Dmitry Torokhov, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 1659 bytes --] On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > Hi! > > > > > 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. > > > > > > > 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. If maximum is not set, you'll have other issues anyway. But it really boils down on what the default behaviour should be. > 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. > Plus it fails to distinguish between "value not specified in the dt" > and "zero is specified in the dt". Again, default behaviour. > The 3eea8b5d68c801fec788b411582b803463834752 is just bad. You were very welcome to review this patch at the time and/or suggest a fix that pleases everyone. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-06-01 9:55 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 9:55 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 10:34:56PM +0200, Pavel Machek wrote: > Hi! > > > > > 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. > > > > > > > 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. If maximum is not set, you'll have other issues anyway. But it really boils down on what the default behaviour should be. > 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. > Plus it fails to distinguish between "value not specified in the dt" > and "zero is specified in the dt". Again, default behaviour. > The 3eea8b5d68c801fec788b411582b803463834752 is just bad. You were very welcome to review this patch at the time and/or suggest a fix that pleases everyone. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150601/a21a0745/attachment-0001.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 9:55 ` Maxime Ripard (?) @ 2015-06-01 14:06 ` Pavel Machek 2015-06-01 14:58 ` Maxime Ripard -1 siblings, 1 reply; 77+ messages in thread From: Pavel Machek @ 2015-06-01 14:06 UTC (permalink / raw) To: Maxime Ripard, Linus Torvalds Cc: Dmitry Torokhov, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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". > 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. > > 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. Dmitry, please revert 3eea8b5d68c801fec788b411582b803463834752 . You'll probably need to revert 0a363a380954e10fece7cd9931b66056eeb07d56 too. Then, Maxime can submit his multitouch patches in a way it does not break existing setups. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 14:06 ` Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen " Pavel Machek @ 2015-06-01 14:58 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 14:58 UTC (permalink / raw) To: Pavel Machek Cc: Linus Torvalds, Dmitry Torokhov, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 3639 bytes --] 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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 14:58 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 14:58 UTC (permalink / raw) To: linux-arm-kernel 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150601/f3f3e427/attachment.sig> ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 14:58 ` Maxime Ripard @ 2015-06-01 15:21 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-01 15:21 UTC (permalink / raw) To: Maxime Ripard Cc: Linus Torvalds, Dmitry Torokhov, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan > > > > 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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 15:21 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-01 15:21 UTC (permalink / raw) To: linux-arm-kernel > > > > 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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 15:21 ` Pavel Machek @ 2015-06-01 17:47 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-01 17:47 UTC (permalink / raw) To: Pavel Machek Cc: Maxime Ripard, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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.torokhov@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 <pavel@ucw.cz> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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, "touchscreen-size-y", + input_abs_get_maximum(axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-y", + input_abs_get_fuzz(axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); - maximum = of_get_optional_u32(np, "touchscreen-max-pressure"); - fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); - } + axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE; + data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure", + input_abs_get_maximum(axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-pressure", + input_abs_get_fuzz(axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); } EXPORT_SYMBOL(touchscreen_parse_of_params); diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c index 72657c5..d8c025b 100644 --- a/drivers/input/touchscreen/tsc2005.c +++ b/drivers/input/touchscreen/tsc2005.c @@ -709,7 +709,7 @@ static int tsc2005_probe(struct spi_device *spi) input_set_abs_params(input_dev, ABS_PRESSURE, 0, max_p, fudge_p, 0); if (np) - touchscreen_parse_of_params(input_dev); + touchscreen_parse_of_params(input_dev, false); input_dev->open = tsc2005_open; input_dev->close = tsc2005_close; diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h index 08a5ef6..eecc9ea 100644 --- a/include/linux/input/touchscreen.h +++ b/include/linux/input/touchscreen.h @@ -12,9 +12,10 @@ #include <linux/input.h> #ifdef CONFIG_OF -void touchscreen_parse_of_params(struct input_dev *dev); +void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch); #else -static inline void touchscreen_parse_of_params(struct input_dev *dev) +static inline void touchscreen_parse_of_params(struct input_dev *dev, + bool multitouch) { } #endif ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 17:47 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-01 17:47 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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, "touchscreen-size-y", + input_abs_get_maximum(axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-y", + input_abs_get_fuzz(axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); - maximum = of_get_optional_u32(np, "touchscreen-max-pressure"); - fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); - } + axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE; + data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure", + input_abs_get_maximum(axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-pressure", + input_abs_get_fuzz(axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); } EXPORT_SYMBOL(touchscreen_parse_of_params); diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c index 72657c5..d8c025b 100644 --- a/drivers/input/touchscreen/tsc2005.c +++ b/drivers/input/touchscreen/tsc2005.c @@ -709,7 +709,7 @@ static int tsc2005_probe(struct spi_device *spi) input_set_abs_params(input_dev, ABS_PRESSURE, 0, max_p, fudge_p, 0); if (np) - touchscreen_parse_of_params(input_dev); + touchscreen_parse_of_params(input_dev, false); input_dev->open = tsc2005_open; input_dev->close = tsc2005_close; diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h index 08a5ef6..eecc9ea 100644 --- a/include/linux/input/touchscreen.h +++ b/include/linux/input/touchscreen.h @@ -12,9 +12,10 @@ #include <linux/input.h> #ifdef CONFIG_OF -void touchscreen_parse_of_params(struct input_dev *dev); +void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch); #else -static inline void touchscreen_parse_of_params(struct input_dev *dev) +static inline void touchscreen_parse_of_params(struct input_dev *dev, + bool multitouch) { } #endif ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 17:47 ` Dmitry Torokhov @ 2015-06-01 20:27 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-01 20:27 UTC (permalink / raw) To: Dmitry Torokhov Cc: Maxime Ripard, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 20:27 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-01 20:27 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 20:27 ` Pavel Machek @ 2015-06-01 20:45 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-01 20:45 UTC (permalink / raw) To: Pavel Machek Cc: Maxime Ripard, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 20:45 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-01 20:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 20:45 ` Dmitry Torokhov @ 2015-06-01 20:54 ` Tony Lindgren -1 siblings, 0 replies; 77+ messages in thread From: Tony Lindgren @ 2015-06-01 20:54 UTC (permalink / raw) To: Dmitry Torokhov Cc: Pavel Machek, Maxime Ripard, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan * Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 20:54 ` Tony Lindgren 0 siblings, 0 replies; 77+ messages in thread From: Tony Lindgren @ 2015-06-01 20:54 UTC (permalink / raw) To: linux-arm-kernel * Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 17:47 ` Dmitry Torokhov @ 2015-06-01 21:22 ` Maxime Ripard -1 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 21:22 UTC (permalink / raw) To: Dmitry Torokhov Cc: Pavel Machek, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 3603 bytes --] 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.torokhov@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 <pavel@ucw.cz> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 21:22 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 21:22 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150601/02f632fa/attachment-0001.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 21:22 ` Maxime Ripard @ 2015-06-01 21:32 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-01 21:32 UTC (permalink / raw) To: Maxime Ripard Cc: Pavel Machek, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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.torokhov@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 <pavel@ucw.cz> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-01 21:32 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-01 21:32 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 21:32 ` Dmitry Torokhov @ 2015-06-02 8:25 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-02 8:25 UTC (permalink / raw) To: Dmitry Torokhov Cc: Maxime Ripard, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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.torokhov@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 <pavel@ucw.cz> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-02 8:25 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-02 8:25 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-01 21:32 ` Dmitry Torokhov @ 2015-06-02 9:44 ` Maxime Ripard -1 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-02 9:44 UTC (permalink / raw) To: Dmitry Torokhov Cc: Pavel Machek, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 4306 bytes --] 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.torokhov@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 <pavel@ucw.cz> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-02 9:44 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-02 9:44 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150602/79e21402/attachment-0001.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-02 9:44 ` Maxime Ripard @ 2015-06-02 17:58 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-02 17:58 UTC (permalink / raw) To: Maxime Ripard Cc: Pavel Machek, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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.torokhov@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 <pavel@ucw.cz> > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-02 17:58 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-02 17:58 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-02 17:58 ` Dmitry Torokhov @ 2015-06-02 18:08 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-02 18:08 UTC (permalink / raw) To: Maxime Ripard Cc: Pavel Machek, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan 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.torokhov@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 <pavel@ucw.cz> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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, maximum, fuzz); - } + axis = multitouch ? ABS_MT_POSITION_X : ABS_X; + data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x", + input_abs_get_max(dev, axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-x", + input_abs_get_fuzz(dev, 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, "touchscreen-size-y", + input_abs_get_max(dev, axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-y", + input_abs_get_fuzz(dev, axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); - maximum = of_get_optional_u32(np, "touchscreen-max-pressure"); - fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); - } + axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE; + data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure", + input_abs_get_max(dev, axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-pressure", + input_abs_get_fuzz(dev, axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); } EXPORT_SYMBOL(touchscreen_parse_of_params); diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c index 72657c5..d8c025b 100644 --- a/drivers/input/touchscreen/tsc2005.c +++ b/drivers/input/touchscreen/tsc2005.c @@ -709,7 +709,7 @@ static int tsc2005_probe(struct spi_device *spi) input_set_abs_params(input_dev, ABS_PRESSURE, 0, max_p, fudge_p, 0); if (np) - touchscreen_parse_of_params(input_dev); + touchscreen_parse_of_params(input_dev, false); input_dev->open = tsc2005_open; input_dev->close = tsc2005_close; diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h index 08a5ef6..eecc9ea 100644 --- a/include/linux/input/touchscreen.h +++ b/include/linux/input/touchscreen.h @@ -12,9 +12,10 @@ #include <linux/input.h> #ifdef CONFIG_OF -void touchscreen_parse_of_params(struct input_dev *dev); +void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch); #else -static inline void touchscreen_parse_of_params(struct input_dev *dev) +static inline void touchscreen_parse_of_params(struct input_dev *dev, + bool multitouch) { } #endif ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-02 18:08 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-06-02 18:08 UTC (permalink / raw) To: linux-arm-kernel 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.torokhov@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 <pavel@ucw.cz> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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, maximum, fuzz); - } + axis = multitouch ? ABS_MT_POSITION_X : ABS_X; + data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x", + input_abs_get_max(dev, axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-x", + input_abs_get_fuzz(dev, 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, "touchscreen-size-y", + input_abs_get_max(dev, axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-y", + input_abs_get_fuzz(dev, axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); - maximum = of_get_optional_u32(np, "touchscreen-max-pressure"); - fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); - } + axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE; + data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure", + input_abs_get_max(dev, axis), + &maximum) | + touchscreen_get_prop_u32(np, "touchscreen-fuzz-pressure", + input_abs_get_fuzz(dev, axis), + &fuzz); + if (data_present) + touchscreen_set_params(dev, axis, maximum, fuzz); } EXPORT_SYMBOL(touchscreen_parse_of_params); diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c index 72657c5..d8c025b 100644 --- a/drivers/input/touchscreen/tsc2005.c +++ b/drivers/input/touchscreen/tsc2005.c @@ -709,7 +709,7 @@ static int tsc2005_probe(struct spi_device *spi) input_set_abs_params(input_dev, ABS_PRESSURE, 0, max_p, fudge_p, 0); if (np) - touchscreen_parse_of_params(input_dev); + touchscreen_parse_of_params(input_dev, false); input_dev->open = tsc2005_open; input_dev->close = tsc2005_close; diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h index 08a5ef6..eecc9ea 100644 --- a/include/linux/input/touchscreen.h +++ b/include/linux/input/touchscreen.h @@ -12,9 +12,10 @@ #include <linux/input.h> #ifdef CONFIG_OF -void touchscreen_parse_of_params(struct input_dev *dev); +void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch); #else -static inline void touchscreen_parse_of_params(struct input_dev *dev) +static inline void touchscreen_parse_of_params(struct input_dev *dev, + bool multitouch) { } #endif ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. 2015-06-02 18:08 ` Dmitry Torokhov @ 2015-06-25 20:25 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-25 20:25 UTC (permalink / raw) To: Dmitry Torokhov Cc: Maxime Ripard, Linus Torvalds, Felipe Balbi, Sebastian Reichel, kernel list, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan Hi! > By the way, the previous version was busted, this one should compile. > Input: improve parsing OF parameters for touchscreens > > From: Dmitry Torokhov <dmitry.torokhov@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 <pavel@ucw.cz> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Tested-by: Pavel Machek <pavel@ucw.cz> Thanks! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. @ 2015-06-25 20:25 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-25 20:25 UTC (permalink / raw) To: linux-arm-kernel Hi! > By the way, the previous version was busted, this one should compile. > Input: improve parsing OF parameters for touchscreens > > From: Dmitry Torokhov <dmitry.torokhov@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 <pavel@ucw.cz> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Tested-by: Pavel Machek <pavel@ucw.cz> Thanks! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:56 ` Pavel Machek @ 2015-05-29 20:22 ` Dmitry Torokhov -1 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 20:22 UTC (permalink / raw) To: Pavel Machek Cc: Felipe Balbi, Sebastian Reichel, kernel list, maxime.ripard, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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: > > Agreed. In parent email, I have list of two commits that should be > reverted. Well, not exactly. The kernel has never parsed touchscreen-max-x and touchscreen-max-y, however we did indeed change behavior of touchscreen_parse_of_params() to reset both max and fuzz if either one is present... I guess we can adjust the behavior it for the sake of tsc2005. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:22 ` Dmitry Torokhov 0 siblings, 0 replies; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 20:22 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 09:56:29PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 14:49:55, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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: > > Agreed. In parent email, I have list of two commits that should be > reverted. Well, not exactly. The kernel has never parsed touchscreen-max-x and touchscreen-max-y, however we did indeed change behavior of touchscreen_parse_of_params() to reset both max and fuzz if either one is present... I guess we can adjust the behavior it for the sake of tsc2005. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:49 ` Felipe Balbi @ 2015-05-29 20:03 ` Maxime Ripard -1 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-05-29 20:03 UTC (permalink / raw) To: Felipe Balbi Cc: Pavel Machek, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 1838 bytes --] On Fri, May 29, 2015 at 02:49:55PM -0500, Felipe Balbi wrote: > Hi, > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > 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 <pavel@ucw.cz> > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > index 4b641c7..09089a6 100644 > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > @@ -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. 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. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:03 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-05-29 20:03 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 02:49:55PM -0500, Felipe Balbi wrote: > Hi, > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > 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 <pavel@ucw.cz> > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > index 4b641c7..09089a6 100644 > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > @@ -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. 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. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150529/0eb41258/attachment.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:03 ` Maxime Ripard @ 2015-05-29 20:18 ` Tony Lindgren -1 siblings, 0 replies; 77+ messages in thread From: Tony Lindgren @ 2015-05-29 20:18 UTC (permalink / raw) To: Maxime Ripard Cc: Felipe Balbi, Pavel Machek, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan * Maxime Ripard <maxime.ripard@free-electrons.com> [150529 13:06]: > On Fri, May 29, 2015 at 02:49:55PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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. > > 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. Tony ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:18 ` Tony Lindgren 0 siblings, 0 replies; 77+ messages in thread From: Tony Lindgren @ 2015-05-29 20:18 UTC (permalink / raw) To: linux-arm-kernel * Maxime Ripard <maxime.ripard@free-electrons.com> [150529 13:06]: > On Fri, May 29, 2015 at 02:49:55PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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. > > 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. Tony ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:18 ` Tony Lindgren @ 2015-05-29 20:30 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 20:30 UTC (permalink / raw) To: Tony Lindgren Cc: Maxime Ripard, Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan > > > 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. > > OK thanks for confirming. Will apply this into omap-for-v4.1/fixes. Thanks! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-29 20:30 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 20:30 UTC (permalink / raw) To: linux-arm-kernel > > > 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. > > OK thanks for confirming. Will apply this into omap-for-v4.1/fixes. Thanks! Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 20:03 ` Maxime Ripard @ 2015-05-30 10:14 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-30 10:14 UTC (permalink / raw) To: Maxime Ripard Cc: Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Fri 2015-05-29 22:03:06, Maxime Ripard wrote: > On Fri, May 29, 2015 at 02:49:55PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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. > > 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. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-30 10:14 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-30 10:14 UTC (permalink / raw) To: linux-arm-kernel On Fri 2015-05-29 22:03:06, Maxime Ripard wrote: > On Fri, May 29, 2015 at 02:49:55PM -0500, Felipe Balbi wrote: > > Hi, > > > > On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > > > 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 <pavel@ucw.cz> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > index 4b641c7..09089a6 100644 > > > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > > > @@ -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. > > 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. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-30 10:14 ` Pavel Machek @ 2015-06-01 9:49 ` Maxime Ripard -1 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 9:49 UTC (permalink / raw) To: Pavel Machek Cc: Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 683 bytes --] On Sat, May 30, 2015 at 12:14:30PM +0200, Pavel Machek wrote: > 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. Feel free to suggest any better solution. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-06-01 9:49 ` Maxime Ripard 0 siblings, 0 replies; 77+ messages in thread From: Maxime Ripard @ 2015-06-01 9:49 UTC (permalink / raw) To: linux-arm-kernel On Sat, May 30, 2015 at 12:14:30PM +0200, Pavel Machek wrote: > 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. Feel free to suggest any better solution. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150601/ed3687f0/attachment.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-06-01 9:49 ` Maxime Ripard @ 2015-06-01 9:54 ` Pavel Machek -1 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-01 9:54 UTC (permalink / raw) To: Maxime Ripard Cc: Felipe Balbi, Sebastian Reichel, kernel list, dmitry.torokhov, pali.rohar, sre, sre, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan On Mon 2015-06-01 11:49:19, Maxime Ripard wrote: > On Sat, May 30, 2015 at 12:14:30PM +0200, Pavel Machek wrote: > > 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. It is "whatever was in the structure in the first place", as it was before you patched the code. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-06-01 9:54 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-06-01 9:54 UTC (permalink / raw) To: linux-arm-kernel On Mon 2015-06-01 11:49:19, Maxime Ripard wrote: > On Sat, May 30, 2015 at 12:14:30PM +0200, Pavel Machek wrote: > > 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. It is "whatever was in the structure in the first place", as it was before you patched the code. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 2015-05-29 19:32 ` Pavel Machek @ 2015-05-30 2:21 ` Sebastian Reichel -1 siblings, 0 replies; 77+ messages in thread From: Sebastian Reichel @ 2015-05-30 2:21 UTC (permalink / raw) To: Pavel Machek Cc: kernel list, maxime.ripard, dmitry.torokhov, pali.rohar, linux-arm-kernel, linux-omap, tony, khilman, aaro.koskinen, ivo.g.dimitrov.75, patrikbachan [-- Attachment #1: Type: text/plain, Size: 1857 bytes --] Hi, On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > 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 <pavel@ucw.cz> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > index 4b641c7..09089a6 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > @@ -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>; > diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts > index 5c16145..5f5e0f3 100644 > --- a/arch/arm/boot/dts/omap3-n900.dts > +++ b/arch/arm/boot/dts/omap3-n900.dts > @@ -832,8 +832,8 @@ > 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>; Acked-By: Sebastian Reichel <sre@kernel.org> max-x/fuzz-x is a leftover from development of the generic bindings (Initially I used min and max). If 3eea8b5d68c801fec788b411582b803463834752's behaviour should become the new behaviour it should be documented, that maximum and fuzz values are dependent on each other. Instead I suggest to restore the old behaviour. -- Sebastian [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 @ 2015-05-30 2:21 ` Sebastian Reichel 0 siblings, 0 replies; 77+ messages in thread From: Sebastian Reichel @ 2015-05-30 2:21 UTC (permalink / raw) To: linux-arm-kernel Hi, On Fri, May 29, 2015 at 09:32:11PM +0200, Pavel Machek wrote: > 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 <pavel@ucw.cz> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > index 4b641c7..09089a6 100644 > --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2005.txt > @@ -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>; > diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts > index 5c16145..5f5e0f3 100644 > --- a/arch/arm/boot/dts/omap3-n900.dts > +++ b/arch/arm/boot/dts/omap3-n900.dts > @@ -832,8 +832,8 @@ > 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>; Acked-By: Sebastian Reichel <sre@kernel.org> max-x/fuzz-x is a leftover from development of the generic bindings (Initially I used min and max). If 3eea8b5d68c801fec788b411582b803463834752's behaviour should become the new behaviour it should be documented, that maximum and fuzz values are dependent on each other. Instead I suggest to restore the old behaviour. -- Sebastian -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150530/2f9f6d69/attachment.sig> ^ permalink raw reply [flat|nested] 77+ messages in thread
* [PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values 2015-05-30 2:21 ` Sebastian Reichel (?) @ 2015-05-30 2:24 ` Sebastian Reichel 2015-05-30 6:26 ` Pavel Machek -1 siblings, 1 reply; 77+ messages in thread From: Sebastian Reichel @ 2015-05-30 2:24 UTC (permalink / raw) To: dmitry.torokhov Cc: Pavel Machek, maxime.ripard, kernel list, linux-omap, linux-input, Sebastian Reichel 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 <pavel@ucw.cz> Fixes: 3eea8b5d68c8 (Input: of_touchscreen - rework the DT parsing function) Signed-off-by: Sebastian Reichel <sre@kernel.org> --- drivers/input/touchscreen/of_touchscreen.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c index b82b520..7d2cf59 100644 --- a/drivers/input/touchscreen/of_touchscreen.c +++ b/drivers/input/touchscreen/of_touchscreen.c @@ -42,8 +42,11 @@ static void touchscreen_set_params(struct input_dev *dev, } absinfo = &dev->absinfo[axis]; - absinfo->maximum = max; - absinfo->fuzz = fuzz; + + if (max) + absinfo->maximum = max; + if (fuzz) + absinfo->fuzz = fuzz; } /** @@ -65,23 +68,17 @@ void touchscreen_parse_of_params(struct input_dev *dev) maximum = of_get_optional_u32(np, "touchscreen-size-x"); fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_X, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_POSITION_X, maximum, fuzz); - } + touchscreen_set_params(dev, ABS_X, maximum, fuzz); + touchscreen_set_params(dev, ABS_MT_POSITION_X, 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); - } + touchscreen_set_params(dev, ABS_Y, maximum, fuzz); + touchscreen_set_params(dev, ABS_MT_POSITION_Y, maximum, fuzz); maximum = of_get_optional_u32(np, "touchscreen-max-pressure"); fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); - } + touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); + touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); } EXPORT_SYMBOL(touchscreen_parse_of_params); -- 2.1.4 ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: [PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values 2015-05-30 2:24 ` [PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values Sebastian Reichel @ 2015-05-30 6:26 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-30 6:26 UTC (permalink / raw) To: Sebastian Reichel Cc: dmitry.torokhov, maxime.ripard, kernel list, linux-omap, linux-input On Sat 2015-05-30 04:24:34, Sebastian Reichel wrote: > 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 <pavel@ucw.cz> > Fixes: 3eea8b5d68c8 (Input: of_touchscreen - rework the DT parsing function) > Signed-off-by: Sebastian Reichel <sre@kernel.org> > --- > drivers/input/touchscreen/of_touchscreen.c | 25 +++++++++++-------------- > 1 file changed, 11 insertions(+), 14 deletions(-) > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c > index b82b520..7d2cf59 100644 > --- a/drivers/input/touchscreen/of_touchscreen.c > +++ b/drivers/input/touchscreen/of_touchscreen.c > @@ -42,8 +42,11 @@ static void touchscreen_set_params(struct input_dev *dev, > } > > absinfo = &dev->absinfo[axis]; > - absinfo->maximum = max; > - absinfo->fuzz = fuzz; > + > + if (max) > + absinfo->maximum = max; > + if (fuzz) > + absinfo->fuzz = fuzz; If driver sets absinfo->fuzz to 5, and then device tree specifies fuzz of 0, this means it will not be overwritten. > @@ -65,23 +68,17 @@ void touchscreen_parse_of_params(struct input_dev *dev) > > maximum = of_get_optional_u32(np, "touchscreen-size-x"); > fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x"); > - if (maximum || fuzz) { > - touchscreen_set_params(dev, ABS_X, maximum, fuzz); > - touchscreen_set_params(dev, ABS_MT_POSITION_X, maximum, fuzz); > - } > + touchscreen_set_params(dev, ABS_X, maximum, fuzz); > + touchscreen_set_params(dev, ABS_MT_POSITION_X, maximum, fuzz); This will introduce warn_on() on axis that are not mentioned in the device tree. Same problem Dmitry did not like on my patch. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] 2015-05-29 19:25 ` Pavel Machek 2015-05-29 19:32 ` Pavel Machek @ 2015-05-29 19:57 ` Maxime Ripard 2015-05-29 20:29 ` Pavel Machek 2015-05-29 21:17 ` Pavel Machek 1 sibling, 2 replies; 77+ messages in thread From: Maxime Ripard @ 2015-05-29 19:57 UTC (permalink / raw) To: Pavel Machek Cc: Sebastian Reichel, kernel list, dmitry.torokhov, Pali Rohár, Ivaylo Dimitrov [-- Attachment #1: Type: text/plain, Size: 1850 bytes --] On Fri, May 29, 2015 at 09:25:05PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 21:08:16, Pavel Machek wrote: > > Hi! > > > > > 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..) > > 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. What do you mean? touchscreen-max-* _is_ device tree data for an axis. > 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. No, both maximum and fuzz are optional. You can perfectly have one without another. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] 2015-05-29 19:57 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Maxime Ripard @ 2015-05-29 20:29 ` Pavel Machek 2015-05-29 21:17 ` Pavel Machek 1 sibling, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 20:29 UTC (permalink / raw) To: Maxime Ripard Cc: Sebastian Reichel, kernel list, dmitry.torokhov, Pali Rohár, Ivaylo Dimitrov On Fri 2015-05-29 21:57:40, Maxime Ripard wrote: > On Fri, May 29, 2015 at 09:25:05PM +0200, Pavel Machek wrote: > > On Fri 2015-05-29 21:08:16, Pavel Machek wrote: > > > Hi! > > > > > > > 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..) > > > > 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. > > What do you mean? touchscreen-max-* _is_ device tree data for an axis. > > > 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. > > No, both maximum and fuzz are optional. You can perfectly have one > without another. Yes, and after your "cleanup", you overwrite maximum with zero with fuzz is specified. ->maximum contained 4096 before, your code sees that it is not specified in the device tree, and you go ahead and replace ->maximum with 0, anyway. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] 2015-05-29 19:57 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Maxime Ripard 2015-05-29 20:29 ` Pavel Machek @ 2015-05-29 21:17 ` Pavel Machek 2015-05-29 21:36 ` Dmitry Torokhov 1 sibling, 1 reply; 77+ messages in thread From: Pavel Machek @ 2015-05-29 21:17 UTC (permalink / raw) To: Maxime Ripard Cc: Sebastian Reichel, kernel list, dmitry.torokhov, Pali Rohár, Ivaylo Dimitrov On Fri 2015-05-29 21:57:40, Maxime Ripard wrote: > On Fri, May 29, 2015 at 09:25:05PM +0200, Pavel Machek wrote: > > On Fri 2015-05-29 21:08:16, Pavel Machek wrote: > > > Hi! > > > > > > > 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..) > > > > 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. > > What do you mean? touchscreen-max-* _is_ device tree data for an axis. > > > 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. > > 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 <pavel@ucw.cz> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c index b82b520..e626c8a 100644 --- a/drivers/input/touchscreen/of_touchscreen.c +++ b/drivers/input/touchscreen/of_touchscreen.c @@ -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)) { - /* - * 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"); } absinfo = &dev->absinfo[axis]; - absinfo->maximum = max; - absinfo->fuzz = fuzz; + of_property_read_u32(np, max, &absinfo->maximum); + of_property_read_u32(np, fuzz, &absinfo->fuzz); } /** @@ -56,32 +40,14 @@ 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; 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); - } - - 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); - } - - maximum = of_get_optional_u32(np, "touchscreen-max-pressure"); - fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure"); - if (maximum || fuzz) { - touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz); - touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz); - } + touchscreen_set_params(dev, ABS_X, "touchscreen-size-x", "touchscreen-fuzz-x"); + touchscreen_set_params(dev, ABS_Y, "touchscreen-size-y", "touchscreen-fuzz-y"); + touchscreen_set_params(dev, ABS_PRESSURE, "touchscreen-max-pressure", "touchscreen-fuzz-pressure"); } EXPORT_SYMBOL(touchscreen_parse_of_params); -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply related [flat|nested] 77+ messages in thread
* Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] 2015-05-29 21:17 ` Pavel Machek @ 2015-05-29 21:36 ` Dmitry Torokhov 2015-05-29 21:58 ` Pavel Machek 0 siblings, 1 reply; 77+ messages in thread From: Dmitry Torokhov @ 2015-05-29 21:36 UTC (permalink / raw) To: Pavel Machek Cc: Maxime Ripard, Sebastian Reichel, kernel list, Pali Rohár, Ivaylo Dimitrov On Fri, May 29, 2015 at 11:17:22PM +0200, Pavel Machek wrote: > On Fri 2015-05-29 21:57:40, Maxime Ripard wrote: > > On Fri, May 29, 2015 at 09:25:05PM +0200, Pavel Machek wrote: > > > On Fri 2015-05-29 21:08:16, Pavel Machek wrote: > > > > Hi! > > > > > > > > > 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..) > > > > > > 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. > > > > What do you mean? touchscreen-max-* _is_ device tree data for an axis. > > > > > 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. > > > > 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 <pavel@ucw.cz> > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c > index b82b520..e626c8a 100644 > --- a/drivers/input/touchscreen/of_touchscreen.c > +++ b/drivers/input/touchscreen/of_touchscreen.c > @@ -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)) { > - /* > - * 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). Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 77+ messages in thread
* Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] 2015-05-29 21:36 ` Dmitry Torokhov @ 2015-05-29 21:58 ` Pavel Machek 0 siblings, 0 replies; 77+ messages in thread From: Pavel Machek @ 2015-05-29 21:58 UTC (permalink / raw) To: Dmitry Torokhov Cc: Maxime Ripard, Sebastian Reichel, kernel list, Pali Rohár, Ivaylo Dimitrov On Fri 2015-05-29 14:36:49, Dmitry Torokhov wrote: > On Fri, May 29, 2015 at 11:17:22PM +0200, Pavel Machek wrote: > > On Fri 2015-05-29 21:57:40, Maxime Ripard wrote: > > > On Fri, May 29, 2015 at 09:25:05PM +0200, Pavel Machek wrote: > > > > On Fri 2015-05-29 21:08:16, Pavel Machek wrote: > > > > > Hi! > > > > > > > > > > > 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..) > > > > > > > > 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. > > > > > > What do you mean? touchscreen-max-* _is_ device tree data for an axis. > > > > > > > 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. > > > > > > 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 <pavel@ucw.cz> > > > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c > > index b82b520..e626c8a 100644 > > --- a/drivers/input/touchscreen/of_touchscreen.c > > +++ b/drivers/input/touchscreen/of_touchscreen.c > > @@ -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)) { > > - /* > > - * 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. Well, we should not bitch at all, that should have been followup patch. > Plus the code structure won't work well if we decide to add more to it > (such as min, or flat, or resolution). Do you want to try to cook up something that works? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 77+ messages in thread
end of thread, other threads:[~2015-06-25 20:25 UTC | newest] Thread overview: 77+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <201505241444.41039@pali> [not found] ` <20150527132545.GA23434@amd> [not found] ` <20150527133311.GJ30798@pali> [not found] ` <20150527143722.GA28108@amd> [not found] ` <20150527145837.GA13223@earth> 2015-05-29 19:08 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Pavel Machek 2015-05-29 19:25 ` Pavel Machek 2015-05-29 19:32 ` [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 Pavel Machek 2015-05-29 19:32 ` Pavel Machek 2015-05-29 19:32 ` Pavel Machek 2015-05-29 19:49 ` Felipe Balbi 2015-05-29 19:49 ` Felipe Balbi 2015-05-29 19:49 ` Felipe Balbi 2015-05-29 19:56 ` Pavel Machek 2015-05-29 19:56 ` Pavel Machek 2015-05-29 20:17 ` Maxime Ripard 2015-05-29 20:17 ` Maxime Ripard 2015-05-29 20:21 ` Felipe Balbi 2015-05-29 20:21 ` Felipe Balbi 2015-05-29 20:21 ` Felipe Balbi 2015-05-29 20:29 ` Dmitry Torokhov 2015-05-29 20:29 ` Dmitry Torokhov 2015-05-29 20:34 ` Pavel Machek 2015-05-29 20:34 ` Pavel Machek 2015-05-29 20:48 ` Dmitry Torokhov 2015-05-29 20:48 ` Dmitry Torokhov 2015-05-29 21:02 ` Pavel Machek 2015-05-29 21:02 ` Pavel Machek 2015-05-29 21:38 ` Dmitry Torokhov 2015-05-29 21:38 ` Dmitry Torokhov 2015-06-01 9:55 ` Maxime Ripard 2015-06-01 9:55 ` Maxime Ripard 2015-06-01 14:06 ` Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen " Pavel Machek 2015-06-01 14:58 ` Maxime Ripard 2015-06-01 14:58 ` Maxime Ripard 2015-06-01 15:21 ` Pavel Machek 2015-06-01 15:21 ` Pavel Machek 2015-06-01 17:47 ` Dmitry Torokhov 2015-06-01 17:47 ` Dmitry Torokhov 2015-06-01 20:27 ` Pavel Machek 2015-06-01 20:27 ` Pavel Machek 2015-06-01 20:45 ` Dmitry Torokhov 2015-06-01 20:45 ` Dmitry Torokhov 2015-06-01 20:54 ` Tony Lindgren 2015-06-01 20:54 ` Tony Lindgren 2015-06-01 21:22 ` Maxime Ripard 2015-06-01 21:22 ` Maxime Ripard 2015-06-01 21:32 ` Dmitry Torokhov 2015-06-01 21:32 ` Dmitry Torokhov 2015-06-02 8:25 ` Pavel Machek 2015-06-02 8:25 ` Pavel Machek 2015-06-02 9:44 ` Maxime Ripard 2015-06-02 9:44 ` Maxime Ripard 2015-06-02 17:58 ` Dmitry Torokhov 2015-06-02 17:58 ` Dmitry Torokhov 2015-06-02 18:08 ` Dmitry Torokhov 2015-06-02 18:08 ` Dmitry Torokhov 2015-06-25 20:25 ` Pavel Machek 2015-06-25 20:25 ` Pavel Machek 2015-05-29 20:22 ` [PATCH] fix n900 dts file to work around 4.1 touchscreen regression " Dmitry Torokhov 2015-05-29 20:22 ` Dmitry Torokhov 2015-05-29 20:03 ` Maxime Ripard 2015-05-29 20:03 ` Maxime Ripard 2015-05-29 20:18 ` Tony Lindgren 2015-05-29 20:18 ` Tony Lindgren 2015-05-29 20:30 ` Pavel Machek 2015-05-29 20:30 ` Pavel Machek 2015-05-30 10:14 ` Pavel Machek 2015-05-30 10:14 ` Pavel Machek 2015-06-01 9:49 ` Maxime Ripard 2015-06-01 9:49 ` Maxime Ripard 2015-06-01 9:54 ` Pavel Machek 2015-06-01 9:54 ` Pavel Machek 2015-05-30 2:21 ` Sebastian Reichel 2015-05-30 2:21 ` Sebastian Reichel 2015-05-30 2:24 ` [PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values Sebastian Reichel 2015-05-30 6:26 ` Pavel Machek 2015-05-29 19:57 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Maxime Ripard 2015-05-29 20:29 ` Pavel Machek 2015-05-29 21:17 ` Pavel Machek 2015-05-29 21:36 ` Dmitry Torokhov 2015-05-29 21:58 ` Pavel Machek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.