Hi Dmitry, On 12/8/21 09:51, Dmitry Torokhov wrote: > On Wed, Dec 8, 2021 at 12:37 AM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> >>>> --- >>>> 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. 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
