[PATCH 1/3] Input: of_touchscreen - always issue warning if axis is not set up

[PATCH 1/3] Input: of_touchscreen - always issue warning if axis is not set up

[Dmitry Torokhov]

Do issue warning about axis that is present in device tree but not specified
by the driver even in case of multi-touch axis as callers now tell us if they
expect multi-touch data or not.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/of_touchscreen.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index 806cd0a..759cf4b 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -39,13 +39,9 @@ static void touchscreen_set_params(struct input_dev *dev,
 	struct input_absinfo *absinfo;
 
 	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");
+		dev_warn(&dev->dev,
+			 "DT specifies parameters but the axis %lu is not set up\n",
+			 axis);
 		return;
 	}
 
-- 
2.4.3.573.g4eafbef

[PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Dmitry Torokhov]

The binding specification says that "touchscreen-size-x" and "-y" specify
horizontal and vertical resolution of the touchscreen and therefore maximum
absolute coordinates should be reduced by 1 since we are starting with 0.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index 759cf4b..50bc0f2 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
 
 	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
 	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
-						input_abs_get_max(dev, axis),
+						input_abs_get_max(dev,
+								  axis) + 1,
 						&maximum) |
 		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-x",
 						input_abs_get_fuzz(dev, axis),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(dev, axis, maximum, fuzz);
+		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
 
 	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
 	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-y",
-						input_abs_get_max(dev, axis),
+						input_abs_get_max(dev,
+								  axis) + 1,
 						&maximum) |
 		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-y",
 						input_abs_get_fuzz(dev, axis),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(dev, axis, maximum, fuzz);
+		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
 
 	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
 	data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure",
-- 
2.4.3.573.g4eafbef

[PATCH 3/3] Input: of_touchscreen - switch to using device properties

[Dmitry Torokhov]

Let's switch form OF to device properties so that common parsing code could
work not only on device tree but also on ACPI-based platforms.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/Kconfig          |  4 +--
 drivers/input/touchscreen/Makefile         |  2 +-
 drivers/input/touchscreen/edt-ft5x06.c     |  2 +-
 drivers/input/touchscreen/of_touchscreen.c | 56 ++++++++++++++++--------------
 drivers/input/touchscreen/tsc2005.c        |  2 +-
 include/linux/input/touchscreen.h          | 11 ++----
 6 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 2dc9d62..38d0f99 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -11,9 +11,9 @@ menuconfig INPUT_TOUCHSCREEN
 
 if INPUT_TOUCHSCREEN
 
-config OF_TOUCHSCREEN
+config TOUCHSCREEN_PROPERTIES
 	def_tristate INPUT
-	depends on INPUT && OF
+	depends on INPUT
 
 config TOUCHSCREEN_88PM860X
 	tristate "Marvell 88PM860x touchscreen"
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index fa3d33b..c85aae2 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -6,7 +6,7 @@
 
 wm97xx-ts-y := wm97xx-core.o
 
-obj-$(CONFIG_OF_TOUCHSCREEN)		+= of_touchscreen.o
+obj-$(CONFIG_TOUCHSCREEN_PROPERTIES)	+= of_touchscreen.o
 obj-$(CONFIG_TOUCHSCREEN_88PM860X)	+= 88pm860x-ts.o
 obj-$(CONFIG_TOUCHSCREEN_AD7877)	+= ad7877.o
 obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 394b1de..8f8f319 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, true);
