All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis
@ 2020-03-03 18:09 Andy Shevchenko
  2020-03-03 18:09 ` [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set() Andy Shevchenko
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-03 18:09 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input, Henrik Rydberg; +Cc: Andy Shevchenko

The 'axis + 1' calculation is implicit and potentially error prone.
Moreover, few lines before the axis is set explicitly for both X and Y.

Do the same when retrieving different properties for X and Y.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/touchscreen/of_touchscreen.c | 35 +++++++++++-----------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
index e16ec4c7043a..97342e14b4f1 100644
--- a/drivers/input/touchscreen/of_touchscreen.c
+++ b/drivers/input/touchscreen/of_touchscreen.c
@@ -66,7 +66,7 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 {
 	struct device *dev = input->dev.parent;
 	struct input_absinfo *absinfo;
-	unsigned int axis;
+	unsigned int axis, axis_x, axis_y;
 	unsigned int minimum, maximum, fuzz;
 	bool data_present;
 
@@ -74,33 +74,34 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	if (!input->absinfo)
 		return;
 
-	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
+	axis_x = multitouch ? ABS_MT_POSITION_X : ABS_X;
+	axis_y = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
+
 	data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
-						input_abs_get_min(input, axis),
+						input_abs_get_min(input, axis_x),
 						&minimum) |
 		       touchscreen_get_prop_u32(dev, "touchscreen-size-x",
 						input_abs_get_max(input,
-								  axis) + 1,
+								  axis_x) + 1,
 						&maximum) |
 		       touchscreen_get_prop_u32(dev, "touchscreen-fuzz-x",
-						input_abs_get_fuzz(input, axis),
+						input_abs_get_fuzz(input, axis_x),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(input, axis, minimum, maximum - 1, fuzz);
+		touchscreen_set_params(input, axis_x, minimum, maximum - 1, fuzz);
 
-	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
 	data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
-						input_abs_get_min(input, axis),
+						input_abs_get_min(input, axis_y),
 						&minimum) |
 		       touchscreen_get_prop_u32(dev, "touchscreen-size-y",
 						input_abs_get_max(input,
-								  axis) + 1,
+								  axis_y) + 1,
 						&maximum) |
 		       touchscreen_get_prop_u32(dev, "touchscreen-fuzz-y",
-						input_abs_get_fuzz(input, axis),
+						input_abs_get_fuzz(input, axis_y),
 						&fuzz);
 	if (data_present)
-		touchscreen_set_params(input, axis, minimum, maximum - 1, fuzz);
+		touchscreen_set_params(input, axis_y, minimum, maximum - 1, fuzz);
 
 	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
 	data_present = touchscreen_get_prop_u32(dev,
@@ -117,15 +118,13 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	if (!prop)
 		return;
 
-	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
-
-	prop->max_x = input_abs_get_max(input, axis);
-	prop->max_y = input_abs_get_max(input, axis + 1);
+	prop->max_x = input_abs_get_max(input, axis_x);
+	prop->max_y = input_abs_get_max(input, axis_y);
 
 	prop->invert_x =
 		device_property_read_bool(dev, "touchscreen-inverted-x");
 	if (prop->invert_x) {
-		absinfo = &input->absinfo[axis];
+		absinfo = &input->absinfo[axis_x];
 		absinfo->maximum -= absinfo->minimum;
 		absinfo->minimum = 0;
 	}
@@ -133,7 +132,7 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	prop->invert_y =
 		device_property_read_bool(dev, "touchscreen-inverted-y");
 	if (prop->invert_y) {
-		absinfo = &input->absinfo[axis + 1];
+		absinfo = &input->absinfo[axis_y];
 		absinfo->maximum -= absinfo->minimum;
 		absinfo->minimum = 0;
 	}
@@ -141,7 +140,7 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
 	prop->swap_x_y =
 		device_property_read_bool(dev, "touchscreen-swapped-x-y");
 	if (prop->swap_x_y)
-		swap(input->absinfo[axis], input->absinfo[axis + 1]);
+		swap(input->absinfo[axis_x], input->absinfo[axis_y]);
 }
 EXPORT_SYMBOL(touchscreen_parse_properties);
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set()
  2020-03-03 18:09 [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Andy Shevchenko
@ 2020-03-03 18:09 ` Andy Shevchenko
  2020-03-07  1:11   ` Dmitry Torokhov
  2020-03-03 18:09 ` [PATCH v1 3/5] Input: edt-ft5x06 - use U16_MAX instead of -1 Andy Shevchenko
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-03 18:09 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input, Henrik Rydberg; +Cc: Andy Shevchenko

