Re: [PATCH 4/4] Input: goodix - Add pen support

Hi Dmitry,

On 12/8/21 09:51, Dmitry Torokhov wrote:

On Wed, Dec 8, 2021 at 12:37 AM Hans de Goede [off-list ref] wrote:

quoted

Hi Dmitry,

Thank you for the review.

On 12/7/21 18:50, Dmitry Torokhov wrote:

quoted

On Mon, Dec 06, 2021 at 05:47:47PM +0100, Hans de Goede wrote:

quoted

Some Goodix touchscreens have support for a (Goodix) active pen, add
support for this. The info on how to detect when a pen is down and to
detect when the stylus buttons are pressed was lifted from the out
of tree Goodix driver with pen support written by Adya:
https://gitlab.com/AdyaAdya/goodix-touchscreen-linux-driver/

Since there is no way to tell if pen support is present, the registering
of the pen input_dev is delayed till the first pen event is detected.

This has been tested on a Trekstor Surftab duo W1, a Chuwi Hi13 and
a Cyberbook T116 tablet.

Link: https://gitlab.com/AdyaAdya/goodix-touchscreen-linux-driver/
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=202161
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204513
Signed-off-by: Hans de Goede <redacted>
---
 drivers/input/touchscreen/goodix.c | 122 ++++++++++++++++++++++++++++-
 drivers/input/touchscreen/goodix.h |   1 +
 2 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 2d38a941e7e4..691e4505cf4a 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c

@@ -298,6 +298,107 @@ 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)
+{
+    struct device *dev = &ts->client->dev;
+    struct input_dev *input;
+
+    input = devm_input_allocate_device(dev);
+    if (!input)
+            return NULL;
+
+    input_alloc_absinfo(input);
+    if (!input->absinfo) {
+            input_free_device(input);
+            return NULL;
+    }

Please drop this as input_abs_set_max() will do allocation and
input_register_device() will reject device with ABS_* events without
absinfo allocated.

quoted

+
+    input->absinfo[ABS_X] = ts->input_dev->absinfo[ABS_MT_POSITION_X];

      input_abs_set_max(input, ABS_X,
              input_abs_get_max(input, ABS_MT_POSITION_X);

or even maybe

      input_set_abs_params(input, ABS_X,
              0, input_abs_get_max(input, ABS_MT_POSITION_X), 0, 0);

The reason why I'm just copying the entire absinfo struct
(and thus need the NULL check above) is because this driver uses
touchscreen_parse_properties(), so the min and fuzz values
might (theoretically) also be set through device-properties and
I wanted to cover that.

Since you don't like the above approach, I will go with the following
for the next version:

        input_set_abs_params(input, ABS_X,
                             input_abs_get_min(ts->input_dev, ABS_MT_POSITION_X),
                             input_abs_get_max(ts->input_dev, ABS_MT_POSITION_X),
                             input_abs_get_fuzz(ts->input_dev, ABS_MT_POSITION_X),
                             input_abs_get_flat(ts->input_dev, ABS_MT_POSITION_X));

(and the same for the Y axis).

Ah, sorry, I misread the code. It is fine as is then, or we could even
consider adding input_copy_abs(input, axis, src) that would allocate
absinfo if needed, set capability, and do the copy.

Oh, I think that adding an input_copy_abs() helper for this is actually
a good idea. I'll do that for the next version (in a separate patch of course).

Regards,

Hans