[RESEND,1/2] HID: Add driver for RC Simulator Controllers

Message ID	20220822060936.769855-1-marcus.folkesson@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-input-owner@kernel.org> From: Marcus Folkesson <marcus.folkesson@gmail.com> To: Jonathan Corbet <corbet@lwn.net>, Marcus Folkesson <marcus.folkesson@gmail.com>, Jiri Kosina <jikos@kernel.org>, Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org Subject: [RESEND PATCH 1/2] HID: Add driver for RC Simulator Controllers Date: Mon, 22 Aug 2022 08:09:35 +0200 Message-Id: <20220822060936.769855-1-marcus.folkesson@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	[RESEND,1/2] HID: Add driver for RC Simulator Controllers \| expand [RESEND,1/2] HID: Add driver for RC Simulator Controllers [RESEND,2/2] MAINTAINERS: Add entry for RC Simulator Controllers

Marcus Folkesson Aug. 22, 2022, 6:09 a.m. UTC

Several RC Simulator Controllers are HID compliant with similar
interface.

Add support for these controllers:
 - Phoenix RC (HID variant)
 - Car VRC2.0
 - Real Flight G5/G6/G7
 - Aero Fly, FMS
 - OrangeRX FrSky

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 Documentation/hid/index.rst |   1 +
 Documentation/hid/rcsim.rst | 142 ++++++++++++++++
 drivers/hid/Kconfig         |  11 ++
 drivers/hid/Makefile        |   1 +
 drivers/hid/hid-ids.h       |   5 +
 drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
 6 files changed, 475 insertions(+)
 create mode 100644 Documentation/hid/rcsim.rst
 create mode 100644 drivers/hid/hid-rcsim.c

Benjamin Tissoires Aug. 23, 2022, 9:49 a.m. UTC | #1

Hi Marcus,

[and sorry for the delay in the review of your patches]

On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Several RC Simulator Controllers are HID compliant with similar
> interface.
>
> Add support for these controllers:
>  - Phoenix RC (HID variant)
>  - Car VRC2.0
>  - Real Flight G5/G6/G7
>  - Aero Fly, FMS
>  - OrangeRX FrSky
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  Documentation/hid/index.rst |   1 +
>  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
>  drivers/hid/Kconfig         |  11 ++
>  drivers/hid/Makefile        |   1 +
>  drivers/hid/hid-ids.h       |   5 +
>  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
>  6 files changed, 475 insertions(+)
>  create mode 100644 Documentation/hid/rcsim.rst
>  create mode 100644 drivers/hid/hid-rcsim.c
>
> diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> index e50f513c579c..e5813d264f37 100644
> --- a/Documentation/hid/index.rst
> +++ b/Documentation/hid/index.rst
> @@ -17,3 +17,4 @@ Human Interface Devices (HID)
>     hid-alps
>     intel-ish-hid
>     amd-sfh-hid
> +   rcsim
> diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> new file mode 100644
> index 000000000000..1a031f7189cb
> --- /dev/null
> +++ b/Documentation/hid/rcsim.rst
> @@ -0,0 +1,142 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=================================
> +rcsim - RC Simulator Controllers
> +=================================
> +
> +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> +
> +This driver let you use your own RC controller plugged
> +into your computer using an HID compatible USB dongle.
> +
> +There are several HID compatible USB dongles from different
> +vendors. The driver currently supports:
> +
> +- Phoenix RC (HID variant) (8ch)
> +- Car VRC2.0 (2ch)
> +- Real Flight G5/G6/G7 (6ch)
> +- Aero Fly, FMS (8ch)
> +- OrangeRX FrSky (6ch)
> +
> +Many RC controllers is able to configure which stick goes to which channel.
> +This is also configurable in most simulators, so a matching is not necessary.
> +
> +Supported dongles
> +==================
> +
> +PhoenixRC
> +----------
> +
> +The PhoenixRC has one HID compatible variant which is supported by this driver.
> +The controller has support for 8 analog channels.
> +
> +The driver is generating the following input event for on channels:
> +
> ++---------+----------------+
> +| Channel |      Event     |
> ++=========+================+
> +|     1   |  ABS_Y         |
> ++---------+----------------+
> +|     2   |  ABS_X         |
> ++---------+----------------+
> +|     3   |  ABS_RY        |
> ++---------+----------------+
> +|     4   |  ABS_RX        |
> ++---------+----------------+
> +|     5   |  ABS_RUDDER    |
> ++---------+----------------+
> +|     6   |  ABS_THROTTLE  |
> ++---------+----------------+
> +|     7   |  ABS_Z         |
> ++---------+----------------+
> +|     8   |  ABS_RZ        |
> ++---------+----------------+
> +
> +VRC2.0
> +----------
> +VRC2.0 is a controller for RC Cars.
> +The controller has support for 2 analog channels.
> +
> +The driver is generating the following input event for on channels:
> +
> ++---------+----------------+
> +| Channel |      Event     |
> ++=========+================+
> +|     1   |  ABS_GAS       |
> ++---------+----------------+
> +|     2   |  ABS_WHEEL     |
> ++---------+----------------+
> +
> +RealFlight
> +----------
> +
> +This driver supports Realflight G4-G7 and above
> +The controller has support for 4 analog channels and two buttons.
> +
> +The driver is generating the following input event for on channels:
> +
> ++---------+----------------+
> +| Channel |      Event     |
> ++=========+================+
> +|     1   |  ABS_Y         |
> ++---------+----------------+
> +|     2   |  ABS_X         |
> ++---------+----------------+
> +|     3   |  ABS_RY        |
> ++---------+----------------+
> +|     4   |  ABS_RX        |
> ++---------+----------------+
> +|     5   |  BTN_A         |
> ++---------+----------------+
> +|     6   |  BTN_B         |
> ++---------+----------------+
> +
> +XTR+G2+FMS Controllers
> +--------------------------------
> +
> +The controllers has support for 8 analog channels.
> +
> +The driver is generating the following input event for on channels:
> +
> ++---------+----------------+
> +| Channel |      Event     |
> ++=========+================+
> +|     1   |  ABS_Y         |
> ++---------+----------------+
> +|     2   |  ABS_X         |
> ++---------+----------------+
> +|     3   |  ABS_RY        |
> ++---------+----------------+
> +|     4   |  ABS_RX        |
> ++---------+----------------+
> +|     5   |  ABS_RUDDER    |
> ++---------+----------------+
> +|     6   |  ABS_THROTTLE  |
> ++---------+----------------+
> +|     7   |  ABS_Z         |
> ++---------+----------------+
> +|     8   |  ABS_RZ        |
> ++---------+----------------+
> +
> +OrangeRX
> +----------
> +
> +The controllers has support for 6 analog channels.
> +
> +The driver is generating the following input event for on channels:
> +
> ++---------+----------------+
> +| Channel |      Event     |
> ++=========+================+
> +|     1   |  ABS_Y         |
> ++---------+----------------+
> +|     2   |  ABS_X         |
> ++---------+----------------+
> +|     3   |  ABS_RY        |
> ++---------+----------------+
> +|     4   |  ABS_RX        |
> ++---------+----------------+
> +|     5   |  ABS_RUDDER    |
> ++---------+----------------+
> +|     6   |  ABS_THROTTLE  |
> ++---------+----------------+
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 70da5931082f..d8313d36086c 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -957,6 +957,17 @@ config HID_RAZER
>         Support for Razer devices that are not fully compliant with the
>         HID standard.
>
> +config HID_RCSIM
> +       tristate "RC Simulator Controllers"
> +       depends on HID
> +       help
> +       Support for several HID compatible RC Simulator Controllers including
> +         - Phoenix RC
> +         - Car VRC2.0
> +         - Real Flight
> +         - Aero Fly, FMS
> +         - OrangeRX FrSky
> +
>  config HID_PRIMAX
>         tristate "Primax non-fully HID-compliant devices"
>         depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index cac2cbe26d11..85d50ab352ee 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
>  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
>  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
>  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o

General rule of thumbs, we try to name the drivers after their
vendors, unless we know we have a generic driver.

Here, this driver seems to be really tied to a small set of devices,
and thus I don't think we can call it "generic".

>  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
>  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
>  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index d9eb676abe96..baf5f74d5bed 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1381,6 +1381,11 @@
>
>  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
>  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> +
> +#define USB_VENDOR_ID_DIPLING  0x0B9B
> +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
>
>  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
>  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> new file mode 100644
> index 000000000000..0f214cb5816a
> --- /dev/null
> +++ b/drivers/hid/hid-rcsim.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for several HID compatible RC Simulator Controllers.
> + * Currently supported controllers are:
> + *
> + * - Phoenix RC (HID variant) (8ch)
> + * - Car VRC2.0 (2ch)
> + * - Real Flight G5/G6/G7 (6ch)
> + * - Aero Fly, FMS (8ch)
> + * - OrangeRX FrSky (6ch)
> + *
> + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +
> +#include "hid-ids.h"
> +
> +/*
> + * Some of these VID/PID are probably "borrowed", so keep them locally and
> + * do not populate hid-ids.h with those.
> + */
> +
> +/* PHOENIXRC Controlloer (HID variant) */
> +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> +#define PHOENIXRC_DSIZE        (8)
> +
> +/* VRC2 Controlloer */
> +#define VRC2_VID       (0x07c0)
> +#define VRC2_PID       (0x1125)
> +#define VRC2_DSIZE     (7)
> +
> +/* Realflight G4-&7 and Above Controller */
> +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> +#define REALFLIGHT_DSIZE       (8)
> +
> +#define REALFLIGHT_BTN_A       BIT(0)
> +#define REALFLIGHT_BTN_B       BIT(1)
> +
> +/* XTR+G2+FMS Controller */
> +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> +#define XTRG2FMS_DSIZE (8)
> +
> +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> +
> +/* OrangeRX FrSky */
> +#define ORANGERX_VID   (0x0451)
> +#define ORANGERX_PID   (0x16a5)
> +#define ORANGERX_DSIZE (8)
> +
> +enum rcsim_controller {
> +       PHOENIXRC,
> +       VRC2,
> +       REALFLIGHT,
> +       XTRG2FMS,
> +       ORANGERX
> +};
> +
> +struct rcsim_priv {
> +       struct hid_device *hdev;
> +       struct input_dev *input;
> +       enum rcsim_controller controller;
> +       u8 alt;
> +};
> +
> +static int rcsim_open(struct input_dev *dev)
> +{
> +       struct rcsim_priv *priv = input_get_drvdata(dev);
> +
> +       return hid_hw_open(priv->hdev);
> +}
> +
> +static void rcsim_close(struct input_dev *dev)
> +{
> +       struct rcsim_priv *priv = input_get_drvdata(dev);
> +
> +       hid_hw_close(priv->hdev);
> +}
> +
> +static int rcsim_setup_input(struct rcsim_priv *priv)
> +{
> +       struct input_dev *input;
> +
> +       input = devm_input_allocate_device(&priv->hdev->dev);
> +       if (!input)
> +               return -ENOMEM;
> +
> +       input->id.bustype = priv->hdev->bus;
> +       input->id.vendor  = priv->hdev->vendor;
> +       input->id.product = priv->hdev->product;
> +       input->id.version = priv->hdev->bus;
> +       input->phys = priv->hdev->phys;
> +       input->uniq = priv->hdev->uniq;
> +       input->open = rcsim_open;
> +       input->close = rcsim_close;
> +
> +       input_set_drvdata(input, priv);
> +
> +       switch (priv->controller) {
> +       case PHOENIXRC:
> +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> +               input->name = "RC Simuator Controller PhoenixRC";
> +               break;
> +       case VRC2:
> +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> +               input->name = "RC Simuator Controller VRC2.0";
> +               break;
> +       case REALFLIGHT:
> +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> +               input_set_capability(input, EV_KEY, BTN_A);
> +               input_set_capability(input, EV_KEY, BTN_B);
> +               input->name = "RC Simuator Controller Realflight";
> +               break;
> +       case XTRG2FMS:
> +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> +               input->name = "RC Simuator Controller AeroFly, FMS";
> +               priv->alt = 0;
> +               break;
> +       case ORANGERX:
> +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> +               input->name = "RC Simuator Controller OrangeRX FrSky";
> +               break;
> +       };
> +
> +       priv->input = input;
> +       return input_register_device(priv->input);
> +}

You are basically rewriting hid-input.c, which is suboptimal.

I guess the report descriptor provided by these devices are basically
useless, and so you have to parse the reports yourself in the
raw_event callback.

But instead of manually doing that, why not overwrite the report
descriptor (with .rdesc_fixup) and declare here all of the data that
needs to be exported. You could remove basically everything in this
driver by just providing a fixed report descriptor.

> +
> +static int rcsim_raw_event(struct hid_device *hdev,
> +                              struct hid_report *report,
> +                              u8 *raw_data, int size)
> +{
> +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> +       u16 value;
> +
> +       switch (priv->controller) {
> +       case PHOENIXRC:
> +               if (size != PHOENIXRC_DSIZE)
> +                       break;
> +
> +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> +
> +               /* Z and RZ are sent every other time */
> +               if (priv->alt)
> +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> +               else
> +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> +
> +               priv->alt ^= 1;
> +               break;
> +       case VRC2:
> +               if (size != VRC2_DSIZE)
> +                       break;
> +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> +               input_report_abs(priv->input, ABS_GAS, value);
> +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> +               input_report_abs(priv->input, ABS_WHEEL, value);
> +               break;
> +       case REALFLIGHT:
> +               if (size != REALFLIGHT_DSIZE)
> +                       break;
> +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> +               input_report_key(priv->input, BTN_A,
> +                               raw_data[7] & REALFLIGHT_BTN_A);
> +               input_report_key(priv->input, BTN_B,
> +                               raw_data[7] & REALFLIGHT_BTN_B);
> +               break;
> +       case XTRG2FMS:
> +               if (size != XTRG2FMS_DSIZE)
> +                       break;
> +
> +               /* X, RX, Y and RY are sent every time */
> +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> +               value = (value << 8) | raw_data[1];
> +               input_report_abs(priv->input, ABS_X, value);
> +
> +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> +               value = (value << 8) | raw_data[2];
> +               input_report_abs(priv->input, ABS_Y, value);
> +
> +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> +               value = (value << 8) | raw_data[0];
> +               input_report_abs(priv->input, ABS_RX, value);
> +
> +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> +               value = (value << 8) | raw_data[4];
> +               input_report_abs(priv->input, ABS_RY, value);
> +
> +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> +               value = (value << 8) | raw_data[6];
> +               if (priv->alt)
> +                       input_report_abs(priv->input, ABS_Z, value);
> +               else
> +                       input_report_abs(priv->input, ABS_RUDDER, value);
> +
> +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> +               value = (value << 8) | raw_data[5];
> +               if (priv->alt)
> +                       input_report_abs(priv->input, ABS_RZ, value);
> +               else
> +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> +
> +               priv->alt ^= 1;
> +               break;
> +       case ORANGERX:
> +               if (size != ORANGERX_DSIZE)
> +                       break;
> +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> +               break;
> +       };
> +
> +       input_sync(priv->input);
> +       return 0;
> +}
> +
> +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +       struct device *dev = &hdev->dev;
> +       struct rcsim_priv *priv;
> +       int ret;
> +
> +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> +               return -ENODEV;

You are not accessing anything in the USB stack, so there is no need
to prevent regression tests that could inject uhid devices to your
drivers.

Cheers,
Benjamin

