Re: [PATCH v6 1/4] HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration
From: Jason Gerecke
Date: Tue Jul 21 2026 - 11:47:00 EST
On Mon, Jul 20, 2026 at 5:54 AM Lee Jones <lee@xxxxxxxxxx> wrote:
>
> Is Jason back in this week?
>
> Note that all of the Sashiko review comments are pre-existing issues.
>
I'm back now :)
> > Input subsystem guidelines require that device capabilities are advertised
> > before the input device is registered. The Wacom driver was violating
> > this by advertising the SW_MUTE_DEVICE capability post-registration in
> > wacom_set_shared_values() (and duplicating it in device-specific setup
> > cases).
> >
> > Resolve this by moving the SW_MUTE_DEVICE capability setup to
> > wacom_setup_touch_input_capabilities() for all touch devices that support
> > it, excluding TabletPC devices.
This paragraph is no longer accurate. The TabletPC-specific checks
were removed. It would be more correct to say something more like "for
all HID_GENERIC touch devices that support it" given the new logic.
> >
> > Additionally, replace the lookup-dependent
> > 'wacom_wac->shared->touch->product' references with 'hdev->product'
> > inside wacom_setup_touch_input_capabilities() as 'hdev' is already
> > available and represents the touch device itself.
> >
> > Fixes: d2ec58aee8b1 ("HID: wacom: generic: support generic touch switch")
> > Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> > ---
> >
> > v4 -> v5: New patch used to split out SW_MUTE_DEVICE as per Jason's request
> > v5 -> v6: Unconditionally advertise SW_MUTE_DEVICE on generic touch devices
> >
> > drivers/hid/wacom_sys.c | 6 ------
> > drivers/hid/wacom_wac.c | 19 +++++++++++--------
> > 2 files changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 0eafa483b7f7..6c2a3ddfe366 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -2359,12 +2359,6 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
> > wacom_wac->shared->is_touch_on = true;
> > }
> >
> > - if (wacom_wac->shared->has_mute_touch_switch &&
> > - wacom_wac->shared->touch_input) {
> > - set_bit(EV_SW, wacom_wac->shared->touch_input->evbit);
> > - input_set_capability(wacom_wac->shared->touch_input, EV_SW,
> > - SW_MUTE_DEVICE);
> > - }
> > }
> >
> > static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
> > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> > index a29bf051ada7..32c4fd03479e 100644
> > --- a/drivers/hid/wacom_wac.c
> > +++ b/drivers/hid/wacom_wac.c
> > @@ -3953,6 +3953,8 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> > int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> > struct wacom_wac *wacom_wac)
> > {
> > + struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
> > + struct hid_device *hdev = wacom->hdev;
> > struct wacom_features *features = &wacom_wac->features;
> >
> > if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
> > @@ -3963,9 +3965,12 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> > else
> > __set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >
> > - if (features->type == HID_GENERIC)
> > + if (features->type == HID_GENERIC) {
> > + input_dev->evbit[0] |= BIT_MASK(EV_SW);
> > + __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> > /* setup has already been done */
> > return 0;
> > + }
I'm still concerned about this not working properly. I don't see how
you are preserving the desired behavior where SW_MUTE_DEVICE is only
set on devices where `has_mute_touch_switch` is true (e.g. when a
WACOM_HID_WD_TOUCHONOFF usage is discovered in the HID descriptor). It
seems like this change will result in *all* HID_GENERIC touch devices
gaining SW_MUTE_DEVICE, which is not correct.
For devices that have SW_MUTE_DEVICE on the touch interface, I think
it would be sufficient to simply add a `has_mute_touch_switch` check
here, since the HID descriptor will have already been parsed by this
point. **But** I think the trickier part is handling the case where
SW_MUTE_DEVICE for the touch appears on a different interface,
endpoint, or (potentially) sibling USB device... (The SW_MUTE_DEVICE
usage is usually found in the "pad" collection, which is appears in
the HID descriptor for the pen interface/endpoint/device)
Jason (she/they)
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
> >
> > input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > __set_bit(BTN_TOUCH, input_dev->keybit);
> > @@ -3997,19 +4002,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> > input_dev->evbit[0] |= BIT_MASK(EV_SW);
> > __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> >
> > - if (wacom_wac->shared->touch->product == 0x361) {
> > + if (hdev->product == 0x361) {
> > input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> > 0, 12440, 4, 0);
> > input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> > 0, 8640, 4, 0);
> > - }
> > - else if (wacom_wac->shared->touch->product == 0x360) {
> > + } else if (hdev->product == 0x360) {
> > input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> > 0, 8960, 4, 0);
> > input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> > 0, 5920, 4, 0);
> > - }
> > - else if (wacom_wac->shared->touch->product == 0x393) {
> > + } else if (hdev->product == 0x393) {
> > input_set_abs_params(input_dev, ABS_MT_POSITION_X,
> > 0, 6400, 4, 0);
> > input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
> > @@ -4039,8 +4042,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> > fallthrough;
> >
> > case WACOM_27QHDT:
> > - if (wacom_wac->shared->touch->product == 0x32C ||
> > - wacom_wac->shared->touch->product == 0xF6) {
> > + if (hdev->product == 0x32C ||
> > + hdev->product == 0xF6) {
> > input_dev->evbit[0] |= BIT_MASK(EV_SW);
> > __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
> > wacom_wac->has_mute_touch_switch = true;
> > --
> > 2.55.0.141.g00534a21ce-goog
> >
>
> --
> Lee Jones
>