Re: [PATCH 1/2] xpad: XTYPE_XBOX: Report analog buttons

From: Rahul Rameshbabu
Date: Sat Aug 26 2023 - 13:01:31 EST


Hi,

You will want to update the commit message subject to use the prefix
"Input: xpad -" instead of "xpad:".

https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/drivers/input/joystick/xpad.c

On Sun, 27 Aug, 2023 00:21:10 +0900 "Max Staudt" <max@xxxxxxxxx> wrote:
> The original Xbox controllers (XTYPE_XBOX) report 8 buttons in an analog
> fashion, in addition to the digital on/off state:
>
> - Action buttons A/B/X/Y/black/white
> - Triggers L/R
>
> Up until now, only the triggers L/R are reported as values 0-255. The
> other pressure sensitive buttons are reported as digital buttons, as
> found on other controllers.
>
> This change exposes these buttons as axes in a way that is as backwards
> compatible as possible.
> The new axes are merely added, and numbered after any existing axes.
> This way, libraries like SDL which renumber axes in enumeration order,
> can keep their button/axis mapping as-is. Userspace can keep working as
> before, and can optionally use the new values when handling this type of
> gamepad.

FWIW, I like the way you handled adding support for the range of the
analog buttons.

>
> - BTN_A..BTN_Z mapped to ABS_MISC+0..ABS_MISC+5, 0 to 255
>
> Signed-off-by: Max Staudt <max@xxxxxxxxx>
> ---
> drivers/input/joystick/xpad.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index cdb193317c3b..609c06f795de 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -420,6 +420,14 @@ static const signed short xpad_abs_triggers[] = {
> -1
> };
>
> +/* used for analog face buttons mapped to axes */
> +static const signed short xpad_abs_analog_face_buttons[] = {
> + ABS_MISC + 0, ABS_MISC + 1, /* A, B */
> + ABS_MISC + 3, ABS_MISC + 4, /* X, Y */
> + ABS_MISC + 2, ABS_MISC + 5, /* C, Z */
> + -1
> +};

Would it make more sense to use an enum for this?
Something like the below enum.

enum xpad_abs_analog_face_btn {
XPAD_ABS_ANALOG_FACE_BTN_A = ABS_MISC,
XPAD_ABS_ANALOG_FACE_BTN_B,
XPAD_ABS_ANALOG_FACE_BTN_C,
XPAD_ABS_ANALOG_FACE_BTN_X,
XPAD_ABS_ANALOG_FACE_BTN_Y,
XPAD_ABS_ANALOG_FACE_BTN_Z,
XPAD_ABS_ANALOG_FACE_BTN_END, /* Must remain as the last element */
};

This would clean up both xpad_process_packet and xpad_set_up_abs a bit
in my opinion. Your loop for xpad_set_up_abs would look like the
following.

enum xpad_abs_analog_face_btn btn;

...

for (btn = XPAD_ABS_ANALOG_FACE_BTN_A; btn != XPAD_ABS_ANALOG_FACE_BTN_END; ++btn)
xpad_set_up_abs(input_dev, btn);

> +
> /* used when the controller has extra paddle buttons */
> static const signed short xpad_btn_paddles[] = {
> BTN_TRIGGER_HAPPY5, BTN_TRIGGER_HAPPY6, /* paddle upper right, lower right */
> @@ -784,6 +792,15 @@ static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *d
> input_report_key(dev, BTN_C, data[8]);
> input_report_key(dev, BTN_Z, data[9]);
>
> + /* analog buttons A, B, X, Y as axes */
> + input_report_abs(dev, xpad_abs_analog_face_buttons[0], data[4]); /* A */
> + input_report_abs(dev, xpad_abs_analog_face_buttons[1], data[5]); /* B */
> + input_report_abs(dev, xpad_abs_analog_face_buttons[2], data[6]); /* X */
> + input_report_abs(dev, xpad_abs_analog_face_buttons[3], data[7]); /* Y */
> +
> + /* analog buttons black, white (C, Z) as axes */
> + input_report_abs(dev, xpad_abs_analog_face_buttons[4], data[8]); /* C */
> + input_report_abs(dev, xpad_abs_analog_face_buttons[5], data[9]); /* Z */
>
> input_sync(dev);
> }
> @@ -1827,6 +1844,14 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
> case ABS_HAT0Y: /* the d-pad (only if dpad is mapped to axes */
> input_set_abs_params(input_dev, abs, -1, 1, 0, 0);
> break;
> + case ABS_MISC + 0:
> + case ABS_MISC + 1:
> + case ABS_MISC + 2:
> + case ABS_MISC + 3:
> + case ABS_MISC + 4:
> + case ABS_MISC + 5:
> + input_set_abs_params(input_dev, abs, 0, 255, 0, 0);
> + break;
> case ABS_PROFILE: /* 4 value profile button (such as on XAC) */
> input_set_abs_params(input_dev, abs, 0, 4, 0, 0);
> break;
> @@ -1928,6 +1953,10 @@ static int xpad_init_input(struct usb_xpad *xpad)
> xpad_set_up_abs(input_dev, xpad_abs_triggers[i]);
> }
>
> + if (xpad->xtype == XTYPE_XBOX)
> + for (i = 0; xpad_abs_analog_face_buttons[i] >= 0; i++)
> + xpad_set_up_abs(input_dev, xpad_abs_analog_face_buttons[i]);
> +
> /* setup profile button as an axis with 4 possible values */
> if (xpad->mapping & MAP_PROFILE_BUTTON)
> xpad_set_up_abs(input_dev, ABS_PROFILE);

--
Thanks,

Rahul Rameshbabu