> +
> +       ret = hid_parse(hdev);
> +       if (ret)
> +               return ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->hdev = hdev;
> +       priv->controller = id->driver_data;
> +       hid_set_drvdata(hdev, priv);
> +
> +       ret = rcsim_setup_input(priv);
> +       if (ret)
> +               return ret;
> +
> +       return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +}
> +
> +static const struct hid_device_id rcsim_devices[] = {
> +       { HID_USB_DEVICE(PHOENIXRC_VID, PHOENIXRC_PID), .driver_data = PHOENIXRC },
> +       { HID_USB_DEVICE(VRC2_VID, VRC2_PID), .driver_data = VRC2 },
> +       { HID_USB_DEVICE(REALFLIGHT_VID, REALFLIGHT_PID), .driver_data = REALFLIGHT },
> +       { HID_USB_DEVICE(XTRG2FMS_VID, XTRG2FMS_PID), .driver_data = XTRG2FMS },
> +       { HID_USB_DEVICE(ORANGERX_VID, ORANGERX_PID), .driver_data = ORANGERX },
> +       { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(hid, rcsim_devices);
> +
> +static struct hid_driver rcsim_driver = {
> +       .name = "rcsim",
> +       .id_table = rcsim_devices,
> +       .probe = rcsim_probe,
> +       .raw_event = rcsim_raw_event,
> +};
> +module_hid_driver(rcsim_driver);
> +
> +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.37.1
>

Marcus Folkesson Aug. 23, 2022, 3:18 p.m. UTC | #2

Thank you  Benjamin,

On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> Hi Marcus,
> 
> [and sorry for the delay in the review of your patches]
> 
> On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > Several RC Simulator Controllers are HID compliant with similar
> > interface.
> >
> > Add support for these controllers:
> >  - Phoenix RC (HID variant)
> >  - Car VRC2.0
> >  - Real Flight G5/G6/G7
> >  - Aero Fly, FMS
> >  - OrangeRX FrSky
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >  Documentation/hid/index.rst |   1 +
> >  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
> >  drivers/hid/Kconfig         |  11 ++
> >  drivers/hid/Makefile        |   1 +
> >  drivers/hid/hid-ids.h       |   5 +
> >  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
> >  6 files changed, 475 insertions(+)
> >  create mode 100644 Documentation/hid/rcsim.rst
> >  create mode 100644 drivers/hid/hid-rcsim.c
> >
> > diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> > index e50f513c579c..e5813d264f37 100644
> > --- a/Documentation/hid/index.rst
> > +++ b/Documentation/hid/index.rst
> > @@ -17,3 +17,4 @@ Human Interface Devices (HID)
> >     hid-alps
> >     intel-ish-hid
> >     amd-sfh-hid
> > +   rcsim
> > diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> > new file mode 100644
> > index 000000000000..1a031f7189cb
> > --- /dev/null
> > +++ b/Documentation/hid/rcsim.rst
> > @@ -0,0 +1,142 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=================================
> > +rcsim - RC Simulator Controllers
> > +=================================
> > +
> > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +
> > +This driver let you use your own RC controller plugged
> > +into your computer using an HID compatible USB dongle.
> > +
> > +There are several HID compatible USB dongles from different
> > +vendors. The driver currently supports:
> > +
> > +- Phoenix RC (HID variant) (8ch)
> > +- Car VRC2.0 (2ch)
> > +- Real Flight G5/G6/G7 (6ch)
> > +- Aero Fly, FMS (8ch)
> > +- OrangeRX FrSky (6ch)
> > +
> > +Many RC controllers is able to configure which stick goes to which channel.
> > +This is also configurable in most simulators, so a matching is not necessary.
> > +
> > +Supported dongles
> > +==================
> > +
> > +PhoenixRC
> > +----------
> > +
> > +The PhoenixRC has one HID compatible variant which is supported by this driver.
> > +The controller has support for 8 analog channels.
> > +
> > +The driver is generating the following input event for on channels:
> > +
> > ++---------+----------------+
> > +| Channel |      Event     |
> > ++=========+================+
> > +|     1   |  ABS_Y         |
> > ++---------+----------------+
> > +|     2   |  ABS_X         |
> > ++---------+----------------+
> > +|     3   |  ABS_RY        |
> > ++---------+----------------+
> > +|     4   |  ABS_RX        |
> > ++---------+----------------+
> > +|     5   |  ABS_RUDDER    |
> > ++---------+----------------+
> > +|     6   |  ABS_THROTTLE  |
> > ++---------+----------------+
> > +|     7   |  ABS_Z         |
> > ++---------+----------------+
> > +|     8   |  ABS_RZ        |
> > ++---------+----------------+
> > +
> > +VRC2.0
> > +----------
> > +VRC2.0 is a controller for RC Cars.
> > +The controller has support for 2 analog channels.
> > +
> > +The driver is generating the following input event for on channels:
> > +
> > ++---------+----------------+
> > +| Channel |      Event     |
> > ++=========+================+
> > +|     1   |  ABS_GAS       |
> > ++---------+----------------+
> > +|     2   |  ABS_WHEEL     |
> > ++---------+----------------+
> > +
> > +RealFlight
> > +----------
> > +
> > +This driver supports Realflight G4-G7 and above
> > +The controller has support for 4 analog channels and two buttons.
> > +
> > +The driver is generating the following input event for on channels:
> > +
> > ++---------+----------------+
> > +| Channel |      Event     |
> > ++=========+================+
> > +|     1   |  ABS_Y         |
> > ++---------+----------------+
> > +|     2   |  ABS_X         |
> > ++---------+----------------+
> > +|     3   |  ABS_RY        |
> > ++---------+----------------+
> > +|     4   |  ABS_RX        |
> > ++---------+----------------+
> > +|     5   |  BTN_A         |
> > ++---------+----------------+
> > +|     6   |  BTN_B         |
> > ++---------+----------------+
> > +
> > +XTR+G2+FMS Controllers
> > +--------------------------------
> > +
> > +The controllers has support for 8 analog channels.
> > +
> > +The driver is generating the following input event for on channels:
> > +
> > ++---------+----------------+
> > +| Channel |      Event     |
> > ++=========+================+
> > +|     1   |  ABS_Y         |
> > ++---------+----------------+
> > +|     2   |  ABS_X         |
> > ++---------+----------------+
> > +|     3   |  ABS_RY        |
> > ++---------+----------------+
> > +|     4   |  ABS_RX        |
> > ++---------+----------------+
> > +|     5   |  ABS_RUDDER    |
> > ++---------+----------------+
> > +|     6   |  ABS_THROTTLE  |
> > ++---------+----------------+
> > +|     7   |  ABS_Z         |
> > ++---------+----------------+
> > +|     8   |  ABS_RZ        |
> > ++---------+----------------+
> > +
> > +OrangeRX
> > +----------
> > +
> > +The controllers has support for 6 analog channels.
> > +
> > +The driver is generating the following input event for on channels:
> > +
> > ++---------+----------------+
> > +| Channel |      Event     |
> > ++=========+================+
> > +|     1   |  ABS_Y         |
> > ++---------+----------------+
> > +|     2   |  ABS_X         |
> > ++---------+----------------+
> > +|     3   |  ABS_RY        |
> > ++---------+----------------+
> > +|     4   |  ABS_RX        |
> > ++---------+----------------+
> > +|     5   |  ABS_RUDDER    |
> > ++---------+----------------+
> > +|     6   |  ABS_THROTTLE  |
> > ++---------+----------------+
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 70da5931082f..d8313d36086c 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -957,6 +957,17 @@ config HID_RAZER
> >         Support for Razer devices that are not fully compliant with the
> >         HID standard.
> >
> > +config HID_RCSIM
> > +       tristate "RC Simulator Controllers"
> > +       depends on HID
> > +       help
> > +       Support for several HID compatible RC Simulator Controllers including
> > +         - Phoenix RC
> > +         - Car VRC2.0
> > +         - Real Flight
> > +         - Aero Fly, FMS
> > +         - OrangeRX FrSky
> > +
> >  config HID_PRIMAX
> >         tristate "Primax non-fully HID-compliant devices"
> >         depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index cac2cbe26d11..85d50ab352ee 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
> >  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
> >  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> >  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> > +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o
> 
> General rule of thumbs, we try to name the drivers after their
> vendors, unless we know we have a generic driver.
> 
> Here, this driver seems to be really tied to a small set of devices,
> and thus I don't think we can call it "generic".

Got it.

> 
> >  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
> >  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> >  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index d9eb676abe96..baf5f74d5bed 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -1381,6 +1381,11 @@
> >
> >  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
> >  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> > +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> > +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> > +
> > +#define USB_VENDOR_ID_DIPLING  0x0B9B
> > +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
> >
> >  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
> >  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> > diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> > new file mode 100644
> > index 000000000000..0f214cb5816a
> > --- /dev/null
> > +++ b/drivers/hid/hid-rcsim.c
> > @@ -0,0 +1,315 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Driver for several HID compatible RC Simulator Controllers.
> > + * Currently supported controllers are:
> > + *
> > + * - Phoenix RC (HID variant) (8ch)
> > + * - Car VRC2.0 (2ch)
> > + * - Real Flight G5/G6/G7 (6ch)
> > + * - Aero Fly, FMS (8ch)
> > + * - OrangeRX FrSky (6ch)
> > + *
> > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/usb.h>
> > +
> > +#include "hid-ids.h"
> > +
> > +/*
> > + * Some of these VID/PID are probably "borrowed", so keep them locally and
> > + * do not populate hid-ids.h with those.
> > + */
> > +
> > +/* PHOENIXRC Controlloer (HID variant) */
> > +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> > +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> > +#define PHOENIXRC_DSIZE        (8)
> > +
> > +/* VRC2 Controlloer */
> > +#define VRC2_VID       (0x07c0)
> > +#define VRC2_PID       (0x1125)
> > +#define VRC2_DSIZE     (7)
> > +
> > +/* Realflight G4-&7 and Above Controller */
> > +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> > +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> > +#define REALFLIGHT_DSIZE       (8)
> > +
> > +#define REALFLIGHT_BTN_A       BIT(0)
> > +#define REALFLIGHT_BTN_B       BIT(1)
> > +
> > +/* XTR+G2+FMS Controller */
> > +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> > +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> > +#define XTRG2FMS_DSIZE (8)
> > +
> > +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> > +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> > +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> > +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> > +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> > +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> > +
> > +/* OrangeRX FrSky */
> > +#define ORANGERX_VID   (0x0451)
> > +#define ORANGERX_PID   (0x16a5)
> > +#define ORANGERX_DSIZE (8)
> > +
> > +enum rcsim_controller {
> > +       PHOENIXRC,
> > +       VRC2,
> > +       REALFLIGHT,
> > +       XTRG2FMS,
> > +       ORANGERX
> > +};
> > +
> > +struct rcsim_priv {
> > +       struct hid_device *hdev;
> > +       struct input_dev *input;
> > +       enum rcsim_controller controller;
> > +       u8 alt;
> > +};
> > +
> > +static int rcsim_open(struct input_dev *dev)
> > +{
> > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > +
> > +       return hid_hw_open(priv->hdev);
> > +}
> > +
> > +static void rcsim_close(struct input_dev *dev)
> > +{
> > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > +
> > +       hid_hw_close(priv->hdev);
> > +}
> > +
> > +static int rcsim_setup_input(struct rcsim_priv *priv)
> > +{
> > +       struct input_dev *input;
> > +
> > +       input = devm_input_allocate_device(&priv->hdev->dev);
> > +       if (!input)
> > +               return -ENOMEM;
> > +
> > +       input->id.bustype = priv->hdev->bus;
> > +       input->id.vendor  = priv->hdev->vendor;
> > +       input->id.product = priv->hdev->product;
> > +       input->id.version = priv->hdev->bus;
> > +       input->phys = priv->hdev->phys;
> > +       input->uniq = priv->hdev->uniq;
> > +       input->open = rcsim_open;
> > +       input->close = rcsim_close;
> > +
> > +       input_set_drvdata(input, priv);
> > +
> > +       switch (priv->controller) {
> > +       case PHOENIXRC:
> > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > +               input->name = "RC Simuator Controller PhoenixRC";
> > +               break;
> > +       case VRC2:
> > +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> > +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> > +               input->name = "RC Simuator Controller VRC2.0";
> > +               break;
> > +       case REALFLIGHT:
> > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > +               input_set_capability(input, EV_KEY, BTN_A);
> > +               input_set_capability(input, EV_KEY, BTN_B);
> > +               input->name = "RC Simuator Controller Realflight";
> > +               break;
> > +       case XTRG2FMS:
> > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> > +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> > +               input->name = "RC Simuator Controller AeroFly, FMS";
> > +               priv->alt = 0;
> > +               break;
> > +       case ORANGERX:
> > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > +               input->name = "RC Simuator Controller OrangeRX FrSky";
> > +               break;
> > +       };
> > +
> > +       priv->input = input;
> > +       return input_register_device(priv->input);
> > +}
> 
> You are basically rewriting hid-input.c, which is suboptimal.

Ouch. I will have a look at hid-input, thanks.

> 
> I guess the report descriptor provided by these devices are basically
> useless, and so you have to parse the reports yourself in the
> raw_event callback.

Yep.

> 
> But instead of manually doing that, why not overwrite the report
> descriptor (with .rdesc_fixup) and declare here all of the data that
 Do you mean .report_fixup?

> needs to be exported. You could remove basically everything in this
> driver by just providing a fixed report descriptor.

What you are aiming for is to fixup the report descriptor and let the
generic hid-raw driver handle the rest, or do I get you wrong?

How is the report mapped to certain events then?

I do read at [1] but it is not obvious how to put it together.
Most drivers I've looked at that is using .report_fixup just fix broken
reports. I guess these reports are not "broken", just.. odd?


> 
> > +
> > +static int rcsim_raw_event(struct hid_device *hdev,
> > +                              struct hid_report *report,
> > +                              u8 *raw_data, int size)
> > +{
> > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > +       u16 value;
> > +
> > +       switch (priv->controller) {
> > +       case PHOENIXRC:
> > +               if (size != PHOENIXRC_DSIZE)
> > +                       break;
> > +
> > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > +
> > +               /* Z and RZ are sent every other time */
> > +               if (priv->alt)
> > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > +               else
> > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > +
> > +               priv->alt ^= 1;
> > +               break;
> > +       case VRC2:
> > +               if (size != VRC2_DSIZE)
> > +                       break;
> > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > +               input_report_abs(priv->input, ABS_GAS, value);
> > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > +               break;
> > +       case REALFLIGHT:
> > +               if (size != REALFLIGHT_DSIZE)
> > +                       break;
> > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > +               input_report_key(priv->input, BTN_A,
> > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > +               input_report_key(priv->input, BTN_B,
> > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > +               break;
> > +       case XTRG2FMS:
> > +               if (size != XTRG2FMS_DSIZE)
> > +                       break;
> > +
> > +               /* X, RX, Y and RY are sent every time */
> > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > +               value = (value << 8) | raw_data[1];
> > +               input_report_abs(priv->input, ABS_X, value);
> > +
> > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > +               value = (value << 8) | raw_data[2];
> > +               input_report_abs(priv->input, ABS_Y, value);
> > +
> > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > +               value = (value << 8) | raw_data[0];
> > +               input_report_abs(priv->input, ABS_RX, value);
> > +
> > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > +               value = (value << 8) | raw_data[4];
> > +               input_report_abs(priv->input, ABS_RY, value);
> > +
> > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > +               value = (value << 8) | raw_data[6];
> > +               if (priv->alt)
> > +                       input_report_abs(priv->input, ABS_Z, value);
> > +               else
> > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > +
> > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > +               value = (value << 8) | raw_data[5];
> > +               if (priv->alt)
> > +                       input_report_abs(priv->input, ABS_RZ, value);
> > +               else
> > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > +
> > +               priv->alt ^= 1;
> > +               break;
> > +       case ORANGERX:
> > +               if (size != ORANGERX_DSIZE)
> > +                       break;
> > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > +               break;
> > +       };
> > +
> > +       input_sync(priv->input);
> > +       return 0;
> > +}
> > +
> > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > +       struct device *dev = &hdev->dev;
> > +       struct rcsim_priv *priv;
> > +       int ret;
> > +
> > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > +               return -ENODEV;
> 
> You are not accessing anything in the USB stack, so there is no need
> to prevent regression tests that could inject uhid devices to your
> drivers.

Ok, thanks.

> 
> Cheers,
> Benjamin
> 

Best regards,
Marcus Folkesson

[1] https://www.usb.org/hid

Benjamin Tissoires Aug. 23, 2022, 4:43 p.m. UTC | #3

On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Thank you  Benjamin,
>
> On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > Hi Marcus,
> >
> > [and sorry for the delay in the review of your patches]
> >
> > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:
> > >
> > > Several RC Simulator Controllers are HID compliant with similar
> > > interface.
> > >
> > > Add support for these controllers:
> > >  - Phoenix RC (HID variant)
> > >  - Car VRC2.0
> > >  - Real Flight G5/G6/G7
> > >  - Aero Fly, FMS
> > >  - OrangeRX FrSky
> > >
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > ---
> > >  Documentation/hid/index.rst |   1 +
> > >  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
> > >  drivers/hid/Kconfig         |  11 ++
> > >  drivers/hid/Makefile        |   1 +
> > >  drivers/hid/hid-ids.h       |   5 +
> > >  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
> > >  6 files changed, 475 insertions(+)
> > >  create mode 100644 Documentation/hid/rcsim.rst
> > >  create mode 100644 drivers/hid/hid-rcsim.c
> > >
> > > diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> > > index e50f513c579c..e5813d264f37 100644
> > > --- a/Documentation/hid/index.rst
> > > +++ b/Documentation/hid/index.rst
> > > @@ -17,3 +17,4 @@ Human Interface Devices (HID)
> > >     hid-alps
> > >     intel-ish-hid
> > >     amd-sfh-hid
> > > +   rcsim
> > > diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> > > new file mode 100644
> > > index 000000000000..1a031f7189cb
> > > --- /dev/null
> > > +++ b/Documentation/hid/rcsim.rst
> > > @@ -0,0 +1,142 @@
> > > +.. SPDX-License-Identifier: GPL-2.0
> > > +
> > > +=================================
> > > +rcsim - RC Simulator Controllers
> > > +=================================
> > > +
> > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > +
> > > +This driver let you use your own RC controller plugged
> > > +into your computer using an HID compatible USB dongle.
> > > +
> > > +There are several HID compatible USB dongles from different
> > > +vendors. The driver currently supports:
> > > +
> > > +- Phoenix RC (HID variant) (8ch)
> > > +- Car VRC2.0 (2ch)
> > > +- Real Flight G5/G6/G7 (6ch)
> > > +- Aero Fly, FMS (8ch)
> > > +- OrangeRX FrSky (6ch)
> > > +
> > > +Many RC controllers is able to configure which stick goes to which channel.
> > > +This is also configurable in most simulators, so a matching is not necessary.
> > > +
> > > +Supported dongles
> > > +==================
> > > +
> > > +PhoenixRC
> > > +----------
> > > +
> > > +The PhoenixRC has one HID compatible variant which is supported by this driver.
> > > +The controller has support for 8 analog channels.
> > > +
> > > +The driver is generating the following input event for on channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel |      Event     |
> > > ++=========+================+
> > > +|     1   |  ABS_Y         |
> > > ++---------+----------------+
> > > +|     2   |  ABS_X         |
> > > ++---------+----------------+
> > > +|     3   |  ABS_RY        |
> > > ++---------+----------------+
> > > +|     4   |  ABS_RX        |
> > > ++---------+----------------+
> > > +|     5   |  ABS_RUDDER    |
> > > ++---------+----------------+
> > > +|     6   |  ABS_THROTTLE  |
> > > ++---------+----------------+
> > > +|     7   |  ABS_Z         |
> > > ++---------+----------------+
> > > +|     8   |  ABS_RZ        |
> > > ++---------+----------------+
> > > +
> > > +VRC2.0
> > > +----------
> > > +VRC2.0 is a controller for RC Cars.
> > > +The controller has support for 2 analog channels.
> > > +
> > > +The driver is generating the following input event for on channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel |      Event     |
> > > ++=========+================+
> > > +|     1   |  ABS_GAS       |
> > > ++---------+----------------+
> > > +|     2   |  ABS_WHEEL     |
> > > ++---------+----------------+
> > > +
> > > +RealFlight
> > > +----------
> > > +
> > > +This driver supports Realflight G4-G7 and above
> > > +The controller has support for 4 analog channels and two buttons.
> > > +
> > > +The driver is generating the following input event for on channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel |      Event     |
> > > ++=========+================+
> > > +|     1   |  ABS_Y         |
> > > ++---------+----------------+
> > > +|     2   |  ABS_X         |
> > > ++---------+----------------+
> > > +|     3   |  ABS_RY        |
> > > ++---------+----------------+
> > > +|     4   |  ABS_RX        |
> > > ++---------+----------------+
> > > +|     5   |  BTN_A         |
> > > ++---------+----------------+
> > > +|     6   |  BTN_B         |
> > > ++---------+----------------+
> > > +
> > > +XTR+G2+FMS Controllers
> > > +--------------------------------
> > > +
> > > +The controllers has support for 8 analog channels.
> > > +
> > > +The driver is generating the following input event for on channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel |      Event     |
> > > ++=========+================+
> > > +|     1   |  ABS_Y         |
> > > ++---------+----------------+
> > > +|     2   |  ABS_X         |
> > > ++---------+----------------+
> > > +|     3   |  ABS_RY        |
> > > ++---------+----------------+
> > > +|     4   |  ABS_RX        |
> > > ++---------+----------------+
> > > +|     5   |  ABS_RUDDER    |
> > > ++---------+----------------+
> > > +|     6   |  ABS_THROTTLE  |
> > > ++---------+----------------+
> > > +|     7   |  ABS_Z         |
> > > ++---------+----------------+
> > > +|     8   |  ABS_RZ        |
> > > ++---------+----------------+
> > > +
> > > +OrangeRX
> > > +----------
> > > +
> > > +The controllers has support for 6 analog channels.
> > > +
> > > +The driver is generating the following input event for on channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel |      Event     |
> > > ++=========+================+
> > > +|     1   |  ABS_Y         |
> > > ++---------+----------------+
> > > +|     2   |  ABS_X         |
> > > ++---------+----------------+
> > > +|     3   |  ABS_RY        |
> > > ++---------+----------------+
> > > +|     4   |  ABS_RX        |
> > > ++---------+----------------+
> > > +|     5   |  ABS_RUDDER    |
> > > ++---------+----------------+
> > > +|     6   |  ABS_THROTTLE  |
> > > ++---------+----------------+
> > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > index 70da5931082f..d8313d36086c 100644
> > > --- a/drivers/hid/Kconfig
> > > +++ b/drivers/hid/Kconfig
> > > @@ -957,6 +957,17 @@ config HID_RAZER
> > >         Support for Razer devices that are not fully compliant with the
> > >         HID standard.
> > >
> > > +config HID_RCSIM
> > > +       tristate "RC Simulator Controllers"
> > > +       depends on HID
> > > +       help
> > > +       Support for several HID compatible RC Simulator Controllers including
> > > +         - Phoenix RC
> > > +         - Car VRC2.0
> > > +         - Real Flight
> > > +         - Aero Fly, FMS
> > > +         - OrangeRX FrSky
> > > +
> > >  config HID_PRIMAX
> > >         tristate "Primax non-fully HID-compliant devices"
> > >         depends on HID
> > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > index cac2cbe26d11..85d50ab352ee 100644
> > > --- a/drivers/hid/Makefile
> > > +++ b/drivers/hid/Makefile
> > > @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
> > >  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
> > >  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> > >  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> > > +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o
> >
> > General rule of thumbs, we try to name the drivers after their
> > vendors, unless we know we have a generic driver.
> >
> > Here, this driver seems to be really tied to a small set of devices,
> > and thus I don't think we can call it "generic".
>
> Got it.
>
> >
> > >  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
> > >  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> > >  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > index d9eb676abe96..baf5f74d5bed 100644
> > > --- a/drivers/hid/hid-ids.h
> > > +++ b/drivers/hid/hid-ids.h
> > > @@ -1381,6 +1381,11 @@
> > >
> > >  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
> > >  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> > > +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> > > +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> > > +
> > > +#define USB_VENDOR_ID_DIPLING  0x0B9B
> > > +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
> > >
> > >  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
> > >  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> > > diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> > > new file mode 100644
> > > index 000000000000..0f214cb5816a
> > > --- /dev/null
> > > +++ b/drivers/hid/hid-rcsim.c
> > > @@ -0,0 +1,315 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Driver for several HID compatible RC Simulator Controllers.
> > > + * Currently supported controllers are:
> > > + *
> > > + * - Phoenix RC (HID variant) (8ch)
> > > + * - Car VRC2.0 (2ch)
> > > + * - Real Flight G5/G6/G7 (6ch)
> > > + * - Aero Fly, FMS (8ch)
> > > + * - OrangeRX FrSky (6ch)
> > > + *
> > > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/device.h>
> > > +#include <linux/input.h>
> > > +#include <linux/hid.h>
> > > +#include <linux/module.h>
> > > +#include <linux/usb.h>
> > > +
> > > +#include "hid-ids.h"
> > > +
> > > +/*
> > > + * Some of these VID/PID are probably "borrowed", so keep them locally and
> > > + * do not populate hid-ids.h with those.
> > > + */
> > > +
> > > +/* PHOENIXRC Controlloer (HID variant) */
> > > +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> > > +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> > > +#define PHOENIXRC_DSIZE        (8)
> > > +
> > > +/* VRC2 Controlloer */
> > > +#define VRC2_VID       (0x07c0)
> > > +#define VRC2_PID       (0x1125)
> > > +#define VRC2_DSIZE     (7)
> > > +
> > > +/* Realflight G4-&7 and Above Controller */
> > > +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> > > +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> > > +#define REALFLIGHT_DSIZE       (8)
> > > +
> > > +#define REALFLIGHT_BTN_A       BIT(0)
> > > +#define REALFLIGHT_BTN_B       BIT(1)
> > > +
> > > +/* XTR+G2+FMS Controller */
> > > +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> > > +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> > > +#define XTRG2FMS_DSIZE (8)
> > > +
> > > +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> > > +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> > > +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> > > +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> > > +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> > > +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> > > +
> > > +/* OrangeRX FrSky */
> > > +#define ORANGERX_VID   (0x0451)
> > > +#define ORANGERX_PID   (0x16a5)
> > > +#define ORANGERX_DSIZE (8)
> > > +
> > > +enum rcsim_controller {
> > > +       PHOENIXRC,
> > > +       VRC2,
> > > +       REALFLIGHT,
> > > +       XTRG2FMS,
> > > +       ORANGERX
> > > +};
> > > +
> > > +struct rcsim_priv {
> > > +       struct hid_device *hdev;
> > > +       struct input_dev *input;
> > > +       enum rcsim_controller controller;
> > > +       u8 alt;
> > > +};
> > > +
> > > +static int rcsim_open(struct input_dev *dev)
> > > +{
> > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > +
> > > +       return hid_hw_open(priv->hdev);
> > > +}
> > > +
> > > +static void rcsim_close(struct input_dev *dev)
> > > +{
> > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > +
> > > +       hid_hw_close(priv->hdev);
> > > +}
> > > +
> > > +static int rcsim_setup_input(struct rcsim_priv *priv)
> > > +{
> > > +       struct input_dev *input;
> > > +
> > > +       input = devm_input_allocate_device(&priv->hdev->dev);
> > > +       if (!input)
> > > +               return -ENOMEM;
> > > +
> > > +       input->id.bustype = priv->hdev->bus;
> > > +       input->id.vendor  = priv->hdev->vendor;
> > > +       input->id.product = priv->hdev->product;
> > > +       input->id.version = priv->hdev->bus;
> > > +       input->phys = priv->hdev->phys;
> > > +       input->uniq = priv->hdev->uniq;
> > > +       input->open = rcsim_open;
> > > +       input->close = rcsim_close;
> > > +
> > > +       input_set_drvdata(input, priv);
> > > +
> > > +       switch (priv->controller) {
> > > +       case PHOENIXRC:
> > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > +               input->name = "RC Simuator Controller PhoenixRC";
> > > +               break;
> > > +       case VRC2:
> > > +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> > > +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> > > +               input->name = "RC Simuator Controller VRC2.0";
> > > +               break;
> > > +       case REALFLIGHT:
> > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > +               input_set_capability(input, EV_KEY, BTN_A);
> > > +               input_set_capability(input, EV_KEY, BTN_B);
> > > +               input->name = "RC Simuator Controller Realflight";
> > > +               break;
> > > +       case XTRG2FMS:
> > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> > > +               input->name = "RC Simuator Controller AeroFly, FMS";
> > > +               priv->alt = 0;
> > > +               break;
> > > +       case ORANGERX:
> > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > +               input->name = "RC Simuator Controller OrangeRX FrSky";
> > > +               break;
> > > +       };
> > > +
> > > +       priv->input = input;
> > > +       return input_register_device(priv->input);
> > > +}
> >
> > You are basically rewriting hid-input.c, which is suboptimal.
>
> Ouch. I will have a look at hid-input, thanks.
>
> >
> > I guess the report descriptor provided by these devices are basically
> > useless, and so you have to parse the reports yourself in the
> > raw_event callback.
>
> Yep.
>
> >
> > But instead of manually doing that, why not overwrite the report
> > descriptor (with .rdesc_fixup) and declare here all of the data that
>  Do you mean .report_fixup?

yes, sorry :/

>
> > needs to be exported. You could remove basically everything in this
> > driver by just providing a fixed report descriptor.
>
> What you are aiming for is to fixup the report descriptor and let the
> generic hid-raw driver handle the rest, or do I get you wrong?

yep, exactly

>
> How is the report mapped to certain events then?

Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
events are mapped to input events with a one to one mapping.

>
> I do read at [1] but it is not obvious how to put it together.
> Most drivers I've looked at that is using .report_fixup just fix broken
> reports. I guess these reports are not "broken", just.. odd?

Have a look at [2], lots of full report descriptors :)

And in your case, the reports are incomplete, not odd.

>
>
> >
> > > +
> > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > +                              struct hid_report *report,
> > > +                              u8 *raw_data, int size)
> > > +{
> > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > +       u16 value;
> > > +
> > > +       switch (priv->controller) {
> > > +       case PHOENIXRC:
> > > +               if (size != PHOENIXRC_DSIZE)
> > > +                       break;
> > > +
> > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > +
> > > +               /* Z and RZ are sent every other time */
> > > +               if (priv->alt)
> > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > +               else
> > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > +
> > > +               priv->alt ^= 1;
> > > +               break;
> > > +       case VRC2:
> > > +               if (size != VRC2_DSIZE)
> > > +                       break;
> > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > +               break;
> > > +       case REALFLIGHT:
> > > +               if (size != REALFLIGHT_DSIZE)
> > > +                       break;
> > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > +               input_report_key(priv->input, BTN_A,
> > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > +               input_report_key(priv->input, BTN_B,
> > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > +               break;
> > > +       case XTRG2FMS:
> > > +               if (size != XTRG2FMS_DSIZE)
> > > +                       break;
> > > +
> > > +               /* X, RX, Y and RY are sent every time */
> > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > +               value = (value << 8) | raw_data[1];
> > > +               input_report_abs(priv->input, ABS_X, value);
> > > +
> > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > +               value = (value << 8) | raw_data[2];
> > > +               input_report_abs(priv->input, ABS_Y, value);
> > > +
> > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > +               value = (value << 8) | raw_data[0];
> > > +               input_report_abs(priv->input, ABS_RX, value);
> > > +
> > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > +               value = (value << 8) | raw_data[4];
> > > +               input_report_abs(priv->input, ABS_RY, value);
> > > +
> > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > +               value = (value << 8) | raw_data[6];
> > > +               if (priv->alt)
> > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > +               else
> > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > +
> > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > +               value = (value << 8) | raw_data[5];
> > > +               if (priv->alt)
> > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > +               else
> > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > +
> > > +               priv->alt ^= 1;
> > > +               break;
> > > +       case ORANGERX:
> > > +               if (size != ORANGERX_DSIZE)
> > > +                       break;
> > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > +               break;
> > > +       };
> > > +
> > > +       input_sync(priv->input);
> > > +       return 0;
> > > +}
> > > +
> > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > +{
> > > +       struct device *dev = &hdev->dev;
> > > +       struct rcsim_priv *priv;
> > > +       int ret;
> > > +
> > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > +               return -ENODEV;
> >
> > You are not accessing anything in the USB stack, so there is no need
> > to prevent regression tests that could inject uhid devices to your
> > drivers.
>
> Ok, thanks.
>
> >
> > Cheers,
> > Benjamin
> >
>
> Best regards,
> Marcus Folkesson
>
> [1] https://www.usb.org/hid
>

If you need help in writing report descriptors, I can give you some,
but the easiest might be for you to start from the report descriptor
in hid-sony.c. I used to have a tool to dynamically write a report
descriptor, but I'm not sure it still works...

FYI, I just re-read rcsim_raw_event() and there is stuff that would
require more than just a report descriptor fixup (the fact that some
data is sent every other report is not good and will need some manual
handling though).

Cheers,
Benjamin

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817

Marcus Folkesson Aug. 25, 2022, 6:48 a.m. UTC | #4

Hi Benjamin,

Thank you!
I have a few questions regarding the report descriptor, please see
below.

On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
> On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > Thank you  Benjamin,
> >
> > On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > > Hi Marcus,
> > >
> > > [and sorry for the delay in the review of your patches]
> > >
> > > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > > <marcus.folkesson@gmail.com> wrote:

[...]

> > > > +       };
> > > > +
> > > > +       priv->input = input;
> > > > +       return input_register_device(priv->input);
> > > > +}
> > >
> > > You are basically rewriting hid-input.c, which is suboptimal.
> >
> > Ouch. I will have a look at hid-input, thanks.
> >
> > >
> > > I guess the report descriptor provided by these devices are basically
> > > useless, and so you have to parse the reports yourself in the
> > > raw_event callback.
> >
> > Yep.
> >
> > >
> > > But instead of manually doing that, why not overwrite the report
> > > descriptor (with .rdesc_fixup) and declare here all of the data that
> >  Do you mean .report_fixup?
> 
> yes, sorry :/
> 
> >
> > > needs to be exported. You could remove basically everything in this
> > > driver by just providing a fixed report descriptor.
> >
> > What you are aiming for is to fixup the report descriptor and let the
> > generic hid-raw driver handle the rest, or do I get you wrong?
> 
> yep, exactly
> 
> >
> > How is the report mapped to certain events then?
> 
> Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
> events are mapped to input events with a one to one mapping.
> 
> >
> > I do read at [1] but it is not obvious how to put it together.
> > Most drivers I've looked at that is using .report_fixup just fix broken
> > reports. I guess these reports are not "broken", just.. odd?
> 
> Have a look at [2], lots of full report descriptors :)
> 
> And in your case, the reports are incomplete, not odd.

Got it.
I've parsed [4] the report descriptor for VRC2, and I guess it does not looks
that good:

0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
0x09, 0x00,        // Usage (Undefined)
0xA1, 0x01,        // Collection (Application)
0x15, 0x00,        //   Logical Minimum (0)
0x25, 0x01,        //   Logical Maximum (1)
0x75, 0x01,        //   Report Size (1)
0x05, 0x09,        //   Usage Page (Button)
0x19, 0x01,        //   Usage Minimum (0x01)
0x29, 0x3F,        //   Usage Maximum (0x3F)
0x95, 0x40,        //   Report Count (64)
0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0xC0,              // End Collection

The Usage should rather be Joystick than undefined, the other
fields does also looks wrong to me.

The data for each axis (WHEEL and GAS) is 11 bit long (Logical maximum
2047 ?), how does the report descriptor map to the actual data in terms
of offset, order and length?

> 
> >
> >
> > >
> > > > +
> > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > +                              struct hid_report *report,
> > > > +                              u8 *raw_data, int size)
> > > > +{
> > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > +       u16 value;
> > > > +
> > > > +       switch (priv->controller) {
> > > > +       case PHOENIXRC:
> > > > +               if (size != PHOENIXRC_DSIZE)
> > > > +                       break;
> > > > +
> > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > +
> > > > +               /* Z and RZ are sent every other time */
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > +
> > > > +               priv->alt ^= 1;
> > > > +               break;
> > > > +       case VRC2:
> > > > +               if (size != VRC2_DSIZE)
> > > > +                       break;
> > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > +               break;
> > > > +       case REALFLIGHT:
> > > > +               if (size != REALFLIGHT_DSIZE)
> > > > +                       break;
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > +               input_report_key(priv->input, BTN_A,
> > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > +               input_report_key(priv->input, BTN_B,
> > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > +               break;
> > > > +       case XTRG2FMS:
> > > > +               if (size != XTRG2FMS_DSIZE)
> > > > +                       break;
> > > > +
> > > > +               /* X, RX, Y and RY are sent every time */
> > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[1];
> > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[2];
> > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[0];
> > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[4];
> > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > +
> > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > +               value = (value << 8) | raw_data[6];
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > +               value = (value << 8) | raw_data[5];
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > +
> > > > +               priv->alt ^= 1;
> > > > +               break;
> > > > +       case ORANGERX:
> > > > +               if (size != ORANGERX_DSIZE)
> > > > +                       break;
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > +               break;
> > > > +       };
> > > > +
> > > > +       input_sync(priv->input);
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > +{
> > > > +       struct device *dev = &hdev->dev;
> > > > +       struct rcsim_priv *priv;
> > > > +       int ret;
> > > > +
> > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > +               return -ENODEV;
> > >
> > > You are not accessing anything in the USB stack, so there is no need
> > > to prevent regression tests that could inject uhid devices to your
> > > drivers.
> >
> > Ok, thanks.
> >
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > Best regards,
> > Marcus Folkesson
> >
> > [1] https://www.usb.org/hid
> >
> 
> If you need help in writing report descriptors, I can give you some,
> but the easiest might be for you to start from the report descriptor
> in hid-sony.c. I used to have a tool to dynamically write a report
> descriptor, but I'm not sure it still works...

I think at least some advice would be great :-)

The VRC2 would be the most simple of those, it only has 2 axis with
resolution of 11-bit.
If you have time, would you please give some advice what a report descriptor would look
like and I could probably come up with something for the others.

> 
> FYI, I just re-read rcsim_raw_event() and there is stuff that would
> require more than just a report descriptor fixup (the fact that some
> data is sent every other report is not good and will need some manual
> handling though).


Is the fact that more than one button share the same
byte hard to describe in the report?

  value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
  value = (value << 8) | raw_data[6];

...
  value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
  value = (value << 8) | raw_data[5];


> 
> Cheers,
> Benjamin
> 

Best regards
Marcus Folkesson

> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
[4] https://eleccelerator.com/usbdescreqparser/

Marcus Folkesson Aug. 28, 2022, 2:18 p.m. UTC | #5

Hi,

On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
> On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:

> >
> > Thank you  Benjamin,
> >
> > On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > > Hi Marcus,
> > >
> > > [and sorry for the delay in the review of your patches]
> > >
> > > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > > <marcus.folkesson@gmail.com> wrote:
> > > >
> > > > Several RC Simulator Controllers are HID compliant with similar
> > > > interface.
> > > >
> > > > Add support for these controllers:
> > > >  - Phoenix RC (HID variant)
> > > >  - Car VRC2.0
> > > >  - Real Flight G5/G6/G7
> > > >  - Aero Fly, FMS
> > > >  - OrangeRX FrSky
> > > >
> > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > ---
> > > >  Documentation/hid/index.rst |   1 +
> > > >  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
> > > >  drivers/hid/Kconfig         |  11 ++
> > > >  drivers/hid/Makefile        |   1 +
> > > >  drivers/hid/hid-ids.h       |   5 +
> > > >  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
> > > >  6 files changed, 475 insertions(+)
> > > >  create mode 100644 Documentation/hid/rcsim.rst
> > > >  create mode 100644 drivers/hid/hid-rcsim.c
> > > >
> > > > diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> > > > index e50f513c579c..e5813d264f37 100644
> > > > --- a/Documentation/hid/index.rst
> > > > +++ b/Documentation/hid/index.rst
> > > > @@ -17,3 +17,4 @@ Human Interface Devices (HID)
> > > >     hid-alps
> > > >     intel-ish-hid
> > > >     amd-sfh-hid
> > > > +   rcsim
> > > > diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> > > > new file mode 100644
> > > > index 000000000000..1a031f7189cb
> > > > --- /dev/null
> > > > +++ b/Documentation/hid/rcsim.rst
> > > > @@ -0,0 +1,142 @@
> > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +=================================
> > > > +rcsim - RC Simulator Controllers
> > > > +=================================
> > > > +
> > > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > +
> > > > +This driver let you use your own RC controller plugged
> > > > +into your computer using an HID compatible USB dongle.
> > > > +
> > > > +There are several HID compatible USB dongles from different
> > > > +vendors. The driver currently supports:
> > > > +
> > > > +- Phoenix RC (HID variant) (8ch)
> > > > +- Car VRC2.0 (2ch)
> > > > +- Real Flight G5/G6/G7 (6ch)
> > > > +- Aero Fly, FMS (8ch)
> > > > +- OrangeRX FrSky (6ch)
> > > > +
> > > > +Many RC controllers is able to configure which stick goes to which channel.
> > > > +This is also configurable in most simulators, so a matching is not necessary.
> > > > +
> > > > +Supported dongles
> > > > +==================
> > > > +
> > > > +PhoenixRC
> > > > +----------
> > > > +
> > > > +The PhoenixRC has one HID compatible variant which is supported by this driver.
> > > > +The controller has support for 8 analog channels.
> > > > +
> > > > +The driver is generating the following input event for on channels:
> > > > +
> > > > ++---------+----------------+
> > > > +| Channel |      Event     |
> > > > ++=========+================+
> > > > +|     1   |  ABS_Y         |
> > > > ++---------+----------------+
> > > > +|     2   |  ABS_X         |
> > > > ++---------+----------------+
> > > > +|     3   |  ABS_RY        |
> > > > ++---------+----------------+
> > > > +|     4   |  ABS_RX        |
> > > > ++---------+----------------+
> > > > +|     5   |  ABS_RUDDER    |
> > > > ++---------+----------------+
> > > > +|     6   |  ABS_THROTTLE  |
> > > > ++---------+----------------+
> > > > +|     7   |  ABS_Z         |
> > > > ++---------+----------------+
> > > > +|     8   |  ABS_RZ        |
> > > > ++---------+----------------+
> > > > +
> > > > +VRC2.0
> > > > +----------
> > > > +VRC2.0 is a controller for RC Cars.
> > > > +The controller has support for 2 analog channels.
> > > > +
> > > > +The driver is generating the following input event for on channels:
> > > > +
> > > > ++---------+----------------+
> > > > +| Channel |      Event     |
> > > > ++=========+================+
> > > > +|     1   |  ABS_GAS       |
> > > > ++---------+----------------+
> > > > +|     2   |  ABS_WHEEL     |
> > > > ++---------+----------------+
> > > > +
> > > > +RealFlight
> > > > +----------
> > > > +
> > > > +This driver supports Realflight G4-G7 and above
> > > > +The controller has support for 4 analog channels and two buttons.
> > > > +
> > > > +The driver is generating the following input event for on channels:
> > > > +
> > > > ++---------+----------------+
> > > > +| Channel |      Event     |
> > > > ++=========+================+
> > > > +|     1   |  ABS_Y         |
> > > > ++---------+----------------+
> > > > +|     2   |  ABS_X         |
> > > > ++---------+----------------+
> > > > +|     3   |  ABS_RY        |
> > > > ++---------+----------------+
> > > > +|     4   |  ABS_RX        |
> > > > ++---------+----------------+
> > > > +|     5   |  BTN_A         |
> > > > ++---------+----------------+
> > > > +|     6   |  BTN_B         |
> > > > ++---------+----------------+
> > > > +
> > > > +XTR+G2+FMS Controllers
> > > > +--------------------------------
> > > > +
> > > > +The controllers has support for 8 analog channels.
> > > > +
> > > > +The driver is generating the following input event for on channels:
> > > > +
> > > > ++---------+----------------+
> > > > +| Channel |      Event     |
> > > > ++=========+================+
> > > > +|     1   |  ABS_Y         |
> > > > ++---------+----------------+
> > > > +|     2   |  ABS_X         |
> > > > ++---------+----------------+
> > > > +|     3   |  ABS_RY        |
> > > > ++---------+----------------+
> > > > +|     4   |  ABS_RX        |
> > > > ++---------+----------------+
> > > > +|     5   |  ABS_RUDDER    |
> > > > ++---------+----------------+
> > > > +|     6   |  ABS_THROTTLE  |
> > > > ++---------+----------------+
> > > > +|     7   |  ABS_Z         |
> > > > ++---------+----------------+
> > > > +|     8   |  ABS_RZ        |
> > > > ++---------+----------------+
> > > > +
> > > > +OrangeRX
> > > > +----------
> > > > +
> > > > +The controllers has support for 6 analog channels.
> > > > +
> > > > +The driver is generating the following input event for on channels:
> > > > +
> > > > ++---------+----------------+
> > > > +| Channel |      Event     |
> > > > ++=========+================+
> > > > +|     1   |  ABS_Y         |
> > > > ++---------+----------------+
> > > > +|     2   |  ABS_X         |
> > > > ++---------+----------------+
> > > > +|     3   |  ABS_RY        |
> > > > ++---------+----------------+
> > > > +|     4   |  ABS_RX        |
> > > > ++---------+----------------+
> > > > +|     5   |  ABS_RUDDER    |
> > > > ++---------+----------------+
> > > > +|     6   |  ABS_THROTTLE  |
> > > > ++---------+----------------+
> > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > index 70da5931082f..d8313d36086c 100644
> > > > --- a/drivers/hid/Kconfig
> > > > +++ b/drivers/hid/Kconfig
> > > > @@ -957,6 +957,17 @@ config HID_RAZER
> > > >         Support for Razer devices that are not fully compliant with the
> > > >         HID standard.
> > > >
> > > > +config HID_RCSIM
> > > > +       tristate "RC Simulator Controllers"
> > > > +       depends on HID
> > > > +       help
> > > > +       Support for several HID compatible RC Simulator Controllers including
> > > > +         - Phoenix RC
> > > > +         - Car VRC2.0
> > > > +         - Real Flight
> > > > +         - Aero Fly, FMS
> > > > +         - OrangeRX FrSky
> > > > +
> > > >  config HID_PRIMAX
> > > >         tristate "Primax non-fully HID-compliant devices"
> > > >         depends on HID
> > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > index cac2cbe26d11..85d50ab352ee 100644
> > > > --- a/drivers/hid/Makefile
> > > > +++ b/drivers/hid/Makefile
> > > > @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
> > > >  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
> > > >  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> > > >  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> > > > +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o
> > >
> > > General rule of thumbs, we try to name the drivers after their
> > > vendors, unless we know we have a generic driver.
> > >
> > > Here, this driver seems to be really tied to a small set of devices,
> > > and thus I don't think we can call it "generic".
> >
> > Got it.
> >
> > >
> > > >  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
> > > >  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> > > >  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > index d9eb676abe96..baf5f74d5bed 100644
> > > > --- a/drivers/hid/hid-ids.h
> > > > +++ b/drivers/hid/hid-ids.h
> > > > @@ -1381,6 +1381,11 @@
> > > >
> > > >  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
> > > >  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> > > > +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> > > > +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> > > > +
> > > > +#define USB_VENDOR_ID_DIPLING  0x0B9B
> > > > +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
> > > >
> > > >  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
> > > >  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> > > > diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> > > > new file mode 100644
> > > > index 000000000000..0f214cb5816a
> > > > --- /dev/null
> > > > +++ b/drivers/hid/hid-rcsim.c
> > > > @@ -0,0 +1,315 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Driver for several HID compatible RC Simulator Controllers.
> > > > + * Currently supported controllers are:
> > > > + *
> > > > + * - Phoenix RC (HID variant) (8ch)
> > > > + * - Car VRC2.0 (2ch)
> > > > + * - Real Flight G5/G6/G7 (6ch)
> > > > + * - Aero Fly, FMS (8ch)
> > > > + * - OrangeRX FrSky (6ch)
> > > > + *
> > > > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > + */
> > > > +
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/hid.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/usb.h>
> > > > +
> > > > +#include "hid-ids.h"
> > > > +
> > > > +/*
> > > > + * Some of these VID/PID are probably "borrowed", so keep them locally and
> > > > + * do not populate hid-ids.h with those.
> > > > + */
> > > > +
> > > > +/* PHOENIXRC Controlloer (HID variant) */
> > > > +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> > > > +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> > > > +#define PHOENIXRC_DSIZE        (8)
> > > > +
> > > > +/* VRC2 Controlloer */
> > > > +#define VRC2_VID       (0x07c0)
> > > > +#define VRC2_PID       (0x1125)
> > > > +#define VRC2_DSIZE     (7)
> > > > +
> > > > +/* Realflight G4-&7 and Above Controller */
> > > > +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> > > > +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> > > > +#define REALFLIGHT_DSIZE       (8)
> > > > +
> > > > +#define REALFLIGHT_BTN_A       BIT(0)
> > > > +#define REALFLIGHT_BTN_B       BIT(1)
> > > > +
> > > > +/* XTR+G2+FMS Controller */
> > > > +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> > > > +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> > > > +#define XTRG2FMS_DSIZE (8)
> > > > +
> > > > +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> > > > +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> > > > +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> > > > +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> > > > +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> > > > +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> > > > +
> > > > +/* OrangeRX FrSky */
> > > > +#define ORANGERX_VID   (0x0451)
> > > > +#define ORANGERX_PID   (0x16a5)
> > > > +#define ORANGERX_DSIZE (8)
> > > > +
> > > > +enum rcsim_controller {
> > > > +       PHOENIXRC,
> > > > +       VRC2,
> > > > +       REALFLIGHT,
> > > > +       XTRG2FMS,
> > > > +       ORANGERX
> > > > +};
> > > > +
> > > > +struct rcsim_priv {
> > > > +       struct hid_device *hdev;
> > > > +       struct input_dev *input;
> > > > +       enum rcsim_controller controller;
> > > > +       u8 alt;
> > > > +};
> > > > +
> > > > +static int rcsim_open(struct input_dev *dev)
> > > > +{
> > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > +
> > > > +       return hid_hw_open(priv->hdev);
> > > > +}
> > > > +
> > > > +static void rcsim_close(struct input_dev *dev)
> > > > +{
> > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > +
> > > > +       hid_hw_close(priv->hdev);
> > > > +}
> > > > +
> > > > +static int rcsim_setup_input(struct rcsim_priv *priv)
> > > > +{
> > > > +       struct input_dev *input;
> > > > +
> > > > +       input = devm_input_allocate_device(&priv->hdev->dev);
> > > > +       if (!input)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       input->id.bustype = priv->hdev->bus;
> > > > +       input->id.vendor  = priv->hdev->vendor;
> > > > +       input->id.product = priv->hdev->product;
> > > > +       input->id.version = priv->hdev->bus;
> > > > +       input->phys = priv->hdev->phys;
> > > > +       input->uniq = priv->hdev->uniq;
> > > > +       input->open = rcsim_open;
> > > > +       input->close = rcsim_close;
> > > > +
> > > > +       input_set_drvdata(input, priv);
> > > > +
> > > > +       switch (priv->controller) {
> > > > +       case PHOENIXRC:
> > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > +               input->name = "RC Simuator Controller PhoenixRC";
> > > > +               break;
> > > > +       case VRC2:
> > > > +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> > > > +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> > > > +               input->name = "RC Simuator Controller VRC2.0";
> > > > +               break;
> > > > +       case REALFLIGHT:
> > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > +               input_set_capability(input, EV_KEY, BTN_A);
> > > > +               input_set_capability(input, EV_KEY, BTN_B);
> > > > +               input->name = "RC Simuator Controller Realflight";
> > > > +               break;
> > > > +       case XTRG2FMS:
> > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> > > > +               input->name = "RC Simuator Controller AeroFly, FMS";
> > > > +               priv->alt = 0;
> > > > +               break;
> > > > +       case ORANGERX:
> > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > +               input->name = "RC Simuator Controller OrangeRX FrSky";
> > > > +               break;
> > > > +       };
> > > > +
> > > > +       priv->input = input;
> > > > +       return input_register_device(priv->input);
> > > > +}
> > >
> > > You are basically rewriting hid-input.c, which is suboptimal.
> >
> > Ouch. I will have a look at hid-input, thanks.
> >
> > >
> > > I guess the report descriptor provided by these devices are basically
> > > useless, and so you have to parse the reports yourself in the
> > > raw_event callback.
> >
> > Yep.
> >
> > >
> > > But instead of manually doing that, why not overwrite the report
> > > descriptor (with .rdesc_fixup) and declare here all of the data that
> >  Do you mean .report_fixup?
> 
> yes, sorry :/
> 
> >
> > > needs to be exported. You could remove basically everything in this
> > > driver by just providing a fixed report descriptor.
> >
> > What you are aiming for is to fixup the report descriptor and let the
> > generic hid-raw driver handle the rest, or do I get you wrong?
> 
> yep, exactly
> 
> >
> > How is the report mapped to certain events then?
> 
> Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
> events are mapped to input events with a one to one mapping.
> 
> >
> > I do read at [1] but it is not obvious how to put it together.
> > Most drivers I've looked at that is using .report_fixup just fix broken
> > reports. I guess these reports are not "broken", just.. odd?
> 
> Have a look at [2], lots of full report descriptors :)
> 
> And in your case, the reports are incomplete, not odd.
> 
> >
> >
> > >
> > > > +
> > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > +                              struct hid_report *report,
> > > > +                              u8 *raw_data, int size)
> > > > +{
> > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > +       u16 value;
> > > > +
> > > > +       switch (priv->controller) {
> > > > +       case PHOENIXRC:
> > > > +               if (size != PHOENIXRC_DSIZE)
> > > > +                       break;
> > > > +
> > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > +
> > > > +               /* Z and RZ are sent every other time */
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > +
> > > > +               priv->alt ^= 1;
> > > > +               break;
> > > > +       case VRC2:
> > > > +               if (size != VRC2_DSIZE)
> > > > +                       break;
> > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > +               break;
> > > > +       case REALFLIGHT:
> > > > +               if (size != REALFLIGHT_DSIZE)
> > > > +                       break;
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > +               input_report_key(priv->input, BTN_A,
> > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > +               input_report_key(priv->input, BTN_B,
> > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > +               break;
> > > > +       case XTRG2FMS:
> > > > +               if (size != XTRG2FMS_DSIZE)
> > > > +                       break;
> > > > +
> > > > +               /* X, RX, Y and RY are sent every time */
> > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[1];
> > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[2];
> > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[0];
> > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > +               value = (value << 8) | raw_data[4];
> > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > +
> > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > +               value = (value << 8) | raw_data[6];
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > +
> > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > +               value = (value << 8) | raw_data[5];
> > > > +               if (priv->alt)
> > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > +               else
> > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > +
> > > > +               priv->alt ^= 1;
> > > > +               break;
> > > > +       case ORANGERX:
> > > > +               if (size != ORANGERX_DSIZE)
> > > > +                       break;
> > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > +               break;
> > > > +       };
> > > > +
> > > > +       input_sync(priv->input);
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > +{
> > > > +       struct device *dev = &hdev->dev;
> > > > +       struct rcsim_priv *priv;
> > > > +       int ret;
> > > > +
> > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > +               return -ENODEV;
> > >
> > > You are not accessing anything in the USB stack, so there is no need
> > > to prevent regression tests that could inject uhid devices to your
> > > drivers.
> >
> > Ok, thanks.
> >
> > >
> > > Cheers,
> > > Benjamin
> > >
> >
> > Best regards,
> > Marcus Folkesson
> >
> > [1] https://www.usb.org/hid
> >
> 
> If you need help in writing report descriptors, I can give you some,
> but the easiest might be for you to start from the report descriptor
> in hid-sony.c. I used to have a tool to dynamically write a report
> descriptor, but I'm not sure it still works...

I've worked with the report descriptor for VRC2, and have come up with
something that works.


static __u8 vrc2_rdesc_fixed[] = {
	0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
	0x09, 0x04,        // Usage (Joystick)
	0xA1, 0x01,        // Collection (Application)
	0x09, 0x01,        //   Usage (Pointer)
	0xA1, 0x00,        //   Collection (Physical)
	0x09, 0x30,        //     Usage (X)
	0x09, 0x31,        //     Usage (Y)
	0x15, 0x00,        //     Logical Minimum (0)
	0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
	0x35, 0x00,        //     Physical Minimum (0)
	0x46, 0xFF, 0x00,  //     Physical Maximum (255)
	0x75, 0x10,        //     Report Size (16)
	0x95, 0x02,        //     Report Count (2)
	0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
	0xC0,              //   End Collection
	0x95, 0x01,        //   Report Count (1)
	0x75, 0x18,        //   Report Size (24)
	0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
	0x09, 0x00,        //   Usage (Undefined)
	0x15, 0x00,        //   Logical Minimum (0)
	0x26, 0xFF, 0x00,  //   Logical Maximum (255)
	0x75, 0x08,        //   Report Size (8)
	0x95, 0x08,        //   Report Count (8)
	0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
	0xC0,              // End Collection
};


It is a result of reading specifications and looking at various
examples. 

These lines are taken from an example, and I do not know why I need
those. I think the constant report size is wrong (should be 32 to
fill up) and I do not know what the Feature is used for.

	0x95, 0x01,        //   Report Count (1)
	0x75, 0x18,        //   Report Size (24)
	0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
	0x09, 0x00,        //   Usage (Undefined)
	0x15, 0x00,        //   Logical Minimum (0)
	0x26, 0xFF, 0x00,  //   Logical Maximum (255)
	0x75, 0x08,        //   Report Size (8)
	0x95, 0x08,        //   Report Count (8)
	0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)

If I omit any of these lines then I only get tons of

kernel: usb 3-12.3.1.3.3: input irq status -75 received

In the kernel log.


> 
> FYI, I just re-read rcsim_raw_event() and there is stuff that would
> require more than just a report descriptor fixup (the fact that some
> data is sent every other report is not good and will need some manual
> handling though).
> 
> Cheers,
> Benjamin
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
> 


Best regards
Marcus Folkesson

Benjamin Tissoires Aug. 30, 2022, 12:35 p.m. UTC | #6

Hi Marcus,

On Sun, Aug 28, 2022 at 4:13 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Hi,
>
> On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
> > On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:
>
> > >
> > > Thank you  Benjamin,
> > >
> > > On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > > > Hi Marcus,
> > > >
> > > > [and sorry for the delay in the review of your patches]
> > > >
> > > > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > > > <marcus.folkesson@gmail.com> wrote:
> > > > >
> > > > > Several RC Simulator Controllers are HID compliant with similar
> > > > > interface.
> > > > >
> > > > > Add support for these controllers:
> > > > >  - Phoenix RC (HID variant)
> > > > >  - Car VRC2.0
> > > > >  - Real Flight G5/G6/G7
> > > > >  - Aero Fly, FMS
> > > > >  - OrangeRX FrSky
> > > > >
> > > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > ---
> > > > >  Documentation/hid/index.rst |   1 +
> > > > >  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
> > > > >  drivers/hid/Kconfig         |  11 ++
> > > > >  drivers/hid/Makefile        |   1 +
> > > > >  drivers/hid/hid-ids.h       |   5 +
> > > > >  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
> > > > >  6 files changed, 475 insertions(+)
> > > > >  create mode 100644 Documentation/hid/rcsim.rst
> > > > >  create mode 100644 drivers/hid/hid-rcsim.c
> > > > >
> > > > > diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> > > > > index e50f513c579c..e5813d264f37 100644
> > > > > --- a/Documentation/hid/index.rst
> > > > > +++ b/Documentation/hid/index.rst
> > > > > @@ -17,3 +17,4 @@ Human Interface Devices (HID)
> > > > >     hid-alps
> > > > >     intel-ish-hid
> > > > >     amd-sfh-hid
> > > > > +   rcsim
> > > > > diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> > > > > new file mode 100644
> > > > > index 000000000000..1a031f7189cb
> > > > > --- /dev/null
> > > > > +++ b/Documentation/hid/rcsim.rst
> > > > > @@ -0,0 +1,142 @@
> > > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > > +
> > > > > +=================================
> > > > > +rcsim - RC Simulator Controllers
> > > > > +=================================
> > > > > +
> > > > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > +
> > > > > +This driver let you use your own RC controller plugged
> > > > > +into your computer using an HID compatible USB dongle.
> > > > > +
> > > > > +There are several HID compatible USB dongles from different
> > > > > +vendors. The driver currently supports:
> > > > > +
> > > > > +- Phoenix RC (HID variant) (8ch)
> > > > > +- Car VRC2.0 (2ch)
> > > > > +- Real Flight G5/G6/G7 (6ch)
> > > > > +- Aero Fly, FMS (8ch)
> > > > > +- OrangeRX FrSky (6ch)
> > > > > +
> > > > > +Many RC controllers is able to configure which stick goes to which channel.
> > > > > +This is also configurable in most simulators, so a matching is not necessary.
> > > > > +
> > > > > +Supported dongles
> > > > > +==================
> > > > > +
> > > > > +PhoenixRC
> > > > > +----------
> > > > > +
> > > > > +The PhoenixRC has one HID compatible variant which is supported by this driver.
> > > > > +The controller has support for 8 analog channels.
> > > > > +
> > > > > +The driver is generating the following input event for on channels:
> > > > > +
> > > > > ++---------+----------------+
> > > > > +| Channel |      Event     |
> > > > > ++=========+================+
> > > > > +|     1   |  ABS_Y         |
> > > > > ++---------+----------------+
> > > > > +|     2   |  ABS_X         |
> > > > > ++---------+----------------+
> > > > > +|     3   |  ABS_RY        |
> > > > > ++---------+----------------+
> > > > > +|     4   |  ABS_RX        |
> > > > > ++---------+----------------+
> > > > > +|     5   |  ABS_RUDDER    |
> > > > > ++---------+----------------+
> > > > > +|     6   |  ABS_THROTTLE  |
> > > > > ++---------+----------------+
> > > > > +|     7   |  ABS_Z         |
> > > > > ++---------+----------------+
> > > > > +|     8   |  ABS_RZ        |
> > > > > ++---------+----------------+
> > > > > +
> > > > > +VRC2.0
> > > > > +----------
> > > > > +VRC2.0 is a controller for RC Cars.
> > > > > +The controller has support for 2 analog channels.
> > > > > +
> > > > > +The driver is generating the following input event for on channels:
> > > > > +
> > > > > ++---------+----------------+
> > > > > +| Channel |      Event     |
> > > > > ++=========+================+
> > > > > +|     1   |  ABS_GAS       |
> > > > > ++---------+----------------+
> > > > > +|     2   |  ABS_WHEEL     |
> > > > > ++---------+----------------+
> > > > > +
> > > > > +RealFlight
> > > > > +----------
> > > > > +
> > > > > +This driver supports Realflight G4-G7 and above
> > > > > +The controller has support for 4 analog channels and two buttons.
> > > > > +
> > > > > +The driver is generating the following input event for on channels:
> > > > > +
> > > > > ++---------+----------------+
> > > > > +| Channel |      Event     |
> > > > > ++=========+================+
> > > > > +|     1   |  ABS_Y         |
> > > > > ++---------+----------------+
> > > > > +|     2   |  ABS_X         |
> > > > > ++---------+----------------+
> > > > > +|     3   |  ABS_RY        |
> > > > > ++---------+----------------+
> > > > > +|     4   |  ABS_RX        |
> > > > > ++---------+----------------+
> > > > > +|     5   |  BTN_A         |
> > > > > ++---------+----------------+
> > > > > +|     6   |  BTN_B         |
> > > > > ++---------+----------------+
> > > > > +
> > > > > +XTR+G2+FMS Controllers
> > > > > +--------------------------------
> > > > > +
> > > > > +The controllers has support for 8 analog channels.
> > > > > +
> > > > > +The driver is generating the following input event for on channels:
> > > > > +
> > > > > ++---------+----------------+
> > > > > +| Channel |      Event     |
> > > > > ++=========+================+
> > > > > +|     1   |  ABS_Y         |
> > > > > ++---------+----------------+
> > > > > +|     2   |  ABS_X         |
> > > > > ++---------+----------------+
> > > > > +|     3   |  ABS_RY        |
> > > > > ++---------+----------------+
> > > > > +|     4   |  ABS_RX        |
> > > > > ++---------+----------------+
> > > > > +|     5   |  ABS_RUDDER    |
> > > > > ++---------+----------------+
> > > > > +|     6   |  ABS_THROTTLE  |
> > > > > ++---------+----------------+
> > > > > +|     7   |  ABS_Z         |
> > > > > ++---------+----------------+
> > > > > +|     8   |  ABS_RZ        |
> > > > > ++---------+----------------+
> > > > > +
> > > > > +OrangeRX
> > > > > +----------
> > > > > +
> > > > > +The controllers has support for 6 analog channels.
> > > > > +
> > > > > +The driver is generating the following input event for on channels:
> > > > > +
> > > > > ++---------+----------------+
> > > > > +| Channel |      Event     |
> > > > > ++=========+================+
> > > > > +|     1   |  ABS_Y         |
> > > > > ++---------+----------------+
> > > > > +|     2   |  ABS_X         |
> > > > > ++---------+----------------+
> > > > > +|     3   |  ABS_RY        |
> > > > > ++---------+----------------+
> > > > > +|     4   |  ABS_RX        |
> > > > > ++---------+----------------+
> > > > > +|     5   |  ABS_RUDDER    |
> > > > > ++---------+----------------+
> > > > > +|     6   |  ABS_THROTTLE  |
> > > > > ++---------+----------------+
> > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > > index 70da5931082f..d8313d36086c 100644
> > > > > --- a/drivers/hid/Kconfig
> > > > > +++ b/drivers/hid/Kconfig
> > > > > @@ -957,6 +957,17 @@ config HID_RAZER
> > > > >         Support for Razer devices that are not fully compliant with the
> > > > >         HID standard.
> > > > >
> > > > > +config HID_RCSIM
> > > > > +       tristate "RC Simulator Controllers"
> > > > > +       depends on HID
> > > > > +       help
> > > > > +       Support for several HID compatible RC Simulator Controllers including
> > > > > +         - Phoenix RC
> > > > > +         - Car VRC2.0
> > > > > +         - Real Flight
> > > > > +         - Aero Fly, FMS
> > > > > +         - OrangeRX FrSky
> > > > > +
> > > > >  config HID_PRIMAX
> > > > >         tristate "Primax non-fully HID-compliant devices"
> > > > >         depends on HID
> > > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > > index cac2cbe26d11..85d50ab352ee 100644
> > > > > --- a/drivers/hid/Makefile
> > > > > +++ b/drivers/hid/Makefile
> > > > > @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
> > > > >  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
> > > > >  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> > > > >  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> > > > > +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o
> > > >
> > > > General rule of thumbs, we try to name the drivers after their
> > > > vendors, unless we know we have a generic driver.
> > > >
> > > > Here, this driver seems to be really tied to a small set of devices,
> > > > and thus I don't think we can call it "generic".
> > >
> > > Got it.
> > >
> > > >
> > > > >  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
> > > > >  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> > > > >  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > > index d9eb676abe96..baf5f74d5bed 100644
> > > > > --- a/drivers/hid/hid-ids.h
> > > > > +++ b/drivers/hid/hid-ids.h
> > > > > @@ -1381,6 +1381,11 @@
> > > > >
> > > > >  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
> > > > >  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> > > > > +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> > > > > +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> > > > > +
> > > > > +#define USB_VENDOR_ID_DIPLING  0x0B9B
> > > > > +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
> > > > >
> > > > >  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
> > > > >  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> > > > > diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> > > > > new file mode 100644
> > > > > index 000000000000..0f214cb5816a
> > > > > --- /dev/null
> > > > > +++ b/drivers/hid/hid-rcsim.c
> > > > > @@ -0,0 +1,315 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * Driver for several HID compatible RC Simulator Controllers.
> > > > > + * Currently supported controllers are:
> > > > > + *
> > > > > + * - Phoenix RC (HID variant) (8ch)
> > > > > + * - Car VRC2.0 (2ch)
> > > > > + * - Real Flight G5/G6/G7 (6ch)
> > > > > + * - Aero Fly, FMS (8ch)
> > > > > + * - OrangeRX FrSky (6ch)
> > > > > + *
> > > > > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > + */
> > > > > +
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/input.h>
> > > > > +#include <linux/hid.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/usb.h>
> > > > > +
> > > > > +#include "hid-ids.h"
> > > > > +
> > > > > +/*
> > > > > + * Some of these VID/PID are probably "borrowed", so keep them locally and
> > > > > + * do not populate hid-ids.h with those.
> > > > > + */
> > > > > +
> > > > > +/* PHOENIXRC Controlloer (HID variant) */
> > > > > +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> > > > > +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> > > > > +#define PHOENIXRC_DSIZE        (8)
> > > > > +
> > > > > +/* VRC2 Controlloer */
> > > > > +#define VRC2_VID       (0x07c0)
> > > > > +#define VRC2_PID       (0x1125)
> > > > > +#define VRC2_DSIZE     (7)
> > > > > +
> > > > > +/* Realflight G4-&7 and Above Controller */
> > > > > +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> > > > > +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> > > > > +#define REALFLIGHT_DSIZE       (8)
> > > > > +
> > > > > +#define REALFLIGHT_BTN_A       BIT(0)
> > > > > +#define REALFLIGHT_BTN_B       BIT(1)
> > > > > +
> > > > > +/* XTR+G2+FMS Controller */
> > > > > +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> > > > > +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> > > > > +#define XTRG2FMS_DSIZE (8)
> > > > > +
> > > > > +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> > > > > +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> > > > > +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> > > > > +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> > > > > +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> > > > > +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> > > > > +
> > > > > +/* OrangeRX FrSky */
> > > > > +#define ORANGERX_VID   (0x0451)
> > > > > +#define ORANGERX_PID   (0x16a5)
> > > > > +#define ORANGERX_DSIZE (8)
> > > > > +
> > > > > +enum rcsim_controller {
> > > > > +       PHOENIXRC,
> > > > > +       VRC2,
> > > > > +       REALFLIGHT,
> > > > > +       XTRG2FMS,
> > > > > +       ORANGERX
> > > > > +};
> > > > > +
> > > > > +struct rcsim_priv {
> > > > > +       struct hid_device *hdev;
> > > > > +       struct input_dev *input;
> > > > > +       enum rcsim_controller controller;
> > > > > +       u8 alt;
> > > > > +};
> > > > > +
> > > > > +static int rcsim_open(struct input_dev *dev)
> > > > > +{
> > > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > > +
> > > > > +       return hid_hw_open(priv->hdev);
> > > > > +}
> > > > > +
> > > > > +static void rcsim_close(struct input_dev *dev)
> > > > > +{
> > > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > > +
> > > > > +       hid_hw_close(priv->hdev);
> > > > > +}
> > > > > +
> > > > > +static int rcsim_setup_input(struct rcsim_priv *priv)
> > > > > +{
> > > > > +       struct input_dev *input;
> > > > > +
> > > > > +       input = devm_input_allocate_device(&priv->hdev->dev);
> > > > > +       if (!input)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       input->id.bustype = priv->hdev->bus;
> > > > > +       input->id.vendor  = priv->hdev->vendor;
> > > > > +       input->id.product = priv->hdev->product;
> > > > > +       input->id.version = priv->hdev->bus;
> > > > > +       input->phys = priv->hdev->phys;
> > > > > +       input->uniq = priv->hdev->uniq;
> > > > > +       input->open = rcsim_open;
> > > > > +       input->close = rcsim_close;
> > > > > +
> > > > > +       input_set_drvdata(input, priv);
> > > > > +
> > > > > +       switch (priv->controller) {
> > > > > +       case PHOENIXRC:
> > > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > > +               input->name = "RC Simuator Controller PhoenixRC";
> > > > > +               break;
> > > > > +       case VRC2:
> > > > > +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> > > > > +               input->name = "RC Simuator Controller VRC2.0";
> > > > > +               break;
> > > > > +       case REALFLIGHT:
> > > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > > +               input_set_capability(input, EV_KEY, BTN_A);
> > > > > +               input_set_capability(input, EV_KEY, BTN_B);
> > > > > +               input->name = "RC Simuator Controller Realflight";
> > > > > +               break;
> > > > > +       case XTRG2FMS:
> > > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> > > > > +               input->name = "RC Simuator Controller AeroFly, FMS";
> > > > > +               priv->alt = 0;
> > > > > +               break;
> > > > > +       case ORANGERX:
> > > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > > +               input->name = "RC Simuator Controller OrangeRX FrSky";
> > > > > +               break;
> > > > > +       };
> > > > > +
> > > > > +       priv->input = input;
> > > > > +       return input_register_device(priv->input);
> > > > > +}
> > > >
> > > > You are basically rewriting hid-input.c, which is suboptimal.
> > >
> > > Ouch. I will have a look at hid-input, thanks.
> > >
> > > >
> > > > I guess the report descriptor provided by these devices are basically
> > > > useless, and so you have to parse the reports yourself in the
> > > > raw_event callback.
> > >
> > > Yep.
> > >
> > > >
> > > > But instead of manually doing that, why not overwrite the report
> > > > descriptor (with .rdesc_fixup) and declare here all of the data that
> > >  Do you mean .report_fixup?
> >
> > yes, sorry :/
> >
> > >
> > > > needs to be exported. You could remove basically everything in this
> > > > driver by just providing a fixed report descriptor.
> > >
> > > What you are aiming for is to fixup the report descriptor and let the
> > > generic hid-raw driver handle the rest, or do I get you wrong?
> >
> > yep, exactly
> >
> > >
> > > How is the report mapped to certain events then?
> >
> > Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
> > events are mapped to input events with a one to one mapping.
> >
> > >
> > > I do read at [1] but it is not obvious how to put it together.
> > > Most drivers I've looked at that is using .report_fixup just fix broken
> > > reports. I guess these reports are not "broken", just.. odd?
> >
> > Have a look at [2], lots of full report descriptors :)
> >
> > And in your case, the reports are incomplete, not odd.
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > > +                              struct hid_report *report,
> > > > > +                              u8 *raw_data, int size)
> > > > > +{
> > > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > > +       u16 value;
> > > > > +
> > > > > +       switch (priv->controller) {
> > > > > +       case PHOENIXRC:
> > > > > +               if (size != PHOENIXRC_DSIZE)
> > > > > +                       break;
> > > > > +
> > > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > +
> > > > > +               /* Z and RZ are sent every other time */
> > > > > +               if (priv->alt)
> > > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > > +               else
> > > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > > +
> > > > > +               priv->alt ^= 1;
> > > > > +               break;
> > > > > +       case VRC2:
> > > > > +               if (size != VRC2_DSIZE)
> > > > > +                       break;
> > > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > > +               break;
> > > > > +       case REALFLIGHT:
> > > > > +               if (size != REALFLIGHT_DSIZE)
> > > > > +                       break;
> > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > > +               input_report_key(priv->input, BTN_A,
> > > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > > +               input_report_key(priv->input, BTN_B,
> > > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > > +               break;
> > > > > +       case XTRG2FMS:
> > > > > +               if (size != XTRG2FMS_DSIZE)
> > > > > +                       break;
> > > > > +
> > > > > +               /* X, RX, Y and RY are sent every time */
> > > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[1];
> > > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[2];
> > > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[0];
> > > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[4];
> > > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > > +
> > > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > > +               value = (value << 8) | raw_data[6];
> > > > > +               if (priv->alt)
> > > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > > +               else
> > > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > > +               value = (value << 8) | raw_data[5];
> > > > > +               if (priv->alt)
> > > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > > +               else
> > > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > > +
> > > > > +               priv->alt ^= 1;
> > > > > +               break;
> > > > > +       case ORANGERX:
> > > > > +               if (size != ORANGERX_DSIZE)
> > > > > +                       break;
> > > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > +               break;
> > > > > +       };
> > > > > +
> > > > > +       input_sync(priv->input);
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > > +{
> > > > > +       struct device *dev = &hdev->dev;
> > > > > +       struct rcsim_priv *priv;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > > +               return -ENODEV;
> > > >
> > > > You are not accessing anything in the USB stack, so there is no need
> > > > to prevent regression tests that could inject uhid devices to your
> > > > drivers.
> > >
> > > Ok, thanks.
> > >
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > >
> > > Best regards,
> > > Marcus Folkesson
> > >
> > > [1] https://www.usb.org/hid
> > >
> >
> > If you need help in writing report descriptors, I can give you some,
> > but the easiest might be for you to start from the report descriptor
> > in hid-sony.c. I used to have a tool to dynamically write a report
> > descriptor, but I'm not sure it still works...
>
> I've worked with the report descriptor for VRC2, and have come up with
> something that works.

Great that you managed to fill in most of the gaps :)

>
>
> static __u8 vrc2_rdesc_fixed[] = {
>         0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
>         0x09, 0x04,        // Usage (Joystick)
>         0xA1, 0x01,        // Collection (Application)
>         0x09, 0x01,        //   Usage (Pointer)
>         0xA1, 0x00,        //   Collection (Physical)
>         0x09, 0x30,        //     Usage (X)
>         0x09, 0x31,        //     Usage (Y)
>         0x15, 0x00,        //     Logical Minimum (0)
>         0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
>         0x35, 0x00,        //     Physical Minimum (0)
>         0x46, 0xFF, 0x00,  //     Physical Maximum (255)
>         0x75, 0x10,        //     Report Size (16)
>         0x95, 0x02,        //     Report Count (2)
>         0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
>         0xC0,              //   End Collection
>         0x95, 0x01,        //   Report Count (1)
>         0x75, 0x18,        //   Report Size (24)
>         0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
>         0x09, 0x00,        //   Usage (Undefined)
>         0x15, 0x00,        //   Logical Minimum (0)
>         0x26, 0xFF, 0x00,  //   Logical Maximum (255)
>         0x75, 0x08,        //   Report Size (8)
>         0x95, 0x08,        //   Report Count (8)
>         0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
>         0xC0,              // End Collection
> };
>
>
> It is a result of reading specifications and looking at various
> examples.
>
> These lines are taken from an example, and I do not know why I need
> those. I think the constant report size is wrong (should be 32 to

Do you have an example of the actual data sent on the wire? hexdump or
hid-recorder should be enough for me to get what is missing.

As I read the report descriptors, we had:
* old one:
  - opening of application collection of usage undefined
    - 64 buttons (logical max of 1), usage being being 0x01 and 0x3F
(63) -> so the last bit is ignored

Which would translate to a report of the following (each 0 being a
separate button, | is marking bytes):

0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0,
0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0,
0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, X

-> 64 * 1 = 64 bits

()last bit being ignored because of its duplicated usage with the previous one)

* new one:
  - opening of an application collection of usage Joystick
    - opening of a physical collection of type Pointer
      - 16 bits for X, with logical max of 2047 and physical max of 255
      - 16 bits for Y, with logical max of 2047 and physical max of 255
    - 1 const element of size 24 that has to be ignored
    - one feature that should not be there :)

So it would translate into the following (c being constant bit):
X (2 bytes) | Y (2 bytes) | c, c, c, c, c, c, c, c | c, c, c, c, c, c,
c, c | c, c, c, c, c, c, c, c

-> 24 + 2*8 + 2*8 = 56 bits

So I agree with you, you are missing 16 bits

> fill up) and I do not know what the Feature is used for.

You should not have to declare a feature. A feature is a special HID
endpoint that you can use to talk to the HID device. By emitting
special HID commands, (HID_REPORT_GET/HID_REPORT_SET) you can control
how the device works. But if your current report descriptor doesn't
have a feature declared, you should not declare it.

>
>         0x95, 0x01,        //   Report Count (1)
>         0x75, 0x18,        //   Report Size (24)
>         0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
>         0x09, 0x00,        //   Usage (Undefined)
>         0x15, 0x00,        //   Logical Minimum (0)
>         0x26, 0xFF, 0x00,  //   Logical Maximum (255)
>         0x75, 0x08,        //   Report Size (8)
>         0x95, 0x08,        //   Report Count (8)
>         0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
>
> If I omit any of these lines then I only get tons of
>
> kernel: usb 3-12.3.1.3.3: input irq status -75 received

I assume the feature in this particular case helps the driver to
understand that you need to allocate a 64 bits chunk of memory to
communicate with the device.

I would advise to use the following report instead:

----
static __u8 vrc2_rdesc_fixed[] = {
        0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
        0x09, 0x04,        // Usage (Joystick)
        0xA1, 0x01,        // Collection (Application)
        0x09, 0x01,        //   Usage (Pointer)
        0xA1, 0x00,        //   Collection (Physical)
        0x09, 0x30,        //     Usage (X)
        0x09, 0x31,        //     Usage (Y)
        0x15, 0x00,        //     Logical Minimum (0)
        0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
        0x35, 0x00,        //     Physical Minimum (0)
        0x46, 0xFF, 0x00,  //     Physical Maximum (255)
        0x75, 0x10,        //     Report Size (16)
        0x95, 0x02,        //     Report Count (2)
        0x81, 0x02,        //     Input (Data,Var,Abs,No
Wrap,Linear,Preferred State,No Null Position)
        0xC0,              //   End Collection
        0x75, 0x08,        //   Report Size (8)
       0x95, 0x04,        //   Report Count (4)
        0x81, 0x03,        //   Input (Cnst,Var,Abs)
        0xC0,              // End Collection
};
---

Few changes:
- use of 4 bytes of padding, instead of 1 24bits slot of padding
- changed the padding from 0x01 to 0x03 to remove the "array" const
definition which is tricky to handle
- removed the feature report.

Cheers,
Benjamin

>
> In the kernel log.
>
>
> >
> > FYI, I just re-read rcsim_raw_event() and there is stuff that would
> > require more than just a report descriptor fixup (the fact that some
> > data is sent every other report is not good and will need some manual
> > handling though).
> >
> > Cheers,
> > Benjamin
> >
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
> >
>
>
> Best regards
> Marcus Folkesson

Benjamin Tissoires Aug. 30, 2022, 12:45 p.m. UTC | #7

On Thu, Aug 25, 2022 at 8:44 AM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Hi Benjamin,
>
> Thank you!
> I have a few questions regarding the report descriptor, please see
> below.
>
> On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
[...]
>
> Got it.
> I've parsed [4] the report descriptor for VRC2, and I guess it does not looks
> that good:
>
> 0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
> 0x09, 0x00,        // Usage (Undefined)
> 0xA1, 0x01,        // Collection (Application)
> 0x15, 0x00,        //   Logical Minimum (0)
> 0x25, 0x01,        //   Logical Maximum (1)
> 0x75, 0x01,        //   Report Size (1)
> 0x05, 0x09,        //   Usage Page (Button)
> 0x19, 0x01,        //   Usage Minimum (0x01)
> 0x29, 0x3F,        //   Usage Maximum (0x3F)
> 0x95, 0x40,        //   Report Count (64)
> 0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0xC0,              // End Collection
>
> The Usage should rather be Joystick than undefined, the other
> fields does also looks wrong to me.

yep, as you saw already :)

>
> The data for each axis (WHEEL and GAS) is 11 bit long (Logical maximum
> 2047 ?), how does the report descriptor map to the actual data in terms
> of offset, order and length?

You probably already got it, but it all depends on how much data is
used on the wire:
- if you have 2 bytes used per each value, then the report size will
be 16 and you set the logical max to 2047
- if the values are packed, then the report size is 11, and then you
probably have to add padding *after* the 22 bits.

>
> >
> > >
> > >
> > > >
> > > > > +
> > > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > > +                              struct hid_report *report,
> > > > > +                              u8 *raw_data, int size)
> > > > > +{
> > > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > > +       u16 value;
> > > > > +
> > > > > +       switch (priv->controller) {
> > > > > +       case PHOENIXRC:
> > > > > +               if (size != PHOENIXRC_DSIZE)
> > > > > +                       break;
> > > > > +
> > > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > +
> > > > > +               /* Z and RZ are sent every other time */
> > > > > +               if (priv->alt)
> > > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > > +               else
> > > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > > +
> > > > > +               priv->alt ^= 1;
> > > > > +               break;
> > > > > +       case VRC2:
> > > > > +               if (size != VRC2_DSIZE)
> > > > > +                       break;
> > > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > > +               break;
> > > > > +       case REALFLIGHT:
> > > > > +               if (size != REALFLIGHT_DSIZE)
> > > > > +                       break;
> > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > > +               input_report_key(priv->input, BTN_A,
> > > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > > +               input_report_key(priv->input, BTN_B,
> > > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > > +               break;
> > > > > +       case XTRG2FMS:
> > > > > +               if (size != XTRG2FMS_DSIZE)
> > > > > +                       break;
> > > > > +
> > > > > +               /* X, RX, Y and RY are sent every time */
> > > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[1];
> > > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[2];
> > > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[0];
> > > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > > +               value = (value << 8) | raw_data[4];
> > > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > > +
> > > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > > +               value = (value << 8) | raw_data[6];
> > > > > +               if (priv->alt)
> > > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > > +               else
> > > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > > +
> > > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > > +               value = (value << 8) | raw_data[5];
> > > > > +               if (priv->alt)
> > > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > > +               else
> > > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > > +
> > > > > +               priv->alt ^= 1;
> > > > > +               break;
> > > > > +       case ORANGERX:
> > > > > +               if (size != ORANGERX_DSIZE)
> > > > > +                       break;
> > > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > +               break;
> > > > > +       };
> > > > > +
> > > > > +       input_sync(priv->input);
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > > +{
> > > > > +       struct device *dev = &hdev->dev;
> > > > > +       struct rcsim_priv *priv;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > > +               return -ENODEV;
> > > >
> > > > You are not accessing anything in the USB stack, so there is no need
> > > > to prevent regression tests that could inject uhid devices to your
> > > > drivers.
> > >
> > > Ok, thanks.
> > >
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > >
> > > Best regards,
> > > Marcus Folkesson
> > >
> > > [1] https://www.usb.org/hid
> > >
> >
> > If you need help in writing report descriptors, I can give you some,
> > but the easiest might be for you to start from the report descriptor
> > in hid-sony.c. I used to have a tool to dynamically write a report
> > descriptor, but I'm not sure it still works...
>
> I think at least some advice would be great :-)
>
> The VRC2 would be the most simple of those, it only has 2 axis with
> resolution of 11-bit.
> If you have time, would you please give some advice what a report descriptor would look
> like and I could probably come up with something for the others.
>
> >
> > FYI, I just re-read rcsim_raw_event() and there is stuff that would
> > require more than just a report descriptor fixup (the fact that some
> > data is sent every other report is not good and will need some manual
> > handling though).
>
>
> Is the fact that more than one button share the same
> byte hard to describe in the report?

No, this is actually easy to describe. You say that there is one usage
of "something" which has a report size of 1 bit, and then you have
another usage of "something else" with the same report size.

But usually you have to add padding after to make up to 8 bits (so 6
bits in that case).

I was referring to the case  where you are parsing the same bit on the
wire, and give a different usage based if you have received an odd or
an even number of reports. In that case, we probably need to use move
this bit to a const field in the original report descriptor and say
that the data is now not const:

- initial report (completely random example):
  X (2 bytes) | Y (2 bytes) | button this_or_that (1 bit, depending of
odd or even received reports) | 7 bits of padding
- we can declare it as:
  X (2 bytes) | Y (2 bytes) | button this (1 bit) | button that (1
bit) | 6 bits of padding

and have raw_event do the translation for the button "this_or_that"
that would populate the correct bit in the translated report.

Cheers,
Benjamin

>
>   value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
>   value = (value << 8) | raw_data[6];
>
> ...
>   value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
>   value = (value << 8) | raw_data[5];
>
>
> >
> > Cheers,
> > Benjamin
> >
>
> Best regards
> Marcus Folkesson
>
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
> [4] https://eleccelerator.com/usbdescreqparser/

Marcus Folkesson Aug. 30, 2022, 2:43 p.m. UTC | #8

Hi Benjamin,

Thank you for your detailed response!

On Tue, Aug 30, 2022 at 02:35:54PM +0200, Benjamin Tissoires wrote:
> Hi Marcus,
> 
> On Sun, Aug 28, 2022 at 4:13 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > Hi,
> >
> > On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
> > > On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
> > > <marcus.folkesson@gmail.com> wrote:
> >
> > > >
> > > > Thank you  Benjamin,
> > > >
> > > > On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > > > > Hi Marcus,
> > > > >
> > > > > [and sorry for the delay in the review of your patches]
> > > > >
> > > > > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > > > > <marcus.folkesson@gmail.com> wrote:
> > > > > >
> > > > > > Several RC Simulator Controllers are HID compliant with similar
> > > > > > interface.
> > > > > >
> > > > > > Add support for these controllers:
> > > > > >  - Phoenix RC (HID variant)
> > > > > >  - Car VRC2.0
> > > > > >  - Real Flight G5/G6/G7
> > > > > >  - Aero Fly, FMS
> > > > > >  - OrangeRX FrSky
> > > > > >
> > > > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > > ---
> > > > > >  Documentation/hid/index.rst |   1 +
> > > > > >  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
> > > > > >  drivers/hid/Kconfig         |  11 ++
> > > > > >  drivers/hid/Makefile        |   1 +
> > > > > >  drivers/hid/hid-ids.h       |   5 +
> > > > > >  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
> > > > > >  6 files changed, 475 insertions(+)
> > > > > >  create mode 100644 Documentation/hid/rcsim.rst
> > > > > >  create mode 100644 drivers/hid/hid-rcsim.c
> > > > > >
> > > > > > diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> > > > > > index e50f513c579c..e5813d264f37 100644
> > > > > > --- a/Documentation/hid/index.rst
> > > > > > +++ b/Documentation/hid/index.rst
> > > > > > @@ -17,3 +17,4 @@ Human Interface Devices (HID)
> > > > > >     hid-alps
> > > > > >     intel-ish-hid
> > > > > >     amd-sfh-hid
> > > > > > +   rcsim
> > > > > > diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> > > > > > new file mode 100644
> > > > > > index 000000000000..1a031f7189cb
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/hid/rcsim.rst
> > > > > > @@ -0,0 +1,142 @@
> > > > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > > > +
> > > > > > +=================================
> > > > > > +rcsim - RC Simulator Controllers
> > > > > > +=================================
> > > > > > +
> > > > > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > > +
> > > > > > +This driver let you use your own RC controller plugged
> > > > > > +into your computer using an HID compatible USB dongle.
> > > > > > +
> > > > > > +There are several HID compatible USB dongles from different
> > > > > > +vendors. The driver currently supports:
> > > > > > +
> > > > > > +- Phoenix RC (HID variant) (8ch)
> > > > > > +- Car VRC2.0 (2ch)
> > > > > > +- Real Flight G5/G6/G7 (6ch)
> > > > > > +- Aero Fly, FMS (8ch)
> > > > > > +- OrangeRX FrSky (6ch)
> > > > > > +
> > > > > > +Many RC controllers is able to configure which stick goes to which channel.
> > > > > > +This is also configurable in most simulators, so a matching is not necessary.
> > > > > > +
> > > > > > +Supported dongles
> > > > > > +==================
> > > > > > +
> > > > > > +PhoenixRC
> > > > > > +----------
> > > > > > +
> > > > > > +The PhoenixRC has one HID compatible variant which is supported by this driver.
> > > > > > +The controller has support for 8 analog channels.
> > > > > > +
> > > > > > +The driver is generating the following input event for on channels:
> > > > > > +
> > > > > > ++---------+----------------+
> > > > > > +| Channel |      Event     |
> > > > > > ++=========+================+
> > > > > > +|     1   |  ABS_Y         |
> > > > > > ++---------+----------------+
> > > > > > +|     2   |  ABS_X         |
> > > > > > ++---------+----------------+
> > > > > > +|     3   |  ABS_RY        |
> > > > > > ++---------+----------------+
> > > > > > +|     4   |  ABS_RX        |
> > > > > > ++---------+----------------+
> > > > > > +|     5   |  ABS_RUDDER    |
> > > > > > ++---------+----------------+
> > > > > > +|     6   |  ABS_THROTTLE  |
> > > > > > ++---------+----------------+
> > > > > > +|     7   |  ABS_Z         |
> > > > > > ++---------+----------------+
> > > > > > +|     8   |  ABS_RZ        |
> > > > > > ++---------+----------------+
> > > > > > +
> > > > > > +VRC2.0
> > > > > > +----------
> > > > > > +VRC2.0 is a controller for RC Cars.
> > > > > > +The controller has support for 2 analog channels.
> > > > > > +
> > > > > > +The driver is generating the following input event for on channels:
> > > > > > +
> > > > > > ++---------+----------------+
> > > > > > +| Channel |      Event     |
> > > > > > ++=========+================+
> > > > > > +|     1   |  ABS_GAS       |
> > > > > > ++---------+----------------+
> > > > > > +|     2   |  ABS_WHEEL     |
> > > > > > ++---------+----------------+
> > > > > > +
> > > > > > +RealFlight
> > > > > > +----------
> > > > > > +
> > > > > > +This driver supports Realflight G4-G7 and above
> > > > > > +The controller has support for 4 analog channels and two buttons.
> > > > > > +
> > > > > > +The driver is generating the following input event for on channels:
> > > > > > +
> > > > > > ++---------+----------------+
> > > > > > +| Channel |      Event     |
> > > > > > ++=========+================+
> > > > > > +|     1   |  ABS_Y         |
> > > > > > ++---------+----------------+
> > > > > > +|     2   |  ABS_X         |
> > > > > > ++---------+----------------+
> > > > > > +|     3   |  ABS_RY        |
> > > > > > ++---------+----------------+
> > > > > > +|     4   |  ABS_RX        |
> > > > > > ++---------+----------------+
> > > > > > +|     5   |  BTN_A         |
> > > > > > ++---------+----------------+
> > > > > > +|     6   |  BTN_B         |
> > > > > > ++---------+----------------+
> > > > > > +
> > > > > > +XTR+G2+FMS Controllers
> > > > > > +--------------------------------
> > > > > > +
> > > > > > +The controllers has support for 8 analog channels.
> > > > > > +
> > > > > > +The driver is generating the following input event for on channels:
> > > > > > +
> > > > > > ++---------+----------------+
> > > > > > +| Channel |      Event     |
> > > > > > ++=========+================+
> > > > > > +|     1   |  ABS_Y         |
> > > > > > ++---------+----------------+
> > > > > > +|     2   |  ABS_X         |
> > > > > > ++---------+----------------+
> > > > > > +|     3   |  ABS_RY        |
> > > > > > ++---------+----------------+
> > > > > > +|     4   |  ABS_RX        |
> > > > > > ++---------+----------------+
> > > > > > +|     5   |  ABS_RUDDER    |
> > > > > > ++---------+----------------+
> > > > > > +|     6   |  ABS_THROTTLE  |
> > > > > > ++---------+----------------+
> > > > > > +|     7   |  ABS_Z         |
> > > > > > ++---------+----------------+
> > > > > > +|     8   |  ABS_RZ        |
> > > > > > ++---------+----------------+
> > > > > > +
> > > > > > +OrangeRX
> > > > > > +----------
> > > > > > +
> > > > > > +The controllers has support for 6 analog channels.
> > > > > > +
> > > > > > +The driver is generating the following input event for on channels:
> > > > > > +
> > > > > > ++---------+----------------+
> > > > > > +| Channel |      Event     |
> > > > > > ++=========+================+
> > > > > > +|     1   |  ABS_Y         |
> > > > > > ++---------+----------------+
> > > > > > +|     2   |  ABS_X         |
> > > > > > ++---------+----------------+
> > > > > > +|     3   |  ABS_RY        |
> > > > > > ++---------+----------------+
> > > > > > +|     4   |  ABS_RX        |
> > > > > > ++---------+----------------+
> > > > > > +|     5   |  ABS_RUDDER    |
> > > > > > ++---------+----------------+
> > > > > > +|     6   |  ABS_THROTTLE  |
> > > > > > ++---------+----------------+
> > > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > > > index 70da5931082f..d8313d36086c 100644
> > > > > > --- a/drivers/hid/Kconfig
> > > > > > +++ b/drivers/hid/Kconfig
> > > > > > @@ -957,6 +957,17 @@ config HID_RAZER
> > > > > >         Support for Razer devices that are not fully compliant with the
> > > > > >         HID standard.
> > > > > >
> > > > > > +config HID_RCSIM
> > > > > > +       tristate "RC Simulator Controllers"
> > > > > > +       depends on HID
> > > > > > +       help
> > > > > > +       Support for several HID compatible RC Simulator Controllers including
> > > > > > +         - Phoenix RC
> > > > > > +         - Car VRC2.0
> > > > > > +         - Real Flight
> > > > > > +         - Aero Fly, FMS
> > > > > > +         - OrangeRX FrSky
> > > > > > +
> > > > > >  config HID_PRIMAX
> > > > > >         tristate "Primax non-fully HID-compliant devices"
> > > > > >         depends on HID
> > > > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > > > index cac2cbe26d11..85d50ab352ee 100644
> > > > > > --- a/drivers/hid/Makefile
> > > > > > +++ b/drivers/hid/Makefile
> > > > > > @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
> > > > > >  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
> > > > > >  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> > > > > >  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> > > > > > +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o
> > > > >
> > > > > General rule of thumbs, we try to name the drivers after their
> > > > > vendors, unless we know we have a generic driver.
> > > > >
> > > > > Here, this driver seems to be really tied to a small set of devices,
> > > > > and thus I don't think we can call it "generic".
> > > >
> > > > Got it.
> > > >
> > > > >
> > > > > >  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
> > > > > >  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> > > > > >  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > > > index d9eb676abe96..baf5f74d5bed 100644
> > > > > > --- a/drivers/hid/hid-ids.h
> > > > > > +++ b/drivers/hid/hid-ids.h
> > > > > > @@ -1381,6 +1381,11 @@
> > > > > >
> > > > > >  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
> > > > > >  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> > > > > > +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> > > > > > +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> > > > > > +
> > > > > > +#define USB_VENDOR_ID_DIPLING  0x0B9B
> > > > > > +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
> > > > > >
> > > > > >  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
> > > > > >  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> > > > > > diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..0f214cb5816a
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/hid/hid-rcsim.c
> > > > > > @@ -0,0 +1,315 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +/*
> > > > > > + * Driver for several HID compatible RC Simulator Controllers.
> > > > > > + * Currently supported controllers are:
> > > > > > + *
> > > > > > + * - Phoenix RC (HID variant) (8ch)
> > > > > > + * - Car VRC2.0 (2ch)
> > > > > > + * - Real Flight G5/G6/G7 (6ch)
> > > > > > + * - Aero Fly, FMS (8ch)
> > > > > > + * - OrangeRX FrSky (6ch)
> > > > > > + *
> > > > > > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/bitfield.h>
> > > > > > +#include <linux/device.h>
> > > > > > +#include <linux/input.h>
> > > > > > +#include <linux/hid.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/usb.h>
> > > > > > +
> > > > > > +#include "hid-ids.h"
> > > > > > +
> > > > > > +/*
> > > > > > + * Some of these VID/PID are probably "borrowed", so keep them locally and
> > > > > > + * do not populate hid-ids.h with those.
> > > > > > + */
> > > > > > +
> > > > > > +/* PHOENIXRC Controlloer (HID variant) */
> > > > > > +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> > > > > > +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> > > > > > +#define PHOENIXRC_DSIZE        (8)
> > > > > > +
> > > > > > +/* VRC2 Controlloer */
> > > > > > +#define VRC2_VID       (0x07c0)
> > > > > > +#define VRC2_PID       (0x1125)
> > > > > > +#define VRC2_DSIZE     (7)
> > > > > > +
> > > > > > +/* Realflight G4-&7 and Above Controller */
> > > > > > +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> > > > > > +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> > > > > > +#define REALFLIGHT_DSIZE       (8)
> > > > > > +
> > > > > > +#define REALFLIGHT_BTN_A       BIT(0)
> > > > > > +#define REALFLIGHT_BTN_B       BIT(1)
> > > > > > +
> > > > > > +/* XTR+G2+FMS Controller */
> > > > > > +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> > > > > > +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> > > > > > +#define XTRG2FMS_DSIZE (8)
> > > > > > +
> > > > > > +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> > > > > > +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> > > > > > +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> > > > > > +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> > > > > > +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> > > > > > +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> > > > > > +
> > > > > > +/* OrangeRX FrSky */
> > > > > > +#define ORANGERX_VID   (0x0451)
> > > > > > +#define ORANGERX_PID   (0x16a5)
> > > > > > +#define ORANGERX_DSIZE (8)
> > > > > > +
> > > > > > +enum rcsim_controller {
> > > > > > +       PHOENIXRC,
> > > > > > +       VRC2,
> > > > > > +       REALFLIGHT,
> > > > > > +       XTRG2FMS,
> > > > > > +       ORANGERX
> > > > > > +};
> > > > > > +
> > > > > > +struct rcsim_priv {
> > > > > > +       struct hid_device *hdev;
> > > > > > +       struct input_dev *input;
> > > > > > +       enum rcsim_controller controller;
> > > > > > +       u8 alt;
> > > > > > +};
> > > > > > +
> > > > > > +static int rcsim_open(struct input_dev *dev)
> > > > > > +{
> > > > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > > > +
> > > > > > +       return hid_hw_open(priv->hdev);
> > > > > > +}
> > > > > > +
> > > > > > +static void rcsim_close(struct input_dev *dev)
> > > > > > +{
> > > > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > > > +
> > > > > > +       hid_hw_close(priv->hdev);
> > > > > > +}
> > > > > > +
> > > > > > +static int rcsim_setup_input(struct rcsim_priv *priv)
> > > > > > +{
> > > > > > +       struct input_dev *input;
> > > > > > +
> > > > > > +       input = devm_input_allocate_device(&priv->hdev->dev);
> > > > > > +       if (!input)
> > > > > > +               return -ENOMEM;
> > > > > > +
> > > > > > +       input->id.bustype = priv->hdev->bus;
> > > > > > +       input->id.vendor  = priv->hdev->vendor;
> > > > > > +       input->id.product = priv->hdev->product;
> > > > > > +       input->id.version = priv->hdev->bus;
> > > > > > +       input->phys = priv->hdev->phys;
> > > > > > +       input->uniq = priv->hdev->uniq;
> > > > > > +       input->open = rcsim_open;
> > > > > > +       input->close = rcsim_close;
> > > > > > +
> > > > > > +       input_set_drvdata(input, priv);
> > > > > > +
> > > > > > +       switch (priv->controller) {
> > > > > > +       case PHOENIXRC:
> > > > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > > > +               input->name = "RC Simuator Controller PhoenixRC";
> > > > > > +               break;
> > > > > > +       case VRC2:
> > > > > > +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> > > > > > +               input->name = "RC Simuator Controller VRC2.0";
> > > > > > +               break;
> > > > > > +       case REALFLIGHT:
> > > > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > > > +               input_set_capability(input, EV_KEY, BTN_A);
> > > > > > +               input_set_capability(input, EV_KEY, BTN_B);
> > > > > > +               input->name = "RC Simuator Controller Realflight";
> > > > > > +               break;
> > > > > > +       case XTRG2FMS:
> > > > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> > > > > > +               input->name = "RC Simuator Controller AeroFly, FMS";
> > > > > > +               priv->alt = 0;
> > > > > > +               break;
> > > > > > +       case ORANGERX:
> > > > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > > > +               input->name = "RC Simuator Controller OrangeRX FrSky";
> > > > > > +               break;
> > > > > > +       };
> > > > > > +
> > > > > > +       priv->input = input;
> > > > > > +       return input_register_device(priv->input);
> > > > > > +}
> > > > >
> > > > > You are basically rewriting hid-input.c, which is suboptimal.
> > > >
> > > > Ouch. I will have a look at hid-input, thanks.
> > > >
> > > > >
> > > > > I guess the report descriptor provided by these devices are basically
> > > > > useless, and so you have to parse the reports yourself in the
> > > > > raw_event callback.
> > > >
> > > > Yep.
> > > >
> > > > >
> > > > > But instead of manually doing that, why not overwrite the report
> > > > > descriptor (with .rdesc_fixup) and declare here all of the data that
> > > >  Do you mean .report_fixup?
> > >
> > > yes, sorry :/
> > >
> > > >
> > > > > needs to be exported. You could remove basically everything in this
> > > > > driver by just providing a fixed report descriptor.
> > > >
> > > > What you are aiming for is to fixup the report descriptor and let the
> > > > generic hid-raw driver handle the rest, or do I get you wrong?
> > >
> > > yep, exactly
> > >
> > > >
> > > > How is the report mapped to certain events then?
> > >
> > > Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
> > > events are mapped to input events with a one to one mapping.
> > >
> > > >
> > > > I do read at [1] but it is not obvious how to put it together.
> > > > Most drivers I've looked at that is using .report_fixup just fix broken
> > > > reports. I guess these reports are not "broken", just.. odd?
> > >
> > > Have a look at [2], lots of full report descriptors :)
> > >
> > > And in your case, the reports are incomplete, not odd.
> > >
> > > >
> > > >
> > > > >
> > > > > > +
> > > > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > > > +                              struct hid_report *report,
> > > > > > +                              u8 *raw_data, int size)
> > > > > > +{
> > > > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > > > +       u16 value;
> > > > > > +
> > > > > > +       switch (priv->controller) {
> > > > > > +       case PHOENIXRC:
> > > > > > +               if (size != PHOENIXRC_DSIZE)
> > > > > > +                       break;
> > > > > > +
> > > > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > > +
> > > > > > +               /* Z and RZ are sent every other time */
> > > > > > +               if (priv->alt)
> > > > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > > > +               else
> > > > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > > > +
> > > > > > +               priv->alt ^= 1;
> > > > > > +               break;
> > > > > > +       case VRC2:
> > > > > > +               if (size != VRC2_DSIZE)
> > > > > > +                       break;
> > > > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > > > +               break;
> > > > > > +       case REALFLIGHT:
> > > > > > +               if (size != REALFLIGHT_DSIZE)
> > > > > > +                       break;
> > > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > > > +               input_report_key(priv->input, BTN_A,
> > > > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > > > +               input_report_key(priv->input, BTN_B,
> > > > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > > > +               break;
> > > > > > +       case XTRG2FMS:
> > > > > > +               if (size != XTRG2FMS_DSIZE)
> > > > > > +                       break;
> > > > > > +
> > > > > > +               /* X, RX, Y and RY are sent every time */
> > > > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > > > +               value = (value << 8) | raw_data[1];
> > > > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > > > +
> > > > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > > > +               value = (value << 8) | raw_data[2];
> > > > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > > > +
> > > > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > > > +               value = (value << 8) | raw_data[0];
> > > > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > > > +
> > > > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > > > +               value = (value << 8) | raw_data[4];
> > > > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > > > +
> > > > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > > > +               value = (value << 8) | raw_data[6];
> > > > > > +               if (priv->alt)
> > > > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > > > +               else
> > > > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > > > +
> > > > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > > > +               value = (value << 8) | raw_data[5];
> > > > > > +               if (priv->alt)
> > > > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > > > +               else
> > > > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > > > +
> > > > > > +               priv->alt ^= 1;
> > > > > > +               break;
> > > > > > +       case ORANGERX:
> > > > > > +               if (size != ORANGERX_DSIZE)
> > > > > > +                       break;
> > > > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > > +               break;
> > > > > > +       };
> > > > > > +
> > > > > > +       input_sync(priv->input);
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > > > +{
> > > > > > +       struct device *dev = &hdev->dev;
> > > > > > +       struct rcsim_priv *priv;
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > > > +               return -ENODEV;
> > > > >
> > > > > You are not accessing anything in the USB stack, so there is no need
> > > > > to prevent regression tests that could inject uhid devices to your
> > > > > drivers.
> > > >
> > > > Ok, thanks.
> > > >
> > > > >
> > > > > Cheers,
> > > > > Benjamin
> > > > >
> > > >
> > > > Best regards,
> > > > Marcus Folkesson
> > > >
> > > > [1] https://www.usb.org/hid
> > > >
> > >
> > > If you need help in writing report descriptors, I can give you some,
> > > but the easiest might be for you to start from the report descriptor
> > > in hid-sony.c. I used to have a tool to dynamically write a report
> > > descriptor, but I'm not sure it still works...
> >
> > I've worked with the report descriptor for VRC2, and have come up with
> > something that works.
> 
> Great that you managed to fill in most of the gaps :)
> 
> >
> >
> > static __u8 vrc2_rdesc_fixed[] = {
> >         0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
> >         0x09, 0x04,        // Usage (Joystick)
> >         0xA1, 0x01,        // Collection (Application)
> >         0x09, 0x01,        //   Usage (Pointer)
> >         0xA1, 0x00,        //   Collection (Physical)
> >         0x09, 0x30,        //     Usage (X)
> >         0x09, 0x31,        //     Usage (Y)
> >         0x15, 0x00,        //     Logical Minimum (0)
> >         0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
> >         0x35, 0x00,        //     Physical Minimum (0)
> >         0x46, 0xFF, 0x00,  //     Physical Maximum (255)
> >         0x75, 0x10,        //     Report Size (16)
> >         0x95, 0x02,        //     Report Count (2)
> >         0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> >         0xC0,              //   End Collection
> >         0x95, 0x01,        //   Report Count (1)
> >         0x75, 0x18,        //   Report Size (24)
> >         0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
> >         0x09, 0x00,        //   Usage (Undefined)
> >         0x15, 0x00,        //   Logical Minimum (0)
> >         0x26, 0xFF, 0x00,  //   Logical Maximum (255)
> >         0x75, 0x08,        //   Report Size (8)
> >         0x95, 0x08,        //   Report Count (8)
> >         0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> >         0xC0,              // End Collection
> > };
> >
> >
> > It is a result of reading specifications and looking at various
> > examples.
> >
> > These lines are taken from an example, and I do not know why I need
> > those. I think the constant report size is wrong (should be 32 to
> 
> Do you have an example of the actual data sent on the wire? hexdump or
> hid-recorder should be enough for me to get what is missing.

Sorry, this will be a little confusing.

I just realized that there are actually two hidraw devices showing
up.

One with this descriptor:
0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
0x09, 0x00,        // Usage (Undefined)
0xA1, 0x01,        // Collection (Application)
0x15, 0x00,        //   Logical Minimum (0)
0x25, 0x01,        //   Logical Maximum (1)
0x75, 0x01,        //   Report Size (1)
0x05, 0x09,        //   Usage Page (Button)
0x19, 0x01,        //   Usage Minimum (0x01)
0x29, 0x3F,        //   Usage Maximum (0x3F)
0x95, 0x40,        //   Report Count (64)
0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0xC0,              // End Collection

Output from hid-recorder:
E: 000074.735891 8 20 00 ed 03 00 00 00 cc
#  Button: 0  0  0  0  0  1  0  0  0  0  0  0  0  0  0  0  0  0  0  1  0  1  1  1  1  1  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  1  0  0  1 , 1

At a first glance, it did look as the button bits follows the same
pattern as the joystick axis in the descriptor below. It is hard to
debug though, as it causes random clicks and button presses
everywhere... and it driving me nuts.


The other one looks like this:
0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
0x09, 0x04,        // Usage (Joystick)
0xA1, 0x01,        // Collection (Application)
0x09, 0x01,        //   Usage (Pointer)
0xA1, 0x00,        //   Collection (Physical)
0x09, 0x30,        //     Usage (X)
0x09, 0x31,        //     Usage (Y)
0x09, 0x32,        //     Usage (Z)
0x15, 0x00,        //     Logical Minimum (0)
0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
0x35, 0x00,        //     Physical Minimum (0)
0x46, 0xFF, 0x00,  //     Physical Maximum (255)
0x75, 0x10,        //     Report Size (16)
0x95, 0x03,        //     Report Count (3)
0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0xC0,              //   End Collection
0x05, 0x09,        //   Usage Page (Button)
0x19, 0x01,        //   Usage Minimum (0x01)
0x29, 0x02,        //   Usage Maximum (0x02)
0x15, 0x00,        //   Logical Minimum (0)
0x25, 0x01,        //   Logical Maximum (1)
0x95, 0x02,        //   Report Count (2)
0x75, 0x01,        //   Report Size (1)
0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0x95, 0x01,        //   Report Count (1)
0x75, 0x06,        //   Report Size (6)
0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
0x09, 0x00,        //   Usage (0x00)
0x15, 0x00,        //   Logical Minimum (0)
0x26, 0xFF, 0x00,  //   Logical Maximum (255)
0x75, 0x08,        //   Report Size (8)
0x95, 0x08,        //   Report Count (8)
0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
0xC0,              // End Collection

Output from hid-recorder:
E: 000013.240054 7 00 00 00 00 80 00 00
#  X:      110 | Y:      122 | Z:    0   | Button: 0  0 | #


The controller does only support 2 axis (X/Y) and no buttons, so this
descriptor is wrong in any way.
I see that it makes use of Feature as well.
Does this feature have something to do with the first HID device?

> 
> As I read the report descriptors, we had:
> * old one:
>   - opening of application collection of usage undefined
>     - 64 buttons (logical max of 1), usage being being 0x01 and 0x3F
> (63) -> so the last bit is ignored
> 
> Which would translate to a report of the following (each 0 being a
> separate button, | is marking bytes):
> 
> 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0,
> 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0,
> 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, X
> 
> -> 64 * 1 = 64 bits
> 
> ()last bit being ignored because of its duplicated usage with the previous one)
> 
> * new one:
>   - opening of an application collection of usage Joystick
>     - opening of a physical collection of type Pointer
>       - 16 bits for X, with logical max of 2047 and physical max of 255
>       - 16 bits for Y, with logical max of 2047 and physical max of 255
>     - 1 const element of size 24 that has to be ignored
>     - one feature that should not be there :)
> 
> So it would translate into the following (c being constant bit):
> X (2 bytes) | Y (2 bytes) | c, c, c, c, c, c, c, c | c, c, c, c, c, c,
> c, c | c, c, c, c, c, c, c, c
> 
> -> 24 + 2*8 + 2*8 = 56 bits
> 
> So I agree with you, you are missing 16 bits
> 
> > fill up) and I do not know what the Feature is used for.
> 
> You should not have to declare a feature. A feature is a special HID
> endpoint that you can use to talk to the HID device. By emitting
> special HID commands, (HID_REPORT_GET/HID_REPORT_SET) you can control
> how the device works. But if your current report descriptor doesn't
> have a feature declared, you should not declare it.
> 
> >
> >         0x95, 0x01,        //   Report Count (1)
> >         0x75, 0x18,        //   Report Size (24)
> >         0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
> >         0x09, 0x00,        //   Usage (Undefined)
> >         0x15, 0x00,        //   Logical Minimum (0)
> >         0x26, 0xFF, 0x00,  //   Logical Maximum (255)
> >         0x75, 0x08,        //   Report Size (8)
> >         0x95, 0x08,        //   Report Count (8)
> >         0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> >
> > If I omit any of these lines then I only get tons of
> >
> > kernel: usb 3-12.3.1.3.3: input irq status -75 received
> 
> I assume the feature in this particular case helps the driver to
> understand that you need to allocate a 64 bits chunk of memory to
> communicate with the device.
> 
> I would advise to use the following report instead:
> 
> ----
> static __u8 vrc2_rdesc_fixed[] = {
>         0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
>         0x09, 0x04,        // Usage (Joystick)
>         0xA1, 0x01,        // Collection (Application)
>         0x09, 0x01,        //   Usage (Pointer)
>         0xA1, 0x00,        //   Collection (Physical)
>         0x09, 0x30,        //     Usage (X)
>         0x09, 0x31,        //     Usage (Y)
>         0x15, 0x00,        //     Logical Minimum (0)
>         0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
>         0x35, 0x00,        //     Physical Minimum (0)
>         0x46, 0xFF, 0x00,  //     Physical Maximum (255)
>         0x75, 0x10,        //     Report Size (16)
>         0x95, 0x02,        //     Report Count (2)
>         0x81, 0x02,        //     Input (Data,Var,Abs,No
> Wrap,Linear,Preferred State,No Null Position)
>         0xC0,              //   End Collection
>         0x75, 0x08,        //   Report Size (8)
>        0x95, 0x04,        //   Report Count (4)
>         0x81, 0x03,        //   Input (Cnst,Var,Abs)
>         0xC0,              // End Collection
> };
> ---

This report is more of what I want it to look like.

I got the same "usb 3-12.3.1.3.1: input irq status -75 received" error
when using this descriptor though.
I think it is beacause it is applied to both hid-devices?


> 
> Few changes:
> - use of 4 bytes of padding, instead of 1 24bits slot of padding
> - changed the padding from 0x01 to 0x03 to remove the "array" const
> definition which is tricky to handle
> - removed the feature report.
> 
> Cheers,
> Benjamin
> 
> >
> > In the kernel log.
> >
> >
> > >
> > > FYI, I just re-read rcsim_raw_event() and there is stuff that would
> > > require more than just a report descriptor fixup (the fact that some
> > > data is sent every other report is not good and will need some manual
> > > handling though).
> > >
> > > Cheers,
> > > Benjamin
> > >
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> > > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
> > >
> >
> >
> > Best regards
> > Marcus Folkesson
> 

Best regards,
Marcus Folkesson

Benjamin Tissoires Aug. 30, 2022, 3:01 p.m. UTC | #9

On Tue, Aug 30, 2022 at 4:38 PM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Hi Benjamin,
>
> Thank you for your detailed response!
>
> On Tue, Aug 30, 2022 at 02:35:54PM +0200, Benjamin Tissoires wrote:
> > Hi Marcus,
> >
> > On Sun, Aug 28, 2022 at 4:13 PM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Aug 23, 2022 at 06:43:47PM +0200, Benjamin Tissoires wrote:
> > > > On Tue, Aug 23, 2022 at 5:13 PM Marcus Folkesson
> > > > <marcus.folkesson@gmail.com> wrote:
> > >
> > > > >
> > > > > Thank you  Benjamin,
> > > > >
> > > > > On Tue, Aug 23, 2022 at 11:49:59AM +0200, Benjamin Tissoires wrote:
> > > > > > Hi Marcus,
> > > > > >
> > > > > > [and sorry for the delay in the review of your patches]
> > > > > >
> > > > > > On Mon, Aug 22, 2022 at 8:04 AM Marcus Folkesson
> > > > > > <marcus.folkesson@gmail.com> wrote:
> > > > > > >
> > > > > > > Several RC Simulator Controllers are HID compliant with similar
> > > > > > > interface.
> > > > > > >
> > > > > > > Add support for these controllers:
> > > > > > >  - Phoenix RC (HID variant)
> > > > > > >  - Car VRC2.0
> > > > > > >  - Real Flight G5/G6/G7
> > > > > > >  - Aero Fly, FMS
> > > > > > >  - OrangeRX FrSky
> > > > > > >
> > > > > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > > > ---
> > > > > > >  Documentation/hid/index.rst |   1 +
> > > > > > >  Documentation/hid/rcsim.rst | 142 ++++++++++++++++
> > > > > > >  drivers/hid/Kconfig         |  11 ++
> > > > > > >  drivers/hid/Makefile        |   1 +
> > > > > > >  drivers/hid/hid-ids.h       |   5 +
> > > > > > >  drivers/hid/hid-rcsim.c     | 315 ++++++++++++++++++++++++++++++++++++
> > > > > > >  6 files changed, 475 insertions(+)
> > > > > > >  create mode 100644 Documentation/hid/rcsim.rst
> > > > > > >  create mode 100644 drivers/hid/hid-rcsim.c
> > > > > > >
> > > > > > > diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> > > > > > > index e50f513c579c..e5813d264f37 100644
> > > > > > > --- a/Documentation/hid/index.rst
> > > > > > > +++ b/Documentation/hid/index.rst
> > > > > > > @@ -17,3 +17,4 @@ Human Interface Devices (HID)
> > > > > > >     hid-alps
> > > > > > >     intel-ish-hid
> > > > > > >     amd-sfh-hid
> > > > > > > +   rcsim
> > > > > > > diff --git a/Documentation/hid/rcsim.rst b/Documentation/hid/rcsim.rst
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..1a031f7189cb
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/hid/rcsim.rst
> > > > > > > @@ -0,0 +1,142 @@
> > > > > > > +.. SPDX-License-Identifier: GPL-2.0
> > > > > > > +
> > > > > > > +=================================
> > > > > > > +rcsim - RC Simulator Controllers
> > > > > > > +=================================
> > > > > > > +
> > > > > > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > > > +
> > > > > > > +This driver let you use your own RC controller plugged
> > > > > > > +into your computer using an HID compatible USB dongle.
> > > > > > > +
> > > > > > > +There are several HID compatible USB dongles from different
> > > > > > > +vendors. The driver currently supports:
> > > > > > > +
> > > > > > > +- Phoenix RC (HID variant) (8ch)
> > > > > > > +- Car VRC2.0 (2ch)
> > > > > > > +- Real Flight G5/G6/G7 (6ch)
> > > > > > > +- Aero Fly, FMS (8ch)
> > > > > > > +- OrangeRX FrSky (6ch)
> > > > > > > +
> > > > > > > +Many RC controllers is able to configure which stick goes to which channel.
> > > > > > > +This is also configurable in most simulators, so a matching is not necessary.
> > > > > > > +
> > > > > > > +Supported dongles
> > > > > > > +==================
> > > > > > > +
> > > > > > > +PhoenixRC
> > > > > > > +----------
> > > > > > > +
> > > > > > > +The PhoenixRC has one HID compatible variant which is supported by this driver.
> > > > > > > +The controller has support for 8 analog channels.
> > > > > > > +
> > > > > > > +The driver is generating the following input event for on channels:
> > > > > > > +
> > > > > > > ++---------+----------------+
> > > > > > > +| Channel |      Event     |
> > > > > > > ++=========+================+
> > > > > > > +|     1   |  ABS_Y         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     2   |  ABS_X         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     3   |  ABS_RY        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     4   |  ABS_RX        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     5   |  ABS_RUDDER    |
> > > > > > > ++---------+----------------+
> > > > > > > +|     6   |  ABS_THROTTLE  |
> > > > > > > ++---------+----------------+
> > > > > > > +|     7   |  ABS_Z         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     8   |  ABS_RZ        |
> > > > > > > ++---------+----------------+
> > > > > > > +
> > > > > > > +VRC2.0
> > > > > > > +----------
> > > > > > > +VRC2.0 is a controller for RC Cars.
> > > > > > > +The controller has support for 2 analog channels.
> > > > > > > +
> > > > > > > +The driver is generating the following input event for on channels:
> > > > > > > +
> > > > > > > ++---------+----------------+
> > > > > > > +| Channel |      Event     |
> > > > > > > ++=========+================+
> > > > > > > +|     1   |  ABS_GAS       |
> > > > > > > ++---------+----------------+
> > > > > > > +|     2   |  ABS_WHEEL     |
> > > > > > > ++---------+----------------+
> > > > > > > +
> > > > > > > +RealFlight
> > > > > > > +----------
> > > > > > > +
> > > > > > > +This driver supports Realflight G4-G7 and above
> > > > > > > +The controller has support for 4 analog channels and two buttons.
> > > > > > > +
> > > > > > > +The driver is generating the following input event for on channels:
> > > > > > > +
> > > > > > > ++---------+----------------+
> > > > > > > +| Channel |      Event     |
> > > > > > > ++=========+================+
> > > > > > > +|     1   |  ABS_Y         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     2   |  ABS_X         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     3   |  ABS_RY        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     4   |  ABS_RX        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     5   |  BTN_A         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     6   |  BTN_B         |
> > > > > > > ++---------+----------------+
> > > > > > > +
> > > > > > > +XTR+G2+FMS Controllers
> > > > > > > +--------------------------------
> > > > > > > +
> > > > > > > +The controllers has support for 8 analog channels.
> > > > > > > +
> > > > > > > +The driver is generating the following input event for on channels:
> > > > > > > +
> > > > > > > ++---------+----------------+
> > > > > > > +| Channel |      Event     |
> > > > > > > ++=========+================+
> > > > > > > +|     1   |  ABS_Y         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     2   |  ABS_X         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     3   |  ABS_RY        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     4   |  ABS_RX        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     5   |  ABS_RUDDER    |
> > > > > > > ++---------+----------------+
> > > > > > > +|     6   |  ABS_THROTTLE  |
> > > > > > > ++---------+----------------+
> > > > > > > +|     7   |  ABS_Z         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     8   |  ABS_RZ        |
> > > > > > > ++---------+----------------+
> > > > > > > +
> > > > > > > +OrangeRX
> > > > > > > +----------
> > > > > > > +
> > > > > > > +The controllers has support for 6 analog channels.
> > > > > > > +
> > > > > > > +The driver is generating the following input event for on channels:
> > > > > > > +
> > > > > > > ++---------+----------------+
> > > > > > > +| Channel |      Event     |
> > > > > > > ++=========+================+
> > > > > > > +|     1   |  ABS_Y         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     2   |  ABS_X         |
> > > > > > > ++---------+----------------+
> > > > > > > +|     3   |  ABS_RY        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     4   |  ABS_RX        |
> > > > > > > ++---------+----------------+
> > > > > > > +|     5   |  ABS_RUDDER    |
> > > > > > > ++---------+----------------+
> > > > > > > +|     6   |  ABS_THROTTLE  |
> > > > > > > ++---------+----------------+
> > > > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > > > > index 70da5931082f..d8313d36086c 100644
> > > > > > > --- a/drivers/hid/Kconfig
> > > > > > > +++ b/drivers/hid/Kconfig
> > > > > > > @@ -957,6 +957,17 @@ config HID_RAZER
> > > > > > >         Support for Razer devices that are not fully compliant with the
> > > > > > >         HID standard.
> > > > > > >
> > > > > > > +config HID_RCSIM
> > > > > > > +       tristate "RC Simulator Controllers"
> > > > > > > +       depends on HID
> > > > > > > +       help
> > > > > > > +       Support for several HID compatible RC Simulator Controllers including
> > > > > > > +         - Phoenix RC
> > > > > > > +         - Car VRC2.0
> > > > > > > +         - Real Flight
> > > > > > > +         - Aero Fly, FMS
> > > > > > > +         - OrangeRX FrSky
> > > > > > > +
> > > > > > >  config HID_PRIMAX
> > > > > > >         tristate "Primax non-fully HID-compliant devices"
> > > > > > >         depends on HID
> > > > > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > > > > index cac2cbe26d11..85d50ab352ee 100644
> > > > > > > --- a/drivers/hid/Makefile
> > > > > > > +++ b/drivers/hid/Makefile
> > > > > > > @@ -102,6 +102,7 @@ obj-$(CONFIG_HID_PLANTRONICS)       += hid-plantronics.o
> > > > > > >  obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
> > > > > > >  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
> > > > > > >  obj-$(CONFIG_HID_RAZER)        += hid-razer.o
> > > > > > > +obj-$(CONFIG_HID_RCSIM)        += hid-rcsim.o
> > > > > >
> > > > > > General rule of thumbs, we try to name the drivers after their
> > > > > > vendors, unless we know we have a generic driver.
> > > > > >
> > > > > > Here, this driver seems to be really tied to a small set of devices,
> > > > > > and thus I don't think we can call it "generic".
> > > > >
> > > > > Got it.
> > > > >
> > > > > >
> > > > > > >  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
> > > > > > >  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> > > > > > >  obj-$(CONFIG_HID_ROCCAT)       += hid-roccat.o hid-roccat-common.o \
> > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > > > > index d9eb676abe96..baf5f74d5bed 100644
> > > > > > > --- a/drivers/hid/hid-ids.h
> > > > > > > +++ b/drivers/hid/hid-ids.h
> > > > > > > @@ -1381,6 +1381,11 @@
> > > > > > >
> > > > > > >  #define USB_VENDOR_ID_MULTIPLE_1781    0x1781
> > > > > > >  #define USB_DEVICE_ID_RAPHNET_4NES4SNES_OLD    0x0a9d
> > > > > > > +#define USB_DEVICE_ID_PHOENIXRC        0x0898
> > > > > > > +#define USB_DEVICE_ID_REALFLIGHT       0x0e56
> > > > > > > +
> > > > > > > +#define USB_VENDOR_ID_DIPLING  0x0B9B
> > > > > > > +#define USB_DEVICE_ID_DIPLING_RCCONTROLLER     0x4012
> > > > > > >
> > > > > > >  #define USB_VENDOR_ID_DRACAL_RAPHNET   0x289b
> > > > > > >  #define USB_DEVICE_ID_RAPHNET_2NES2SNES        0x0002
> > > > > > > diff --git a/drivers/hid/hid-rcsim.c b/drivers/hid/hid-rcsim.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..0f214cb5816a
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/hid/hid-rcsim.c
> > > > > > > @@ -0,0 +1,315 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > > +/*
> > > > > > > + * Driver for several HID compatible RC Simulator Controllers.
> > > > > > > + * Currently supported controllers are:
> > > > > > > + *
> > > > > > > + * - Phoenix RC (HID variant) (8ch)
> > > > > > > + * - Car VRC2.0 (2ch)
> > > > > > > + * - Real Flight G5/G6/G7 (6ch)
> > > > > > > + * - Aero Fly, FMS (8ch)
> > > > > > > + * - OrangeRX FrSky (6ch)
> > > > > > > + *
> > > > > > > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/bitfield.h>
> > > > > > > +#include <linux/device.h>
> > > > > > > +#include <linux/input.h>
> > > > > > > +#include <linux/hid.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/usb.h>
> > > > > > > +
> > > > > > > +#include "hid-ids.h"
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Some of these VID/PID are probably "borrowed", so keep them locally and
> > > > > > > + * do not populate hid-ids.h with those.
> > > > > > > + */
> > > > > > > +
> > > > > > > +/* PHOENIXRC Controlloer (HID variant) */
> > > > > > > +#define PHOENIXRC_VID  (USB_VENDOR_ID_MULTIPLE_1781)
> > > > > > > +#define PHOENIXRC_PID  (USB_DEVICE_ID_PHOENIXRC)
> > > > > > > +#define PHOENIXRC_DSIZE        (8)
> > > > > > > +
> > > > > > > +/* VRC2 Controlloer */
> > > > > > > +#define VRC2_VID       (0x07c0)
> > > > > > > +#define VRC2_PID       (0x1125)
> > > > > > > +#define VRC2_DSIZE     (7)
> > > > > > > +
> > > > > > > +/* Realflight G4-&7 and Above Controller */
> > > > > > > +#define REALFLIGHT_VID (USB_VENDOR_ID_MULTIPLE_1781)
> > > > > > > +#define REALFLIGHT_PID (USB_DEVICE_ID_REALFLIGHT)
> > > > > > > +#define REALFLIGHT_DSIZE       (8)
> > > > > > > +
> > > > > > > +#define REALFLIGHT_BTN_A       BIT(0)
> > > > > > > +#define REALFLIGHT_BTN_B       BIT(1)
> > > > > > > +
> > > > > > > +/* XTR+G2+FMS Controller */
> > > > > > > +#define XTRG2FMS_VID   (USB_VENDOR_ID_DIPLING)
> > > > > > > +#define XTRG2FMS_PID   (USB_DEVICE_ID_DIPLING_RCCONTROLLER)
> > > > > > > +#define XTRG2FMS_DSIZE (8)
> > > > > > > +
> > > > > > > +#define XTRG2FMS_X_HI  GENMASK(3, 2)
> > > > > > > +#define XTRG2FMS_Y_HI  GENMASK(1, 0)
> > > > > > > +#define XTRG2FMS_RX_HI GENMASK(7, 6)
> > > > > > > +#define XTRG2FMS_RY_HI GENMASK(5, 4)
> > > > > > > +#define XTRG2FMS_ALT1_HI       GENMASK(3, 2)
> > > > > > > +#define XTRG2FMS_ALT2_HI       GENMASK(1, 0)
> > > > > > > +
> > > > > > > +/* OrangeRX FrSky */
> > > > > > > +#define ORANGERX_VID   (0x0451)
> > > > > > > +#define ORANGERX_PID   (0x16a5)
> > > > > > > +#define ORANGERX_DSIZE (8)
> > > > > > > +
> > > > > > > +enum rcsim_controller {
> > > > > > > +       PHOENIXRC,
> > > > > > > +       VRC2,
> > > > > > > +       REALFLIGHT,
> > > > > > > +       XTRG2FMS,
> > > > > > > +       ORANGERX
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct rcsim_priv {
> > > > > > > +       struct hid_device *hdev;
> > > > > > > +       struct input_dev *input;
> > > > > > > +       enum rcsim_controller controller;
> > > > > > > +       u8 alt;
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int rcsim_open(struct input_dev *dev)
> > > > > > > +{
> > > > > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > > > > +
> > > > > > > +       return hid_hw_open(priv->hdev);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void rcsim_close(struct input_dev *dev)
> > > > > > > +{
> > > > > > > +       struct rcsim_priv *priv = input_get_drvdata(dev);
> > > > > > > +
> > > > > > > +       hid_hw_close(priv->hdev);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int rcsim_setup_input(struct rcsim_priv *priv)
> > > > > > > +{
> > > > > > > +       struct input_dev *input;
> > > > > > > +
> > > > > > > +       input = devm_input_allocate_device(&priv->hdev->dev);
> > > > > > > +       if (!input)
> > > > > > > +               return -ENOMEM;
> > > > > > > +
> > > > > > > +       input->id.bustype = priv->hdev->bus;
> > > > > > > +       input->id.vendor  = priv->hdev->vendor;
> > > > > > > +       input->id.product = priv->hdev->product;
> > > > > > > +       input->id.version = priv->hdev->bus;
> > > > > > > +       input->phys = priv->hdev->phys;
> > > > > > > +       input->uniq = priv->hdev->uniq;
> > > > > > > +       input->open = rcsim_open;
> > > > > > > +       input->close = rcsim_close;
> > > > > > > +
> > > > > > > +       input_set_drvdata(input, priv);
> > > > > > > +
> > > > > > > +       switch (priv->controller) {
> > > > > > > +       case PHOENIXRC:
> > > > > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > > > > +               input->name = "RC Simuator Controller PhoenixRC";
> > > > > > > +               break;
> > > > > > > +       case VRC2:
> > > > > > > +               input_set_abs_params(input, ABS_GAS, 0, 2048, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_WHEEL, 0, 2048, 0, 0);
> > > > > > > +               input->name = "RC Simuator Controller VRC2.0";
> > > > > > > +               break;
> > > > > > > +       case REALFLIGHT:
> > > > > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > > > > +               input_set_capability(input, EV_KEY, BTN_A);
> > > > > > > +               input_set_capability(input, EV_KEY, BTN_B);
> > > > > > > +               input->name = "RC Simuator Controller Realflight";
> > > > > > > +               break;
> > > > > > > +       case XTRG2FMS:
> > > > > > > +               input_set_abs_params(input, ABS_X, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_Y, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RX, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RY, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_Z, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RZ, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 1024, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 1024, 0, 0);
> > > > > > > +               input->name = "RC Simuator Controller AeroFly, FMS";
> > > > > > > +               priv->alt = 0;
> > > > > > > +               break;
> > > > > > > +       case ORANGERX:
> > > > > > > +               input_set_abs_params(input, ABS_X, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_Y, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RX, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RY, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_RUDDER, 0, 255, 0, 0);
> > > > > > > +               input_set_abs_params(input, ABS_THROTTLE, 0, 255, 0, 0);
> > > > > > > +               input->name = "RC Simuator Controller OrangeRX FrSky";
> > > > > > > +               break;
> > > > > > > +       };
> > > > > > > +
> > > > > > > +       priv->input = input;
> > > > > > > +       return input_register_device(priv->input);
> > > > > > > +}
> > > > > >
> > > > > > You are basically rewriting hid-input.c, which is suboptimal.
> > > > >
> > > > > Ouch. I will have a look at hid-input, thanks.
> > > > >
> > > > > >
> > > > > > I guess the report descriptor provided by these devices are basically
> > > > > > useless, and so you have to parse the reports yourself in the
> > > > > > raw_event callback.
> > > > >
> > > > > Yep.
> > > > >
> > > > > >
> > > > > > But instead of manually doing that, why not overwrite the report
> > > > > > descriptor (with .rdesc_fixup) and declare here all of the data that
> > > > >  Do you mean .report_fixup?
> > > >
> > > > yes, sorry :/
> > > >
> > > > >
> > > > > > needs to be exported. You could remove basically everything in this
> > > > > > driver by just providing a fixed report descriptor.
> > > > >
> > > > > What you are aiming for is to fixup the report descriptor and let the
> > > > > generic hid-raw driver handle the rest, or do I get you wrong?
> > > >
> > > > yep, exactly
> > > >
> > > > >
> > > > > How is the report mapped to certain events then?
> > > >
> > > > Have a look at hid_configure_usage in hid-input.c [3]. Most of HID
> > > > events are mapped to input events with a one to one mapping.
> > > >
> > > > >
> > > > > I do read at [1] but it is not obvious how to put it together.
> > > > > Most drivers I've looked at that is using .report_fixup just fix broken
> > > > > reports. I guess these reports are not "broken", just.. odd?
> > > >
> > > > Have a look at [2], lots of full report descriptors :)
> > > >
> > > > And in your case, the reports are incomplete, not odd.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +static int rcsim_raw_event(struct hid_device *hdev,
> > > > > > > +                              struct hid_report *report,
> > > > > > > +                              u8 *raw_data, int size)
> > > > > > > +{
> > > > > > > +       struct rcsim_priv *priv = hid_get_drvdata(hdev);
> > > > > > > +       u16 value;
> > > > > > > +
> > > > > > > +       switch (priv->controller) {
> > > > > > > +       case PHOENIXRC:
> > > > > > > +               if (size != PHOENIXRC_DSIZE)
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               /* X, RX, Y and RY, RUDDER and THROTTLE are sent every time */
> > > > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[0]);
> > > > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[4]);
> > > > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > > > +
> > > > > > > +               /* Z and RZ are sent every other time */
> > > > > > > +               if (priv->alt)
> > > > > > > +                       input_report_abs(priv->input, ABS_Z, raw_data[7]);
> > > > > > > +               else
> > > > > > > +                       input_report_abs(priv->input, ABS_RZ, raw_data[7]);
> > > > > > > +
> > > > > > > +               priv->alt ^= 1;
> > > > > > > +               break;
> > > > > > > +       case VRC2:
> > > > > > > +               if (size != VRC2_DSIZE)
> > > > > > > +                       break;
> > > > > > > +               value = (raw_data[1] << 8 | raw_data[0]) & GENMASK(10, 0);
> > > > > > > +               input_report_abs(priv->input, ABS_GAS, value);
> > > > > > > +               value = (raw_data[3] << 8 | raw_data[2]) & GENMASK(10, 0);
> > > > > > > +               input_report_abs(priv->input, ABS_WHEEL, value);
> > > > > > > +               break;
> > > > > > > +       case REALFLIGHT:
> > > > > > > +               if (size != REALFLIGHT_DSIZE)
> > > > > > > +                       break;
> > > > > > > +               input_report_abs(priv->input, ABS_X, raw_data[2]);
> > > > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[1]);
> > > > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[5]);
> > > > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[3]);
> > > > > > > +               input_report_abs(priv->input, ABS_MISC, raw_data[4]);
> > > > > > > +               input_report_key(priv->input, BTN_A,
> > > > > > > +                               raw_data[7] & REALFLIGHT_BTN_A);
> > > > > > > +               input_report_key(priv->input, BTN_B,
> > > > > > > +                               raw_data[7] & REALFLIGHT_BTN_B);
> > > > > > > +               break;
> > > > > > > +       case XTRG2FMS:
> > > > > > > +               if (size != XTRG2FMS_DSIZE)
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               /* X, RX, Y and RY are sent every time */
> > > > > > > +               value = FIELD_GET(XTRG2FMS_X_HI, raw_data[3]);
> > > > > > > +               value = (value << 8) | raw_data[1];
> > > > > > > +               input_report_abs(priv->input, ABS_X, value);
> > > > > > > +
> > > > > > > +               value = FIELD_GET(XTRG2FMS_Y_HI, raw_data[3]);
> > > > > > > +               value = (value << 8) | raw_data[2];
> > > > > > > +               input_report_abs(priv->input, ABS_Y, value);
> > > > > > > +
> > > > > > > +               value = FIELD_GET(XTRG2FMS_RX_HI, raw_data[3]);
> > > > > > > +               value = (value << 8) | raw_data[0];
> > > > > > > +               input_report_abs(priv->input, ABS_RX, value);
> > > > > > > +
> > > > > > > +               value = FIELD_GET(XTRG2FMS_RY_HI, raw_data[3]);
> > > > > > > +               value = (value << 8) | raw_data[4];
> > > > > > > +               input_report_abs(priv->input, ABS_RY, value);
> > > > > > > +
> > > > > > > +               /* Z, RZ, RUDDER and THROTTLE are sent every other time */
> > > > > > > +               value = FIELD_GET(XTRG2FMS_ALT1_HI, raw_data[7]);
> > > > > > > +               value = (value << 8) | raw_data[6];
> > > > > > > +               if (priv->alt)
> > > > > > > +                       input_report_abs(priv->input, ABS_Z, value);
> > > > > > > +               else
> > > > > > > +                       input_report_abs(priv->input, ABS_RUDDER, value);
> > > > > > > +
> > > > > > > +               value = FIELD_GET(XTRG2FMS_ALT2_HI, raw_data[7]);
> > > > > > > +               value = (value << 8) | raw_data[5];
> > > > > > > +               if (priv->alt)
> > > > > > > +                       input_report_abs(priv->input, ABS_RZ, value);
> > > > > > > +               else
> > > > > > > +                       input_report_abs(priv->input, ABS_THROTTLE, value);
> > > > > > > +
> > > > > > > +               priv->alt ^= 1;
> > > > > > > +               break;
> > > > > > > +       case ORANGERX:
> > > > > > > +               if (size != ORANGERX_DSIZE)
> > > > > > > +                       break;
> > > > > > > +               input_report_abs(priv->input, ABS_X, raw_data[0]);
> > > > > > > +               input_report_abs(priv->input, ABS_Y, raw_data[2]);
> > > > > > > +               input_report_abs(priv->input, ABS_RX, raw_data[3]);
> > > > > > > +               input_report_abs(priv->input, ABS_RY, raw_data[1]);
> > > > > > > +               input_report_abs(priv->input, ABS_RUDDER, raw_data[5]);
> > > > > > > +               input_report_abs(priv->input, ABS_THROTTLE, raw_data[6]);
> > > > > > > +               break;
> > > > > > > +       };
> > > > > > > +
> > > > > > > +       input_sync(priv->input);
> > > > > > > +       return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int rcsim_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > > > > > +{
> > > > > > > +       struct device *dev = &hdev->dev;
> > > > > > > +       struct rcsim_priv *priv;
> > > > > > > +       int ret;
> > > > > > > +
> > > > > > > +       if (!hid_is_using_ll_driver(hdev, &usb_hid_driver))
> > > > > > > +               return -ENODEV;
> > > > > >
> > > > > > You are not accessing anything in the USB stack, so there is no need
> > > > > > to prevent regression tests that could inject uhid devices to your
> > > > > > drivers.
> > > > >
> > > > > Ok, thanks.
> > > > >
> > > > > >
> > > > > > Cheers,
> > > > > > Benjamin
> > > > > >
> > > > >
> > > > > Best regards,
> > > > > Marcus Folkesson
> > > > >
> > > > > [1] https://www.usb.org/hid
> > > > >
> > > >
> > > > If you need help in writing report descriptors, I can give you some,
> > > > but the easiest might be for you to start from the report descriptor
> > > > in hid-sony.c. I used to have a tool to dynamically write a report
> > > > descriptor, but I'm not sure it still works...
> > >
> > > I've worked with the report descriptor for VRC2, and have come up with
> > > something that works.
> >
> > Great that you managed to fill in most of the gaps :)
> >
> > >
> > >
> > > static __u8 vrc2_rdesc_fixed[] = {
> > >         0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
> > >         0x09, 0x04,        // Usage (Joystick)
> > >         0xA1, 0x01,        // Collection (Application)
> > >         0x09, 0x01,        //   Usage (Pointer)
> > >         0xA1, 0x00,        //   Collection (Physical)
> > >         0x09, 0x30,        //     Usage (X)
> > >         0x09, 0x31,        //     Usage (Y)
> > >         0x15, 0x00,        //     Logical Minimum (0)
> > >         0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
> > >         0x35, 0x00,        //     Physical Minimum (0)
> > >         0x46, 0xFF, 0x00,  //     Physical Maximum (255)
> > >         0x75, 0x10,        //     Report Size (16)
> > >         0x95, 0x02,        //     Report Count (2)
> > >         0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> > >         0xC0,              //   End Collection
> > >         0x95, 0x01,        //   Report Count (1)
> > >         0x75, 0x18,        //   Report Size (24)
> > >         0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
> > >         0x09, 0x00,        //   Usage (Undefined)
> > >         0x15, 0x00,        //   Logical Minimum (0)
> > >         0x26, 0xFF, 0x00,  //   Logical Maximum (255)
> > >         0x75, 0x08,        //   Report Size (8)
> > >         0x95, 0x08,        //   Report Count (8)
> > >         0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> > >         0xC0,              // End Collection
> > > };
> > >
> > >
> > > It is a result of reading specifications and looking at various
> > > examples.
> > >
> > > These lines are taken from an example, and I do not know why I need
> > > those. I think the constant report size is wrong (should be 32 to
> >
> > Do you have an example of the actual data sent on the wire? hexdump or
> > hid-recorder should be enough for me to get what is missing.
>
> Sorry, this will be a little confusing.
>
> I just realized that there are actually two hidraw devices showing
> up.
>
> One with this descriptor:
> 0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
> 0x09, 0x00,        // Usage (Undefined)
> 0xA1, 0x01,        // Collection (Application)
> 0x15, 0x00,        //   Logical Minimum (0)
> 0x25, 0x01,        //   Logical Maximum (1)
> 0x75, 0x01,        //   Report Size (1)
> 0x05, 0x09,        //   Usage Page (Button)
> 0x19, 0x01,        //   Usage Minimum (0x01)
> 0x29, 0x3F,        //   Usage Maximum (0x3F)
> 0x95, 0x40,        //   Report Count (64)
> 0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0xC0,              // End Collection
>
> Output from hid-recorder:
> E: 000074.735891 8 20 00 ed 03 00 00 00 cc
> #  Button: 0  0  0  0  0  1  0  0  0  0  0  0  0  0  0  0  0  0  0  1  0  1  1  1  1  1  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  1  1  0  0  1 , 1
>
> At a first glance, it did look as the button bits follows the same
> pattern as the joystick axis in the descriptor below. It is hard to
> debug though, as it causes random clicks and button presses
> everywhere... and it driving me nuts.

Maybe the easiest is to ignore that hid device in .probe:
- if the report descriptor size is 23, return -ENODEV.

And then see if you get all the correct events with the second HID device.

>
>
> The other one looks like this:
> 0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
> 0x09, 0x04,        // Usage (Joystick)
> 0xA1, 0x01,        // Collection (Application)
> 0x09, 0x01,        //   Usage (Pointer)
> 0xA1, 0x00,        //   Collection (Physical)
> 0x09, 0x30,        //     Usage (X)
> 0x09, 0x31,        //     Usage (Y)
> 0x09, 0x32,        //     Usage (Z)
> 0x15, 0x00,        //     Logical Minimum (0)
> 0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
> 0x35, 0x00,        //     Physical Minimum (0)
> 0x46, 0xFF, 0x00,  //     Physical Maximum (255)
> 0x75, 0x10,        //     Report Size (16)
> 0x95, 0x03,        //     Report Count (3)
> 0x81, 0x02,        //     Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0xC0,              //   End Collection
> 0x05, 0x09,        //   Usage Page (Button)
> 0x19, 0x01,        //   Usage Minimum (0x01)
> 0x29, 0x02,        //   Usage Maximum (0x02)
> 0x15, 0x00,        //   Logical Minimum (0)
> 0x25, 0x01,        //   Logical Maximum (1)
> 0x95, 0x02,        //   Report Count (2)
> 0x75, 0x01,        //   Report Size (1)
> 0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0x95, 0x01,        //   Report Count (1)
> 0x75, 0x06,        //   Report Size (6)
> 0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0x09, 0x00,        //   Usage (0x00)
> 0x15, 0x00,        //   Logical Minimum (0)
> 0x26, 0xFF, 0x00,  //   Logical Maximum (255)
> 0x75, 0x08,        //   Report Size (8)
> 0x95, 0x08,        //   Report Count (8)
> 0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> 0xC0,              // End Collection
>
> Output from hid-recorder:
> E: 000013.240054 7 00 00 00 00 80 00 00
> #  X:      110 | Y:      122 | Z:    0   | Button: 0  0 | #
>
>
> The controller does only support 2 axis (X/Y) and no buttons, so this
> descriptor is wrong in any way.

Well, if the values are always 0, it is not so terrible. It will look
like a proper controller with buttons that are never pressed, which is
good enough in 99% of the cases.

> I see that it makes use of Feature as well.
> Does this feature have something to do with the first HID device?

The fact that you have 2 hidraw devices, means that you have 2
separate USB endpoints on the device. Virtually, you have 2 entirely
separated USB devices in one.
Features are defined per USB endpoint. So you can not define a feature
on one device if the device doesn't support it, and the other way
around.

>
> >
> > As I read the report descriptors, we had:
> > * old one:
> >   - opening of application collection of usage undefined
> >     - 64 buttons (logical max of 1), usage being being 0x01 and 0x3F
> > (63) -> so the last bit is ignored
> >
> > Which would translate to a report of the following (each 0 being a
> > separate button, | is marking bytes):
> >
> > 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0,
> > 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0,
> > 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, 0 | 0, 0, 0, 0, 0, 0, 0, X
> >
> > -> 64 * 1 = 64 bits
> >
> > ()last bit being ignored because of its duplicated usage with the previous one)
> >
> > * new one:
> >   - opening of an application collection of usage Joystick
> >     - opening of a physical collection of type Pointer
> >       - 16 bits for X, with logical max of 2047 and physical max of 255
> >       - 16 bits for Y, with logical max of 2047 and physical max of 255
> >     - 1 const element of size 24 that has to be ignored
> >     - one feature that should not be there :)
> >
> > So it would translate into the following (c being constant bit):
> > X (2 bytes) | Y (2 bytes) | c, c, c, c, c, c, c, c | c, c, c, c, c, c,
> > c, c | c, c, c, c, c, c, c, c
> >
> > -> 24 + 2*8 + 2*8 = 56 bits
> >
> > So I agree with you, you are missing 16 bits
> >
> > > fill up) and I do not know what the Feature is used for.
> >
> > You should not have to declare a feature. A feature is a special HID
> > endpoint that you can use to talk to the HID device. By emitting
> > special HID commands, (HID_REPORT_GET/HID_REPORT_SET) you can control
> > how the device works. But if your current report descriptor doesn't
> > have a feature declared, you should not declare it.
> >
> > >
> > >         0x95, 0x01,        //   Report Count (1)
> > >         0x75, 0x18,        //   Report Size (24)
> > >         0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
> > >         0x09, 0x00,        //   Usage (Undefined)
> > >         0x15, 0x00,        //   Logical Minimum (0)
> > >         0x26, 0xFF, 0x00,  //   Logical Maximum (255)
> > >         0x75, 0x08,        //   Report Size (8)
> > >         0x95, 0x08,        //   Report Count (8)
> > >         0xB1, 0x02,        //   Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> > >
> > > If I omit any of these lines then I only get tons of
> > >
> > > kernel: usb 3-12.3.1.3.3: input irq status -75 received
> >
> > I assume the feature in this particular case helps the driver to
> > understand that you need to allocate a 64 bits chunk of memory to
> > communicate with the device.
> >
> > I would advise to use the following report instead:
> >
> > ----
> > static __u8 vrc2_rdesc_fixed[] = {
> >         0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
> >         0x09, 0x04,        // Usage (Joystick)
> >         0xA1, 0x01,        // Collection (Application)
> >         0x09, 0x01,        //   Usage (Pointer)
> >         0xA1, 0x00,        //   Collection (Physical)
> >         0x09, 0x30,        //     Usage (X)
> >         0x09, 0x31,        //     Usage (Y)
> >         0x15, 0x00,        //     Logical Minimum (0)
> >         0x26, 0xFF, 0x07,  //     Logical Maximum (2047)
> >         0x35, 0x00,        //     Physical Minimum (0)
> >         0x46, 0xFF, 0x00,  //     Physical Maximum (255)
> >         0x75, 0x10,        //     Report Size (16)
> >         0x95, 0x02,        //     Report Count (2)
> >         0x81, 0x02,        //     Input (Data,Var,Abs,No
> > Wrap,Linear,Preferred State,No Null Position)
> >         0xC0,              //   End Collection
> >         0x75, 0x08,        //   Report Size (8)
> >        0x95, 0x04,        //   Report Count (4)
> >         0x81, 0x03,        //   Input (Cnst,Var,Abs)
> >         0xC0,              // End Collection
> > };
> > ---
>
> This report is more of what I want it to look like.
>
> I got the same "usb 3-12.3.1.3.1: input irq status -75 received" error
> when using this descriptor though.
> I think it is beacause it is applied to both hid-devices?