For better reading unroll nested conditions to simple if-else-if.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index d2587724c52a..cb67104c6934 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -666,10 +666,10 @@ static int edt_ft5x06_debugfs_mode_set(void *data, u64 mode)
 
 	mutex_lock(&tsdata->mutex);
 
-	if (mode != tsdata->factory_mode) {
-		retval = mode ? edt_ft5x06_factory_mode(tsdata) :
-				edt_ft5x06_work_mode(tsdata);
-	}
+	if (mode && !tsdata->factory_mode)
+		retval = edt_ft5x06_factory_mode(tsdata);
+	else if (!mode && tsdata->factory_mode)
+		retval = edt_ft5x06_work_mode(tsdata);
 
 	mutex_unlock(&tsdata->mutex);
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 3/5] Input: edt-ft5x06 - use U16_MAX instead of -1
  2020-03-03 18:09 [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Andy Shevchenko
  2020-03-03 18:09 ` [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set() Andy Shevchenko
@ 2020-03-03 18:09 ` Andy Shevchenko
  2020-05-11 10:09   ` Andy Shevchenko
  2020-03-03 18:09 ` [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory Andy Shevchenko
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-03 18:09 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input, Henrik Rydberg; +Cc: Andy Shevchenko

Explicitly show what the value we supply for the touchscreen resolution
when it can't be detected. -1 is hard to compare with when unsigned short
type is in use. The change will help to avoid signed vs. unsigned error
prone comparisons.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index cb67104c6934..a05c6b597d43 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -23,6 +23,7 @@
 #include <linux/input/touchscreen.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
+#include <linux/limits.h>
 #include <linux/module.h>
 #include <linux/ratelimit.h>
 #include <linux/regulator/consumer.h>
@@ -983,8 +984,8 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
 		tsdata->num_y = edt_ft5x06_register_read(tsdata,
 							 reg_addr->reg_num_y);
 	} else {
-		tsdata->num_x = -1;
-		tsdata->num_y = -1;
+		tsdata->num_x = U16_MAX;
+		tsdata->num_y = U16_MAX;
 	}
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory
  2020-03-03 18:09 [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Andy Shevchenko
  2020-03-03 18:09 ` [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set() Andy Shevchenko
  2020-03-03 18:09 ` [PATCH v1 3/5] Input: edt-ft5x06 - use U16_MAX instead of -1 Andy Shevchenko
@ 2020-03-03 18:09 ` Andy Shevchenko
  2020-03-07  1:08   ` Dmitry Torokhov
  2020-03-03 18:09 ` [PATCH v1 5/5] Input: edt-ft5x06 - allocate buffer once for debugging Andy Shevchenko
  2020-03-07  1:12 ` [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Dmitry Torokhov
  4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-03 18:09 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input, Henrik Rydberg; +Cc: Andy Shevchenko

When mode switch happens we try to allocate too much memory in case
when num_x and num_y are being assigned to their maximum.

Since the resolution should come from property in such case, reassign
values back to num_x and num_y to prevent too much memory allocation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index a05c6b597d43..1023d4134b8d 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1178,6 +1178,13 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 
 	touchscreen_parse_properties(input, true, &tsdata->prop);
 
+	if (tsdata->num_x == U16_MAX && tsdata->prop.max_x &&
+	    tsdata->num_y == U16_MAX && tsdata->prop.max_y) {
+		/* Reassign num_x and num_y from properties */
+		tsdata->num_x = tsdata->prop.max_x;
+		tsdata->num_y = tsdata->prop.max_y;
+	}
+
 	error = input_mt_init_slots(input, tsdata->max_support_points,
 				INPUT_MT_DIRECT);
 	if (error) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v1 5/5] Input: edt-ft5x06 - allocate buffer once for debugging
  2020-03-03 18:09 [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Andy Shevchenko
                   ` (2 preceding siblings ...)
  2020-03-03 18:09 ` [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory Andy Shevchenko
@ 2020-03-03 18:09 ` Andy Shevchenko
  2020-03-07  0:57   ` Dmitry Torokhov
  2020-03-07  1:12 ` [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Dmitry Torokhov
  4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2020-03-03 18:09 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input, Henrik Rydberg; +Cc: Andy Shevchenko

There is no need to allocate buffer each time we switch modes. First of all,
the code is protected by checking the factory_mode state. The size of the
buffer is static and can't be changed after ->probe() anyway.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 1023d4134b8d..3895e194006a 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -544,16 +544,6 @@ static int edt_ft5x06_factory_mode(struct edt_ft5x06_ts_data *tsdata)
 
 	disable_irq(client->irq);
 
-	if (!tsdata->raw_buffer) {
-		tsdata->raw_bufsize = tsdata->num_x * tsdata->num_y *
-				      sizeof(u16);
-		tsdata->raw_buffer = kzalloc(tsdata->raw_bufsize, GFP_KERNEL);
-		if (!tsdata->raw_buffer) {
-			error = -ENOMEM;
-			goto err_out;
-		}
-	}
-
 	/* mode register is 0x3c when in the work mode */
 	error = edt_ft5x06_register_write(tsdata, WORK_REGISTER_OPMODE, 0x03);
 	if (error) {
@@ -581,8 +571,6 @@ static int edt_ft5x06_factory_mode(struct edt_ft5x06_ts_data *tsdata)
 	return 0;
 
 err_out:
-	kfree(tsdata->raw_buffer);
-	tsdata->raw_buffer = NULL;
 	tsdata->factory_mode = false;
 	enable_irq(client->irq);
 
@@ -622,9 +610,6 @@ static int edt_ft5x06_work_mode(struct edt_ft5x06_ts_data *tsdata)
 		return -EIO;
 	}
 
-	kfree(tsdata->raw_buffer);
-	tsdata->raw_buffer = NULL;
-
 	/* restore parameters */
 	edt_ft5x06_register_write(tsdata, reg_addr->reg_threshold,
 				  tsdata->threshold);
@@ -697,7 +682,7 @@ static ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
 
 	mutex_lock(&tsdata->mutex);
 
-	if (!tsdata->factory_mode || !tsdata->raw_buffer) {
+	if (!tsdata->factory_mode) {
 		error = -EIO;
 		goto out;
 	}
@@ -774,6 +759,12 @@ edt_ft5x06_ts_prepare_debugfs(struct edt_ft5x06_ts_data *tsdata,
 
 	debugfs_create_file("mode", S_IRUSR | S_IWUSR,
 			    tsdata->debug_dir, tsdata, &debugfs_mode_fops);
+
+	tsdata->raw_bufsize = tsdata->num_x * tsdata->num_y * sizeof(u16);
+	tsdata->raw_buffer = kzalloc(tsdata->raw_bufsize, GFP_KERNEL);
+	if (!tsdata->raw_buffer)
+		return;
+
 	debugfs_create_file("raw_data", S_IRUSR,
 			    tsdata->debug_dir, tsdata, &debugfs_raw_data_fops);
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 5/5] Input: edt-ft5x06 - allocate buffer once for debugging
  2020-03-03 18:09 ` [PATCH v1 5/5] Input: edt-ft5x06 - allocate buffer once for debugging Andy Shevchenko
@ 2020-03-07  0:57   ` Dmitry Torokhov
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-03-07  0:57 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-input, Henrik Rydberg

Hi Andy,

On Tue, Mar 03, 2020 at 08:09:17PM +0200, Andy Shevchenko wrote:
> There is no need to allocate buffer each time we switch modes. First of all,
> the code is protected by checking the factory_mode state. The size of the
> buffer is static and can't be changed after ->probe() anyway.

Why do we need to keep memory allocated if it is not going to be used
majority of the time? How much is the code savings vs. allocated memory
size (without considering having multiple devices connected to the same
system).

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory
  2020-03-03 18:09 ` [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory Andy Shevchenko
@ 2020-03-07  1:08   ` Dmitry Torokhov
  2020-05-11 10:08     ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2020-03-07  1:08 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-input, Henrik Rydberg

On Tue, Mar 03, 2020 at 08:09:16PM +0200, Andy Shevchenko wrote:
> When mode switch happens we try to allocate too much memory in case
> when num_x and num_y are being assigned to their maximum.
> 
> Since the resolution should come from property in such case, reassign
> values back to num_x and num_y to prevent too much memory allocation.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index a05c6b597d43..1023d4134b8d 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1178,6 +1178,13 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  
>  	touchscreen_parse_properties(input, true, &tsdata->prop);
>  
> +	if (tsdata->num_x == U16_MAX && tsdata->prop.max_x &&
> +	    tsdata->num_y == U16_MAX && tsdata->prop.max_y) {
> +		/* Reassign num_x and num_y from properties */
> +		tsdata->num_x = tsdata->prop.max_x;
> +		tsdata->num_y = tsdata->prop.max_y;

No. num_x and num_y reprsenet number of electrodes on a given axis and
we should not be assigning maximum coordinates to them.

Moreover, the factory mode can only be activated on M06, where we do
read these values from registers, so we will not be allocating too much
memory. If anything, we should add error handling for
edt_ft5x06_register_read() when trying to fetch num_x and num_y.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set()
  2020-03-03 18:09 ` [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set() Andy Shevchenko
@ 2020-03-07  1:11   ` Dmitry Torokhov
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-03-07  1:11 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-input, Henrik Rydberg

On Tue, Mar 03, 2020 at 08:09:14PM +0200, Andy Shevchenko wrote:
> For better reading unroll nested conditions to simple if-else-if.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index d2587724c52a..cb67104c6934 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -666,10 +666,10 @@ static int edt_ft5x06_debugfs_mode_set(void *data, u64 mode)
>  
>  	mutex_lock(&tsdata->mutex);
>  
> -	if (mode != tsdata->factory_mode) {
> -		retval = mode ? edt_ft5x06_factory_mode(tsdata) :
> -				edt_ft5x06_work_mode(tsdata);
> -	}
> +	if (mode && !tsdata->factory_mode)
> +		retval = edt_ft5x06_factory_mode(tsdata);
> +	else if (!mode && tsdata->factory_mode)
> +		retval = edt_ft5x06_work_mode(tsdata);

Sorry, I find the original better, as I read it

	if mode changing then
		activate desired mode (either factory or normal mode)

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis
  2020-03-03 18:09 [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Andy Shevchenko
                   ` (3 preceding siblings ...)
  2020-03-03 18:09 ` [PATCH v1 5/5] Input: edt-ft5x06 - allocate buffer once for debugging Andy Shevchenko
@ 2020-03-07  1:12 ` Dmitry Torokhov
  4 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-03-07  1:12 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-input, Henrik Rydberg

On Tue, Mar 03, 2020 at 08:09:13PM +0200, Andy Shevchenko wrote:
> The 'axis + 1' calculation is implicit and potentially error prone.
> Moreover, few lines before the axis is set explicitly for both X and Y.
> 
> Do the same when retrieving different properties for X and Y.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/of_touchscreen.c | 35 +++++++++++-----------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/of_touchscreen.c b/drivers/input/touchscreen/of_touchscreen.c
> index e16ec4c7043a..97342e14b4f1 100644
> --- a/drivers/input/touchscreen/of_touchscreen.c
> +++ b/drivers/input/touchscreen/of_touchscreen.c
> @@ -66,7 +66,7 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
>  {
>  	struct device *dev = input->dev.parent;
>  	struct input_absinfo *absinfo;
> -	unsigned int axis;
> +	unsigned int axis, axis_x, axis_y;
>  	unsigned int minimum, maximum, fuzz;
>  	bool data_present;
>  
> @@ -74,33 +74,34 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
>  	if (!input->absinfo)
>  		return;
>  
> -	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
> +	axis_x = multitouch ? ABS_MT_POSITION_X : ABS_X;
> +	axis_y = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
> +
>  	data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
> -						input_abs_get_min(input, axis),
> +						input_abs_get_min(input, axis_x),
>  						&minimum) |
>  		       touchscreen_get_prop_u32(dev, "touchscreen-size-x",
>  						input_abs_get_max(input,
> -								  axis) + 1,
> +								  axis_x) + 1,
>  						&maximum) |
>  		       touchscreen_get_prop_u32(dev, "touchscreen-fuzz-x",
> -						input_abs_get_fuzz(input, axis),
> +						input_abs_get_fuzz(input, axis_x),
>  						&fuzz);
>  	if (data_present)
> -		touchscreen_set_params(input, axis, minimum, maximum - 1, fuzz);
> +		touchscreen_set_params(input, axis_x, minimum, maximum - 1, fuzz);
>  
> -	axis = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
>  	data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
> -						input_abs_get_min(input, axis),
> +						input_abs_get_min(input, axis_y),
>  						&minimum) |
>  		       touchscreen_get_prop_u32(dev, "touchscreen-size-y",
>  						input_abs_get_max(input,
> -								  axis) + 1,
> +								  axis_y) + 1,
>  						&maximum) |
>  		       touchscreen_get_prop_u32(dev, "touchscreen-fuzz-y",
> -						input_abs_get_fuzz(input, axis),
> +						input_abs_get_fuzz(input, axis_y),
>  						&fuzz);
>  	if (data_present)
> -		touchscreen_set_params(input, axis, minimum, maximum - 1, fuzz);
> +		touchscreen_set_params(input, axis_y, minimum, maximum - 1, fuzz);
>  
>  	axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
>  	data_present = touchscreen_get_prop_u32(dev,
> @@ -117,15 +118,13 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
>  	if (!prop)
>  		return;
>  
> -	axis = multitouch ? ABS_MT_POSITION_X : ABS_X;
> -
> -	prop->max_x = input_abs_get_max(input, axis);
> -	prop->max_y = input_abs_get_max(input, axis + 1);
> +	prop->max_x = input_abs_get_max(input, axis_x);
> +	prop->max_y = input_abs_get_max(input, axis_y);
>  
>  	prop->invert_x =
>  		device_property_read_bool(dev, "touchscreen-inverted-x");
>  	if (prop->invert_x) {
> -		absinfo = &input->absinfo[axis];
> +		absinfo = &input->absinfo[axis_x];
>  		absinfo->maximum -= absinfo->minimum;
>  		absinfo->minimum = 0;
>  	}
> @@ -133,7 +132,7 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
>  	prop->invert_y =
>  		device_property_read_bool(dev, "touchscreen-inverted-y");
>  	if (prop->invert_y) {
> -		absinfo = &input->absinfo[axis + 1];
> +		absinfo = &input->absinfo[axis_y];
>  		absinfo->maximum -= absinfo->minimum;
>  		absinfo->minimum = 0;
>  	}
> @@ -141,7 +140,7 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
>  	prop->swap_x_y =
>  		device_property_read_bool(dev, "touchscreen-swapped-x-y");
>  	if (prop->swap_x_y)
> -		swap(input->absinfo[axis], input->absinfo[axis + 1]);
> +		swap(input->absinfo[axis_x], input->absinfo[axis_y]);
>  }
>  EXPORT_SYMBOL(touchscreen_parse_properties);
>  
> -- 
> 2.25.1
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory
  2020-03-07  1:08   ` Dmitry Torokhov
@ 2020-05-11 10:08     ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2020-05-11 10:08 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, Henrik Rydberg

On Fri, Mar 06, 2020 at 05:08:37PM -0800, Dmitry Torokhov wrote:
> On Tue, Mar 03, 2020 at 08:09:16PM +0200, Andy Shevchenko wrote:
> > When mode switch happens we try to allocate too much memory in case
> > when num_x and num_y are being assigned to their maximum.
> > 
> > Since the resolution should come from property in such case, reassign
> > values back to num_x and num_y to prevent too much memory allocation.

> > +	if (tsdata->num_x == U16_MAX && tsdata->prop.max_x &&
> > +	    tsdata->num_y == U16_MAX && tsdata->prop.max_y) {
> > +		/* Reassign num_x and num_y from properties */
> > +		tsdata->num_x = tsdata->prop.max_x;
> > +		tsdata->num_y = tsdata->prop.max_y;
> 
> No. num_x and num_y reprsenet number of electrodes on a given axis and
> we should not be assigning maximum coordinates to them.

Thank you for explanation.

> Moreover, the factory mode can only be activated on M06, where we do
> read these values from registers, so we will not be allocating too much
> memory. If anything, we should add error handling for
> edt_ft5x06_register_read() when trying to fetch num_x and num_y.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 3/5] Input: edt-ft5x06 - use U16_MAX instead of -1
  2020-03-03 18:09 ` [PATCH v1 3/5] Input: edt-ft5x06 - use U16_MAX instead of -1 Andy Shevchenko
@ 2020-05-11 10:09   ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2020-05-11 10:09 UTC (permalink / raw)
  To: Dmitry Torokhov, linux-input, Henrik Rydberg

On Tue, Mar 03, 2020 at 08:09:15PM +0200, Andy Shevchenko wrote:
> Explicitly show what the value we supply for the touchscreen resolution
> when it can't be detected. -1 is hard to compare with when unsigned short
> type is in use. The change will help to avoid signed vs. unsigned error
> prone comparisons.
> 

So, this left without comment, does it mean you are going to apply it?

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index cb67104c6934..a05c6b597d43 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -23,6 +23,7 @@
>  #include <linux/input/touchscreen.h>
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
> +#include <linux/limits.h>
>  #include <linux/module.h>
>  #include <linux/ratelimit.h>
>  #include <linux/regulator/consumer.h>
> @@ -983,8 +984,8 @@ edt_ft5x06_ts_get_parameters(struct edt_ft5x06_ts_data *tsdata)
>  		tsdata->num_y = edt_ft5x06_register_read(tsdata,
>  							 reg_addr->reg_num_y);
>  	} else {
> -		tsdata->num_x = -1;
> -		tsdata->num_y = -1;
> +		tsdata->num_x = U16_MAX;
> +		tsdata->num_y = U16_MAX;
>  	}
>  }
>  
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-11 10:09 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 18:09 [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Andy Shevchenko
2020-03-03 18:09 ` [PATCH v1 2/5] Input: edt-ft5x06 - refactor condition in edt_ft5x06_debugfs_mode_set() Andy Shevchenko
2020-03-07  1:11   ` Dmitry Torokhov
2020-03-03 18:09 ` [PATCH v1 3/5] Input: edt-ft5x06 - use U16_MAX instead of -1 Andy Shevchenko
2020-05-11 10:09   ` Andy Shevchenko
2020-03-03 18:09 ` [PATCH v1 4/5] Input: edt-ft5x06 - do not try to allocate too much memory Andy Shevchenko
2020-03-07  1:08   ` Dmitry Torokhov
2020-05-11 10:08     ` Andy Shevchenko
2020-03-03 18:09 ` [PATCH v1 5/5] Input: edt-ft5x06 - allocate buffer once for debugging Andy Shevchenko
2020-03-07  0:57   ` Dmitry Torokhov
2020-03-07  1:12 ` [PATCH v1 1/5] Input: of_touchscreen - explicitly choose axis Dmitry Torokhov

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.