linux-kernel - Re: [PATCH 1/2] xpad: XTYPE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87fs45u4o2.fsf@protonmail.com>
Date:   Sat, 26 Aug 2023 16:52:51 +0000
From:   Rahul Rameshbabu <sergeantsagara@...tonmail.com>
To:     Max Staudt <max@...as.org>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Vicki Pfau <vi@...rift.com>,
        Pavel Rojtberg <rojtberg@...il.com>,
        Roderick Colenbrander <roderick@...kai.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] xpad: XTYPE_XBOX: Report analog buttons

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@...as.org> 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@...as.org>
> ---
>  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