Yes, you need to check which one you are applying the fixup. The
easiest for that is to check for the current report descriptor size.
If it's the one you expect, change it. If not, ignore it.

But let's first have a look at ignoring the first hidraw device
entirely and see if it solves the problem for this one.

Cheers,
Benjamin

>
>
> >
> > Few changes:
> > - use of 4 bytes of padding, instead of 1 24bits slot of padding
> > - changed the padding from 0x01 to 0x03 to remove the "array" const
> > definition which is tricky to handle
> > - removed the feature report.
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > In the kernel log.
> > >
> > >
> > > >
> > > > FYI, I just re-read rcsim_raw_event() and there is stuff that would
> > > > require more than just a report descriptor fixup (the fact that some
> > > > data is sent every other report is not good and will need some manual
> > > > handling though).
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-uclogic-rdesc.c
> > > > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-input.c#n817
> > > >
> > >
> > >
> > > Best regards
> > > Marcus Folkesson
> >
>
> Best regards,
> Marcus Folkesson

Marcus Folkesson Sept. 15, 2022, 7:34 a.m. UTC | #10

Hi Benjamin,

On Tue, Aug 30, 2022 at 02:45:11PM +0200, Benjamin Tissoires wrote:
> On Thu, Aug 25, 2022 at 8:44 AM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >


