Re: [RESEND PATCH v2] input: pxrc: new driver for PhoenixRC Flight Controller Adapter

Dmitry Torokhov
2018-01-12T08:05:42Z

On Thu, Jan 11, 2018 at 01:07:42PM +0100, Marcus Folkesson wrote:
> Hi Dmitry,
>
> Thank you for your review!
>
> On Wed, Jan 10, 2018 at 04:37:14PM -0800, Dmitry Torokhov wrote:
> > Hi Marcus,
> >
> > On Wed, Jan 10, 2018 at 02:11:39PM +0100, Marcus Folkesson wrote:
> > > This driver let you plug in your RC controller to the adapter and
> > > use it as input device in various RC simulators.
> > >
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > ---
> > > v2:
> > > - Change module license to GPLv2 to match SPDX tag
> > >
> > > Documentation/input/devices/pxrc.rst | 57 ++++++++
> > > drivers/input/joystick/Kconfig | 9 ++
> > > drivers/input/joystick/Makefile | 1 +
> > > drivers/input/joystick/pxrc.c | 254 +++++++++++++++++++++++++++++++++++
> > > 4 files changed, 321 insertions(+)
> > > create mode 100644 Documentation/input/devices/pxrc.rst
> > > create mode 100644 drivers/input/joystick/pxrc.c
> > >
> > > diff --git a/Documentation/input/devices/pxrc.rst b/Documentation/input/devices/pxrc.rst
> > > new file mode 100644
> > > index 000000000000..0ec466c58958
> > > --- /dev/null
> > > +++ b/Documentation/input/devices/pxrc.rst
> > > @@ -0,0 +1,57 @@
> > > +=======================================================
> > > +pxrc - PhoenixRC Flight Controller Adapter
> > > +=======================================================
> > > +
> > > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > +
> > > +This driver let you use your own RC controller plugged into the
> > > +adapter that comes with PhoenixRC [1]_ or other compatible adapters.
> > > +
> > > +The adapter supports 7 analog channels and 1 digital input switch.
> > > +
> > > +Notes
> > > +=====
> > > +
> > > +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.
> > > +
> > > +The driver is generating the following input event for analog channels:
> > > +
> > > ++---------+----------------+
> > > +| Channel | Event |
> > > ++=========+================+
> > > +| 1 | ABS_X |
> > > ++---------+----------------+
> > > +| 2 | ABS_Y |
> > > ++---------+----------------+
> > > +| 3 | ABS_RX |
> > > ++---------+----------------+
> > > +| 4 | ABS_RY |
> > > ++---------+----------------+
> > > +| 5 | ABS_TILT_X |
> > > ++---------+----------------+
> > > +| 6 | ABS_TILT_Y |
> > > ++---------+----------------+
> >
> > TILT are normally reserved for stylus/pen. Do we have better event codes
> > maybe? Is there a picture of the RC so I can make more sense of the
> > proposed event assignment?
> >
>
> Ok, maybe RUDDER and MISC?

OK.

