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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Wed, Dec 8, 2021 at 12:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Dmitry,
>
> Thank you for the review.
>
> On 12/7/21 18:50, Dmitry Torokhov wrote:
> > On Mon, Dec 06, 2021 at 05:47:47PM +0100, Hans de Goede wrote:
> >> 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 <hdegoede@redhat.com>
> >> ---
> >>  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.
> >
> >> +
> >> +    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.

Thanks.

-- 
Dmitry