+		touchscreen_parse_properties(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 50bc0f2..bb6f2fe 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -9,12 +9,12 @@
  *
  */
 
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/input.h>
 #include <linux/input/mt.h>
 #include <linux/input/touchscreen.h>
 
-static bool touchscreen_get_prop_u32(struct device_node *np,
+static bool touchscreen_get_prop_u32(struct device *dev,
 				     const char *property,
 				     unsigned int default_value,
 				     unsigned int *value)
@@ -22,7 +22,7 @@ static bool touchscreen_get_prop_u32(struct device_node *np,
 	u32 val;
 	int error;
 
-	error = of_property_read_u32(np, property, &val);
+	error = device_property_read_u32(dev, property, &val);
 	if (error) {
 		*value = default_value;
 		return false;
@@ -51,54 +51,58 @@ static void touchscreen_set_params(struct input_dev *dev,
 }
 
 /**
- * touchscreen_parse_of_params - parse common touchscreen DT properties
- * @dev: device that should be parsed
+ * touchscreen_parse_properties - parse common touchscreen DT properties
+ * @input: input device that should be parsed
+ * @multitouch: specifies whether parsed properties should be applied to
+ *	single-touch or multi-touch axes
  *
  * This function parses common DT properties for touchscreens and setups the
- * input device accordingly. The function keeps previously setuped default
+ * input device accordingly. The function keeps previously set up default
  * values if no value is specified via DT.
  */
-void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
+void touchscreen_parse_properties(struct input_dev *input, bool multitouch)
 {
-	struct device_node *np = dev->dev.parent->of_node;
+	struct device *dev = input->dev.parent;
 	unsigned int axis;
 	unsigned int maximum, fuzz;
 	bool data_present;
 
-	input_alloc_absinfo(dev);
-	if (!dev->absinfo)
+	input_alloc_absinfo(input);
+	if (!input->absinfo)
 		return;
 
 	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
-	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
-						input_abs_get_max(dev,
+	data_present = touchscreen_get_prop_u32(dev, "touchscreen-size-x",
+						input_abs_get_max(input,
 								  axis) + 1,
 						&maximum) |
-		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-x",
-						input_abs_get_fuzz(dev, axis),
+		       touchscreen_get_prop_u32(dev, "touchscreen-fuzz-x",
+						input_abs_get_fuzz(input, axis),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
+		touchscreen_set_params(input, axis, maximum - 1, fuzz);
 
 	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
-	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-y",
-						input_abs_get_max(dev,
+	data_present = touchscreen_get_prop_u32(dev, "touchscreen-size-y",
+						input_abs_get_max(input,
 								  axis) + 1,
 						&maximum) |
-		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-y",
-						input_abs_get_fuzz(dev, axis),
+		       touchscreen_get_prop_u32(dev, "touchscreen-fuzz-y",
+						input_abs_get_fuzz(input, axis),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
+		touchscreen_set_params(input, axis, maximum - 1, fuzz);
 
 	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
-	data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure",
-						input_abs_get_max(dev, axis),
+	data_present = touchscreen_get_prop_u32(dev,
+						"touchscreen-max-pressure",
+						input_abs_get_max(input, axis),
 						&maximum) |
-		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-pressure",
-						input_abs_get_fuzz(dev, axis),
+		       touchscreen_get_prop_u32(dev,
+						"touchscreen-fuzz-pressure",
+						input_abs_get_fuzz(input, axis),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(dev, axis, maximum, fuzz);
+		touchscreen_set_params(input, axis, maximum, fuzz);
 }
-EXPORT_SYMBOL(touchscreen_parse_of_params);
+EXPORT_SYMBOL(touchscreen_parse_properties);
diff --git a/drivers/input/touchscreen/tsc2005.c b/drivers/input/touchscreen/tsc2005.c
index d8c025b..aaf94752 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, false);
+		touchscreen_parse_properties(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 eecc9ea..c91e137 100644
--- a/include/linux/input/touchscreen.h
+++ b/include/linux/input/touchscreen.h
@@ -9,15 +9,8 @@
 #ifndef _TOUCHSCREEN_H
 #define _TOUCHSCREEN_H
 
-#include <linux/input.h>
+struct input_dev;
 
-#ifdef CONFIG_OF
-void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch);
-#else
-static inline void touchscreen_parse_of_params(struct input_dev *dev,
-					       bool multitouch)
-{
-}
-#endif
+void touchscreen_parse_properties(struct input_dev *dev, bool multitouch);
 
 #endif
-- 
2.4.3.573.g4eafbef

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Roger Quadros]

Hi Dmitry,

On 07/07/15 03:27, Dmitry Torokhov wrote:
> The binding specification says that "touchscreen-size-x" and "-y" specify
> horizontal and vertical resolution of the touchscreen and therefore maximum
> absolute coordinates should be reduced by 1 since we are starting with 0.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>   drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> index 759cf4b..50bc0f2 100644
> --- a/drivers/input/touchscreen/of_touchscreen.c
> +++ b/drivers/input/touchscreen/of_touchscreen.c
> @@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
>
>   	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
>   	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
> -						input_abs_get_max(dev, axis),
> +						input_abs_get_max(dev,
> +								  axis) + 1,

Why do we need to pass default_value to touchscreen_get_prop_u32()?
If the property doesn't exist we are not updating the parameter anyway 
right?

>   						&maximum) |
>   		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-x",
>   						input_abs_get_fuzz(dev, axis),
>   						&fuzz);
>   	if (data_present)
> -		touchscreen_set_params(dev, axis, maximum, fuzz);
> +		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
>
>   	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
>   	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-y",
> -						input_abs_get_max(dev, axis),
> +						input_abs_get_max(dev,
> +								  axis) + 1,
>   						&maximum) |
>   		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-y",
>   						input_abs_get_fuzz(dev, axis),
>   						&fuzz);
>   	if (data_present)
> -		touchscreen_set_params(dev, axis, maximum, fuzz);
> +		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
>
>   	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
>   	data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure",
>

cheers,
-roger

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Dmitry Torokhov]

Hi Roger,

On Tue, Jul 07, 2015 at 12:37:31PM +0300, Roger Quadros wrote:
> Hi Dmitry,
> 
> On 07/07/15 03:27, Dmitry Torokhov wrote:
> >The binding specification says that "touchscreen-size-x" and "-y" specify
> >horizontal and vertical resolution of the touchscreen and therefore maximum
> >absolute coordinates should be reduced by 1 since we are starting with 0.
> >
> >Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >---
> >  drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> >index 759cf4b..50bc0f2 100644
> >--- a/drivers/input/touchscreen/of_touchscreen.c
> >+++ b/drivers/input/touchscreen/of_touchscreen.c
> >@@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
> >
> >  	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
> >  	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
> >-						input_abs_get_max(dev, axis),
> >+						input_abs_get_max(dev,
> >+								  axis) + 1,
> 
> Why do we need to pass default_value to touchscreen_get_prop_u32()?
> If the property doesn't exist we are not updating the parameter
> anyway right?

The binding can specify max, fuzz, both, or neither. If only one is
specified we do not want to "undo" whatever the driver did (for example
tsc2005 sets up the default maximums before trying to parse OF), so we
fetch the current value and pass it on as default one.

> 
> >  						&maximum) |
> >  		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-x",
> >  						input_abs_get_fuzz(dev, axis),
> >  						&fuzz);
> >  	if (data_present)
> >-		touchscreen_set_params(dev, axis, maximum, fuzz);
> >+		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
> >
> >  	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
> >  	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-y",
> >-						input_abs_get_max(dev, axis),
> >+						input_abs_get_max(dev,
> >+								  axis) + 1,
> >  						&maximum) |
> >  		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-y",
> >  						input_abs_get_fuzz(dev, axis),
> >  						&fuzz);
> >  	if (data_present)
> >-		touchscreen_set_params(dev, axis, maximum, fuzz);
> >+		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
> >
> >  	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
> >  	data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure",
> >
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Roger Quadros]

