From: Maxime Ripard <maxime.ripard@free-electrons.com> To: Pavel Machek <pavel@ucw.cz> Cc: Linus Torvalds <torvalds@linux-foundation.org>, Dmitry Torokhov <dmitry.torokhov@gmail.com>, Felipe Balbi <balbi@ti.com>, Sebastian Reichel <sre@kernel.org>, kernel list <linux-kernel@vger.kernel.org>, pali.rohar@gmail.com, sre@debian.org, sre@ring0.de, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, linux-omap@vger.kernel.org, tony@atomide.com, khilman@kernel.org, aaro.koskinen@iki.fi, ivo.g.dimitrov.75@gmail.com, patrikbachan@gmail.com Subject: Re: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. Date: Mon, 1 Jun 2015 16:58:25 +0200 [thread overview] Message-ID: <20150601145825.GA20557@lukather> (raw) In-Reply-To: <20150601140605.GA26908@amd> [-- Attachment #1: Type: text/plain, Size: 3639 bytes --] On Mon, Jun 01, 2015 at 04:06:06PM +0200, Pavel Machek wrote: > Hi! > > > > But that's not what I'm asking. See a changelog of > > > 3eea8b5d68c801fec788b411582b803463834752 and compare it with what it > > > actually does. > > > > > > It is buggy. If fuzz is specified but maximum is not, it overwites > > > maximum with zero. > > > > If maximum is not set, you'll have other issues anyway. But it really > > boils down on what the default behaviour should be. > > It was not broken before commit > 3eea8b5d68c801fec788b411582b803463834752. Maximum was set, but after > your patch, it is overwritten with zero. > > > > Plus it introduces new failure "if (!test_bit(axis, dev->absbit))". > > > > It's not a new failure, it's testing against stupid code. > > Yes. In a commit marked "cleanup". We call this "undocumented > feature". It has never been said that it was a "cleanup" (whatever the double quotes mean) but a rework. And it's not a feature either. > > If an axis is setup in the DT but not registered in the driver, > > something is wrong, most probably the DT. > > Yes, we have fixed the DT, so that bug you introduced will not happen > on n900 with updated device tree. s/updated/fixed/ > > > Plus it fails to distinguish between "value not specified in the dt" > > > and "zero is specified in the dt". > > > > Again, default behaviour. > > Again, regression from 4.0 kernel, you are not willing to fix. > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad. > > > > You were very welcome to review this patch at the time and/or suggest > > a fix that pleases everyone. > > You should be the one that should suggest fixes, as you broke it in > the first place. But clearly you don't understand that. You actually never asked for a fix, and went head first calling this patch "bad" and asking for nothing but reverting it. It's not really a very good way to move forward and start a productive discussion. I'm sorry if this cause you any issues, but reverting this patch will break users (and this time at the kernel level only, the DT has nothing to do with that) just like you are right now. What about: -- >8 -- diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c index b82b5207c78b..7e98c2e443ab 100644 --- a/drivers/input/touchscreen/of_touchscreen.c +++ b/drivers/input/touchscreen/of_touchscreen.c @@ -14,10 +14,10 @@ #include <linux/input/mt.h> #include <linux/input/touchscreen.h> -static u32 of_get_optional_u32(struct device_node *np, +static int of_get_optional_u32(struct device_node *np, const char *property) { - u32 val = 0; + int val = -1; of_property_read_u32(np, property, &val); @@ -42,8 +42,12 @@ static void touchscreen_set_params(struct input_dev *dev, } absinfo = &dev->absinfo[axis]; - absinfo->maximum = max; - absinfo->fuzz = fuzz; + + if (max >= 0) + absinfo->maximum = max; + + if (fuzz >= 0) + absinfo->fuzz = fuzz; } /** @@ -57,7 +61,7 @@ static void touchscreen_set_params(struct input_dev *dev, void touchscreen_parse_of_params(struct input_dev *dev) { struct device_node *np = dev->dev.parent->of_node; - u32 maximum, fuzz; + int maximum, fuzz; input_alloc_absinfo(dev); if (!dev->absinfo) -- >8 -- That reduces the max size of the screens, but I don't really expect the screen size to reach that order of magnitude before a few years... Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: maxime.ripard@free-electrons.com (Maxime Ripard) To: linux-arm-kernel@lists.infradead.org Subject: Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900. Date: Mon, 1 Jun 2015 16:58:25 +0200 [thread overview] Message-ID: <20150601145825.GA20557@lukather> (raw) In-Reply-To: <20150601140605.GA26908@amd> On Mon, Jun 01, 2015 at 04:06:06PM +0200, Pavel Machek wrote: > Hi! > > > > But that's not what I'm asking. See a changelog of > > > 3eea8b5d68c801fec788b411582b803463834752 and compare it with what it > > > actually does. > > > > > > It is buggy. If fuzz is specified but maximum is not, it overwites > > > maximum with zero. > > > > If maximum is not set, you'll have other issues anyway. But it really > > boils down on what the default behaviour should be. > > It was not broken before commit > 3eea8b5d68c801fec788b411582b803463834752. Maximum was set, but after > your patch, it is overwritten with zero. > > > > Plus it introduces new failure "if (!test_bit(axis, dev->absbit))". > > > > It's not a new failure, it's testing against stupid code. > > Yes. In a commit marked "cleanup". We call this "undocumented > feature". It has never been said that it was a "cleanup" (whatever the double quotes mean) but a rework. And it's not a feature either. > > If an axis is setup in the DT but not registered in the driver, > > something is wrong, most probably the DT. > > Yes, we have fixed the DT, so that bug you introduced will not happen > on n900 with updated device tree. s/updated/fixed/ > > > Plus it fails to distinguish between "value not specified in the dt" > > > and "zero is specified in the dt". > > > > Again, default behaviour. > > Again, regression from 4.0 kernel, you are not willing to fix. > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad. > > > > You were very welcome to review this patch at the time and/or suggest > > a fix that pleases everyone. > > You should be the one that should suggest fixes, as you broke it in > the first place. But clearly you don't understand that. You actually never asked for a fix, and went head first calling this patch "bad" and asking for nothing but reverting it. It's not really a very good way to move forward and start a productive discussion. I'm sorry if this cause you any issues, but reverting this patch will break users (and this time at the kernel level only, the DT has nothing to do with that) just like you are right now. What about: -- >8 -- diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c index b82b5207c78b..7e98c2e443ab 100644 --- a/drivers/input/touchscreen/of_touchscreen.c +++ b/drivers/input/touchscreen/of_touchscreen.c @@ -14,10 +14,10 @@ #include <linux/input/mt.h> #include <linux/input/touchscreen.h> -static u32 of_get_optional_u32(struct device_node *np, +static int of_get_optional_u32(struct device_node *np, const char *property) { - u32 val = 0; + int val = -1; of_property_read_u32(np, property, &val); @@ -42,8 +42,12 @@ static void touchscreen_set_params(struct input_dev *dev, } absinfo = &dev->absinfo[axis]; - absinfo->maximum = max; - absinfo->fuzz = fuzz; + + if (max >= 0) + absinfo->maximum = max; + + if (fuzz >= 0) + absinfo->fuzz = fuzz; } /** @@ -57,7 +61,7 @@ static void touchscreen_set_params(struct input_dev *dev, void touchscreen_parse_of_params(struct input_dev *dev) { struct device_node *np = dev->dev.parent->of_node; - u32 maximum, fuzz; + int maximum, fuzz; input_alloc_absinfo(dev); if (!dev->absinfo) -- >8 -- That reduces the max size of the screens, but I don't really expect the screen size to reach that order of magnitude before a few years... Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150601/f3f3e427/attachment.sig>
next prev parent reply other threads:[~2015-06-01 15:00 UTC|newest] Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <201505241444.41039@pali> [not found] ` <20150527132545.GA23434@amd> [not found] ` <20150527133311.GJ30798@pali> [not found] ` <20150527143722.GA28108@amd> [not found] ` <20150527145837.GA13223@earth> 2015-05-29 19:08 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Pavel Machek 2015-05-29 19:25 ` Pavel Machek 2015-05-29 19:32 ` [PATCH] fix n900 dts file to work around 4.1 touchscreen regression on n900 Pavel Machek 2015-05-29 19:32 ` Pavel Machek 2015-05-29 19:32 ` Pavel Machek 2015-05-29 19:49 ` Felipe Balbi 2015-05-29 19:49 ` Felipe Balbi 2015-05-29 19:49 ` Felipe Balbi 2015-05-29 19:56 ` Pavel Machek 2015-05-29 19:56 ` Pavel Machek 2015-05-29 20:17 ` Maxime Ripard 2015-05-29 20:17 ` Maxime Ripard 2015-05-29 20:21 ` Felipe Balbi 2015-05-29 20:21 ` Felipe Balbi 2015-05-29 20:21 ` Felipe Balbi 2015-05-29 20:29 ` Dmitry Torokhov 2015-05-29 20:29 ` Dmitry Torokhov 2015-05-29 20:34 ` Pavel Machek 2015-05-29 20:34 ` Pavel Machek 2015-05-29 20:48 ` Dmitry Torokhov 2015-05-29 20:48 ` Dmitry Torokhov 2015-05-29 21:02 ` Pavel Machek 2015-05-29 21:02 ` Pavel Machek 2015-05-29 21:38 ` Dmitry Torokhov 2015-05-29 21:38 ` Dmitry Torokhov 2015-06-01 9:55 ` Maxime Ripard 2015-06-01 9:55 ` Maxime Ripard 2015-06-01 14:06 ` Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen " Pavel Machek 2015-06-01 14:58 ` Maxime Ripard [this message] 2015-06-01 14:58 ` Maxime Ripard 2015-06-01 15:21 ` Pavel Machek 2015-06-01 15:21 ` Pavel Machek 2015-06-01 17:47 ` Dmitry Torokhov 2015-06-01 17:47 ` Dmitry Torokhov 2015-06-01 20:27 ` Pavel Machek 2015-06-01 20:27 ` Pavel Machek 2015-06-01 20:45 ` Dmitry Torokhov 2015-06-01 20:45 ` Dmitry Torokhov 2015-06-01 20:54 ` Tony Lindgren 2015-06-01 20:54 ` Tony Lindgren 2015-06-01 21:22 ` Maxime Ripard 2015-06-01 21:22 ` Maxime Ripard 2015-06-01 21:32 ` Dmitry Torokhov 2015-06-01 21:32 ` Dmitry Torokhov 2015-06-02 8:25 ` Pavel Machek 2015-06-02 8:25 ` Pavel Machek 2015-06-02 9:44 ` Maxime Ripard 2015-06-02 9:44 ` Maxime Ripard 2015-06-02 17:58 ` Dmitry Torokhov 2015-06-02 17:58 ` Dmitry Torokhov 2015-06-02 18:08 ` Dmitry Torokhov 2015-06-02 18:08 ` Dmitry Torokhov 2015-06-25 20:25 ` Pavel Machek 2015-06-25 20:25 ` Pavel Machek 2015-05-29 20:22 ` [PATCH] fix n900 dts file to work around 4.1 touchscreen regression " Dmitry Torokhov 2015-05-29 20:22 ` Dmitry Torokhov 2015-05-29 20:03 ` Maxime Ripard 2015-05-29 20:03 ` Maxime Ripard 2015-05-29 20:18 ` Tony Lindgren 2015-05-29 20:18 ` Tony Lindgren 2015-05-29 20:30 ` Pavel Machek 2015-05-29 20:30 ` Pavel Machek 2015-05-30 10:14 ` Pavel Machek 2015-05-30 10:14 ` Pavel Machek 2015-06-01 9:49 ` Maxime Ripard 2015-06-01 9:49 ` Maxime Ripard 2015-06-01 9:54 ` Pavel Machek 2015-06-01 9:54 ` Pavel Machek 2015-05-30 2:21 ` Sebastian Reichel 2015-05-30 2:21 ` Sebastian Reichel 2015-05-30 2:24 ` [PATCH] Input: of_touchscreen - remove interdependence of max/fuzz values Sebastian Reichel 2015-05-30 6:26 ` Pavel Machek 2015-05-29 19:57 ` 4.1 touchscreen regression on n900 -- pinpointed [was Re: linux-n900 v4.1-rc4] Maxime Ripard 2015-05-29 20:29 ` Pavel Machek 2015-05-29 21:17 ` Pavel Machek 2015-05-29 21:36 ` Dmitry Torokhov 2015-05-29 21:58 ` Pavel Machek
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150601145825.GA20557@lukather \ --to=maxime.ripard@free-electrons.com \ --cc=aaro.koskinen@iki.fi \ --cc=balbi@ti.com \ --cc=dmitry.torokhov@gmail.com \ --cc=ivo.g.dimitrov.75@gmail.com \ --cc=khilman@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=pali.rohar@gmail.com \ --cc=patrikbachan@gmail.com \ --cc=pavel@ucw.cz \ --cc=sre@debian.org \ --cc=sre@kernel.org \ --cc=sre@ring0.de \ --cc=tony@atomide.com \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.