Re: [PATCH 1/2] HID: nintendo: fix face button mappings

From: Roderick Colenbrander <thunderbird2k@gmail.com>
To: Max Fletcher <fletcher0max@gmail.com>
Cc: "Daniel J. Ogorchock" <djogorchock@gmail.com>,
	Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	linux-input <linux-input@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] HID: nintendo: fix face button mappings
Date: Fri, 13 May 2022 12:58:08 -0700	[thread overview]
Message-ID: <CAEc3jaBbVT1t1kS_Vvp3EqfheCWr=CAvVgdzw7vkeFyYz9H_7Q@mail.gmail.com> (raw)
In-Reply-To: <20220512001500.16739-1-fletcher0max@gmail.com>

Hi Max,

Thanks for your patch, however I must say the patch is not correct for
2 reasons.

Over the years different controllers have different layouts. The
standard which this driver (as well as others such as
hid-sony/hid-playstation) follow is the Linux gamepad standard (see
Documentation/input/gamepad.rst). It stays away of the debate what is
A/B/X/Y. It talks about North/west/.., (yes they are macros which map
to A/B/X/Y). In case of the Switch it does mean things are flipped,
but it was not meant to represent an Xbox controller. (Technically one
could argue that the Xbox controller should be flipped as it was the
SNES controller back in the days which introduced X/Y and the Switch
is still consistent with that.)

Second, even if the patch was right it would be tricky to merge. The
problem is that a changed mapping breaks user spaces and in general
can't do this unless there is a really good reason. It just would
break existing applications and libraries (often e.g. SDL)

Thanks,
Roderick

On Wed, May 11, 2022 at 8:12 PM Max Fletcher <fletcher0max@gmail.com> wrote:
>
> Previously, A and B would match the Xbox layout, but X and Y were incorrectly swapped. This corrects it so that X and Y match the Xbox layout.
>
> Signed-off-by: Max Fletcher <fletcher0max@gmail.com>
> ---
>  drivers/hid/hid-nintendo.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 2204de889739..7735971ede3f 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -1351,10 +1351,10 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr,
>                 input_report_key(dev, BTN_START, btns & JC_BTN_PLUS);
>                 input_report_key(dev, BTN_THUMBR, btns & JC_BTN_RSTICK);
>                 input_report_key(dev, BTN_MODE, btns & JC_BTN_HOME);
> -               input_report_key(dev, BTN_WEST, btns & JC_BTN_Y);
> -               input_report_key(dev, BTN_NORTH, btns & JC_BTN_X);
> -               input_report_key(dev, BTN_EAST, btns & JC_BTN_A);
> -               input_report_key(dev, BTN_SOUTH, btns & JC_BTN_B);
> +               input_report_key(dev, BTN_X, btns & JC_BTN_Y);
> +               input_report_key(dev, BTN_Y, btns & JC_BTN_X);
> +               input_report_key(dev, BTN_B, btns & JC_BTN_A);
> +               input_report_key(dev, BTN_A, btns & JC_BTN_B);
>         }
>
>         input_sync(dev);
> @@ -1578,7 +1578,7 @@ static const unsigned int joycon_button_inputs_l[] = {
>
>  static const unsigned int joycon_button_inputs_r[] = {
>         BTN_START, BTN_MODE, BTN_THUMBR,
> -       BTN_SOUTH, BTN_EAST, BTN_NORTH, BTN_WEST,
> +       BTN_A, BTN_B, BTN_Y, BTN_X,
>         BTN_TR, BTN_TR2,
>         0 /* 0 signals end of array */
>  };
> --
> 2.35.3
>