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. Thanks. -- Dmitry
