Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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
> > > > > Min0
> > > > > Max0
> > > > > 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
> >
> > 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
> > #include
> > -#include
> > #include
> >
> > -static u32 of_get_optional_u32(struct device_node *np,
> > - const char *property)
> > -{
> > - u32 val = 0;
> > -
> > - of_property_read_u32(np, property, );
> > -
> > - 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,
> > -"DT specifies parameters but the axis is not
> > set up\n");
> > - return;
> > + dev_warn(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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
> > > > Min0
> > > > Max0
> > > > 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
>
> 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
> #include
> -#include
> #include
>
> -static u32 of_get_optional_u32(struct device_node *np,
> -const char *property)
> -{
> - u32 val = 0;
> -
> - of_property_read_u32(np, property, );
> -
> - 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,
> - "DT specifies parameters but the axis is not
> set up\n");
> - return;
> + dev_warn(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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
> > > Min0
> > > Max0
> > > 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
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
#include
-#include
#include
-static u32 of_get_optional_u32(struct device_node *np,
- const char *property)
-{
- u32 val = 0;
-
- of_property_read_u32(np, property, );
-
- 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,
-"DT specifies parameters but the axis is not
set up\n");
- return;
+ dev_warn(>dev,
+"DT specifies parameters but the axis is not set
up\n");
}
absinfo = >absinfo[axis];
- absinfo->maximum = max;
- absinfo->fuzz = fuzz;
+ of_property_read_u32(np, max, >maximum);
+ of_property_read_u32(np, fuzz, >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,
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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 > > > Min0 > > > Max0 > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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 > > Min0 > > Max0 > > 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 signature.asc Description: Digital signature
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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 > Min0 > Max0 > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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
Min0
Max0
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 pa...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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 Min0 Max0 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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 Min0 Max0 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 signature.asc Description: Digital signature
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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 Min0 Max0 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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
Min0
Max0
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 pa...@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) {
-
Re: 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4]
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
Min0
Max0
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 pa...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
