Re: [PATCH v6 1/4] HID: wacom: Advertise SW_MUTE_DEVICE capability prior to registration

[Lee Jones <lee@kernel.org>]

Is Jason back in this week?

Note that all of the Sashiko review comments are pre-existing issues.

> 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.
> 
> 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@kernel.org>
> ---
> 
> 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;
> +	}
>  
>  	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