[PATCH 0/2] Improve GIP support

[PATCH 0/2] Improve GIP support

[Vicki Pfau]

This series contains a new version of the previously submitted "fix PowerA
EnWired Controller guide button" patch to make the failure soft instead of
hard, as well as a further patch to add (and use) constants for the interface
names, based on information gleaned from the xone project.

Vicki Pfau (2):
  Input: xpad - Add constants for GIP interface numbers
  Input: xpad - fix PowerA EnWired Controller guide button

 drivers/input/joystick/xpad.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.40.0

[PATCH 1/2] Input: xpad - Add constants for GIP interface numbers

[Vicki Pfau]

Wired GIP devices present multiple interfaces with the same USB identification
other than the interface number. This adds constants for differentiating two of
them and uses them where appropriate

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/input/joystick/xpad.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 2d86ca0c1ace..698224e1948f 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -561,6 +561,9 @@ struct xboxone_init_packet {
 #define GIP_MOTOR_LT BIT(3)
 #define GIP_MOTOR_ALL (GIP_MOTOR_R | GIP_MOTOR_L | GIP_MOTOR_RT | GIP_MOTOR_LT)
 
+#define GIP_WIRED_INTF_DATA 0
+#define GIP_WIRED_INTF_AUDIO 1
+
 /*
  * This packet is required for all Xbox One pads with 2015
  * or later firmware installed (or present from the factory).
@@ -2004,7 +2007,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	}
 
 	if (xpad->xtype == XTYPE_XBOXONE &&
-	    intf->cur_altsetting->desc.bInterfaceNumber != 0) {
+	    intf->cur_altsetting->desc.bInterfaceNumber != GIP_WIRED_INTF_DATA) {
 		/*
 		 * The Xbox One controller lists three interfaces all with the
 		 * same interface class, subclass and protocol. Differentiate by
-- 
2.40.0

[PATCH 2/2] Input: xpad - fix PowerA EnWired Controller guide button

[Vicki Pfau]

This commit explicitly disables the audio interface the same way the official
driver does. This is needed for some controllers, such as the PowerA Enhanced
Wired Controller for Series X|S (0x20d6:0x200e) to report the guide button.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/input/joystick/xpad.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 698224e1948f..c31fc4e9b310 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1396,6 +1396,14 @@ static int xpad_start_xbox_one(struct usb_xpad *xpad)
 	unsigned long flags;
 	int retval;
 
+	/* Explicitly disable the audio interface. This is needed for some
+	 * controllers, such as the PowerA Enhanced Wired Controller
+	 * for Series X|S (0x20d6:0x200e) to report the guide button */
+	retval = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
+	if (retval)
+		dev_warn(&xpad->dev->dev,
+			 "unable to disable audio interface: %d\n", retval);
+
 	spin_lock_irqsave(&xpad->odata_lock, flags);
 
 	/*
-- 
2.40.0

Re: [PATCH 2/2] Input: xpad - fix PowerA EnWired Controller guide button

[Dmitry Torokhov]

On Wed, Mar 29, 2023 at 07:47:52PM -0700, Vicki Pfau wrote:
> This commit explicitly disables the audio interface the same way the official
> driver does. This is needed for some controllers, such as the PowerA Enhanced
> Wired Controller for Series X|S (0x20d6:0x200e) to report the guide button.
> 
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
>  drivers/input/joystick/xpad.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 698224e1948f..c31fc4e9b310 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -1396,6 +1396,14 @@ static int xpad_start_xbox_one(struct usb_xpad *xpad)
>  	unsigned long flags;
>  	int retval;
>  
> +	/* Explicitly disable the audio interface. This is needed for some
> +	 * controllers, such as the PowerA Enhanced Wired Controller
> +	 * for Series X|S (0x20d6:0x200e) to report the guide button */
> +	retval = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
> +	if (retval)
> +		dev_warn(&xpad->dev->dev,
> +			 "unable to disable audio interface: %d\n", retval);

I would prefer if we first validated that the interface is in fact
present. Can we do something like:

	if (usb_ifnum_to_if(xpad->udev, GIP_WIRED_INTF_AUDIO)) {
		error = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
		if (error)
			...
	}

> +
>  	spin_lock_irqsave(&xpad->odata_lock, flags);
>  
>  	/*
> -- 
> 2.40.0
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: xpad - fix PowerA EnWired Controller guide button

[Vicki Pfau]

On 4/1/23 14:41, Dmitry Torokhov wrote:
> On Wed, Mar 29, 2023 at 07:47:52PM -0700, Vicki Pfau wrote:
>> This commit explicitly disables the audio interface the same way the official
>> driver does. This is needed for some controllers, such as the PowerA Enhanced
>> Wired Controller for Series X|S (0x20d6:0x200e) to report the guide button.
>>
>> Signed-off-by: Vicki Pfau <vi@endrift.com>
>> ---
>>  drivers/input/joystick/xpad.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
>> index 698224e1948f..c31fc4e9b310 100644
>> --- a/drivers/input/joystick/xpad.c
>> +++ b/drivers/input/joystick/xpad.c
>> @@ -1396,6 +1396,14 @@ static int xpad_start_xbox_one(struct usb_xpad *xpad)
>>  	unsigned long flags;
>>  	int retval;
>>  
>> +	/* Explicitly disable the audio interface. This is needed for some
>> +	 * controllers, such as the PowerA Enhanced Wired Controller
>> +	 * for Series X|S (0x20d6:0x200e) to report the guide button */
>> +	retval = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
>> +	if (retval)
>> +		dev_warn(&xpad->dev->dev,
>> +			 "unable to disable audio interface: %d\n", retval);
> 
> I would prefer if we first validated that the interface is in fact
> present. Can we do something like:
> 
> 	if (usb_ifnum_to_if(xpad->udev, GIP_WIRED_INTF_AUDIO)) {
> 		error = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
> 		if (error)
> 			...
> 	}
> 

Yup, that makes sense. Wasn't sure what the cleanest way to do that was, though I'm unconvinced that the first party driver would work without this interface. It can't hurt to add the check.

Should I resubmit both patches in the series, or just this one?

>> +
>>  	spin_lock_irqsave(&xpad->odata_lock, flags);
>>  
>>  	/*
>> -- 
>> 2.40.0
>>
> 
> Thanks.
> 

Thanks,
Vicki

Re: [PATCH 2/2] Input: xpad - fix PowerA EnWired Controller guide button

[Dmitry Torokhov]

On Wed, Apr 05, 2023 at 07:40:32PM -0700, Vicki Pfau wrote:
> 
> 
> On 4/1/23 14:41, Dmitry Torokhov wrote:
> > On Wed, Mar 29, 2023 at 07:47:52PM -0700, Vicki Pfau wrote:
> >> This commit explicitly disables the audio interface the same way the official
> >> driver does. This is needed for some controllers, such as the PowerA Enhanced
> >> Wired Controller for Series X|S (0x20d6:0x200e) to report the guide button.
> >>
> >> Signed-off-by: Vicki Pfau <vi@endrift.com>
> >> ---
> >>  drivers/input/joystick/xpad.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> >> index 698224e1948f..c31fc4e9b310 100644
> >> --- a/drivers/input/joystick/xpad.c
> >> +++ b/drivers/input/joystick/xpad.c
> >> @@ -1396,6 +1396,14 @@ static int xpad_start_xbox_one(struct usb_xpad *xpad)
> >>  	unsigned long flags;
> >>  	int retval;
> >>  
> >> +	/* Explicitly disable the audio interface. This is needed for some
> >> +	 * controllers, such as the PowerA Enhanced Wired Controller
> >> +	 * for Series X|S (0x20d6:0x200e) to report the guide button */
> >> +	retval = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
> >> +	if (retval)
> >> +		dev_warn(&xpad->dev->dev,
> >> +			 "unable to disable audio interface: %d\n", retval);
> > 
> > I would prefer if we first validated that the interface is in fact
> > present. Can we do something like:
> > 
> > 	if (usb_ifnum_to_if(xpad->udev, GIP_WIRED_INTF_AUDIO)) {
> > 		error = usb_set_interface(xpad->udev, GIP_WIRED_INTF_AUDIO, 0);
> > 		if (error)
> > 			...
> > 	}
> > 
> 
> Yup, that makes sense. Wasn't sure what the cleanest way to do that was, though I'm unconvinced that the first party driver would work without this interface. It can't hurt to add the check.
> 
> Should I resubmit both patches in the series, or just this one?

Both please.

Thanks.

-- 
Dmitry