Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 Aug 2022 12:56:51 +0200
From:   Marcus Folkesson <marcus.folkesson@gmail.com>
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     Jiri Kosina <jikos@kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] HID: Add driver for VRC-2 Car Controller
Message-ID: <Yw8+cx+0QyHe6K95@gmail.com>
References: <20220830195055.1812192-1-marcus.folkesson@gmail.com>
 <20220830195055.1812192-2-marcus.folkesson@gmail.com>
 <CAO-hwJ+34yUM0nkp8HPnubf8-aHX7RXWt=m5u2qFgXByGg0+Ng@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="3X/soURl3JHM+qm5"
Content-Disposition: inline
In-Reply-To: <CAO-hwJ+34yUM0nkp8HPnubf8-aHX7RXWt=m5u2qFgXByGg0+Ng@mail.gmail.com>
Precedence: bulk


--3X/soURl3JHM+qm5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Aug 31, 2022 at 11:45:21AM +0200, Benjamin Tissoires wrote:
> On Tue, Aug 30, 2022 at 9:46 PM Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> >
> > VRC-2 is 2-axis controller often used in car simulators.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >  MAINTAINERS            |   6 +++
> >  drivers/hid/Kconfig    |   9 ++++
> >  drivers/hid/Makefile   |   1 +
> >  drivers/hid/hid-vrc2.c | 100 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 116 insertions(+)
> >  create mode 100644 drivers/hid/hid-vrc2.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 216b9f021f72..33010f93c993 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -8919,6 +8919,12 @@ F:       drivers/hid/hid-sensor-*
> >  F:     drivers/iio/*/hid-*
> >  F:     include/linux/hid-sensor-*
> >
> > +HID VRC-2 CAR CONTROLLER DRIVER
> > +M:     Marcus Folkesson <marcus.folkesson@gmail.com>
> > +L:     linux-input@vger.kernel.org
> > +S:     Maintained
> > +F:     drivers/hid/hid-vrc2.c
> > +
> >  HID WACOM DRIVER
> >  M:     Ping Cheng <ping.cheng@wacom.com>
> >  M:     Jason Gerecke  <jason.gerecke@wacom.com>
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index d8313d36086c..518e0a5a7852 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -481,6 +481,15 @@ config HID_VIEWSONIC
> >         help
> >           Support for ViewSonic/Signotec PD1011 signature pad.
> >
> > +config HID_VRC2
> > +       tristate "VRC-2 Car Controller"
> > +       depends on HID
> > +       help
> > +        Support for VRC-2 2-axis Car Controller
>=20
> maybe "Support for VRC-2 which is a 2-axis controller often used in
> car simulators." to be a little bit more explicit.

Will change, thank you.

>=20
> > +
> > +        To compile this driver as a module, choose M here: the
> > +        module will be called hid-vrc2.
> > +
> >  config HID_XIAOMI
> >         tristate "Xiaomi"
> >         depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 85d50ab352ee..32ab85b1a776 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -137,6 +137,7 @@ obj-$(CONFIG_HID_XINMO)             +=3D hid-xinmo.o
> >  obj-$(CONFIG_HID_ZEROPLUS)     +=3D hid-zpff.o
> >  obj-$(CONFIG_HID_ZYDACRON)     +=3D hid-zydacron.o
> >  obj-$(CONFIG_HID_VIEWSONIC)    +=3D hid-viewsonic.o
> > +obj-$(CONFIG_HID_VRC2)         +=3D hid-vrc2.o
> >
> >  wacom-objs                     :=3D wacom_wac.o wacom_sys.o
> >  obj-$(CONFIG_HID_WACOM)                +=3D wacom.o
> > diff --git a/drivers/hid/hid-vrc2.c b/drivers/hid/hid-vrc2.c
> > new file mode 100644
> > index 000000000000..43c98607b837
> > --- /dev/null
> > +++ b/drivers/hid/hid-vrc2.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * HID driver for VRC-2 2-axis Car controller
> > + *
> > + * Copyright (C) 2022 Marcus Folkesson <marcus.folkesson@gmail.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +
> > +/*
> > + * VID/PID are probably "borrowed", so keep them locally and
> > + * do not populate hid-ids.h with those.
> > + */
> > +#define USB_VENDOR_ID_VRC2     (0x07c0)
> > +#define USB_DEVICE_ID_VRC2     (0x1125)
> > +
> > +static __u8 vrc2_rdesc_fixed[] =3D {
> > +       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,Pr=
eferred State,No Null Position)
> > +       0xC0,              //   End Collection
> > +       0x75, 0x08,        //   Report Size (8)
> > +       0x95, 0x03,        //   Report Count (3)
> > +       0x81, 0x03,        //   Input (Cnst,Var,Abs)
> > +       0xC0,              // End Collection
> > +};
> > +
> > +static __u8 *vrc2_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > +                               unsigned int *rsize)
> > +{
> > +       if (*rsize =3D=3D 23)
> > +               return rdesc;
>=20
> This test is not required. probe() returned -ENODEV for this
> interface, so we are sure we have the correct one when we are there.

Got it.

>=20
> > +
> > +       hid_info(hdev, "fixing up VRC-2 report descriptor\n");
> > +       *rsize =3D sizeof(vrc2_rdesc_fixed);
> > +       return vrc2_rdesc_fixed;
> > +}
> > +
> > +static int vrc2_probe(struct hid_device *hdev, const struct hid_device=
_id *id)
> > +{
> > +       int ret;
> > +
> > +       /*
> > +        * The device gives us 2 separate USB endpoints.
> > +        * One of those (the one with report descriptor size of 23) is =
just bogus so ignore it
> > +        */
> > +       if (hdev->dev_rsize =3D=3D 23)
> > +               return -ENODEV;
> > +
> > +       ret =3D hid_parse(hdev);
> > +       if (ret) {
> > +               hid_err(hdev, "parse failed\n");
> > +               return ret;
> > +       }
> > +
> > +       ret =3D hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > +       if (ret) {
> > +               hid_err(hdev, "hw start failed\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void vrc2_remove(struct hid_device *hdev)
> > +{
> > +       hid_hw_stop(hdev);
> > +}
>=20
> I am pretty sure you can remove that vrc2_remove(), hid-core should be
> able to call hid_hw_stop() for you.
>=20

Hrm. Looking at this description:

/**
 * hid_hw_start - start underlying HW
 * @hdev: hid device
 * @connect_mask: which outputs to connect, see HID_CONNECT_*
 *
 * Call this in probe function *after* hid_parse. This will setup HW
 * buffers and start the device (if not defeirred to device open).
 * hid_hw_stop must be called if this was successful.
 */

 and


/**
 * hid_hw_stop - stop underlying HW
 * @hdev: hid device
 *
 * This is usually called from remove function or from probe when something
 * failed and hid_hw_start was called already.
 */

Most of the HID drivers actually do call hid_hw_stop() in the their .remove.

hid_device_remove(struct device *dev)
Seems to do this for you though.

Maybe we should clean it up to avoid more drivers to follow this
pattern?


> > +
> > +static const struct hid_device_id vrc2_devices[] =3D {
> > +       { HID_USB_DEVICE(USB_VENDOR_ID_VRC2, USB_DEVICE_ID_VRC2) },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(hid, vrc2_devices);
> > +
> > +static struct hid_driver vrc2_driver =3D {
> > +       .name =3D "vrc2",
> > +       .id_table =3D vrc2_devices,
> > +       .report_fixup =3D vrc2_report_fixup,
> > +       .probe =3D vrc2_probe,
> > +       .remove =3D vrc2_remove,
> > +};
> > +module_hid_driver(vrc2_driver);
> > +
> > +MODULE_AUTHOR("Marcus Folkesson <marcus.folkesson@gmail.com>");
> > +MODULE_DESCRIPTION("HID driver for VRC-2 2-axis Car controller");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.37.1
> >
>=20
> Cheers,
> Benjamin
>=20

Best regards,
Marcus Folkesson

--3X/soURl3JHM+qm5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEBVGi6LZstU1kwSxliIBOb1ldUjIFAmMPPmkACgkQiIBOb1ld
UjJQBg//fWa9zKyzQsNfquM5bXHlPaedUoW6xIuVslE8R2Q5EmYGLt1vzobW1gSf
cqYPIVFyyz3thMa9hCoEQi/+VjWCshsVSq7jnHANp0HMRUj04XklJiv1OzpfXGD+
IuCxeEoeswfviO1gM+qK2TXCArgAPjDv7YptC8vduFG1rviUrKtoNyxZdGcXqe1G
34uNJ3IWrB91vHKYcermn6CWyIOU8rZLA8ARC8Z0ysQnRHrg6sxyH9IHivn4kuqN
a5HsZCroBm5DajvnSw/XjwleswOG8jsCmIUnPAqQyWxnKpkvZ422M6Jy5Xl9jugY
3kF/EGL3hFMtdEWYSuydoAU49b999OJ3BtY1/L20RU0M+wykkrUw/CF/+vBehKBs
+roKB8AfC6PsL2mnJozmr7fJHlRW34bG3MHr3kygYnk6dNAP3kBjqFHmn6Xvk4EN
qC++BSrEljlSox6QFRH/RXfAOAGJcVPt6MEGwNIBWoexBF0dEhw26qe7vUi8VABm
KmetM/r4NVZOJWLMqK4afzDDLrEr3KaR4yYPlxC1VcgEi0OGZKyyfB3ZDxCpUTg6
qsZB+CvNFrV5NGGx2TeytS0ys3j2emgPi7c99nLFox6vIaQYjg3WuNKqxkGDYQg4
RuwRu7+kXCN9lWDK+03DaYruTdajPbirJcIAbP9tsrFIfuqUTFE=
=jkNZ
-----END PGP SIGNATURE-----

--3X/soURl3JHM+qm5--