[PATCH 0/5] Input: goodix - Various fixes and improvements

Hi Dmitry,

Here is a series with some fixes and improvements for the Goodix
touchscreen driver based on your review of my recent patch to
add pen support (which has already been merged).

This is based on goodix.c with all goodix patches from both
input/for-next and input/for-linux applied so that it is tested
against what goodix.c will look like in 5.17-rc1 when both branches
are merged. I don't expect this to cause any problems applying,
but let me know if you want me to rebase on just input/for-next.

Regards,

Hans


Hans de Goede (5):
  Input: Add input_copy_abs() function
  Input: goodix - Use input_copy_abs() helper
  Input: goodix - Improve gpiod_get() error logging
  Input: goodix - 2 small fixes for pen support
  Input: goodix - Fix race on driver unbind

 drivers/input/input.c              | 34 ++++++++++++++++
 drivers/input/touchscreen/goodix.c | 65 +++++++++++++++---------------
 drivers/input/touchscreen/goodix.h |  1 +
 include/linux/input.h              |  2 +
 4 files changed, 69 insertions(+), 33 deletions(-)

-- 
2.33.1

Add a new helper function to copy absinfo from one input_dev to
another input_dev.

This is useful to e.g. setup a pen/stylus input-device for combined
touchscreen/pen hardware where the pen uses the same coordinates as
the touchscreen.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/input.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/input.h |  2 ++
 2 files changed, 36 insertions(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index ccaeb2426385..60f3eb38906f 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c

@@ -526,6 +526,40 @@ void input_set_abs_params(struct input_dev *dev, unsigned int axis,
 }
 EXPORT_SYMBOL(input_set_abs_params);
 
+/**
+ * input_copy_abs - Copy absinfo from one input_dev to another
+ * @dst: Destination input device to copy the abs settings to
+ * @dst_axis: ABS_* value selecting the destination axis
+ * @src: Source input device to copy the abs settings from
+ * @src_axis: ABS_* value selecting the source axis
+ *
+ * Set absinfo for the selected destination axis by copying it from
+ * the specified source input device's source axis.
+ * This is useful to e.g. setup a pen/stylus input-device for combined
+ * touchscreen/pen hardware where the pen uses the same coordinates as
+ * the touchscreen.
+ */
+void input_copy_abs(struct input_dev *dst, unsigned int dst_axis,
+		    const struct input_dev *src, unsigned int src_axis)
+{
+	/*
+	 * input_alloc_absinfo() may have failed for the source. Our caller is
+	 * expected to catch this when registering the input devices, which may
+	 * happen after the input_copy_abs() call.
+	 */
+	if (!src->absinfo)
+		return;
+
+	input_alloc_absinfo(dst);
+	if (!dst->absinfo)
+		return;
+
+	dst->absinfo[dst_axis] = src->absinfo[src_axis];
+
+	__set_bit(EV_ABS, dst->evbit);
+	__set_bit(dst_axis, dst->absbit);
+}
+EXPORT_SYMBOL(input_copy_abs);
 
 /**
  * input_grab_device - grabs device for exclusive use

diff --git a/include/linux/input.h b/include/linux/input.h
index 0354b298d874..49790c1bd2c4 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h

@@ -475,6 +475,8 @@ static inline void input_set_events_per_packet(struct input_dev *dev, int n_even
 void input_alloc_absinfo(struct input_dev *dev);
 void input_set_abs_params(struct input_dev *dev, unsigned int axis,
 			  int min, int max, int fuzz, int flat);
+void input_copy_abs(struct input_dev *dst, unsigned int dst_axis,
+		    const struct input_dev *src, unsigned int src_axis);
 
 #define INPUT_GENERATE_ABS_ACCESSORS(_suffix, _item)			\
 static inline int input_abs_get_##_suffix(struct input_dev *dev,	\

-- 
2.33.1

goodix_get_gpio_config() errors are fatal (abort probe()) so log them
at KERN_ERR level rather then as debug messages.

Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
- Just do s/dev_dbg/dev_err/, rather then switching to dev_err_probe()
---
 drivers/input/touchscreen/goodix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 3a16f78036d5..84406789da2d 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -967,7 +967,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 	if (IS_ERR(gpiod)) {
 		error = PTR_ERR(gpiod);
 		if (error != -EPROBE_DEFER)
-			dev_dbg(dev, "Failed to get %s GPIO: %d\n",
+			dev_err(dev, "Failed to get %s GPIO: %d\n",
 				GOODIX_GPIO_INT_NAME, error);
 		return error;
 	}

@@ -984,7 +984,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
 	if (IS_ERR(gpiod)) {
 		error = PTR_ERR(gpiod);
 		if (error != -EPROBE_DEFER)
-			dev_dbg(dev, "Failed to get %s GPIO: %d\n",
+			dev_err(dev, "Failed to get %s GPIO: %d\n",
 				GOODIX_GPIO_RST_NAME, error);
 		return error;
 	}

-- 
2.33.1

Use the new input_copy_abs() helper and move the 2 input_abs_set_res()
calls up to be directly after the 2 input_copy_abs() calls, so that
the calls initializing the X and Y axis are all together.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/goodix.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 8aec8a8cef7e..3a16f78036d5 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -306,23 +306,8 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
 	if (!input)
 		return NULL;
 
-	input_alloc_absinfo(input);
-	if (!input->absinfo) {
-		input_free_device(input);
-		return NULL;
-	}
-
-	input->absinfo[ABS_X] = ts->input_dev->absinfo[ABS_MT_POSITION_X];
-	input->absinfo[ABS_Y] = ts->input_dev->absinfo[ABS_MT_POSITION_Y];
-	__set_bit(ABS_X, input->absbit);
-	__set_bit(ABS_Y, input->absbit);
-	input_set_abs_params(input, ABS_PRESSURE, 0, 255, 0, 0);
-
-	input_set_capability(input, EV_KEY, BTN_TOUCH);
-	input_set_capability(input, EV_KEY, BTN_TOOL_PEN);
-	input_set_capability(input, EV_KEY, BTN_STYLUS);
-	input_set_capability(input, EV_KEY, BTN_STYLUS2);
-	__set_bit(INPUT_PROP_DIRECT, input->propbit);
+	input_copy_abs(input, ABS_X, ts->input_dev, ABS_MT_POSITION_X);
+	input_copy_abs(input, ABS_Y, ts->input_dev, ABS_MT_POSITION_Y);
 	/*
 	 * The resolution of these touchscreens is about 10 units/mm, the actual
 	 * resolution does not matter much since we set INPUT_PROP_DIRECT.

@@ -330,6 +315,13 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
 	 */
 	input_abs_set_res(input, ABS_X, 10);
 	input_abs_set_res(input, ABS_Y, 10);
+	input_set_abs_params(input, ABS_PRESSURE, 0, 255, 0, 0);
+
+	input_set_capability(input, EV_KEY, BTN_TOUCH);
+	input_set_capability(input, EV_KEY, BTN_TOOL_PEN);
+	input_set_capability(input, EV_KEY, BTN_STYLUS);
+	input_set_capability(input, EV_KEY, BTN_STYLUS2);
+	__set_bit(INPUT_PROP_DIRECT, input->propbit);
 
 	input->name = "Goodix Active Pen";
 	input->phys = "input/pen";

-- 
2.33.1

2 small fixes for pen support

1. Set the id.vendor field for the pen input_dev
2. Fix a typo in a comment

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/goodix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 84406789da2d..04baf5a770f5 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -326,6 +326,7 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
 	input->name = "Goodix Active Pen";
 	input->phys = "input/pen";
 	input->id.bustype = BUS_I2C;
+	input->id.vendor = 0x0416;
 	if (kstrtou16(ts->id, 10, &input->id.product))
 		input->id.product = 0x1001;
 	input->id.version = ts->version;

@@ -468,7 +469,7 @@ static void goodix_process_events(struct goodix_ts_data *ts)
 	if (touch_num == 1 && (point_data[1] & 0x80)) {
 		goodix_ts_report_pen_down(ts, point_data);
 		goodix_ts_release_keys(ts);
-		goto sync; /* Release any previousle registered touches */
+		goto sync; /* Release any previously registered touches */
 	} else {
 		goodix_ts_report_pen_up(ts);
 	}

-- 
2.33.1

Because there is no way to detect if the touchscreen has pen support,
the driver is allocating and registering the input_pen input_dev on
receiving the first pen event.

But this means that the input_dev gets allocated after the request_irq()
call which means that the devm framework will free it before disabling
the irq, leaving a window where the irq handler may run and reference the
free-ed input_dev.

To fix this move the allocation of the input_pen input_dev to before
the request_irq() call, while still only registering it on the first pen
event so that the driver does not advertise pen capability on touchscreens
without it (most goodix touchscreens do not have pen support).

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/goodix.c | 32 ++++++++++++++++++------------
 drivers/input/touchscreen/goodix.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 04baf5a770f5..1ada40fa6de6 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -297,14 +297,14 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 	return -ENOMSG;
 }
 