[...]
> >
> >
> > Is the fact that more than one button share the same
> > byte hard to describe in the report?
> 
> No, this is actually easy to describe. You say that there is one usage
> of "something" which has a report size of 1 bit, and then you have
> another usage of "something else" with the same report size.
> 
> But usually you have to add padding after to make up to 8 bits (so 6
> bits in that case).
> 
> I was referring to the case  where you are parsing the same bit on the
> wire, and give a different usage based if you have received an odd or
> an even number of reports. In that case, we probably need to use move
> this bit to a const field in the original report descriptor and say
> that the data is now not const:
> 
> - initial report (completely random example):
>   X (2 bytes) | Y (2 bytes) | button this_or_that (1 bit, depending of
> odd or even received reports) | 7 bits of padding
> - we can declare it as:
>   X (2 bytes) | Y (2 bytes) | button this (1 bit) | button that (1
> bit) | 6 bits of padding

How about if there is no unused bytes?

The XTRG2FMS has 8 10-bit channels and use every byte in the report.
Should I specify 8 8-bit channels instead and fix that in raw_event?
If so, should I only use 8bit values then?

(Are you at the ELCE conference btw?)

Best regards
Marcus Folkesson

Benjamin Tissoires Sept. 15, 2022, 7:35 a.m. UTC | #11

