Hi José, On Tue, Nov 23, 2021 at 8:12 PM José Expósito <jose.exposito89@xxxxxxxxx> wrote: > > Hi all, > > Historically, libinput has relayed on the INPUT_PROP_BUTTONPAD property > to detect buttonpads. > > Since buttonpads are expected to have only one button (BTN_LEFT), > recently we added a new rule to detect buttonpads: Where a touchpad > maps the BTN_RIGHT bit, libinput assumes it is NOT a buttonpad. > > However, this change leaded to several false possitives, so we ended up > reverting it. For more context: > https://gitlab.freedesktop.org/libinput/libinput/-/issues/704 > > And for a full list of affected hardware, HID reports and bug reports > please see: > https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/726 > > My understanding is that buttonpads should not map BTN_RIGHT and/or > BTN_MIDDLE and to avoid it I would like to fix the required drivers. As long as udev intrinsic is happy with it (and it correctly tags the touchpad as ID_INPUT_something), I'm fine with it. Also, you might want to point at the specification regarding button pads: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-capabilities-feature-report The way I read it: if the device exports the Button type value feature, and it is 0 or 1 (click-pad or pressure-pad), there should not be discrete buttons. > > One option to fix it (this patch) is to clear the bits that might have > been added because of the HID descriptor on every driver. > However, since this code will be common to all drivers, I would like to > ask if you consider it worth it to add a function to handle adding > properties. > > A function similar to input_set_capability but for props could be added > in input.h/c: > > /** > * input_set_property - add a property to the device > * @dev: device to add the property to > * @property: type of the property (INPUT_PROP_POINTER, INPUT_PROP_DIRECT...) > * > * In addition to setting up corresponding bit in dev->propbit the function > * might add or remove related capabilities. > */ > void input_set_property(struct input_dev *dev, unsigned int property) > { > switch (property) { > case INPUT_PROP_POINTER: > case INPUT_PROP_DIRECT: > case INPUT_PROP_SEMI_MT: > case INPUT_PROP_TOPBUTTONPAD: > case INPUT_PROP_POINTING_STICK: > case INPUT_PROP_ACCELEROMETER: > break; > > case INPUT_PROP_BUTTONPAD: > input_set_capability(dev, EV_KEY, BTN_LEFT); > __clear_bit(BTN_RIGHT, dev->keybit); > __clear_bit(BTN_MIDDLE, dev->keybit); > break; > > default: > pr_err("%s: unknown property %u\n", __func__, property); > dump_stack(); > return; > } > > __set_bit(property, dev->propbit); > } > EXPORT_SYMBOL(input_set_property); > > > Which approach do you think is the best? I think it depends if you plan on fixing just hid-multitouch or the others. If you have more than one driver, then yes, adding a new symbol in hid-input.c makes sense. If not, then you are just exposing a new function we won't know if there are users and we won't be able to change without care. Cheers, Benjamin > > Thank you very much in advance, > Jose > > > José Expósito (1): > HID: multitouch: only map BTN_LEFT on buttonpads > > drivers/hid/hid-multitouch.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > -- > 2.25.1 >