Dmitry,

On 07/07/15 19:25, Dmitry Torokhov wrote:
> Hi Roger,
>
> On Tue, Jul 07, 2015 at 12:37:31PM +0300, Roger Quadros wrote:
>> Hi Dmitry,
>>
>> On 07/07/15 03:27, Dmitry Torokhov wrote:
>>> The binding specification says that "touchscreen-size-x" and "-y" specify
>>> horizontal and vertical resolution of the touchscreen and therefore maximum
>>> absolute coordinates should be reduced by 1 since we are starting with 0.
>>>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> ---
>>>   drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
>>> index 759cf4b..50bc0f2 100644
>>> --- a/drivers/input/touchscreen/of_touchscreen.c
>>> +++ b/drivers/input/touchscreen/of_touchscreen.c
>>> @@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
>>>
>>>   	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
>>>   	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
>>> -						input_abs_get_max(dev, axis),
>>> +						input_abs_get_max(dev,
>>> +								  axis) + 1,
>>
>> Why do we need to pass default_value to touchscreen_get_prop_u32()?
>> If the property doesn't exist we are not updating the parameter
>> anyway right?
>
> The binding can specify max, fuzz, both, or neither. If only one is
> specified we do not want to "undo" whatever the driver did (for example

why not? At least that is not what touchscreen_parse_properties() does.
It will update the touchscreen params if any one of the DT property is 
present.

> tsc2005 sets up the default maximums before trying to parse OF), so we
> fetch the current value and pass it on as default one.