On Thu, Sep 15, 2022 at 8:28 AM Marcus Folkesson
<marcus.folkesson@gmail.com> wrote:
>
> Hi Benjamin,
>
> On Tue, Aug 30, 2022 at 02:45:11PM +0200, Benjamin Tissoires wrote:
> > On Thu, Aug 25, 2022 at 8:44 AM Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:
> > >
>
>
> [...]
> > >
> > >
> > > Is the fact that more than one button share the same
> > > byte hard to describe in the report?
> >
> > No, this is actually easy to describe. You say that there is one usage
> > of "something" which has a report size of 1 bit, and then you have
> > another usage of "something else" with the same report size.
> >
> > But usually you have to add padding after to make up to 8 bits (so 6
> > bits in that case).
> >
> > I was referring to the case  where you are parsing the same bit on the
> > wire, and give a different usage based if you have received an odd or
> > an even number of reports. In that case, we probably need to use move
> > this bit to a const field in the original report descriptor and say
> > that the data is now not const:
> >
> > - initial report (completely random example):
> >   X (2 bytes) | Y (2 bytes) | button this_or_that (1 bit, depending of
> > odd or even received reports) | 7 bits of padding
> > - we can declare it as:
> >   X (2 bytes) | Y (2 bytes) | button this (1 bit) | button that (1
> > bit) | 6 bits of padding
>
> How about if there is no unused bytes?
>
> The XTRG2FMS has 8 10-bit channels and use every byte in the report.
> Should I specify 8 8-bit channels instead and fix that in raw_event?
> If so, should I only use 8bit values then?