-static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
+static int goodix_create_pen_input(struct goodix_ts_data *ts)
 {
 	struct device *dev = &ts->client->dev;
 	struct input_dev *input;
 
 	input = devm_input_allocate_device(dev);
 	if (!input)
-		return NULL;
+		return -ENOMEM;
 
 	input_copy_abs(input, ABS_X, ts->input_dev, ABS_MT_POSITION_X);
 	input_copy_abs(input, ABS_Y, ts->input_dev, ABS_MT_POSITION_Y);

@@ -331,23 +331,18 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
 		input->id.product = 0x1001;
 	input->id.version = ts->version;
 
-	if (input_register_device(input) != 0) {
-		input_free_device(input);
-		return NULL;
-	}
-
-	return input;
+	ts->input_pen = input;
+	return 0;
 }
 
 static void goodix_ts_report_pen_down(struct goodix_ts_data *ts, u8 *data)
 {
-	int input_x, input_y, input_w;
+	int input_x, input_y, input_w, error;
 	u8 key_value;
 
-	if (!ts->input_pen) {
-		ts->input_pen = goodix_create_pen_input(ts);
-		if (!ts->input_pen)
-			return;
+	if (!ts->pen_input_registered) {
+		error = input_register_device(ts->input_pen);
+		ts->pen_input_registered = error == 0;
 	}
 
 	if (ts->contact_size == 9) {

@@ -1207,6 +1202,17 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
 		return error;
 	}
 
+	/*
+	 * Create the input_pen device before goodix_request_irq() calls
+	 * devm_request_threaded_irq() so that the devm framework frees
+	 * it after disabling the irq.
+	 * Unfortunately there is no way to detect if the touchscreen has pen
+	 * support, so registering the dev is delayed till the first pen event.
+	 */
+	error = goodix_create_pen_input(ts);
+	if (error)
+		return error;
+
 	ts->irq_flags = goodix_irq_flags[ts->int_trigger_type] | IRQF_ONESHOT;
 	error = goodix_request_irq(ts);
 	if (error) {

diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index fa8602e78a64..30ea24f28f8a 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h

@@ -94,6 +94,7 @@ struct goodix_ts_data {
 	u16 version;
 	bool reset_controller_at_probe;
 	bool load_cfg_from_disk;
+	bool pen_input_registered;
 	struct completion firmware_loading_complete;
 	unsigned long irq_flags;
 	enum goodix_irq_pin_access_method irq_pin_access_method;

-- 
2.33.1

Hi Hans,

On Sun, Dec 12, 2021 at 01:42:42PM +0100, Hans de Goede wrote:

I do not think you want to try and re-register input device if first
registration failed. You probably also don't want to waste time and
return early from here in case of failures.

Thanks.

-- 
Dmitry

On Sun, Dec 12, 2021 at 01:42:40PM +0100, Hans de Goede wrote:

goodix_get_gpio_config() errors are fatal (abort probe()) so log them
at KERN_ERR level rather then as debug messages.

Signed-off-by: Hans de Goede <redacted>

Applied, thank you.

-- 
Dmitry

On Sun, Dec 12, 2021 at 01:42:41PM +0100, Hans de Goede wrote:

2 small fixes for pen support

1. Set the id.vendor field for the pen input_dev
2. Fix a typo in a comment

Signed-off-by: Hans de Goede <redacted>

Applied, thank you.

-- 
Dmitry

Hi Hans,

On Sun, Dec 12, 2021 at 01:42:38PM +0100, Hans de Goede wrote:

I'd probably check if source device actually declared EV_ABS/src_axis
and yelled loudly (WARN?) in such case.

+
+	input_alloc_absinfo(dst);
+	if (!dst->absinfo)
+		return;
+
+	dst->absinfo[dst_axis] = src->absinfo[src_axis];
+
+	__set_bit(EV_ABS, dst->evbit);
+	__set_bit(dst_axis, dst->absbit);

input_set_capability() ?

+}
+EXPORT_SYMBOL(input_copy_abs);

Thanks.

-- 
Dmitry

Hi Dmitry,

Thank you for your review of this patch series; and sorry for
being so slow in getting back to you on this.

On 12/13/21 05:58, Dmitry Torokhov wrote:

Hi Hans,

On Sun, Dec 12, 2021 at 01:42:38PM +0100, Hans de Goede wrote:

quoted

Add a new helper function to copy absinfo from one input_dev to
another input_dev.

This is useful to e.g. setup a pen/stylus input-device for combined
touchscreen/pen hardware where the pen uses the same coordinates as
the touchscreen.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/input.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/input.h |  2 ++
 2 files changed, 36 insertions(+)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index ccaeb2426385..60f3eb38906f 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c

@@ -526,6 +526,40 @@ void input_set_abs_params(struct input_dev *dev, unsigned int axis,
 }
 EXPORT_SYMBOL(input_set_abs_params);
 
+/**
+ * input_copy_abs - Copy absinfo from one input_dev to another
+ * @dst: Destination input device to copy the abs settings to
+ * @dst_axis: ABS_* value selecting the destination axis
+ * @src: Source input device to copy the abs settings from
+ * @src_axis: ABS_* value selecting the source axis
+ *
+ * Set absinfo for the selected destination axis by copying it from
+ * the specified source input device's source axis.
+ * This is useful to e.g. setup a pen/stylus input-device for combined
+ * touchscreen/pen hardware where the pen uses the same coordinates as
+ * the touchscreen.
+ */
+void input_copy_abs(struct input_dev *dst, unsigned int dst_axis,
+		    const struct input_dev *src, unsigned int src_axis)
+{
+	/*
+	 * input_alloc_absinfo() may have failed for the source. Our caller is
+	 * expected to catch this when registering the input devices, which may
+	 * happen after the input_copy_abs() call.
+	 */
+	if (!src->absinfo)
+		return;

I'd probably check if source device actually declared EV_ABS/src_axis
and yelled loudly (WARN?) in such case.

Ack, I will add this for v2.

quoted

+
+	input_alloc_absinfo(dst);
+	if (!dst->absinfo)
+		return;
+
+	dst->absinfo[dst_axis] = src->absinfo[src_axis];
+
+	__set_bit(EV_ABS, dst->evbit);
+	__set_bit(dst_axis, dst->absbit);

input_set_capability() ?

Ack.

Regards,

Hans

quoted

+}
+EXPORT_SYMBOL(input_copy_abs);

Thanks.

Hi,

On 12/13/21 05:56, Dmitry Torokhov wrote:

Hi Hans,

On Sun, Dec 12, 2021 at 01:42:42PM +0100, Hans de Goede wrote:

quoted

Because there is no way to detect if the touchscreen has pen support,
the driver is allocating and registering the input_pen input_dev on
receiving the first pen event.

But this means that the input_dev gets allocated after the request_irq()
call which means that the devm framework will free it before disabling
the irq, leaving a window where the irq handler may run and reference the
free-ed input_dev.

To fix this move the allocation of the input_pen input_dev to before
the request_irq() call, while still only registering it on the first pen
event so that the driver does not advertise pen capability on touchscreens
without it (most goodix touchscreens do not have pen support).

Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/goodix.c | 32 ++++++++++++++++++------------
 drivers/input/touchscreen/goodix.h |  1 +
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 04baf5a770f5..1ada40fa6de6 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -297,14 +297,14 @@ static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
 	return -ENOMSG;
 }
 
-static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
+static int goodix_create_pen_input(struct goodix_ts_data *ts)
 {
 	struct device *dev = &ts->client->dev;
 	struct input_dev *input;
 
 	input = devm_input_allocate_device(dev);
 	if (!input)
-		return NULL;
+		return -ENOMEM;
 
 	input_copy_abs(input, ABS_X, ts->input_dev, ABS_MT_POSITION_X);
 	input_copy_abs(input, ABS_Y, ts->input_dev, ABS_MT_POSITION_Y);

@@ -331,23 +331,18 @@ static struct input_dev *goodix_create_pen_input(struct goodix_ts_data *ts)
 		input->id.product = 0x1001;
 	input->id.version = ts->version;
 
-	if (input_register_device(input) != 0) {
-		input_free_device(input);
-		return NULL;
-	}
-
-	return input;
+	ts->input_pen = input;
+	return 0;
 }
 
 static void goodix_ts_report_pen_down(struct goodix_ts_data *ts, u8 *data)
 {
-	int input_x, input_y, input_w;
+	int input_x, input_y, input_w, error;
 	u8 key_value;
 
-	if (!ts->input_pen) {
-		ts->input_pen = goodix_create_pen_input(ts);
-		if (!ts->input_pen)
-			return;
+	if (!ts->pen_input_registered) {
+		error = input_register_device(ts->input_pen);
+		ts->pen_input_registered = error == 0;

I do not think you want to try and re-register input device if first
registration failed. You probably also don't want to waste time and
return early from here in case of failures.

Ack, I've fixed both for v2 of this patch-series.

Regards,

Hans