This is what would happen in tsc2005 case

-driver first sets ABS_X_max to 0xfff and ABS_Y_max to 0xfff.
-calls touchscreen_parse_properties()
-which calls touchscreen_get_prop_u32(dev, "touchscreen-size-x", 
0xfff+1, &maximum)
-if DT properties are missing, touchscreen_get_prop_u32() changes
ABS_X_max to 0x1000 and returns false.
- as data_present is false we don't do a -1 and so we have ABS_X_max 
changed to 0x1000.

This doesn't look right.

So IMO default_value parameter can be removed from 
touchscreen_get_prop_u32() to make things simpler.

If the property doesn't exist it will not change *value and return false.

cheers,
-roger
-
>
>>
>>>   						&maximum) |
>>>   		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-x",
>>>   						input_abs_get_fuzz(dev, axis),
>>>   						&fuzz);
>>>   	if (data_present)
>>> -		touchscreen_set_params(dev, axis, maximum, fuzz);
>>> +		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
>>>
>>>   	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
>>>   	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-y",
>>> -						input_abs_get_max(dev, axis),
>>> +						input_abs_get_max(dev,
>>> +								  axis) + 1,
>>>   						&maximum) |
>>>   		       touchscreen_get_prop_u32(np, "touchscreen-fuzz-y",
>>>   						input_abs_get_fuzz(dev, axis),
>>>   						&fuzz);
>>>   	if (data_present)
>>> -		touchscreen_set_params(dev, axis, maximum, fuzz);
>>> +		touchscreen_set_params(dev, axis, maximum - 1, fuzz);
>>>
>>>   	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
>>>   	data_present = touchscreen_get_prop_u32(np, "touchscreen-max-pressure",
>>>
>>
>
> Thanks.
>

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Dmitry Torokhov]

On Wed, Jul 08, 2015 at 10:59:04AM +0300, Roger Quadros wrote:
> Dmitry,
> 
> On 07/07/15 19:25, Dmitry Torokhov wrote:
> >Hi Roger,
> >
> >On Tue, Jul 07, 2015 at 12:37:31PM +0300, Roger Quadros wrote:
> >>Hi Dmitry,
> >>
> >>On 07/07/15 03:27, Dmitry Torokhov wrote:
> >>>The binding specification says that "touchscreen-size-x" and "-y" specify
> >>>horizontal and vertical resolution of the touchscreen and therefore maximum
> >>>absolute coordinates should be reduced by 1 since we are starting with 0.
> >>>
> >>>Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>>---
> >>>  drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
> >>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>
> >>>diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> >>>index 759cf4b..50bc0f2 100644
> >>>--- a/drivers/input/touchscreen/of_touchscreen.c
> >>>+++ b/drivers/input/touchscreen/of_touchscreen.c
> >>>@@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
> >>>
> >>>  	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
> >>>  	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
> >>>-						input_abs_get_max(dev, axis),
> >>>+						input_abs_get_max(dev,
> >>>+								  axis) + 1,
> >>
> >>Why do we need to pass default_value to touchscreen_get_prop_u32()?
> >>If the property doesn't exist we are not updating the parameter
> >>anyway right?
> >
> >The binding can specify max, fuzz, both, or neither. If only one is
> >specified we do not want to "undo" whatever the driver did (for example
> 
> why not? At least that is not what touchscreen_parse_properties() does.
> It will update the touchscreen params if any one of the DT property
> is present.

Yes, but while keeping the old properties (max, fuzz) intact if they
have not been specified in dts. That is why we are passing default value
to touchscreen_get_prop_u32.

> 
> >tsc2005 sets up the default maximums before trying to parse OF), so we
> >fetch the current value and pass it on as default one.
> 
> This is what would happen in tsc2005 case
> 
> -driver first sets ABS_X_max to 0xfff and ABS_Y_max to 0xfff.
> -calls touchscreen_parse_properties()
> -which calls touchscreen_get_prop_u32(dev, "touchscreen-size-x",
> 0xfff+1, &maximum)
> -if DT properties are missing, touchscreen_get_prop_u32() changes
> ABS_X_max to 0x1000 and returns false.
> - as data_present is false we don't do a -1 and so we have ABS_X_max
> changed to 0x1000.