> The driver is actually for an "adapter" [1] that you connect your RC to.
> The RC is then mapping its sticks/buttons to different channels. Unlike other
> joysticks, this mapping is not fixed but something you setup in your RC.
>
> For example, I'm using a Turnigy 9xr[2].
>
> The RC is typically used with a RC Flight Simulator software (I'm using
> Heli-X [3] when testing) which also map the channels to events (throttle,
> rudder and so on).
>
>
> > > +| 7 | ABS_THROTTLE |
> > > ++---------+----------------+
> > > +
> > > +The digital input switch is generated as an `BTN_A` event.
> > > +
> > > +Manual Testing
> > > +==============
> > > +
> > > +To test this driver's functionality you may use `input-event` which is part of
> > > +the `input layer utilities` suite [2]_.
> > > +
> > > +For example::
> > > +
> > > + > modprobe pxrc
> > > + > input-events <devnr>
> > > +
> > > +To print all input events from input `devnr`.
> > > +
> > > +References
> > > +==========
> > > +
> > > +.. [1] http://www.phoenix-sim.com/
> > > +.. [2] https://www.kraxel.org/cgit/input/
> > > diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> > > index f3c2f6ea8b44..18ab6dafff41 100644
> > > --- a/drivers/input/joystick/Kconfig
> > > +++ b/drivers/input/joystick/Kconfig
> > > @@ -351,4 +351,13 @@ config JOYSTICK_PSXPAD_SPI_FF
> > >
> > > To drive rumble motor a dedicated power supply is required.
> > >
> > > +config JOYSTICK_PXRC
> > > + tristate "PhoenixRC Flight Controller Adapter"
> > > + depends on USB_ARCH_HAS_HCD
> > > + select USB
> > > + help
> > > + Say Y here if you want to use the PhoenixRC Flight Controller Adapter.
> > > +
> > > + To compile this driver as a module, choose M here: the
> > > + module will be called pxrc.
> > > endif
> > > diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> > > index 67651efda2e1..dd0492ebbed7 100644
> > > --- a/drivers/input/joystick/Makefile
> > > +++ b/drivers/input/joystick/Makefile
> > > @@ -23,6 +23,7 @@ obj-$(CONFIG_JOYSTICK_JOYDUMP) += joydump.o
> > > obj-$(CONFIG_JOYSTICK_MAGELLAN) += magellan.o
> > > obj-$(CONFIG_JOYSTICK_MAPLE) += maplecontrol.o
> > > obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> > > +obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> > > obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> > > obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> > > obj-$(CONFIG_JOYSTICK_SPACEORB) += spaceorb.o
> > > diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> > > new file mode 100644
> > > index 000000000000..2bec99df97e2
> > > --- /dev/null
> > > +++ b/drivers/input/joystick/pxrc.c
> > > @@ -0,0 +1,254 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for Phoenix RC Flight Controller Adapter
> > > + *
> > > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > + *
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/module.h>
> > > +#include <linux/kref.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/usb.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/input.h>
> > > +
> > > +#define PXRC_VENDOR_ID (0x1781)
> > > +#define PXRC_PRODUCT_ID (0x0898)
> > > +
> > > +static const struct usb_device_id pxrc_table[] = {
> > > + { USB_DEVICE(PXRC_VENDOR_ID, PXRC_PRODUCT_ID) },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(usb, pxrc_table);
> > > +
> > > +struct usb_pxrc {
> > > + struct input_dev *input_dev;
> > > + struct usb_device *udev;
> > > + struct usb_interface *interface;
> > > + struct usb_anchor anchor;
> > > + __u8 epaddr;
> > > + char phys[64];
> > > + unsigned char *data;
> > > + size_t bsize;
> > > + struct kref kref;
> > > +};
> > > +
> > > +#define to_pxrc_dev(d) container_of(d, struct usb_pxrc, kref)
> > > +static void pxrc_delete(struct kref *kref)
> > > +{
> > > + struct usb_pxrc *pxrc = to_pxrc_dev(kref);
> > > +
> > > + usb_put_dev(pxrc->udev);
> > > +}
> > > +
> > > +static void pxrc_usb_irq(struct urb *urb)
> > > +{
> > > + struct usb_pxrc *pxrc = urb->context;
> > > +
> > > + switch (urb->status) {
> > > + case 0:
> > > + /* success */
> > > + break;
> > > + case -ETIME:
> > > + /* this urb is timing out */
> > > + dev_dbg(&pxrc->interface->dev,
> > > + "%s - urb timed out - was the device unplugged?\n",
> > > + __func__);
> > > + return;
> > > + case -ECONNRESET:
> > > + case -ENOENT:
> > > + case -ESHUTDOWN:
> > > + case -EPIPE:
> > > + /* this urb is terminated, clean up */
> > > + dev_dbg(&pxrc->interface->dev, "%s - urb shutting down with status: %d\n",
> > > + __func__, urb->status);
> > > + return;
> > > + default:
> > > + dev_dbg(&pxrc->interface->dev, "%s - nonzero urb status received: %d\n",
> > > + __func__, urb->status);
> > > + goto exit;
> > > + }
> > > +
> > > + if (urb->actual_length == 8) {
> > > + input_report_abs(pxrc->input_dev, ABS_X, pxrc->data[0]);
> > > + input_report_abs(pxrc->input_dev, ABS_Y, pxrc->data[2]);
> > > + input_report_abs(pxrc->input_dev, ABS_RX, pxrc->data[3]);
> > > + input_report_abs(pxrc->input_dev, ABS_RY, pxrc->data[4]);
> > > + input_report_abs(pxrc->input_dev, ABS_TILT_X, pxrc->data[5]);
> > > + input_report_abs(pxrc->input_dev, ABS_TILT_Y, pxrc->data[6]);
> > > + input_report_abs(pxrc->input_dev, ABS_THROTTLE, pxrc->data[7]);
> > > +
> > > + input_report_key(pxrc->input_dev, BTN_A, pxrc->data[1]);
> > > + }
> > > +
> > > +exit:
> > > + /* Resubmit to fetch new fresh URBs */
> > > + usb_anchor_urb(urb, &pxrc->anchor);
> > > + if (usb_submit_urb(urb, GFP_ATOMIC) < 0)
> > > + usb_unanchor_urb(urb);
> > > +}
> > > +
> > > +static int pxrc_submit_intr_urb(struct usb_pxrc *pxrc)
> > > +{
> > > + struct urb *urb;
> > > + unsigned int pipe;
> > > + int err;
> > > +
> > > + urb = usb_alloc_urb(0, GFP_KERNEL);
> > > + if (!urb)
> > > + return -ENOMEM;
> > > +
> > > + pipe = usb_rcvintpipe(pxrc->udev, pxrc->epaddr),
> > > + usb_fill_int_urb(urb, pxrc->udev, pipe, pxrc->data, pxrc->bsize,
> > > + pxrc_usb_irq, pxrc, 1);
> > > + usb_anchor_urb(urb, &pxrc->anchor);
> > > + err = usb_submit_urb(urb, GFP_KERNEL);
> > > + if (err < 0)
> > > + usb_unanchor_urb(urb);
> > > +
> > > + usb_free_urb(urb);
> > > + return err;
> > > +}
> > > +
> > > +static int pxrc_open(struct input_dev *input)
> > > +{
> > > + struct usb_pxrc *pxrc = input_get_drvdata(input);
> > > + int err;
> > > +
> > > + err = pxrc_submit_intr_urb(pxrc);
> > > + if (err < 0)
> > > + goto error;
> > > +
> > > + kref_get(&pxrc->kref);
> > > + return 0;
> > > +
> > > +error:
> > > + usb_kill_anchored_urbs(&pxrc->anchor);
> > > + return err;
> > > +}
> > > +
> > > +static void pxrc_close(struct input_dev *input)
> > > +{
> > > + struct usb_pxrc *pxrc = input_get_drvdata(input);
> > > +
> > > + usb_kill_anchored_urbs(&pxrc->anchor);
> > > + kref_put(&pxrc->kref, pxrc_delete);
> >
> > This is way to complex and I am not sure why you need to anchor URBs and
> > have a separate refcounting, etc. I do not think it is even safe with
> > module unloading as kref might be in flight as you run through module
> > unload and devm will destroy most of the objects.
> >
> > Why don't you simply use usb_kill_urb() here and be done.
> > input_unregister_device() that is called as part of devm unwind on
> > device unbind will make sure that pxrc_close() is called.
> >
> > Thanks.
> >
>
> Ok, I drop the refcounting. pxrc_delete() did more before I changed most
> allocations to devm_*.
> I tried to get the refcounting similiar to /drivers/usb/usb-skeleton.c.
>
> I have tried different approaches with the create/delete URBs.
> At first I had the URB in the usb_pxrc struct that I allocated in the
> probe function where I also submitted the URB.
>
> This worked but I found it waste to submit URBs when noone has opened the
> device, so I moved the submitting to pxrc_open().
> The problem was that the same URB was submitted multiple times (gives a
> warning) upon multiple calls to open(2).

open() should only be called once, unless close() was called. So as long
as you kill the URB in close(), you should not see warnings.

>
> So I removed the URB from usb_pxrc and used an anchor to still have a
> reference to it for deletion. This part is heavily inspired by
> drivers/bluetooth/bpa10x.c.
> Maybe I should do it differently?
>
> I must admit that I'm not very familiar with USB drivers.

I'd probably look at drivers/input/mouse/synaptics_usb.c and structured
your driver similarly.

Thanks.

--
Dmitry