If I am not wrong, you should be able to add another byte in the
report descriptor, as long as your raw_event function always adds it.
Though now that I am typing it, I am actually wondering if this will
work. You can always try, there is a chance it'll work, but I can't
remember if it'll result in a timeout on the USB front because it'll
expect one more byte that will never arrive.

>
> (Are you at the ELCE conference btw?)

I was at Plumbers this week, but got an extra day today. But yeah, I'm
in Dublin today.

Cheers,
Benjamin

>
> Best regards
> Marcus Folkesson

Benjamin Tissoires Sept. 19, 2022, 1:32 p.m. UTC | #12

On 9/15/22 09:35, Benjamin Tissoires wrote:
> On Thu, Sep 15, 2022 at 8:28 AM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
>>
>> Hi Benjamin,
>>
>> On Tue, Aug 30, 2022 at 02:45:11PM +0200, Benjamin Tissoires wrote:
>>> On Thu, Aug 25, 2022 at 8:44 AM Marcus Folkesson
>>> <marcus.folkesson@gmail.com> wrote:
>>>>
>>
>>
>> [...]
>>>>
>>>>
>>>> Is the fact that more than one button share the same
>>>> byte hard to describe in the report?
>>>
>>> No, this is actually easy to describe. You say that there is one usage
>>> of "something" which has a report size of 1 bit, and then you have
>>> another usage of "something else" with the same report size.
>>>
>>> But usually you have to add padding after to make up to 8 bits (so 6
>>> bits in that case).
>>>
>>> I was referring to the case  where you are parsing the same bit on the
>>> wire, and give a different usage based if you have received an odd or
>>> an even number of reports. In that case, we probably need to use move
>>> this bit to a const field in the original report descriptor and say
>>> that the data is now not const:
>>>
>>> - initial report (completely random example):
>>>    X (2 bytes) | Y (2 bytes) | button this_or_that (1 bit, depending of
>>> odd or even received reports) | 7 bits of padding
>>> - we can declare it as:
>>>    X (2 bytes) | Y (2 bytes) | button this (1 bit) | button that (1
>>> bit) | 6 bits of padding
>>
>> How about if there is no unused bytes?
>>
>> The XTRG2FMS has 8 10-bit channels and use every byte in the report.
>> Should I specify 8 8-bit channels instead and fix that in raw_event?
>> If so, should I only use 8bit values then?
> 
> If I am not wrong, you should be able to add another byte in the
> report descriptor, as long as your raw_event function always adds it.
> Though now that I am typing it, I am actually wondering if this will
> work. You can always try, there is a chance it'll work, but I can't
> remember if it'll result in a timeout on the USB front because it'll
> expect one more byte that will never arrive.