If data_present is false we do not call input_set_abs_params() at all
ans so nothing changes in input device settings.  Now, if fuzz would be
set up (but size was not) we would be calling input_set_abs_params(),
but given that we subtract 1 from maximum returned by
touchscreen_get_prop_u32() we'd be setting input_set_abs_params(input,
ABS_X, 0, 0x1000 - 1, fuzz) thus preserving 0xfff value set by the
driver.

> 
> This doesn't look right.
> 
> So IMO default_value parameter can be removed from
> touchscreen_get_prop_u32() to make things simpler.
> 
> If the property doesn't exist it will not change *value and return false.

But it will change if one of the properties set. For example, if you set
fuzz but not size.

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Roger Quadros]

Dmitry,

On 08/07/15 18:08, Dmitry Torokhov wrote:
> On Wed, Jul 08, 2015 at 10:59:04AM +0300, Roger Quadros wrote:
>> Dmitry,
>>
>> On 07/07/15 19:25, Dmitry Torokhov wrote:
>>> Hi Roger,
>>>
>>> On Tue, Jul 07, 2015 at 12:37:31PM +0300, Roger Quadros wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 07/07/15 03:27, Dmitry Torokhov wrote:
>>>>> The binding specification says that "touchscreen-size-x" and "-y" specify
>>>>> horizontal and vertical resolution of the touchscreen and therefore maximum
>>>>> absolute coordinates should be reduced by 1 since we are starting with 0.
>>>>>
>>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>> ---
>>>>>  drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
>>>>> index 759cf4b..50bc0f2 100644
>>>>> --- a/drivers/input/touchscreen/of_touchscreen.c
>>>>> +++ b/drivers/input/touchscreen/of_touchscreen.c
>>>>> @@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
>>>>>
>>>>>  	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
>>>>>  	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
>>>>> -						input_abs_get_max(dev, axis),
>>>>> +						input_abs_get_max(dev,
>>>>> +								  axis) + 1,
>>>>
>>>> Why do we need to pass default_value to touchscreen_get_prop_u32()?
>>>> If the property doesn't exist we are not updating the parameter
>>>> anyway right?
>>>
>>> The binding can specify max, fuzz, both, or neither. If only one is
>>> specified we do not want to "undo" whatever the driver did (for example
>>
>> why not? At least that is not what touchscreen_parse_properties() does.
>> It will update the touchscreen params if any one of the DT property
>> is present.
> 
> Yes, but while keeping the old properties (max, fuzz) intact if they
> have not been specified in dts. That is why we are passing default value
> to touchscreen_get_prop_u32.
> 
>>
>>> tsc2005 sets up the default maximums before trying to parse OF), so we
>>> fetch the current value and pass it on as default one.
>>
>> This is what would happen in tsc2005 case
>>
>> -driver first sets ABS_X_max to 0xfff and ABS_Y_max to 0xfff.
>> -calls touchscreen_parse_properties()
>> -which calls touchscreen_get_prop_u32(dev, "touchscreen-size-x",
>> 0xfff+1, &maximum)
>> -if DT properties are missing, touchscreen_get_prop_u32() changes
>> ABS_X_max to 0x1000 and returns false.
>> - as data_present is false we don't do a -1 and so we have ABS_X_max
>> changed to 0x1000.
> 
> If data_present is false we do not call input_set_abs_params() at all
> ans so nothing changes in input device settings.  Now, if fuzz would be

You are right. I missed the fact that we don't call input_set_abs_params()
in data_present is false case hence my confusion.

> set up (but size was not) we would be calling input_set_abs_params(),
> but given that we subtract 1 from maximum returned by
> touchscreen_get_prop_u32() we'd be setting input_set_abs_params(input,
> ABS_X, 0, 0x1000 - 1, fuzz) thus preserving 0xfff value set by the
> driver.
> 
>>
>> This doesn't look right.
>>
>> So IMO default_value parameter can be removed from
>> touchscreen_get_prop_u32() to make things simpler.
>>
>> If the property doesn't exist it will not change *value and return false.
> 
> But it will change if one of the properties set. For example, if you set
> fuzz but not size.

