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

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

[Pavel Machek]

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

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

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

Fix dts to match what the Linux kernel expects. This works around
touchscreen problems in 4.1 linux on Nokia n900.
    
Signed-off-by: Pavel Machek <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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

Fix dts to match what the Linux kernel expects. This works around
touchscreen problems in 4.1 linux on Nokia n900.
    
Signed-off-by: Pavel Machek <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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

Fix dts to match what the Linux kernel expects. This works around
touchscreen problems in 4.1 linux on Nokia n900.
    
Signed-off-by: Pavel Machek <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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Felipe Balbi]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Felipe Balbi]

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>

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Felipe Balbi]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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>

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Felipe Balbi]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Felipe Balbi]

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>

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Felipe Balbi]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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>

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Maxime Ripard]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Maxime Ripard]

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>

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Tony Lindgren]

* 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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Tony Lindgren]

* 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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Maxime Ripard]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Maxime Ripard]

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>

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Maxime Ripard]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Maxime Ripard]

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>

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Dmitry Torokhov]

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

Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Dmitry Torokhov]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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>

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Tony Lindgren]

* 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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Tony Lindgren]

* 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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Maxime Ripard]

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>

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Pavel Machek]

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

Re: [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Sebastian Reichel]

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

[PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900

[Sebastian Reichel]

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>

[PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values

[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

Re: [PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values

[Pavel Machek]

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

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

[Maxime Ripard]

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

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

[Pavel Machek]

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

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

[Pavel Machek]

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

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

[Dmitry Torokhov]

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

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

[Pavel Machek]

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