Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] [v2] Input: Add "Share" button to Microsoft Xbox One
 controller.
To:     Chris Ye <lzye@google.com>,
        =?UTF-8?Q?=c5=81ukasz_Patron?= <priv.luk@gmail.com>,
        Benjamin Valentin <benpicco@googlemail.com>,
        Lee Jones <lee.jones@linaro.org>,
        =?UTF-8?Q?Olivier_Cr=c3=aate?= <olivier.crete@ocrete.ca>,
        Sanjay Govind <sanjay.govind9@gmail.com>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc:     linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        trivial@kernel.org, kernel-team@android.com
References: <20210225053204.697951-1-lzye@google.com>
From:   Cameron Gutman <aicommander@gmail.com>
Message-ID: <78269a7f-f50b-d763-3511-0b3cf80fdbef@gmail.com>
Date:   Sat, 27 Feb 2021 20:01:10 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210225053204.697951-1-lzye@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/24/21 11:32 PM, Chris Ye wrote:
> Add "Share" button input capability and input event mapping for
> Microsoft Xbox One controller.
> Fixed Microsoft Xbox One controller share button not working under USB
> connection.
> 
> Signed-off-by: Chris Ye <lzye@google.com>
> ---
>  drivers/input/joystick/xpad.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 9f0d07dcbf06..0c3374091aff 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -79,6 +79,7 @@
>  #define MAP_DPAD_TO_BUTTONS		(1 << 0)
>  #define MAP_TRIGGERS_TO_BUTTONS		(1 << 1)
>  #define MAP_STICKS_TO_NULL		(1 << 2)
> +#define MAP_SHARE_BUTTON		(1 << 3)
>  #define DANCEPAD_MAP_CONFIG	(MAP_DPAD_TO_BUTTONS |			\
>  				MAP_TRIGGERS_TO_BUTTONS | MAP_STICKS_TO_NULL)
>  
> @@ -130,6 +131,7 @@ static const struct xpad_device {
>  	{ 0x045e, 0x02e3, "Microsoft X-Box One Elite pad", 0, XTYPE_XBOXONE },
>  	{ 0x045e, 0x02ea, "Microsoft X-Box One S pad", 0, XTYPE_XBOXONE },
>  	{ 0x045e, 0x0719, "Xbox 360 Wireless Receiver", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
> +	{ 0x045e, 0x0b12, "Microsoft X-Box One X pad", MAP_SHARE_BUTTON, XTYPE_XBOXONE },

Let's use 'Xbox' for new entries instead of 'X-Box'. There was an effort to
standardize on 'Xbox' (which is what Microsoft uses), but changing device
names can impact userspace which may use these names in mapping heuristics
(SDL does this). We can at least not make the problem worse though.

>  	{ 0x046d, 0xc21d, "Logitech Gamepad F310", 0, XTYPE_XBOX360 },
>  	{ 0x046d, 0xc21e, "Logitech Gamepad F510", 0, XTYPE_XBOX360 },
>  	{ 0x046d, 0xc21f, "Logitech Gamepad F710", 0, XTYPE_XBOX360 },
> @@ -862,6 +864,8 @@ static void xpadone_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char
>  	/* menu/view buttons */
>  	input_report_key(dev, BTN_START,  data[4] & 0x04);
>  	input_report_key(dev, BTN_SELECT, data[4] & 0x08);
> +	if (xpad->mapping & MAP_SHARE_BUTTON)
> +		input_report_key(dev, KEY_RECORD, data[22] & 0x01);
>  

I was worried adding a button to an existing supported gamepad like this
might cause a breaking change to SDL's gamepad mapping for this gamepad,
since SDL assigns each present button an index rather than using the keycodes
directly (adding a new one could change the old indices). Fortunately, SDL
always processes buttons in the BTN_GAMEPAD range first, so this new button
ends up at the end of the list anyway.


>  	/* buttons A,B,X,Y */
>  	input_report_key(dev, BTN_A,	data[4] & 0x10);
> @@ -1669,9 +1673,12 @@ static int xpad_init_input(struct usb_xpad *xpad)
>  
>  	/* set up model-specific ones */
>  	if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX360W ||
> -	    xpad->xtype == XTYPE_XBOXONE) {
> +		xpad->xtype == XTYPE_XBOXONE) {
>  		for (i = 0; xpad360_btn[i] >= 0; i++)
>  			input_set_capability(input_dev, EV_KEY, xpad360_btn[i]);
> +		if (xpad->mapping & MAP_SHARE_BUTTON) {
> +			input_set_capability(input_dev, EV_KEY, KEY_RECORD);
> +		}

Style nit: Drop the uneeded {} here

>  	} else {
>  		for (i = 0; xpad_btn[i] >= 0; i++)
>  			input_set_capability(input_dev, EV_KEY, xpad_btn[i]);
>
LGTM, other than the minor changes suggested above.


Regards,
Cameron