Yes. Thanks for explaining :).

cheers,
-roger

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Dmitry Torokhov]

On Thu, Jul 09, 2015 at 11:32:11AM +0300, Roger Quadros wrote:
> Dmitry,
> 
> On 08/07/15 18:08, Dmitry Torokhov wrote:
> > On Wed, Jul 08, 2015 at 10:59:04AM +0300, Roger Quadros wrote:
> >> Dmitry,
> >>
> >> On 07/07/15 19:25, Dmitry Torokhov wrote:
> >>> Hi Roger,
> >>>
> >>> On Tue, Jul 07, 2015 at 12:37:31PM +0300, Roger Quadros wrote:
> >>>> Hi Dmitry,
> >>>>
> >>>> On 07/07/15 03:27, Dmitry Torokhov wrote:
> >>>>> The binding specification says that "touchscreen-size-x" and "-y" specify
> >>>>> horizontal and vertical resolution of the touchscreen and therefore maximum
> >>>>> absolute coordinates should be reduced by 1 since we are starting with 0.
> >>>>>
> >>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>>>> ---
> >>>>>  drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
> >>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> >>>>> index 759cf4b..50bc0f2 100644
> >>>>> --- a/drivers/input/touchscreen/of_touchscreen.c
> >>>>> +++ b/drivers/input/touchscreen/of_touchscreen.c
> >>>>> @@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
> >>>>>
> >>>>>  	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
> >>>>>  	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
> >>>>> -						input_abs_get_max(dev, axis),
> >>>>> +						input_abs_get_max(dev,
> >>>>> +								  axis) + 1,
> >>>>
> >>>> Why do we need to pass default_value to touchscreen_get_prop_u32()?
> >>>> If the property doesn't exist we are not updating the parameter
> >>>> anyway right?
> >>>
> >>> The binding can specify max, fuzz, both, or neither. If only one is
> >>> specified we do not want to "undo" whatever the driver did (for example
> >>
> >> why not? At least that is not what touchscreen_parse_properties() does.
> >> It will update the touchscreen params if any one of the DT property
> >> is present.
> > 
> > Yes, but while keeping the old properties (max, fuzz) intact if they
> > have not been specified in dts. That is why we are passing default value
> > to touchscreen_get_prop_u32.
> > 
> >>
> >>> tsc2005 sets up the default maximums before trying to parse OF), so we
> >>> fetch the current value and pass it on as default one.
> >>
> >> This is what would happen in tsc2005 case
> >>
> >> -driver first sets ABS_X_max to 0xfff and ABS_Y_max to 0xfff.
> >> -calls touchscreen_parse_properties()
> >> -which calls touchscreen_get_prop_u32(dev, "touchscreen-size-x",
> >> 0xfff+1, &maximum)
> >> -if DT properties are missing, touchscreen_get_prop_u32() changes
> >> ABS_X_max to 0x1000 and returns false.
> >> - as data_present is false we don't do a -1 and so we have ABS_X_max
> >> changed to 0x1000.
> > 
> > If data_present is false we do not call input_set_abs_params() at all
> > ans so nothing changes in input device settings.  Now, if fuzz would be
> 
> You are right. I missed the fact that we don't call input_set_abs_params()
> in data_present is false case hence my confusion.
> 
> > set up (but size was not) we would be calling input_set_abs_params(),
> > but given that we subtract 1 from maximum returned by
> > touchscreen_get_prop_u32() we'd be setting input_set_abs_params(input,
> > ABS_X, 0, 0x1000 - 1, fuzz) thus preserving 0xfff value set by the
> > driver.
> > 
> >>
> >> This doesn't look right.
> >>
> >> So IMO default_value parameter can be removed from
> >> touchscreen_get_prop_u32() to make things simpler.
> >>
> >> If the property doesn't exist it will not change *value and return false.
> > 
> > But it will change if one of the properties set. For example, if you set
> > fuzz but not size.
> 
> Yes. Thanks for explaining :).

No problem. Can I put you down as "reviewed-by" on the of_touchscreen
changes then?

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: of_touchscreen - fix setting max values on X/Y axis

[Roger Quadros]