I am back home, and I just tested that. I had a doubt, and it is indeed
failing. You need the following change for this to be working (I need to
send it as a proper patch after assessing it hasn't side effects)

---

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 13cce286247e..f37ffe2bd488 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -275,6 +275,7 @@ static void hid_irq_in(struct urb *urb)
         int                     status;
  
         switch (urb->status) {
+       case -EOVERFLOW:        /* happens with modified report descriptors */
         case 0:                 /* success */
                 usbhid->retry_delay = 0;
                 if (!test_bit(HID_OPENED, &usbhid->iofl))
---

Cheers,
Benjamin

> 
>>
>> (Are you at the ELCE conference btw?)
> 
> I was at Plumbers this week, but got an extra day today. But yeah, I'm
> in Dublin today.
> 
> Cheers,
> Benjamin
> 
>>
>> Best regards
>> Marcus Folkesson

Marcus Folkesson Jan. 10, 2023, 2:48 p.m. UTC | #13

Hi Benjamin,

On Mon, Sep 19, 2022 at 03:32:37PM +0200, Benjamin Tissoires wrote:
> 
> 

[...]

> 
> I am back home, and I just tested that. I had a doubt, and it is indeed
> failing. You need the following change for this to be working (I need to
> send it as a proper patch after assessing it hasn't side effects)

Did it come up with any side effects? :-)

> 
> ---
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 13cce286247e..f37ffe2bd488 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -275,6 +275,7 @@ static void hid_irq_in(struct urb *urb)
>         int                     status;
>         switch (urb->status) {
> +       case -EOVERFLOW:        /* happens with modified report descriptors */
>         case 0:                 /* success */
>                 usbhid->retry_delay = 0;
>                 if (!test_bit(HID_OPENED, &usbhid->iofl))
> ---
> 
> Cheers,
> Benjamin
> 

Best regards,
Marcus Folkesson

Pavel Machek March 25, 2023, 4:08 p.m. UTC | #14

Hi!

> > +Many RC controllers is able to configure which stick goes to which channel.
> > +This is also configurable in most simulators, so a matching is not necessary.

While it is configurable, we should try to do the good job "by default". Configuring controllers
can take hours, and it is not fun.

> > +The driver is generating the following input event for on channels:
> > +
> > ++---------+----------------+
> > +| Channel |      Event     |
> > ++=========+================+
> > +|     1   |  ABS_Y         |
> > ++---------+----------------+
> > +|     2   |  ABS_X         |
> > ++---------+----------------+
> > +|     3   |  ABS_RY        |
> > ++---------+----------------+
> > +|     4   |  ABS_RX        |
> > ++---------+----------------+
> > +|     5   |  BTN_A         |
> > ++---------+----------------+
> > +|     6   |  BTN_B         |
> > ++---------+----------------+

If one of these is normally used as throttle and rudder, it should be marked as such...

User can then remap it if he does not like the mapping.

Best regards,
									Pavel

[RESEND,1/2] HID: Add driver for RC Simulator Controllers

Commit Message

Comments

Patch