mbox series

[0/2] Improve GIP support

Message ID 20230330024752.2405603-1-vi@endrift.com
Headers show
Series Improve GIP support | expand

Message

Vicki Pfau March 30, 2023, 2:47 a.m. UTC 
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(-)

Comments

Dmitry Torokhov April 1, 2023, 9:41 p.m. UTC | #1 
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.
Vicki Pfau April 6, 2023, 2:40 a.m. UTC | #2 
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
Dmitry Torokhov April 10, 2023, 2:09 a.m. UTC | #3 
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.