On 09/07/15 21:16, Dmitry Torokhov wrote:
> On Thu, Jul 09, 2015 at 11:32:11AM +0300, Roger Quadros wrote:
>> Dmitry,
>>
>> On 08/07/15 18:08, Dmitry Torokhov wrote:
>>> On Wed, Jul 08, 2015 at 10:59:04AM +0300, Roger Quadros wrote:
>>>> Dmitry,
>>>>
>>>> On 07/07/15 19:25, Dmitry Torokhov wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On Tue, Jul 07, 2015 at 12:37:31PM +0300, Roger Quadros wrote:
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 07/07/15 03:27, Dmitry Torokhov wrote:
>>>>>>> The binding specification says that "touchscreen-size-x" and "-y" specify
>>>>>>> horizontal and vertical resolution of the touchscreen and therefore maximum
>>>>>>> absolute coordinates should be reduced by 1 since we are starting with 0.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/input/touchscreen/of_touchscreen.c | 10 ++++++----
>>>>>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
>>>>>>> index 759cf4b..50bc0f2 100644
>>>>>>> --- a/drivers/input/touchscreen/of_touchscreen.c
>>>>>>> +++ b/drivers/input/touchscreen/of_touchscreen.c
>>>>>>> @@ -71,23 +71,25 @@ void touchscreen_parse_of_params(struct input_dev *dev, bool multitouch)
>>>>>>>
>>>>>>>  	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
>>>>>>>  	data_present = touchscreen_get_prop_u32(np, "touchscreen-size-x",
>>>>>>> -						input_abs_get_max(dev, axis),
>>>>>>> +						input_abs_get_max(dev,
>>>>>>> +								  axis) + 1,
>>>>>>
>>>>>> Why do we need to pass default_value to touchscreen_get_prop_u32()?
>>>>>> If the property doesn't exist we are not updating the parameter
>>>>>> anyway right?
>>>>>
>>>>> The binding can specify max, fuzz, both, or neither. If only one is
>>>>> specified we do not want to "undo" whatever the driver did (for example
>>>>
>>>> why not? At least that is not what touchscreen_parse_properties() does.
>>>> It will update the touchscreen params if any one of the DT property
>>>> is present.
>>>
>>> Yes, but while keeping the old properties (max, fuzz) intact if they
>>> have not been specified in dts. That is why we are passing default value
>>> to touchscreen_get_prop_u32.
>>>
>>>>
>>>>> tsc2005 sets up the default maximums before trying to parse OF), so we
>>>>> fetch the current value and pass it on as default one.
>>>>
>>>> This is what would happen in tsc2005 case
>>>>
>>>> -driver first sets ABS_X_max to 0xfff and ABS_Y_max to 0xfff.
>>>> -calls touchscreen_parse_properties()
>>>> -which calls touchscreen_get_prop_u32(dev, "touchscreen-size-x",
>>>> 0xfff+1, &maximum)
>>>> -if DT properties are missing, touchscreen_get_prop_u32() changes
>>>> ABS_X_max to 0x1000 and returns false.
>>>> - as data_present is false we don't do a -1 and so we have ABS_X_max
>>>> changed to 0x1000.
>>>
>>> If data_present is false we do not call input_set_abs_params() at all
>>> ans so nothing changes in input device settings.  Now, if fuzz would be
>>
>> You are right. I missed the fact that we don't call input_set_abs_params()
>> in data_present is false case hence my confusion.
>>
>>> set up (but size was not) we would be calling input_set_abs_params(),
>>> but given that we subtract 1 from maximum returned by
>>> touchscreen_get_prop_u32() we'd be setting input_set_abs_params(input,
>>> ABS_X, 0, 0x1000 - 1, fuzz) thus preserving 0xfff value set by the
>>> driver.
>>>
>>>>
>>>> This doesn't look right.
>>>>
>>>> So IMO default_value parameter can be removed from
>>>> touchscreen_get_prop_u32() to make things simpler.
>>>>
>>>> If the property doesn't exist it will not change *value and return false.
>>>
>>> But it will change if one of the properties set. For example, if you set
>>> fuzz but not size.
>>
>> Yes. Thanks for explaining :).
> 
> No problem. Can I put you down as "reviewed-by" on the of_touchscreen
> changes then?

Yes please.
For all 3 patches.

Reviewed-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger