| Message ID | 20150529193211.GA7599@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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
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
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 <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
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.
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.
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.
> > > 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
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
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.
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...
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.
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
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
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
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
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
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
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>;
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>