Please revert 3eea8b5d68c801fec788b411582b803463834752 as it breaks touchscreen on n900.
diff mbox

Message ID 20150601174730.GA30024@dtor-ws
State New
Headers show

Commit Message

Dmitry Torokhov June 1, 2015, 5:47 p.m. UTC
On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> 
> 
> > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > 
> > > > You were very welcome to review this patch at the time and/or suggest
> > > > a fix that pleases everyone.
> > > 
> > > You should be the one that should suggest fixes, as you broke it in
> > > the first place. But clearly you don't understand that.
> > 
> > You actually never asked for a fix, and went head first calling this
> > patch "bad" and asking for nothing but reverting it.
> 
> Date: Fri, 29 May 2015 21:08:16 +0200
> Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> linux-n900
> ...
> Maxime, can you suggest a fix?

How about we do something like below (it needs a small edt-ft5x06 fixup
that I'll send separately). Not tested.

Thanks.

Comments

Pavel Machek June 1, 2015, 8:27 p.m. UTC | #1
On Mon 2015-06-01 10:47:30, Dmitry Torokhov wrote:
> On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > 
> > 
> > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > 
> > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > a fix that pleases everyone.
> > > > 
> > > > You should be the one that should suggest fixes, as you broke it in
> > > > the first place. But clearly you don't understand that.
> > > 
> > > You actually never asked for a fix, and went head first calling this
> > > patch "bad" and asking for nothing but reverting it.
> > 
> > Date: Fri, 29 May 2015 21:08:16 +0200
> > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > linux-n900
> > ...
> > Maxime, can you suggest a fix?
> 
> How about we do something like below (it needs a small edt-ft5x06 fixup
> that I'll send separately). Not tested.

+       data_present = touchscreen_get_prop_u32(np,
"touchscreen-size-x",
+
input_abs_get_maximum(axis),
+                                               &maximum) |
+                      touchscreen_get_prop_u32(np,
"touchscreen-fuzz-x",
+
input_abs_get_fuzz(axis),
+                                               &fuzz);
+       if (data_present)
+               touchscreen_set_params(dev, axis, maximum, fuzz);

Umm. So you are changing behaviour from "whatever was there" to
"input_abs_get_maximum"... in n900 case. Is that a good idea for a
regression fix this late in release cycle?

Maxime's patch should be easy to fix...
									Pavel
Dmitry Torokhov June 1, 2015, 8:45 p.m. UTC | #2
On Mon, Jun 01, 2015 at 10:27:40PM +0200, Pavel Machek wrote:
> On Mon 2015-06-01 10:47:30, Dmitry Torokhov wrote:
> > On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > > 
> > > 
> > > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > > 
> > > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > > a fix that pleases everyone.
> > > > > 
> > > > > You should be the one that should suggest fixes, as you broke it in
> > > > > the first place. But clearly you don't understand that.
> > > > 
> > > > You actually never asked for a fix, and went head first calling this
> > > > patch "bad" and asking for nothing but reverting it.
> > > 
> > > Date: Fri, 29 May 2015 21:08:16 +0200
> > > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > > linux-n900
> > > ...
> > > Maxime, can you suggest a fix?
> > 
> > How about we do something like below (it needs a small edt-ft5x06 fixup
> > that I'll send separately). Not tested.
> 
> +       data_present = touchscreen_get_prop_u32(np,
> "touchscreen-size-x",
> +
> input_abs_get_maximum(axis),
> +                                               &maximum) |
> +                      touchscreen_get_prop_u32(np,
> "touchscreen-fuzz-x",
> +
> input_abs_get_fuzz(axis),
> +                                               &fuzz);
> +       if (data_present)
> +               touchscreen_set_params(dev, axis, maximum, fuzz);
> 
> Umm. So you are changing behaviour from "whatever was there" to
> "input_abs_get_maximum"... in n900 case.o

That _is_ "whatever was there".

> Is that a good idea for a
> regression fix this late in release cycle?

As Maxime mentioned the new behavior is needed for other touchscreens.
Given that fixed DTS is going into 4.1 (as far as I understand) we do
not need to rush this change into 4.1.

Thanks.
Tony Lindgren June 1, 2015, 8:54 p.m. UTC | #3
* Dmitry Torokhov <dmitry.torokhov@gmail.com> [150601 13:47]:
> On Mon, Jun 01, 2015 at 10:27:40PM +0200, Pavel Machek wrote:
> > On Mon 2015-06-01 10:47:30, Dmitry Torokhov wrote:
> > > On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > > > 
> > > > 
> > > > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > > > 
> > > > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > > > a fix that pleases everyone.
> > > > > > 
> > > > > > You should be the one that should suggest fixes, as you broke it in
> > > > > > the first place. But clearly you don't understand that.
> > > > > 
> > > > > You actually never asked for a fix, and went head first calling this
> > > > > patch "bad" and asking for nothing but reverting it.
> > > > 
> > > > Date: Fri, 29 May 2015 21:08:16 +0200
> > > > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > > > linux-n900
> > > > ...
> > > > Maxime, can you suggest a fix?
> > > 
> > > How about we do something like below (it needs a small edt-ft5x06 fixup
> > > that I'll send separately). Not tested.
> > 
> > +       data_present = touchscreen_get_prop_u32(np,
> > "touchscreen-size-x",
> > +
> > input_abs_get_maximum(axis),
> > +                                               &maximum) |
> > +                      touchscreen_get_prop_u32(np,
> > "touchscreen-fuzz-x",
> > +
> > input_abs_get_fuzz(axis),
> > +                                               &fuzz);
> > +       if (data_present)
> > +               touchscreen_set_params(dev, axis, maximum, fuzz);
> > 
> > Umm. So you are changing behaviour from "whatever was there" to
> > "input_abs_get_maximum"... in n900 case.o
> 
> That _is_ "whatever was there".
> 
> > Is that a good idea for a
> > regression fix this late in release cycle?
> 
> As Maxime mentioned the new behavior is needed for other touchscreens.
> Given that fixed DTS is going into 4.1 (as far as I understand) we do
> not need to rush this change into 4.1.

OK great. I'm planning to send a pull request to arm-soc for Pavel's
DTS fix today along with few other fixes.

Regards,

Tony
--
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
Maxime Ripard June 1, 2015, 9:22 p.m. UTC | #4
Hi Dmitry,

On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > 
> > 
> > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > 
> > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > a fix that pleases everyone.
> > > > 
> > > > You should be the one that should suggest fixes, as you broke it in
> > > > the first place. But clearly you don't understand that.
> > > 
> > > You actually never asked for a fix, and went head first calling this
> > > patch "bad" and asking for nothing but reverting it.
> > 
> > Date: Fri, 29 May 2015 21:08:16 +0200
> > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > linux-n900
> > ...
> > Maxime, can you suggest a fix?
> 
> How about we do something like below (it needs a small edt-ft5x06 fixup
> that I'll send separately). Not tested.
> 
> Thanks.
> 
> -- 
> Dmitry
> 
> 
> Input: improve parsing OF parameters for touchscreens
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> When applying touchscreen parameters specified in device tree let's make
> sure we keep whatever setup was done by the driver and not reset the
> missing values to zero.
> 
> Reported-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c     |    2 -
>  drivers/input/touchscreen/of_touchscreen.c |   67 ++++++++++++++++++----------
>  drivers/input/touchscreen/tsc2005.c        |    2 -
>  include/linux/input/touchscreen.h          |    5 +-
>  4 files changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 29d179a..394b1de 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  			     0, tsdata->num_y * 64 - 1, 0, 0);
>  
>  	if (!pdata)
> -		touchscreen_parse_of_params(input);
> +		touchscreen_parse_of_params(input, true);
>  
>  	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
>  	if (error) {
> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> index b82b520..c132624 100644
> --- a/drivers/input/touchscreen/of_touchscreen.c
> +++ b/drivers/input/touchscreen/of_touchscreen.c
> @@ -14,14 +14,22 @@
>  #include <linux/input/mt.h>
>  #include <linux/input/touchscreen.h>
>  
> -static u32 of_get_optional_u32(struct device_node *np,
> -			       const char *property)
> +static bool touchscreen_get_property_u32(struct device_node *np,
> +					 const char *property,
> +					 unsigned int default_value,
> +					 unsigned int *value)
>  {
>  	u32 val = 0;
> +	int error;
>  
> -	of_property_read_u32(np, property, &val);
> +	error = of_property_read_u32(np, property, &val);
> +	if (error) {
> +		*value = default_value;
> +		return false;
> +	}
>  
> -	return val;
> +	*value = val;
> +	return true;

This looks good.

However, of_property_read_u32 already does the right thing here by not
update val if the property is not found.

So I guess you could just do:

*value = default_value;
return of_property_read_u32(np, property, value) ? true : false;

It looks good otherwise.

Thanks!
Maxime
Dmitry Torokhov June 1, 2015, 9:32 p.m. UTC | #5
On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
> Hi Dmitry,
> 
> On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
> > On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > > 
> > > 
> > > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > > 
> > > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > > a fix that pleases everyone.
> > > > > 
> > > > > You should be the one that should suggest fixes, as you broke it in
> > > > > the first place. But clearly you don't understand that.
> > > > 
> > > > You actually never asked for a fix, and went head first calling this
> > > > patch "bad" and asking for nothing but reverting it.
> > > 
> > > Date: Fri, 29 May 2015 21:08:16 +0200
> > > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > > linux-n900
> > > ...
> > > Maxime, can you suggest a fix?
> > 
> > How about we do something like below (it needs a small edt-ft5x06 fixup
> > that I'll send separately). Not tested.
> > 
> > Thanks.
> > 
> > -- 
> > Dmitry
> > 
> > 
> > Input: improve parsing OF parameters for touchscreens
> > 
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > 
> > When applying touchscreen parameters specified in device tree let's make
> > sure we keep whatever setup was done by the driver and not reset the
> > missing values to zero.
> > 
> > Reported-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/touchscreen/edt-ft5x06.c     |    2 -
> >  drivers/input/touchscreen/of_touchscreen.c |   67 ++++++++++++++++++----------
> >  drivers/input/touchscreen/tsc2005.c        |    2 -
> >  include/linux/input/touchscreen.h          |    5 +-
> >  4 files changed, 48 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > index 29d179a..394b1de 100644
> > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > @@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> >  			     0, tsdata->num_y * 64 - 1, 0, 0);
> >  
> >  	if (!pdata)
> > -		touchscreen_parse_of_params(input);
> > +		touchscreen_parse_of_params(input, true);
> >  
> >  	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
> >  	if (error) {
> > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> > index b82b520..c132624 100644
> > --- a/drivers/input/touchscreen/of_touchscreen.c
> > +++ b/drivers/input/touchscreen/of_touchscreen.c
> > @@ -14,14 +14,22 @@
> >  #include <linux/input/mt.h>
> >  #include <linux/input/touchscreen.h>
> >  
> > -static u32 of_get_optional_u32(struct device_node *np,
> > -			       const char *property)
> > +static bool touchscreen_get_property_u32(struct device_node *np,
> > +					 const char *property,
> > +					 unsigned int default_value,
> > +					 unsigned int *value)
> >  {
> >  	u32 val = 0;
> > +	int error;
> >  
> > -	of_property_read_u32(np, property, &val);
> > +	error = of_property_read_u32(np, property, &val);
> > +	if (error) {
> > +		*value = default_value;
> > +		return false;
> > +	}
> >  
> > -	return val;
> > +	*value = val;
> > +	return true;
> 
> This looks good.
> 
> However, of_property_read_u32 already does the right thing here by not
> update val if the property is not found.

I know but it is not documented anywhere (as far as I know) so I'd
rather not rely on the implementation detail that might change in the
future. This is not a hot path so extra assignment should not hurt.

Thanks.
Pavel Machek June 2, 2015, 8:25 a.m. UTC | #6
On Mon 2015-06-01 14:32:13, Dmitry Torokhov wrote:
> On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
> > > On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > > > 
> > > > 
> > > > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > > > 
> > > > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > > > a fix that pleases everyone.
> > > > > > 
> > > > > > You should be the one that should suggest fixes, as you broke it in
> > > > > > the first place. But clearly you don't understand that.
> > > > > 
> > > > > You actually never asked for a fix, and went head first calling this
> > > > > patch "bad" and asking for nothing but reverting it.
> > > > 
> > > > Date: Fri, 29 May 2015 21:08:16 +0200
> > > > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > > > linux-n900
> > > > ...
> > > > Maxime, can you suggest a fix?
> > > 
> > > How about we do something like below (it needs a small edt-ft5x06 fixup
> > > that I'll send separately). Not tested.
> > > 
> > > Thanks.
> > > 
> > > -- 
> > > Dmitry
> > > 
> > > 
> > > Input: improve parsing OF parameters for touchscreens
> > > 
> > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > 
> > > When applying touchscreen parameters specified in device tree let's make
> > > sure we keep whatever setup was done by the driver and not reset the
> > > missing values to zero.
> > > 
> > > Reported-by: Pavel Machek <pavel@ucw.cz>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/touchscreen/edt-ft5x06.c     |    2 -
> > >  drivers/input/touchscreen/of_touchscreen.c |   67 ++++++++++++++++++----------
> > >  drivers/input/touchscreen/tsc2005.c        |    2 -
> > >  include/linux/input/touchscreen.h          |    5 +-
> > >  4 files changed, 48 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index 29d179a..394b1de 100644
> > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > @@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > >  			     0, tsdata->num_y * 64 - 1, 0, 0);
> > >  
> > >  	if (!pdata)
> > > -		touchscreen_parse_of_params(input);
> > > +		touchscreen_parse_of_params(input, true);
> > >  
> > >  	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
> > >  	if (error) {
> > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> > > index b82b520..c132624 100644
> > > --- a/drivers/input/touchscreen/of_touchscreen.c
> > > +++ b/drivers/input/touchscreen/of_touchscreen.c
> > > @@ -14,14 +14,22 @@
> > >  #include <linux/input/mt.h>
> > >  #include <linux/input/touchscreen.h>
> > >  
> > > -static u32 of_get_optional_u32(struct device_node *np,
> > > -			       const char *property)
> > > +static bool touchscreen_get_property_u32(struct device_node *np,
> > > +					 const char *property,
> > > +					 unsigned int default_value,
> > > +					 unsigned int *value)
> > >  {
> > >  	u32 val = 0;
> > > +	int error;
> > >  
> > > -	of_property_read_u32(np, property, &val);
> > > +	error = of_property_read_u32(np, property, &val);
> > > +	if (error) {
> > > +		*value = default_value;
> > > +		return false;
> > > +	}
> > >  
> > > -	return val;
> > > +	*value = val;
> > > +	return true;
> > 
> > This looks good.
> > 
> > However, of_property_read_u32 already does the right thing here by not
> > update val if the property is not found.
> 
> I know but it is not documented anywhere (as far as I know) so I'd
> rather not rely on the implementation detail that might change in the
> future. This is not a hot path so extra assignment should not hurt.

Seriously? Submit a patch documenting it, instead of writing strange
code. I bet everyone and their dog relies on it.
									Pavel
Maxime Ripard June 2, 2015, 9:44 a.m. UTC | #7
On Mon, Jun 01, 2015 at 02:32:13PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
> > Hi Dmitry,
> > 
> > On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
> > > On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > > > 
> > > > 
> > > > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > > > 
> > > > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > > > a fix that pleases everyone.
> > > > > > 
> > > > > > You should be the one that should suggest fixes, as you broke it in
> > > > > > the first place. But clearly you don't understand that.
> > > > > 
> > > > > You actually never asked for a fix, and went head first calling this
> > > > > patch "bad" and asking for nothing but reverting it.
> > > > 
> > > > Date: Fri, 29 May 2015 21:08:16 +0200
> > > > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > > > linux-n900
> > > > ...
> > > > Maxime, can you suggest a fix?
> > > 
> > > How about we do something like below (it needs a small edt-ft5x06 fixup
> > > that I'll send separately). Not tested.
> > > 
> > > Thanks.
> > > 
> > > -- 
> > > Dmitry
> > > 
> > > 
> > > Input: improve parsing OF parameters for touchscreens
> > > 
> > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > 
> > > When applying touchscreen parameters specified in device tree let's make
> > > sure we keep whatever setup was done by the driver and not reset the
> > > missing values to zero.
> > > 
> > > Reported-by: Pavel Machek <pavel@ucw.cz>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/input/touchscreen/edt-ft5x06.c     |    2 -
> > >  drivers/input/touchscreen/of_touchscreen.c |   67 ++++++++++++++++++----------
> > >  drivers/input/touchscreen/tsc2005.c        |    2 -
> > >  include/linux/input/touchscreen.h          |    5 +-
> > >  4 files changed, 48 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > index 29d179a..394b1de 100644
> > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > @@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > >  			     0, tsdata->num_y * 64 - 1, 0, 0);
> > >  
> > >  	if (!pdata)
> > > -		touchscreen_parse_of_params(input);
> > > +		touchscreen_parse_of_params(input, true);
> > >  
> > >  	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
> > >  	if (error) {
> > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> > > index b82b520..c132624 100644
> > > --- a/drivers/input/touchscreen/of_touchscreen.c
> > > +++ b/drivers/input/touchscreen/of_touchscreen.c
> > > @@ -14,14 +14,22 @@
> > >  #include <linux/input/mt.h>
> > >  #include <linux/input/touchscreen.h>
> > >  
> > > -static u32 of_get_optional_u32(struct device_node *np,
> > > -			       const char *property)
> > > +static bool touchscreen_get_property_u32(struct device_node *np,
> > > +					 const char *property,
> > > +					 unsigned int default_value,
> > > +					 unsigned int *value)
> > >  {
> > >  	u32 val = 0;
> > > +	int error;
> > >  
> > > -	of_property_read_u32(np, property, &val);
> > > +	error = of_property_read_u32(np, property, &val);
> > > +	if (error) {
> > > +		*value = default_value;
> > > +		return false;
> > > +	}
> > >  
> > > -	return val;
> > > +	*value = val;
> > > +	return true;
> > 
> > This looks good.
> > 
> > However, of_property_read_u32 already does the right thing here by not
> > update val if the property is not found.
> 
> I know but it is not documented anywhere (as far as I know) so I'd
> rather not rely on the implementation detail that might change in the
> future. This is not a hot path so extra assignment should not hurt.

It is actually: http://lxr.free-electrons.com/source/drivers/of/base.c#L1231

But you're right that it's mostly a cosmetic change.

Maxime
Dmitry Torokhov June 2, 2015, 5:58 p.m. UTC | #8
On Tue, Jun 02, 2015 at 11:44:47AM +0200, Maxime Ripard wrote:
> On Mon, Jun 01, 2015 at 02:32:13PM -0700, Dmitry Torokhov wrote:
> > On Mon, Jun 01, 2015 at 11:22:26PM +0200, Maxime Ripard wrote:
> > > Hi Dmitry,
> > > 
> > > On Mon, Jun 01, 2015 at 10:47:30AM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Jun 01, 2015 at 05:21:11PM +0200, Pavel Machek wrote:
> > > > > 
> > > > > 
> > > > > > > > > The 3eea8b5d68c801fec788b411582b803463834752 is just bad.
> > > > > > > > 
> > > > > > > > You were very welcome to review this patch at the time and/or suggest
> > > > > > > > a fix that pleases everyone.
> > > > > > > 
> > > > > > > You should be the one that should suggest fixes, as you broke it in
> > > > > > > the first place. But clearly you don't understand that.
> > > > > > 
> > > > > > You actually never asked for a fix, and went head first calling this
> > > > > > patch "bad" and asking for nothing but reverting it.
> > > > > 
> > > > > Date: Fri, 29 May 2015 21:08:16 +0200
> > > > > Subject: 4.1 touchscreen regression on n900 -- pinpointed [was Re:
> > > > > linux-n900
> > > > > ...
> > > > > Maxime, can you suggest a fix?
> > > > 
> > > > How about we do something like below (it needs a small edt-ft5x06 fixup
> > > > that I'll send separately). Not tested.
> > > > 
> > > > Thanks.
> > > > 
> > > > -- 
> > > > Dmitry
> > > > 
> > > > 
> > > > Input: improve parsing OF parameters for touchscreens
> > > > 
> > > > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > 
> > > > When applying touchscreen parameters specified in device tree let's make
> > > > sure we keep whatever setup was done by the driver and not reset the
> > > > missing values to zero.
> > > > 
> > > > Reported-by: Pavel Machek <pavel@ucw.cz>
> > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > ---
> > > >  drivers/input/touchscreen/edt-ft5x06.c     |    2 -
> > > >  drivers/input/touchscreen/of_touchscreen.c |   67 ++++++++++++++++++----------
> > > >  drivers/input/touchscreen/tsc2005.c        |    2 -
> > > >  include/linux/input/touchscreen.h          |    5 +-
> > > >  4 files changed, 48 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> > > > index 29d179a..394b1de 100644
> > > > --- a/drivers/input/touchscreen/edt-ft5x06.c
> > > > +++ b/drivers/input/touchscreen/edt-ft5x06.c
> > > > @@ -1041,7 +1041,7 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
> > > >  			     0, tsdata->num_y * 64 - 1, 0, 0);
> > > >  
> > > >  	if (!pdata)
> > > > -		touchscreen_parse_of_params(input);
> > > > +		touchscreen_parse_of_params(input, true);
> > > >  
> > > >  	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
> > > >  	if (error) {
> > > > diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> > > > index b82b520..c132624 100644
> > > > --- a/drivers/input/touchscreen/of_touchscreen.c
> > > > +++ b/drivers/input/touchscreen/of_touchscreen.c
> > > > @@ -14,14 +14,22 @@
> > > >  #include <linux/input/mt.h>
> > > >  #include <linux/input/touchscreen.h>
> > > >  
> > > > -static u32 of_get_optional_u32(struct device_node *np,
> > > > -			       const char *property)
> > > > +static bool touchscreen_get_property_u32(struct device_node *np,
> > > > +					 const char *property,
> > > > +					 unsigned int default_value,
> > > > +					 unsigned int *value)
> > > >  {
> > > >  	u32 val = 0;
> > > > +	int error;
> > > >  
> > > > -	of_property_read_u32(np, property, &val);
> > > > +	error = of_property_read_u32(np, property, &val);
> > > > +	if (error) {
> > > > +		*value = default_value;
> > > > +		return false;
> > > > +	}
> > > >  
> > > > -	return val;
> > > > +	*value = val;
> > > > +	return true;
> > > 
> > > This looks good.
> > > 
> > > However, of_property_read_u32 already does the right thing here by not
> > > update val if the property is not found.
> > 
> > I know but it is not documented anywhere (as far as I know) so I'd
> > rather not rely on the implementation detail that might change in the
> > future. This is not a hot path so extra assignment should not hurt.
> 
> It is actually: http://lxr.free-electrons.com/source/drivers/of/base.c#L1231

OK, fair enough. But not for ACPI properties (and I think we should
convert the parser to device_property_read_xxx() so it is usable
everywhere).

Thanks.

Patch
diff mbox

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 29d179a..394b1de 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1041,7 +1041,7 @@  static int edt_ft5x06_ts_probe(struct i2c_client *client,
 			     0, tsdata->num_y * 64 - 1, 0, 0);
 
 	if (!pdata)
-		touchscreen_parse_of_params(input);
+		touchscreen_parse_of_params(input, true);
 
 	error = input_mt_init_slots(input, MAX_SUPPORT_POINTS, INPUT_MT_DIRECT);
 	if (error) {
diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index b82b520..c132624 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -14,14 +14,22 @@ 
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
 
-static u32 of_get_optional_u32(struct device_node *np,
-			       const char *property)
+static bool touchscreen_get_property_u32(struct device_node *np,
+					 const char *property,
+					 unsigned int default_value,
+					 unsigned int *value)
 {
 	u32 val = 0;
+	int error;
 
-	of_property_read_u32(np, property, &val);
+	error = of_property_read_u32(np, property, &val);
+	if (error) {
+		*value = default_value;
+		return false;
+	}
 
-	return val;
+	*value = val;
+	return true;
 }
 
 static void touchscreen_set_params(struct input_dev *dev,
@@ -54,34 +62,45 @@  static void touchscreen_set_params(struct input_dev *dev,
  * input device accordingly. The function keeps previously setuped default
  * values if no value is specified via DT.
  */
-void touchscreen_parse_of_params(struct input_dev *dev)
+void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
 {
 	struct device_node *np = dev->dev.parent->of_node;
-	u32 maximum, fuzz;
+	unsigned int maximum, fuzz;
+	int axis;
+	bool present;
 
 	input_alloc_absinfo(dev);
 	if (!dev->absinfo)
 		return;
 
-	maximum = of_get_optional_u32(np, "touchscreen-size-x");
-	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-x");
-	if (maximum || fuzz) {
-		touchscreen_set_params(dev, ABS_X, maximum, fuzz);
-		touchscreen_set_params(dev, ABS_MT_POSITION_X, maximum, fuzz);
-	}
+	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
+	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
+						input_abs_get_maximum(axis),
+						&maximum) |
+		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-x",
+						input_abs_get_fuzz(axis),
+						&fuzz);
+	if (data_present)
+		touchscreen_set_params(dev, axis, maximum, fuzz);
 
-	maximum = of_get_optional_u32(np, "touchscreen-size-y");
-	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-y");
-	if (maximum || fuzz) {
-		touchscreen_set_params(dev, ABS_Y, maximum, fuzz);
-		touchscreen_set_params(dev, ABS_MT_POSITION_Y, maximum, fuzz);
-	}
+	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
+	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-y",
+						input_abs_get_maximum(axis),
+						&maximum) |
+		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-y",
+						input_abs_get_fuzz(axis),
+						&fuzz);
+	if (data_present)
+		touchscreen_set_params(dev, axis, maximum, fuzz);
 
-	maximum = of_get_optional_u32(np, "touchscreen-max-pressure");
-	fuzz = of_get_optional_u32(np, "touchscreen-fuzz-pressure");
-	if (maximum || fuzz) {
-		touchscreen_set_params(dev, ABS_PRESSURE, maximum, fuzz);
-		touchscreen_set_params(dev, ABS_MT_PRESSURE, maximum, fuzz);
-	}
+	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
+	data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure",
+						input_abs_get_maximum(axis),
+						&maximum) |
+		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-pressure",
+						input_abs_get_fuzz(axis),
+						&fuzz);
+	if (data_present)
+		touchscreen_set_params(dev, axis, maximum, fuzz);
 }
 EXPORT_SYMBOL(touchscreen_parse_of_params);
diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index 72657c5..d8c025b 100644
--- a/drivers/input/touchscreen/tsc2005.c
+++ b/drivers/input/touchscreen/tsc2005.c
@@ -709,7 +709,7 @@  static int tsc2005_probe(struct spi_device *spi)
 	input_set_abs_params(input_dev, ABS_PRESSURE, 0, max_p, fudge_p, 0);
 
 	if (np)
-		touchscreen_parse_of_params(input_dev);
+		touchscreen_parse_of_params(input_dev, false);
 
 	input_dev->open = tsc2005_open;
 	input_dev->close = tsc2005_close;
diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h
index 08a5ef6..eecc9ea 100644
--- a/include/linux/input/touchscreen.h
+++ b/include/linux/input/touchscreen.h
@@ -12,9 +12,10 @@ 
 #include <linux/input.h>
 
 #ifdef CONFIG_OF
-void touchscreen_parse_of_params(struct input_dev *dev);
+void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch);
 #else
-static inline void touchscreen_parse_of_params(struct input_dev *dev)
+static inline void touchscreen_parse_of_params(struct input_dev *dev,
+					       bool multitouch)
 {
 }
 #endif