fix n900 dts file to work around 4.1 touchscreen regression on n900
diff mbox

Message ID 20150529193211.GA7599@amd
State New
Headers show

Commit Message

Pavel Machek May 29, 2015, 7:32 p.m. UTC
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>

Comments

Felipe Balbi May 29, 2015, 7:49 p.m. UTC | #1
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>
Pavel Machek May 29, 2015, 7:56 p.m. UTC | #2
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
Maxime Ripard May 29, 2015, 8:03 p.m. UTC | #3
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 May 29, 2015, 8:17 p.m. UTC | #4
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
Tony Lindgren May 29, 2015, 8:18 p.m. UTC | #5
* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi May 29, 2015, 8:21 p.m. UTC | #6
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.
Dmitry Torokhov May 29, 2015, 8:22 p.m. UTC | #7
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 Torokhov May 29, 2015, 8:29 p.m. UTC | #8
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.
Pavel Machek May 29, 2015, 8:30 p.m. UTC | #9
> > > 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
Pavel Machek May 29, 2015, 8:34 p.m. UTC | #10
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
Dmitry Torokhov May 29, 2015, 8:48 p.m. UTC | #11
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.
Pavel Machek May 29, 2015, 9:02 p.m. UTC | #12
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...
Dmitry Torokhov May 29, 2015, 9:38 p.m. UTC | #13
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.
Sebastian Reichel May 30, 2015, 2:21 a.m. UTC | #14
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
Pavel Machek May 30, 2015, 10:14 a.m. UTC | #15
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
Maxime Ripard June 1, 2015, 9:49 a.m. UTC | #16
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
Pavel Machek June 1, 2015, 9:54 a.m. UTC | #17
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
Maxime Ripard June 1, 2015, 9:55 a.m. UTC | #18
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
Pavel Machek June 1, 2015, 2:06 p.m. UTC | #19
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

Patch
diff mbox

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