Re: [PATCH] HID: switchcon: add nintendo switch controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Benjamin,

Thank you for the quick response/feedback. I have been making several
of the changes you've suggested, but I have some questions before
tackling a couple of the major ones. This is the first time I've
attempted submitting a kernel patch, so I'm not very familiar with the
HID subsystem.

Am I correct in assuming that spinlocks are permitted in raw_events,
since they don't sleep? You mention that sleeping is prohibited in the
raw_events and that the mutex locks should be causing problems. I am
puzzled now, since I did run into issues trying to output HID requests
in the raw_events (due to it trying to sleep), but for whatever reason
the mutexes have never caused an issue. Perhaps I've just been getting
incredibly lucky with timings.

I have been putting a lot of thought into your suggestion of using
synchronous transfers/receives. I really like this idea, since it
simplifies the initialization logic. The problem I run into is that
the controllers are constantly sending updates of their full state at
either 60Hz or 120Hz (joycon vs pro controller). I suppose this means
I'd need to filter the responses to the synchronous requests to make
sure I'm getting back the actually response/ACK I'm looking for. I
just wanted to make sure you still think that's a good idea before I
go down that route. At quick glance it looked to me like the hidpp
driver isn't receiving constant, unsolicited data, but I may be
mistaken.

I have a couple more questions/comments inline within the reply:

On Mon, Jan 14, 2019 at 7:55 AM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> () Hi Daniel,
>
> On Mon, Jan 14, 2019 at 2:04 AM Daniel J. Ogorchock
> <djogorchock@xxxxxxxxx> wrote:
> >
> > The switchcon driver supports the Nintendo Switch Pro Controllers and
> > the Joy-Cons. The Pro Controllers can be used over USB or Bluetooth. The
> > Joy-Cons can be used as a pair or individually in their horizontal
> > mode.
>
> Billy (Cc-ed) raised the question about the Switchcon a few weeks ago
> on the mailing list. Adding him to the thread.
> His questions were whether we should have 2 separate devices or merge
> the 2 as one. Adding a few other people from the other thread.
>
> The decision here is to have only one input node for both joycons.
>
> Foreword:
> This is a big driver. I don't know if this is your first contribution
> to the kernel or not, but I have a lot of things to say, and will
> probably have as the code is getting re-spun. So in other words: don't
> panic, and keep pushing even if we get to a high number of revisions
> (others might chime in at v8 and request new changes).
>
> Also, to understand the driver, I couldn't do a linear review. So
> comments are a little bit all over the place, and might not be needed
> once v2 is out.
>
> last, my gmail client made a weird conversion of the symbols at the
> middle (don't know why it used html for part of it). I tried to
> replace the various '&gt;' by '>' that were converted. I hope there is
> nothing wrong in the rest of the review.
>
> Now, getting to the review:
> OK, I think I have a grasp at the driver now.
>
> I put a bunch of comments in the code, but there are a few general I'd
> like you to take into consideration:
> - the driver has a lock of locks and workqueues. This makes it really
> hard to follow the path of the driver.
> - as mentioned below, I am not a big fan of struct switchcon_impl. You
> are using this avoid having 3 switches, but given that you already
> have helpers to call them, I don't think this adds any benefits
> - mutexes are all over the place (well, this is a consequence of using
> multiple workqueues), and you are taking hard locks during
> raw_event(), which can't sleep. This is wrong and I am surprised you
> have not your dmesg full of complains.
>
> I think you should be able to greatly simplify the whole driver by
> taking example on hid-logitech-hidpp.c:
> - write a send_message_sync function (like hidpp_send_message_sync() )
> - then, when you need this in probe, call hid_device_io_start(hdev);
> (after hid_hw_start()), this should allow the events to be processed
> by the kernel while you are still in probe.
> - finally, replace all of the various states/workqueues by synchronous calls.
>
> From a quick look of it, you should not have any need for a workqueue,
> and the only mutex that will stay around is the one for creating the
> input nodes (to be sure you don't create 2 input nodes for each pair).
I think I'll still need some form of lock per switchcon_ctlr and
switchcon_input.
The combining of two joycons requires the input handling to combine the
controller states of two controller structs. Also, either joycon can receive new
input data and trigger a switchcon_input update.
>
> >
> > Signed-off-by: Daniel J. Ogorchock <djogorchock@xxxxxxxxx>
> > ---
> >  MAINTAINERS                 |    6 +
> >  drivers/hid/Kconfig         |   13 +
> >  drivers/hid/Makefile        |    1 +
> >  drivers/hid/hid-ids.h       |    3 +
> >  drivers/hid/hid-quirks.c    |    6 +
> >  drivers/hid/hid-switchcon.c | 1156 +++++++++++++++++++++++++++++++++++
> >  6 files changed, 1185 insertions(+)
> >  create mode 100644 drivers/hid/hid-switchcon.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 39e75bbefc3d..9fc55efd63bc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14572,6 +14572,12 @@ F:     kernel/dma/swiotlb.c
> >  F:     arch/*/kernel/pci-swiotlb.c
> >  F:     include/linux/swiotlb.h
> >
> > +SWITCHCON HID DRIVER
> > +M:     Daniel J. Ogorchock <djogorchock@xxxxxxxxx>
> > +L:     linux-input@xxxxxxxxxxxxxxx
> > +S:     Maintained
> > +F:     drivers/hid/hid-switchcon*
> > +
> >  SWITCHDEV
> >  M:     Jiri Pirko <jiri@xxxxxxxxxxx>
> >  M:     Ivan Vecera <ivecera@xxxxxxxxxx>
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 41e9935fc584..3360267eb5b0 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -943,6 +943,19 @@ config SMARTJOYPLUS_FF
> >         Say Y here if you have a SmartJoy PLUS PS2/USB adapter and want to
> >         enable force feedback support for it.
> >
> > +config HID_SWITCHCON
> > +       tristate "Nintendo Joy-Con and Pro Controller support"
> > +       depends on HID
> > +       depends on USB_HID
> > +       depends on BT_HIDP
>
> Please drop these 2 lines and make sure the driver is transport
> agnostic (no deps on BT or USB low level calls).
> Scrolling down a bit, there is no include related to usb/usbhid nor
> BT, so just drop the 2 references to BT and USBHID, and only keep HID.
>
> > +       help
> > +       Adds support for the Nintendo Switch Joy-Cons and Pro Controller.
> > +       All controllers support bluetooth, and the Pro Controller also supports
> > +       its USB mode.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called hid-switchcon.
> > +
> >  config HID_TIVO
> >         tristate "TiVo Slide Bluetooth remote control support"
> >         depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 896a51ce7ce0..e708e682c4b8 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -103,6 +103,7 @@ obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
> >  obj-$(CONFIG_HID_STEAM)                += hid-steam.o
> >  obj-$(CONFIG_HID_STEELSERIES)  += hid-steelseries.o
> >  obj-$(CONFIG_HID_SUNPLUS)      += hid-sunplus.o
> > +obj-$(CONFIG_HID_SWITCHCON)    += hid-switchcon.o
> >  obj-$(CONFIG_HID_GREENASIA)    += hid-gaff.o
> >  obj-$(CONFIG_HID_THRUSTMASTER) += hid-tmff.o
> >  obj-$(CONFIG_HID_TIVO)         += hid-tivo.o
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index 518fa76414f5..cd41e971801e 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -848,6 +848,9 @@
> >  #define USB_VENDOR_ID_NINTENDO         0x057e
> >  #define USB_DEVICE_ID_NINTENDO_WIIMOTE 0x0306
> >  #define USB_DEVICE_ID_NINTENDO_WIIMOTE2        0x0330
> > +#define USB_DEVICE_ID_NINTENDO_JOYCONL 0x2006
> > +#define USB_DEVICE_ID_NINTENDO_JOYCONR  0x2007
> > +#define USB_DEVICE_ID_NINTENDO_PROCON   0x2009
> >
> >  #define USB_VENDOR_ID_NOVATEK          0x0603
> >  #define USB_DEVICE_ID_NOVATEK_PCT      0x0600
> > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> > index 94088c0ed68a..c22adffa1233 100644
> > --- a/drivers/hid/hid-quirks.c
> > +++ b/drivers/hid/hid-quirks.c
> > @@ -512,6 +512,12 @@ static const struct hid_device_id hid_have_special_driver[] = {
> >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIMOTE) },
> >         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIMOTE2) },
> >  #endif
> > +#if IS_ENABLED(CONFIG_HID_SWITCHCON)
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_JOYCONL) },
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_JOYCONR) },
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_PROCON) },
> > +       { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_PROCON) },
> > +#endif
>
> Please drop this hunk. Since v4.16 or v4.17, this should not be required at all.
>
> >  #if IS_ENABLED(CONFIG_HID_NTI)
> >         { HID_USB_DEVICE(USB_VENDOR_ID_NTI, USB_DEVICE_ID_USB_SUN) },
> >  #endif
> > diff --git a/drivers/hid/hid-switchcon.c b/drivers/hid/hid-switchcon.c
> > new file mode 100644
> > index 000000000000..126f3b39cbfe
> > --- /dev/null
> > +++ b/drivers/hid/hid-switchcon.c
> > @@ -0,0 +1,1156 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * HID driver for Nintendo Switch Joy-Cons and Pro Controllers
> > + *
> > + * Copyright (c) 2019 Daniel J. Ogorchock <djogorchock@xxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the Free
> > + * Software Foundation; either version 2 of the License, or (at your option)
> > + * any later version.
>
> Given that you put the SPDX tag above, there is no point in adding the
> full text of the license.
>
> > + *
> > + * The following resources/projects were referenced for this driver:
> > + *   https://github.com/dekuNukem/Nintendo_Switch_Reverse_Engineering
> > + *   https://gitlab.com/pjranki/joycon-linux-kernel (Peter Rankin)
> > + *   https://github.com/FrotBot/SwitchProConLinuxUSB
> > + *   https://github.com/MTCKC/ProconXInput
> > + *   hid-wiimote kernel hid driver
>
> How different is this one from https://gitlab.com/pjranki/joycon-linux-kernel?
> If you based your work on it, you might want to keep the original copyright.
I actually ended up using a different technique than this driver. It uses a
completely different reporting mode from the controllers. It did help me realize
write the driver in the hid subsystem rather than what I was initially trying in
drivers/input/joystick.
>
> > + *
> > + * This driver supports the Nintendo Switch Joy-Cons and Pro Controllers. The
> > + * Pro Controllers can either be used over USB or Bluetooth. The Joy-Cons can
> > + * be used either as a pair or individually in their horizontal mode.
> > + *
> > + * When a Joy-Con is paired, its LEDs will begin flashing to signal that it is
> > + * in its "searching" mode. By holding the triggers of two Joy-Cons at the same
> > + * time, they will be combined into one input. To use a Joy-Con alone in its
> > + * horizontal mode, hold SL and SR at the same time while it is in the
> > + * "searching" mode.
> > + *
> > + * The driver will retrieve the factory calibration info from the controllers,
> > + * so little to no user calibration should be required.
> > + *
> > + */
> > +
> > +#include "hid-ids.h"
> > +#include <linux delay.h>
> > +#include <linux device.h>
> > +#include <linux hid.h>
> > +#include <linux input.h>
> > +#include <linux module.h>
> > +#include <linux spinlock.h>
> > +
> > +/* Reference the url below for the following HID report defines:
>
> nitpick: the first line of a multi-comment should only have '/*' and
> the last line only '*/' for symmetry.
>
> > + * https://github.com/dekuNukem/Nintendo_Switch_Reverse_Engineering
> > + */
> > +
> > +/* Output Reports */
> > +#define SC_OUTPUT_RUMBLE_AND_SUBCMD    0x01
> > +#define SC_OUTPUT_FW_UPDATE_PKT                0x03
> > +#define        SC_OUTPUT_RUMBLE_ONLY           0x10
>
> a tab hid itself in your code ;)
>
> > +#define SC_OUTPUT_MCU_DATA             0x11
> > +#define SC_OUTPUT_USB_CMD              0x80
> > +
> > +/* Subcommand IDs */
> > +#define SC_SUBCMD_STATE                        0x00
> > +#define SC_SUBCMD_MANUAL_BT_PAIRING    0x01
> > +#define SC_SUBCMD_REQ_DEV_INFO         0x02
> > +#define SC_SUBCMD_SET_REPORT_MODE      0x03
> > +#define SC_SUBCMD_TRIGGERS_ELAPSED     0x04
> > +#define SC_SUBCMD_GET_PAGE_LIST_STATE  0x05
> > +#define SC_SUBCMD_SET_HCI_STATE                0x06
> > +#define SC_SUBCMD_RESET_PAIRING_INFO   0x07
> > +#define SC_SUBCMD_LOW_POWER_MODE       0x08
> > +#define SC_SUBCMD_SPI_FLASH_READ       0x10
> > +#define SC_SUBCMD_SPI_FLASH_WRITE      0x11
> > +#define SC_SUBCMD_RESET_MCU            0x20
> > +#define SC_SUBCMD_SET_MCU_CONFIG       0x21
> > +#define SC_SUBCMD_SET_MCU_STATE                0x22
> > +#define SC_SUBCMD_SET_PLAYER_LIGHTS    0x30
> > +#define SC_SUBCMD_GET_PLAYER_LIGHTS    0x31
> > +#define SC_SUBCMD_SET_HOME_LIGHT       0x38
> > +#define SC_SUBCMD_ENABLE_IMU           0x40
> > +#define SC_SUBCMD_SET_IMU_SENSITIVITY  0x41
> > +#define SC_SUBCMD_WRITE_IMU_REG                0x42
> > +#define SC_SUBCMD_READ_IMU_REG         0x43
> > +#define SC_SUBCMD_ENABLE_VIBRATION     0x48
> > +#define SC_SUBCMD_GET_REGULATED_VOLTAGE        0x50
> > +
> > +/* Input Reports */
> > +#define SC_INPUT_BUTTON_EVENT          0x3F
> > +#define SC_INPUT_SUBCMD_REPLY          0x21
> > +#define SC_INPUT_IMU_DATA              0x30
> > +#define SC_INPUT_MCU_DATA              0x31
> > +#define SC_INPUT_USB_RESPONSE          0x81
> > +
> > +/* Feature Reports */
> > +#define SC_FEATURE_LAST_SUBCMD         0x02
> > +#define SC_FEATURE_OTA_FW_UPGRADE      0x70
> > +#define SC_FEATURE_SETUP_MEM_READ      0x71
> > +#define SC_FEATURE_MEM_READ            0x72
> > +#define SC_FEATURE_ERASE_MEM_SECTOR    0x73
> > +#define SC_FEATURE_MEM_WRITE           0x74
> > +#define SC_FEATURE_LAUNCH              0x75
> > +
> > +/* USB Commands */
> > +#define SC_USB_CMD_CONN_STATUS         0x01
> > +#define SC_USB_CMD_HANDSHAKE           0x02
> > +#define SC_USB_CMD_BAUDRATE_3M         0x03
> > +#define SC_USB_CMD_NO_TIMEOUT          0x04
> > +#define SC_USB_CMD_EN_TIMEOUT          0x05
> > +#define SC_USB_RESET                   0x06
> > +#define SC_USB_PRE_HANDSHAKE           0x91
> > +#define SC_USB_SEND_UART               0x92
> > +
> > +/* SPI storage addresses of factory calibration data */
> > +#define        SC_CAL_DATA_START               0x603d
> > +#define        SC_CAL_DATA_END                 0x604e
>
> Please replace the 2 tabs above by a single space
>
> > +#define SC_CAL_DATA_SIZE       (SC_CAL_DATA_END - SC_CAL_DATA_START + 1)
> > +
> > +
> > +/* The raw analog joystick values will be mapped in terms of this magnitude */
> > +#define SC_MAX_STICK_MAG       32767
> > +#define SC_STICK_FUZZ          250
> > +#define SC_STICK_FLAT          500
> > +
> > +struct switchcon_ctlr;
> > +struct switchcon_input;
> > +
> > +/* States for controller state machine */
> > +enum switchcon_ctlr_state {
> > +       SWITCHCON_CTLR_STATE_INIT,
> > +       SWITCHCON_CTLR_STATE_USB_SET_BAUD,
> > +       SWITCHCON_CTLR_STATE_USB_HANDSHAKE,
> > +       SWITCHCON_CTLR_STATE_CALIBRATION,
> > +       SWITCHCON_CTLR_STATE_POST_CALIBRATION,
> > +       SWITCHCON_CTLR_STATE_SEARCH,
> > +       SWITCHCON_CTLR_STATE_READ,
> > +};
> > +
> > +/* Function pointers will differ based on controller type */
> > +struct switchcon_impl {
> > +       int (*init)(struct switchcon_ctlr *ctlr);
> > +       void (*deinit)(struct switchcon_ctlr *ctlr);
> > +       int (*handle_event)(struct switchcon_ctlr *ctlr, u8 *data, int size);
> > +};
> > +
> > +struct switchcon_output {
> > +       u8      *data;
> > +       u8      size;
> > +};
> > +
> > +struct switchcon_stick_cal {
> > +       s32     x_max;
> > +       s32     x_min;
> > +       s32     x_center;
> > +       s32     y_max;
> > +       s32     y_min;
> > +       s32     y_center;
> > +};
> > +
> > +#define SC_OUTPUT_BUF_SIZE     50
> > +/* Each physical controller is associated with a switchcon_ctlr struct */
> > +struct switchcon_ctlr {
> > +       struct hid_device               *hdev;
> > +       struct switchcon_input          *switchcon_in;
> > +       const struct switchcon_impl     *impl;
> > +       bool                            is_right_joycon;
> > +       bool                            searching;
> > +       enum switchcon_ctlr_state       ctlr_state;
> > +       u8                              subcmd_num;
> > +       struct switchcon_output         output_buf[SC_OUTPUT_BUF_SIZE];
> > +       u8                              output_head;
> > +       u8                              output_tail;
>
> You better use a kfifo directly instead of re-writing yours.
If I end up implementing synchronous calls, I can ditch the fifo right?
>
> > +       spinlock_t                      output_lock;
> > +       struct work_struct              output_worker;
> > +       struct work_struct              create_input_worker;
> > +       struct work_struct              detect_pair_worker;
> > +       struct work_struct              create_horiz_worker;
> > +       struct mutex                    mutex;
> > +
> > +       /* factory calibration data */
> > +       struct switchcon_stick_cal      left_stick_cal;
> > +       struct switchcon_stick_cal      right_stick_cal;
> > +
> > +       /* buttons/sticks */
> > +       u16     left_stick_x;
> > +       u16     left_stick_y;
> > +       u16     right_stick_x;
> > +       u16     right_stick_y;
> > +       bool    but_y;
> > +       bool    but_x;
> > +       bool    but_b;
> > +       bool    but_a;
> > +       bool    but_sr_right_jc;
> > +       bool    but_sl_right_jc;
> > +       bool    but_r;
> > +       bool    but_zr;
> > +       bool    but_l;
> > +       bool    but_zl;
> > +       bool    but_minus;
> > +       bool    but_plus;
> > +       bool    but_rstick;
> > +       bool    but_lstick;
> > +       bool    but_home;
> > +       bool    but_capture;
> > +       bool    but_down;
> > +       bool    but_up;
> > +       bool    but_right;
> > +       bool    but_left;
> > +       bool    but_sr_left_jc;
> > +       bool    but_sl_left_jc;
>
> There is a comment down below, but I think you better keep just a
> bitfields for all of these buttons, if you really need to keep their
> state.
This has simplified things considerably, especially since now combining
two controllers' button states is just a bitwise OR.
>
> > +
> > +};
> > +
> > +enum switchcon_input_type {
> > +       SWITCHCON_INPUT_TYPE_PROCON,
> > +       SWITCHCON_INPUT_TYPE_JOYCONS,
> > +       SWITCHCON_INPUT_TYPE_JOYCON_H,
> > +};
> > +
> > +static const char * const switchcon_input_names[] = {
> > +       "Nintendo Switch Pro Controller",
> > +       "Nintendo Switch Joy-Cons",
> > +       "Nintendo Switch Joy-Con Horizontal",
> > +};
> > +
> > +/* Each linux input device is associated with a switchcon_input */
> > +struct switchcon_input {
> > +       enum switchcon_input_type       type;
> > +       struct input_dev        *input;
> > +       struct switchcon_ctlr   *ctlr_left; /* left joycon or pro controller */
> > +       struct switchcon_ctlr   *ctlr_right; /* only used for joycons */
> > +       struct mutex            mutex;
> > +       struct work_struct      input_worker;
> > +};
> > +
> > +static int switchcon_ctlr_init(struct switchcon_ctlr *ctlr)
> > +{
> > +       if (ctlr->impl &amp;&amp; ctlr->impl->init)
>
> Can this test ever fail?
>
> > +               return ctlr->impl->init(ctlr);
> > +       else
> > +               return -EINVAL;
> > +}
> > +
> > +static void switchcon_ctlr_deinit(struct switchcon_ctlr *ctlr)
> > +{
> > +       if (ctlr->impl &amp;&amp; ctlr->impl->deinit)
>
> likewise, is there a case where this test would fail?
>
> > +               ctlr->impl->deinit(ctlr);
> > +}
> > +
> > +static int switchcon_ctlr_handle_event(struct switchcon_ctlr *ctlr, u8 *data,
> > +                                                                   int size)
> > +{
> > +       if (ctlr->impl &amp;&amp; ctlr->impl->handle_event)
>
> And same here :)
>
> > +               return ctlr->impl->handle_event(ctlr, data, size);
> > +       else
> > +               return -EINVAL;
> > +}
> > +
> > +static int switchcon_hid_queue_send(struct switchcon_ctlr *ctlr,
> > +                                   const u8 *buffer, size_t count)
> > +{
> > +       struct switchcon_output output;
> > +
> > +       output.size = count;
> > +       output.data = kmemdup(buffer, count, GFP_ATOMIC);
> > +       if (!output.data)
> > +               return -ENOMEM;
> > +
> > +       spin_lock(&amp;ctlr->output_lock);
> > +
> > +       ctlr->output_buf[ctlr->output_head++] = output;
> > +       if (ctlr->output_head == SC_OUTPUT_BUF_SIZE)
> > +               ctlr->output_head = 0;
> > +       schedule_work(&amp;ctlr->output_worker);
> > +
> > +       spin_unlock(&amp;ctlr->output_lock);
> > +       return 0;
> > +}
> > +
> > +static int switchcon_queue_subcommand(struct switchcon_ctlr *ctlr,
> > +                                     const u8 *buffer, size_t count)
> > +{
> > +       /* header including packet number and empty rumble data */
> > +       u8 header[10] = {SC_OUTPUT_RUMBLE_AND_SUBCMD, ctlr->subcmd_num};
> > +       struct switchcon_output output;
> > +       u8 *buf;
> > +
> > +       /* the packet number loops after 0xF */
> > +       ctlr->subcmd_num++;
> > +       if (ctlr->subcmd_num > 0xF)
> > +               ctlr->subcmd_num = 0;
> > +
> > +       buf = kzalloc(sizeof(header) + count, GFP_KERNEL);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       memcpy(buf, header, sizeof(header));
> > +       memcpy(buf + sizeof(header), buffer, count);
> > +       output.data = buf;
> > +       output.size = count + sizeof(header);
> > +
> > +       spin_lock(&amp;ctlr->output_lock);
> > +
> > +       ctlr->output_buf[ctlr->output_head++] = output;
> > +       if (ctlr->output_head == SC_OUTPUT_BUF_SIZE)
> > +               ctlr->output_head = 0;
> > +       schedule_work(&amp;ctlr->output_worker);
> > +
> > +       spin_unlock(&amp;ctlr->output_lock);
> > +       return 0;
> > +}
> > +
> > +static void switchcon_set_player_leds(struct switchcon_ctlr *ctlr,
> > +                                     u8 flash, u8 on)
> > +{
> > +       u8 buffer[] = {SC_SUBCMD_SET_PLAYER_LIGHTS, (flash &lt;&lt; 4) | on};
> > +
> > +       switchcon_queue_subcommand(ctlr, buffer, sizeof(buffer));
> > +}
> > +
> > +static void switchcon_request_calibration(struct switchcon_ctlr *ctlr)
> > +{
> > +       u8 cal_subcmd[]         = {SC_SUBCMD_SPI_FLASH_READ,
> > +                                  0xFF &amp; SC_CAL_DATA_START,
> > +                                  0xFF &amp; (SC_CAL_DATA_START >> 8),
> > +                                  0, 0, SC_CAL_DATA_SIZE};
> > +
> > +       switchcon_queue_subcommand(ctlr, cal_subcmd, sizeof(cal_subcmd));
> > +       hid_dbg(ctlr->hdev, "requested cal data\n");
> > +}
> > +
> > +static void switchcon_request_report_mode(struct switchcon_ctlr *ctlr)
> > +{
> > +       u8 inputmode_subcmd[]   = {SC_SUBCMD_SET_REPORT_MODE, 0x30};
> > +
> > +       switchcon_queue_subcommand(ctlr, inputmode_subcmd,
> > +                                  sizeof(inputmode_subcmd));
> > +       hid_dbg(ctlr->hdev, "requested 0x30 report mode\n");
> > +}
> > +
> > +static int switchcon_map_x_stick_val(struct switchcon_stick_cal *cal, s32 val)
> > +{
> > +       s32 center      = cal->x_center;
> > +       s32 min         = cal->x_min;
> > +       s32 max         = cal->x_max;
> > +       s64 newVal;
> > +
> > +       if (val > center) {
> > +               newVal = (val - center) * SC_MAX_STICK_MAG;
> > +               do_div(newVal, max - center);
> > +       } else {
> > +               newVal = (center - val) * SC_MAX_STICK_MAG;
> > +               do_div(newVal, center - min);
> > +               newVal *= -1;
> > +       }
> > +       if (newVal > SC_MAX_STICK_MAG)
> > +               newVal = SC_MAX_STICK_MAG;
> > +       if (newVal &lt; -SC_MAX_STICK_MAG)
> > +               newVal = -SC_MAX_STICK_MAG;
> > +       return (int)newVal;
> > +}
> > +
> > +static int switchcon_map_y_stick_val(struct switchcon_stick_cal *cal, s32 val)
> > +{
> > +       s32 center      = cal->y_center;
> > +       s32 min         = cal->y_min;
> > +       s32 max         = cal->y_max;
> > +       s64 newVal;
> > +
> > +       if (val > center) {
> > +               newVal = (val - center) * SC_MAX_STICK_MAG;
> > +               do_div(newVal, max - center);
> > +       } else {
> > +               newVal = (center - val) * SC_MAX_STICK_MAG;
> > +               do_div(newVal, center - min);
> > +               newVal *= -1;
> > +       }
> > +       if (newVal > SC_MAX_STICK_MAG)
> > +               newVal = SC_MAX_STICK_MAG;
> > +       if (newVal &lt; -SC_MAX_STICK_MAG)
> > +               newVal = -SC_MAX_STICK_MAG;
> > +       return (int)newVal;
> > +}
> > +
> > +static void switchcon_update_input_handler(struct work_struct *ws)
> > +{
> > +       struct switchcon_input *sc_input = container_of(ws,
> > +                                       struct switchcon_input, input_worker);
> > +       struct input_dev *dev = sc_input->input;
> > +       struct switchcon_ctlr *ctlr_l = sc_input->ctlr_left;
> > +       struct switchcon_ctlr *ctlr_r = sc_input->ctlr_right;
> > +       int s_l_x;
> > +       int s_l_y;
> > +       int s_r_x;
> > +       int s_r_y;
> > +
> > +       if (!ctlr_r)
> > +               ctlr_r = ctlr_l;
> > +
> > +       mutex_lock(&amp;sc_input->mutex);
> > +       mutex_lock(&amp;ctlr_l->mutex);
> > +       if (sc_input->ctlr_right)
> > +               mutex_lock(&amp;sc_input->ctlr_right->mutex);
> > +
> > +       /* map the stick values */
> > +       s_l_x = switchcon_map_x_stick_val(&amp;ctlr_l->left_stick_cal,
> > +                                         ctlr_l->left_stick_x);
> > +       s_l_y = switchcon_map_y_stick_val(&amp;ctlr_l->left_stick_cal,
> > +                                         ctlr_l->left_stick_y);
> > +       s_r_x = switchcon_map_x_stick_val(&amp;ctlr_r->right_stick_cal,
> > +                                         ctlr_r->right_stick_x);
> > +       s_r_y = switchcon_map_y_stick_val(&amp;ctlr_r->right_stick_cal,
> > +                                         ctlr_r->right_stick_y);
> > +
> > +       if (sc_input->type == SWITCHCON_INPUT_TYPE_JOYCON_H) {
> > +               if (ctlr_l->is_right_joycon) {
> > +                       /* report stick */
> > +                       input_report_abs(dev, ABS_X, s_r_y);
> > +                       input_report_abs(dev, ABS_Y, -s_r_x);
> > +
> > +                       /* report buttons */
> > +                       input_report_key(dev, BTN_WEST,   ctlr_r->but_b);
> > +                       input_report_key(dev, BTN_NORTH,  ctlr_r->but_y);
> > +                       input_report_key(dev, BTN_EAST,   ctlr_r->but_x);
> > +                       input_report_key(dev, BTN_SOUTH,  ctlr_r->but_a);
> > +                       input_report_key(dev, BTN_THUMBL, ctlr_r->but_rstick);
> > +                       input_report_key(dev, BTN_START,  ctlr_r->but_plus);
> > +                       input_report_key(dev, BTN_MODE,   ctlr_r->but_home);
> > +                       input_report_key(dev, BTN_TR,
> > +                                        ctlr_r->but_sr_right_jc);
> > +                       input_report_key(dev, BTN_TL,
> > +                                        ctlr_r->but_sl_right_jc);
> > +               } else {
> > +                       /* report stick */
> > +                       input_report_abs(dev, ABS_X, -s_l_y);
> > +                       input_report_abs(dev, ABS_Y, s_l_x);
> > +
> > +                       /* report buttons */
> > +                       input_report_key(dev, BTN_WEST,   ctlr_l->but_up);
> > +                       input_report_key(dev, BTN_NORTH,  ctlr_l->but_right);
> > +                       input_report_key(dev, BTN_EAST,   ctlr_l->but_down);
> > +                       input_report_key(dev, BTN_SOUTH,  ctlr_l->but_left);
> > +                       input_report_key(dev, BTN_THUMBL, ctlr_l->but_lstick);
> > +                       input_report_key(dev, BTN_START,  ctlr_l->but_minus);
> > +                       input_report_key(dev, BTN_MODE,   ctlr_l->but_capture);
> > +                       input_report_key(dev, BTN_TR,
> > +                                        ctlr_l->but_sr_left_jc);
> > +                       input_report_key(dev, BTN_TL,
> > +                                        ctlr_l->but_sl_left_jc);
> > +               }
> > +       } else {
> > +               /* report sticks */
> > +               input_report_abs(dev, ABS_X, s_l_x);
> > +               input_report_abs(dev, ABS_Y, s_l_y);
> > +               input_report_abs(dev, ABS_RX, s_r_x);
> > +               input_report_abs(dev, ABS_RY, s_r_y);
> > +
> > +               /* report buttons */
> > +               input_report_key(dev, BTN_WEST,         ctlr_r->but_y);
> > +               input_report_key(dev, BTN_NORTH,        ctlr_r->but_x);
> > +               input_report_key(dev, BTN_EAST,         ctlr_r->but_a);
> > +               input_report_key(dev, BTN_SOUTH,        ctlr_r->but_b);
> > +               input_report_key(dev, BTN_TR,           ctlr_r->but_r);
> > +               input_report_key(dev, BTN_TR2,          ctlr_r->but_zr);
> > +               input_report_key(dev, BTN_TL,           ctlr_l->but_l);
> > +               input_report_key(dev, BTN_TL2,          ctlr_l->but_zl);
> > +               input_report_key(dev, BTN_SELECT,       ctlr_l->but_minus);
> > +               input_report_key(dev, BTN_START,        ctlr_r->but_plus);
> > +               input_report_key(dev, BTN_THUMBL,       ctlr_l->but_lstick);
> > +               input_report_key(dev, BTN_THUMBR,       ctlr_r->but_rstick);
> > +               input_report_key(dev, BTN_MODE,         ctlr_r->but_home);
> > +               input_report_key(dev, BTN_Z,            ctlr_l->but_capture);
> > +               input_report_key(dev, BTN_DPAD_DOWN,    ctlr_l->but_down);
> > +               input_report_key(dev, BTN_DPAD_UP,      ctlr_l->but_up);
> > +               input_report_key(dev, BTN_DPAD_RIGHT,   ctlr_l->but_right);
> > +               input_report_key(dev, BTN_DPAD_LEFT,    ctlr_l->but_left);
> > +       }
> > +
> > +       input_sync(dev);
> > +
> > +       mutex_unlock(&amp;ctlr_l->mutex);
> > +       if (sc_input->ctlr_right)
> > +               mutex_unlock(&amp;sc_input->ctlr_right->mutex);
> > +       mutex_unlock(&amp;sc_input->mutex);
> > +}
> > +
> > +static void switchcon_input_destroy(struct switchcon_input *sc_input)
> > +{
> > +       hid_dbg(sc_input->ctlr_left->hdev, "destroying input\n");
> > +       mutex_lock(&amp;sc_input->mutex);
> > +
> > +       sc_input->ctlr_left->switchcon_in = NULL;
> > +       mutex_lock(&amp;sc_input->ctlr_left->mutex);
> > +       if (sc_input->ctlr_right) {
> > +               mutex_lock(&amp;sc_input->ctlr_right->mutex);
> > +               sc_input->ctlr_right->switchcon_in = NULL;
> > +       }
> > +       /* set the joycon states to searching */
> > +       sc_input->ctlr_left->ctlr_state = SWITCHCON_CTLR_STATE_POST_CALIBRATION;
> > +       if (sc_input->ctlr_right)
> > +               sc_input->ctlr_right->ctlr_state =
> > +                                         SWITCHCON_CTLR_STATE_POST_CALIBRATION;
> > +
> > +       input_unregister_device(sc_input->input);
> > +
> > +       mutex_unlock(&amp;sc_input->ctlr_left->mutex);
> > +       if (sc_input->ctlr_right)
> > +               mutex_unlock(&amp;sc_input->ctlr_right->mutex);
> > +       mutex_unlock(&amp;sc_input->mutex);
> > +       mutex_destroy(&amp;sc_input->mutex);
> > +       kfree(sc_input);
> > +}
> > +
> > +static const unsigned int switchcon_button_inputs[] = {
> > +       BTN_SOUTH, BTN_EAST, BTN_NORTH, BTN_WEST,
> > +       BTN_TL, BTN_TR, BTN_START, BTN_MODE, BTN_THUMBL,
> > +       /* the following buttons do not apply to a single, horizontal joycon */
> > +       BTN_TL2, BTN_TR2, BTN_SELECT, BTN_Z, BTN_THUMBR,
> > +       BTN_DPAD_UP, BTN_DPAD_DOWN, BTN_DPAD_LEFT, BTN_DPAD_RIGHT,
> > +       0 /* 0 signals end of array */
> > +};
> > +
> > +DEFINE_MUTEX(switchcon_input_num_mutex);
> > +static struct switchcon_input *switchcon_input_create(
> > +                                       enum switchcon_input_type type,
> > +                                       struct switchcon_ctlr *ctlr_l,
> > +                                       struct switchcon_ctlr *ctlr_r)
> > +{
> > +       struct switchcon_input *sc_input;
> > +       struct hid_device *hdev;
> > +       static int input_num = 1;
> > +       int i;
> > +       int ret;
> > +
> > +       if (!ctlr_l)
> > +               return ERR_PTR(-EINVAL);
> > +       hdev = ctlr_l->hdev;
> > +       sc_input = kzalloc(sizeof(*sc_input), GFP_KERNEL);
> > +       if (!sc_input)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       sc_input->type          = type;
> > +       sc_input->ctlr_left     = ctlr_l;
> > +       sc_input->ctlr_right    = ctlr_r;
> > +       if (!ctlr_r)
> > +               ctlr_r = ctlr_l;
> > +
> > +       sc_input->input = input_allocate_device();
> > +       if (!sc_input->input) {
> > +               ret = -ENOMEM;
> > +               goto err;
> > +       }
> > +       sc_input->input->dev.parent     = &amp;hdev->dev;
> > +       sc_input->input->id.bustype     = hdev->bus;
> > +       sc_input->input->id.vendor      = hdev->vendor;
> > +       sc_input->input->id.product     = hdev->product;
> > +       sc_input->input->id.version     = hdev->version;
> > +       sc_input->input->name = switchcon_input_names[sc_input->type];
> > +       input_set_drvdata(sc_input->input, sc_input);
> > +
> > +       /* set up button inputs */
> > +       for (i = 0; switchcon_button_inputs[i] > 0; i++) {
> > +               input_set_capability(sc_input->input, EV_KEY,
> > +                                    switchcon_button_inputs[i]);
> > +               if (switchcon_button_inputs[i] == BTN_THUMBL
> > +                &amp;&amp; sc_input->type == SWITCHCON_INPUT_TYPE_JOYCON_H)
> > +                       break;
> > +       }
> > +
> > +       /* set up sticks */
> > +       input_set_abs_params(sc_input->input, ABS_X,
> > +                            -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG,
> > +                            SC_STICK_FUZZ, SC_STICK_FLAT);
> > +       input_set_abs_params(sc_input->input, ABS_Y,
> > +                            -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG,
> > +                            SC_STICK_FUZZ, SC_STICK_FLAT);
> > +       /* set up right stick if necessary for controller type */
> > +       if (sc_input->type != SWITCHCON_INPUT_TYPE_JOYCON_H) {
> > +               input_set_abs_params(sc_input->input, ABS_RX,
> > +                                    -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG,
> > +                                    SC_STICK_FUZZ, SC_STICK_FLAT);
> > +               input_set_abs_params(sc_input->input, ABS_RY,
> > +                                    -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG,
> > +                                    SC_STICK_FUZZ, SC_STICK_FLAT);
> > +       }
> > +
> > +       ret = input_register_device(sc_input->input);
> > +       if (ret)
> > +               goto err_input;
> > +
> > +       mutex_init(&amp;sc_input->mutex);
> > +       INIT_WORK(&amp;sc_input->input_worker, switchcon_update_input_handler);
> > +
> > +       /* Set the controller player leds based on input number */
> > +       mutex_lock(&amp;switchcon_input_num_mutex);
> > +       /* Need to send multiple times to make sure it's not ignored */
> > +       for (i = 0; i &lt; 5; i++) {
> > +               switchcon_set_player_leds(ctlr_l, 0, 0xF >> (4 - input_num));
> > +               switchcon_set_player_leds(ctlr_r, 0, 0xF >> (4 - input_num));
> > +       }
> > +       if (++input_num > 4)
> > +               input_num = 1;
> > +       mutex_unlock(&amp;switchcon_input_num_mutex);
> > +
> > +       return sc_input;
> > +err_input:
> > +       input_free_device(sc_input->input);
> > +err:
> > +       kfree(sc_input);
> > +       return ERR_PTR(ret);
> > +}
> > +
> > +static void switchcon_create_procon_input_handler(struct work_struct *ws)
> > +{
> > +       struct switchcon_ctlr *ctlr = container_of(ws, struct switchcon_ctlr,
> > +                                                       create_input_worker);
> > +       hid_dbg(ctlr->hdev, "create_procon_input_handler\n");
> > +
> > +       mutex_lock(&amp;ctlr->mutex);
> > +
> > +       ctlr->switchcon_in = switchcon_input_create(SWITCHCON_INPUT_TYPE_PROCON,
> > +                                                                   ctlr, NULL);
> > +       if (IS_ERR(ctlr->switchcon_in)) {
> > +               hid_err(ctlr->hdev, "failed to create input\n");
> > +               ctlr->switchcon_in = NULL;
> > +       }
> > +
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +}
> > +
> > +DEFINE_MUTEX(switchcon_detect_pair_mutex);
> > +static void switchcon_detect_pair_handler(struct work_struct *ws)
> > +{
> > +       struct switchcon_ctlr *ctlr = container_of(ws, struct switchcon_ctlr,
> > +                                                       detect_pair_worker);
> > +       static struct switchcon_ctlr *ctlr_l;
> > +       static struct switchcon_ctlr *ctlr_r;
> > +
> > +       mutex_lock(&amp;switchcon_detect_pair_mutex);
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       /* Check if this is a controller no longer searching for partner */
> > +       if (!ctlr->searching) {
> > +               if (ctlr == ctlr_l)
> > +                       ctlr_l = NULL;
> > +               if (ctlr == ctlr_r)
> > +                       ctlr_r = NULL;
> > +       } else {
> > +               if (!ctlr->is_right_joycon &amp;&amp; ctlr_l == NULL)
> > +                       ctlr_l = ctlr;
> > +               if (ctlr->is_right_joycon &amp;&amp; ctlr_r == NULL)
> > +                       ctlr_r = ctlr;
> > +
> > +               /* see if there's a pair */
> > +               if (ctlr_l &amp;&amp; ctlr_r) {
> > +                       if (ctlr == ctlr_l)
> > +                               mutex_lock(&amp;ctlr_r->mutex);
> > +                       else
> > +                               mutex_lock(&amp;ctlr_l->mutex);
> > +                       hid_info(ctlr->hdev, "Joy-Con pair found\n");
> > +
> > +                       ctlr_l->switchcon_in = switchcon_input_create(
> > +                                                  SWITCHCON_INPUT_TYPE_JOYCONS,
> > +                                                  ctlr_l, ctlr_r);
> > +                       ctlr_r->switchcon_in = ctlr_l->switchcon_in;
> > +                       ctlr_l->ctlr_state = SWITCHCON_CTLR_STATE_READ;
> > +                       ctlr_r->ctlr_state = SWITCHCON_CTLR_STATE_READ;
> > +
> > +                       if (IS_ERR(ctlr_l->switchcon_in)) {
> > +                               hid_err(ctlr->hdev, "Failed creating input\n");
> > +                               ctlr_l->switchcon_in = NULL;
> > +                               ctlr_r->switchcon_in = NULL;
> > +                       }
> > +
> > +                       if (ctlr == ctlr_l)
> > +                               mutex_unlock(&amp;ctlr_r->mutex);
> > +                       else
> > +                               mutex_unlock(&amp;ctlr_l->mutex);
> > +                       ctlr_l = NULL;
> > +                       ctlr_r = NULL;
> > +               }
> > +       }
> > +
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       mutex_unlock(&amp;switchcon_detect_pair_mutex);
> > +}
> > +
> > +static void switchcon_create_horiz_handler(struct work_struct *ws)
> > +{
> > +       struct switchcon_ctlr *ctlr = container_of(ws, struct switchcon_ctlr,
> > +                                                       create_horiz_worker);
> > +
> > +       mutex_lock(&amp;ctlr->mutex);
> > +
> > +       ctlr->switchcon_in = switchcon_input_create(
> > +                               SWITCHCON_INPUT_TYPE_JOYCON_H, ctlr, NULL);
> > +       if (IS_ERR(ctlr->switchcon_in)) {
> > +               hid_err(ctlr->hdev, "failed to create input\n");
> > +               ctlr->switchcon_in = NULL;
> > +       }
> > +
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +}
> > +
> > +static void switchcon_send_work_handler(struct work_struct *ws)
> > +{
> > +       struct switchcon_ctlr *ctlr = container_of(ws, struct switchcon_ctlr,
> > +                                                               output_worker);
> > +       struct switchcon_output output;
> > +       struct hid_device *hdev = ctlr->hdev;
> > +       int ret;
> > +
> > +       spin_lock(&amp;ctlr->output_lock);
> > +       while (ctlr->output_tail != ctlr->output_head) {
> > +               output = ctlr->output_buf[ctlr->output_tail];
> > +               ctlr->output_buf[ctlr->output_tail].data = NULL;
> > +               ctlr->output_tail++;
> > +               if (ctlr->output_tail == SC_OUTPUT_BUF_SIZE)
> > +                       ctlr->output_tail = 0;
> > +               spin_unlock(&amp;ctlr->output_lock);
> > +               if (!output.data) {
> > +                       hid_warn(ctlr->hdev, "invalid output in buffer\n");
> > +               } else {
> > +                       ret = hid_hw_output_report(hdev, output.data,
> > +                                                        output.size);
> > +                       if (ret &lt; 0)
> > +                               hid_warn(hdev, "failed output report ret=%d",
> > +                                                                       ret);
> > +                       kfree(output.data);
> > +
> > +               }
> > +               spin_lock(&amp;ctlr->output_lock);
> > +       }
> > +       spin_unlock(&amp;ctlr->output_lock);
> > +}
> > +
> > +static int switchcon_hid_event(struct hid_device *hdev,
> > +                       struct hid_report *report, u8 *raw_data, int size)
> > +{
> > +       struct switchcon_ctlr *ctlr = hid_get_drvdata(hdev);
> > +
> > +       if (size &lt; 1)
> > +               return -EINVAL;
> > +
> > +       return switchcon_ctlr_handle_event(ctlr, raw_data, size);
> > +}
> > +
> > +/* data input must have at least 9 bytes */
> > +static void switchcon_parse_lstick_calibration(u8 *data,
> > +                                              struct switchcon_stick_cal *cal)
> > +{
> > +       s32 x_max_above;
> > +       s32 x_min_below;
> > +       s32 y_max_above;
> > +       s32 y_min_below;
> > +
> > +       x_max_above     = ((data[1] &lt;&lt; 8) &amp; 0xF00) | data[0];
> > +       y_max_above     = (data[2] &lt;&lt; 4) | (data[1] >> 4);
> > +       x_min_below     = ((data[7] &lt;&lt; 8) &amp; 0xF00) | data[6];
> > +       y_min_below     = (data[8] &lt;&lt; 4) | (data[7] >> 4);
> > +       cal->x_center   = ((data[4] &lt;&lt; 8) &amp; 0xF00) | data[3];
> > +       cal->y_center   = (data[5] &lt;&lt; 4) | (data[4] >> 4);
>
> This is all reading u12 in a u8[]. You better use hid_field_extract()
> with the proper parameters. This will make the code much clearer to
> read.
>
> Same goes for all of the u12 readings.
>
> > +       cal->x_max      = cal->x_center + x_max_above;
> > +       cal->x_min      = cal->x_center - x_min_below;
> > +       cal->y_max      = cal->y_center + y_max_above;
> > +       cal->y_min      = cal->y_center - y_min_below;
> > +}
> > +
> > +/* data input must have at least 9 bytes */
> > +static void switchcon_parse_rstick_calibration(u8 *data,
> > +                                              struct switchcon_stick_cal *cal)
> > +{
> > +       s32 x_max_above;
> > +       s32 x_min_below;
> > +       s32 y_max_above;
> > +       s32 y_min_below;
> > +
> > +       cal->x_center   = ((data[1] &lt;&lt; 8) &amp; 0xF00) | data[0];
> > +       cal->y_center   = (data[2] &lt;&lt; 4) | (data[1] >> 4);
> > +       x_max_above     = ((data[7] &lt;&lt; 8) &amp; 0xF00) | data[6];
> > +       y_max_above     = (data[8] &lt;&lt; 4) | (data[7] >> 4);
> > +       x_min_below     = ((data[4] &lt;&lt; 8) &amp; 0xF00) | data[3];
> > +       y_min_below     = (data[5] &lt;&lt; 4) | (data[4] >> 4);
> > +       cal->x_max      = cal->x_center + x_max_above;
> > +       cal->x_min      = cal->x_center - x_min_below;
> > +       cal->y_max      = cal->y_center + y_max_above;
> > +       cal->y_min      = cal->y_center - y_min_below;
> > +}
> > +
> > +/* Common handler for parsing inputs */
> > +static int switchcon_ctlr_read_handler(struct switchcon_ctlr *ctlr,
> > +                                               u8 *data, int size)
> > +{
> > +       int ret = 0;
> > +
> > +       switch (data[0]) {
> > +       case SC_INPUT_SUBCMD_REPLY:
> > +       case SC_INPUT_IMU_DATA:
> > +       case SC_INPUT_MCU_DATA:
> > +               if (size &lt; 12) /* make sure it contains the input report */
> > +                       break;
> > +
> > +               /* Parse analog sticks */
> > +               ctlr->left_stick_x      = data[6] | ((data[7] &amp; 0xF) &lt;&lt; 8);
> > +               ctlr->left_stick_y      = (data[7] >> 4) | (data[8] &lt;&lt; 4);
> > +               ctlr->right_stick_x     = data[9] | ((data[10] &amp; 0xF) &lt;&lt; 8);
> > +               ctlr->right_stick_y     = (data[10] >> 4) | (data[11] &lt;&lt; 4);
> > +
> > +               /* Parse digital buttons */
> > +               ctlr->but_y             = 0x01 &amp; data[3];
> > +               ctlr->but_x             = 0x02 &amp; data[3];
> > +               ctlr->but_b             = 0x04 &amp; data[3];
> > +               ctlr->but_a             = 0x08 &amp; data[3];
> > +               ctlr->but_sr_right_jc   = 0x10 &amp; data[3];
> > +               ctlr->but_sl_right_jc   = 0x20 &amp; data[3];
> > +               ctlr->but_r             = 0x40 &amp; data[3];
> > +               ctlr->but_zr            = 0x80 &amp; data[3];
> > +               ctlr->but_l             = 0x40 &amp; data[5];
> > +               ctlr->but_zl            = 0x80 &amp; data[5];
> > +               ctlr->but_minus         = 0x01 &amp; data[4];
> > +               ctlr->but_plus          = 0x02 &amp; data[4];
> > +               ctlr->but_rstick        = 0x04 &amp; data[4];
> > +               ctlr->but_lstick        = 0x08 &amp; data[4];
> > +               ctlr->but_home          = 0x10 &amp; data[4];
> > +               ctlr->but_capture       = 0x20 &amp; data[4];
> > +               ctlr->but_down          = 0x01 &amp; data[5];
> > +               ctlr->but_up            = 0x02 &amp; data[5];
> > +               ctlr->but_right         = 0x04 &amp; data[5];
> > +               ctlr->but_left          = 0x08 &amp; data[5];
> > +               ctlr->but_sr_left_jc    = 0x10 &amp; data[5];
> > +               ctlr->but_sl_left_jc    = 0x20 &amp; data[5];
>
> Few things here:
> - replace all hex values with their BIT() macro equivalent
> - what is the point of extracting this nice bitfields into booleans?
> - for shorter processing/writing, you might want to store the buttons
> data as a single u32 and have the defines for the various buttons as
> BIT() within this u32.
>
> > +
> > +               break;
> > +       case SC_INPUT_BUTTON_EVENT:
> > +               /* the controller is in wrong request mode */
> > +               switchcon_request_report_mode(ctlr);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               break;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static int switchcon_ctlr_procon_init(struct switchcon_ctlr *ctlr)
> > +{
> > +       int ret;
> > +       u8 baud_cmd[] = {SC_OUTPUT_USB_CMD, SC_USB_CMD_BAUDRATE_3M};
> > +
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_USB_SET_BAUD;
> > +       ret = switchcon_hid_queue_send(ctlr, baud_cmd, sizeof(baud_cmd));
>
> I don't get why do you need to queue this command.
> Your device has started, so you should be able to read/write from it
> while you are in probe.
>
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       return ret;
> > +}
> > +
> > +static int switchcon_ctlr_joyconl_init(struct switchcon_ctlr *ctlr)
> > +{
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       switchcon_request_calibration(ctlr);
> > +       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_CALIBRATION;
> > +       ctlr->is_right_joycon = false;
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       return 0;
> > +}
> > +
> > +static int switchcon_ctlr_joyconr_init(struct switchcon_ctlr *ctlr)
> > +{
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       switchcon_request_calibration(ctlr);
> > +       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_CALIBRATION;
> > +       ctlr->is_right_joycon = true;
> > +       switchcon_request_calibration(ctlr);
>
> Why is this part not symmetrical to the other? (why do you need to
> call twice for the calibration?)
> The call for calibration is  delayed in a worker, so do you really
> need to call it twice?
Yeah. I just somehow missed that repeated call.
>
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       return 0;
> > +}
> > +
> > +void switchcon_ctlr_general_deinit(struct switchcon_ctlr *ctlr)
> > +{
> > +       if (ctlr->switchcon_in)
> > +               switchcon_input_destroy(ctlr->switchcon_in);
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       flush_work(&amp;ctlr->output_worker);
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       mutex_destroy(&amp;ctlr->mutex);
> > +}
> > +
> > +static int switchcon_ctlr_general_handle_event(struct switchcon_ctlr *ctlr,
> > +                                                       u8 *data, int size)
> > +{
> > +       mutex_lock(&amp;ctlr->mutex);
>
> You are called here from .raw_event(). And IIRC, you can't sleep here,
> so taking a mutex is not allowed.
>
> Also, here you just released the mutex in the caller. It would be
> simpler to assume the lock is held and do nothing locking-related
> here.
>
> > +       switch (ctlr->ctlr_state) {
> > +       case SWITCHCON_CTLR_STATE_CALIBRATION:
> > +               if (data[0] != SC_INPUT_SUBCMD_REPLY || size &lt; 38
> > +                || data[13] != 0x90 || data[14] != 0x10) {
> > +                       /* resend request if it wasn't received */
> > +                       switchcon_request_calibration(ctlr);
> > +                       break;
> > +               }
> > +               switchcon_parse_lstick_calibration(data + 20,
> > +                                                 &amp;ctlr->left_stick_cal);
> > +               switchcon_parse_rstick_calibration(data + 29,
> > +                                                 &amp;ctlr->right_stick_cal);
> > +
> > +               hid_info(ctlr->hdev, "l_x_c=%d l_x_max=%d l_x_min=%d\n",
>
> For these 4 printks, please prefix them with 'calibration data: '.
> Also, it would be probably better to regroup them in just one hid_info
> call or you might get interleaved with an other printk in the middle.
>
> > +                                               ctlr->left_stick_cal.x_center,
> > +                                               ctlr->left_stick_cal.x_max,
> > +                                               ctlr->left_stick_cal.x_min);
> > +               hid_info(ctlr->hdev, "l_y_c=%d l_y_max=%d l_y_min=%d\n",
> > +                                               ctlr->left_stick_cal.y_center,
> > +                                               ctlr->left_stick_cal.y_max,
> > +                                               ctlr->left_stick_cal.y_min);
> > +               hid_info(ctlr->hdev, "r_x_c=%d r_x_max=%d r_x_min=%d\n",
> > +                                               ctlr->right_stick_cal.x_center,
> > +                                               ctlr->right_stick_cal.x_max,
> > +                                               ctlr->right_stick_cal.x_min);
> > +               hid_info(ctlr->hdev, "r_y_c=%d r_y_max=%d r_y_min=%d\n",
> > +                                               ctlr->right_stick_cal.y_center,
> > +                                               ctlr->right_stick_cal.y_max,
> > +                                               ctlr->right_stick_cal.y_min);
> > +               ctlr->ctlr_state = SWITCHCON_CTLR_STATE_POST_CALIBRATION;
> > +               switchcon_request_report_mode(ctlr);
> > +               break;
> > +       case SWITCHCON_CTLR_STATE_READ:
> > +               switchcon_ctlr_read_handler(ctlr, data, size);
> > +               if (ctlr->switchcon_in)
> > +                       schedule_work(&amp;ctlr->switchcon_in->input_worker);
>
> Why do you need to defer the processing of the current event in a worker?
> Calling input_event() from .raw_event() should be safe.
>
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +
> > +       return 0;
> > +}
> > +
> > +static int switchcon_ctlr_procon_handle_event(struct switchcon_ctlr *ctlr,
> > +                                                       u8 *data, int size)
> > +{
> > +       int ret;
> > +       u8 handshake_cmd[]      = {SC_OUTPUT_USB_CMD, SC_USB_CMD_HANDSHAKE};
> > +       u8 timeout_cmd[]        = {SC_OUTPUT_USB_CMD, SC_USB_CMD_NO_TIMEOUT};
> > +
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       switch (ctlr->ctlr_state) {
> > +       case SWITCHCON_CTLR_STATE_USB_SET_BAUD:
> > +               if (size &lt; 2 || data[0] != SC_INPUT_USB_RESPONSE
> > +                   || data[1] != SC_USB_CMD_BAUDRATE_3M) {
> > +                       hid_dbg(ctlr->hdev, "No usb response, assume ble\n");
> > +                       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_CALIBRATION;
> > +                       switchcon_request_calibration(ctlr);
> > +                       break;
> > +               }
> > +
> > +               ret = switchcon_hid_queue_send(ctlr, handshake_cmd,
> > +                                               sizeof(handshake_cmd));
> > +               if (!ret) {
> > +                       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_USB_HANDSHAKE;
> > +                       hid_dbg(ctlr->hdev, "sent handshake\n");
> > +               }
> > +               break;
> > +       case SWITCHCON_CTLR_STATE_USB_HANDSHAKE:
> > +               if (size &lt; 2 || data[0] != SC_INPUT_USB_RESPONSE
> > +                   || data[1] != SC_USB_CMD_HANDSHAKE)
> > +                       break;
> > +               ret = switchcon_hid_queue_send(ctlr, timeout_cmd,
> > +                                               sizeof(timeout_cmd));
> > +               if (!ret) {
> > +                       hid_dbg(ctlr->hdev, "sent timeout disable\n");
> > +                       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_CALIBRATION;
> > +                       switchcon_request_calibration(ctlr);
> > +               }
> > +               break;
> > +       case SWITCHCON_CTLR_STATE_POST_CALIBRATION:
> > +               schedule_work(&amp;ctlr->create_input_worker);
> > +               ctlr->ctlr_state = SWITCHCON_CTLR_STATE_READ;
> > +               break;
> > +       default:
> > +               mutex_unlock(&amp;ctlr->mutex);
> > +               return switchcon_ctlr_general_handle_event(ctlr, data, size);
> > +       }
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       return 0;
> > +}
> > +
> > +static int switchcon_ctlr_joycon_handle_event(struct switchcon_ctlr *ctlr,
> > +                                                       u8 *data, int size)
> > +{
> > +       bool last_searching;
> > +       int i;
> > +
> > +       mutex_lock(&amp;ctlr->mutex);
> > +       switch (ctlr->ctlr_state) {
> > +       case SWITCHCON_CTLR_STATE_POST_CALIBRATION:
> > +               ctlr->ctlr_state = SWITCHCON_CTLR_STATE_SEARCH;
> > +               /* flashing leds to indicate searching */
> > +               for (i = 0; i &lt; 10; i++)
> > +                       /* command multiple times to ensure it works */
>
> ouch
> (well, probably not at your code, but it feels wrong to have to send
> it 10 times to ensure it works)
I'll need to probably investigate this problem further. It hasn't happened
during testing of the Pro Controllers so far, only the Joy-Cons.
>
> > +                       switchcon_set_player_leds(ctlr, 0b1111, 0);
> > +               /* intentional fall-through */
> > +       case SWITCHCON_CTLR_STATE_SEARCH:
>
> So the pairing appears to be first in first served. Am I correct?
> Also, you sem to wait for the first event on each controller to do the
> pairing. Can't you store the list of joycons in a static list in the
> driver, and when you get a new one, you check with the previous ones
> if there is a match?
> This way, you should be able to drop the detect_pair_worker and have a
> more straightforward code.
I tried to simulate the joy-con matching process on the actual Switch.
It involves needing to hold the triggers on both Joy-Cons at once to
pair them together. This is why I'm monitoring for the triggers being
released as well.
>
> > +               last_searching = ctlr->searching;
> > +               switchcon_ctlr_read_handler(ctlr, data, size);
> > +               ctlr->searching =  ctlr->but_r || ctlr->but_zr
> > +                               || ctlr->but_l || ctlr->but_zl;
>
> personal taste (and in many other places): I prefer when the logic
> operator ('||' here) is at the end of the previous line.
>
> > +               if (ctlr->searching != last_searching) {
> > +                       schedule_work(&amp;ctlr->detect_pair_worker);
> > +               } else if ((ctlr->but_sr_left_jc &amp;&amp; ctlr->but_sl_left_jc)
> > +                       || (ctlr->but_sr_right_jc &amp;&amp; ctlr->but_sl_right_jc)) {
> > +                       schedule_work(&amp;ctlr->create_horiz_worker);
> > +                       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_READ;
> > +               }
> > +               break;
> > +       default:
> > +               mutex_unlock(&amp;ctlr->mutex);
> > +               return switchcon_ctlr_general_handle_event(ctlr, data, size);
> > +       }
> > +       mutex_unlock(&amp;ctlr->mutex);
> > +       return 0;
> > +}
> > +
> > +/* Implementations for each supported controller type */
> > +static const struct switchcon_impl switchcon_impl_procon = {
> > +       .init           = switchcon_ctlr_procon_init,
> > +       .deinit         = switchcon_ctlr_general_deinit,
> > +       .handle_event   = switchcon_ctlr_procon_handle_event,
> > +};
> > +
> > +static const struct switchcon_impl switchcon_impl_joycon_l = {
> > +       .init           = switchcon_ctlr_joyconl_init,
> > +       .deinit         = switchcon_ctlr_general_deinit,
> > +       .handle_event   = switchcon_ctlr_joycon_handle_event,
> > +};
> > +
> > +static const struct switchcon_impl switchcon_impl_joycon_r = {
> > +       .init           = switchcon_ctlr_joyconr_init,
> > +       .deinit         = switchcon_ctlr_general_deinit,
> > +       .handle_event   = switchcon_ctlr_joycon_handle_event,
> > +};
>
> I am not sure you'll get any benefit by having such callbacks instead
> of having simple switch/case in the code. AFAICT, those callbacks
> could be replaced by 3 switch/case and allow the processor to optimize
> it better.
>
> > +
> > +static struct switchcon_ctlr *switchcon_ctlr_create(struct hid_device *hdev)
> > +{
> > +       struct switchcon_ctlr *ctlr;
> > +
> > +       ctlr = devm_kzalloc(&amp;hdev->dev, sizeof(*ctlr), GFP_KERNEL);
> > +       if (!ctlr)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       switch (hdev->product) {
> > +       case USB_DEVICE_ID_NINTENDO_PROCON:
> > +               hid_info(hdev, "detected pro controller\n");
> > +               ctlr->impl = &amp;switchcon_impl_procon;
> > +               break;
> > +       case USB_DEVICE_ID_NINTENDO_JOYCONL:
> > +               hid_info(hdev, "detected left joy-con\n");
> > +               ctlr->impl = &amp;switchcon_impl_joycon_l;
> > +               break;
> > +       case USB_DEVICE_ID_NINTENDO_JOYCONR:
> > +               hid_info(hdev, "detected right joy-con\n");
> > +               ctlr->impl = &amp;switchcon_impl_joycon_r;
> > +               break;
> > +       default:
> > +               hid_err(hdev, "unknown product id\n");
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +       ctlr->hdev = hdev;
> > +       ctlr->ctlr_state = SWITCHCON_CTLR_STATE_INIT;
> > +       hid_set_drvdata(hdev, ctlr);
> > +       spin_lock_init(&amp;ctlr->output_lock);
> > +       mutex_init(&amp;ctlr->mutex);
> > +       INIT_WORK(&amp;ctlr->output_worker, switchcon_send_work_handler);
> > +       INIT_WORK(&amp;ctlr->create_input_worker,
> > +                 switchcon_create_procon_input_handler);
> > +       INIT_WORK(&amp;ctlr->detect_pair_worker, switchcon_detect_pair_handler);
> > +       INIT_WORK(&amp;ctlr->create_horiz_worker, switchcon_create_horiz_handler);
> > +       return ctlr;
> > +}
> > +
> > +static int switchcon_hid_probe(struct hid_device *hdev,
> > +                              const struct hid_device_id *id)
> > +{
> > +       int ret;
> > +       struct switchcon_ctlr *ctlr;
> > +
> > +       hid_dbg(hdev, "probe - start\n");
> > +       hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
>
> This shouldn't be required since v4.12. Can you double check if you
> really need this?
Looks like I don't. I was just using it because the wiimote driver was.
>
> > +
> > +       ctlr = switchcon_ctlr_create(hdev);
> > +       if (IS_ERR(ctlr)) {
> > +               hid_err(hdev, "Failed to create new controller\n");
> > +               ret = PTR_ERR(ctlr);
> > +               goto err;
> > +       }
> > +
> > +       ret = hid_parse(hdev);
> > +       if (ret) {
> > +               hid_err(hdev, "HID parse failed\n");
> > +               goto err;
> > +       }
> > +
> > +       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > +       if (ret) {
> > +               hid_err(hdev, "HW start failed\n");
> > +               goto err;
> > +       }
> > +
> > +       ret = hid_hw_open(hdev);
> > +       if (ret) {
> > +               hid_err(hdev, "cannot start hardware I/O\n");
> > +               goto err_stop;
> > +       }
> > +
> > +       ret = switchcon_ctlr_init(ctlr);
> > +       if (ret) {
> > +               hid_err(hdev, "failed to initialize ctlr\n");
> > +               goto err_close;
> > +       }
> > +
> > +       hid_dbg(hdev, "probe - success\n");
> > +       return 0;
> > +
> > +err_close:
> > +       switchcon_ctlr_deinit(ctlr);
> > +       hid_hw_close(hdev);
> > +err_stop:
> > +       hid_hw_stop(hdev);
> > +err:
> > +       hid_err(hdev, "probe - fail = %d\n", ret);
> > +       return ret;
> > +}
> > +
> > +static void switchcon_hid_remove(struct hid_device *hdev)
> > +{
> > +       struct switchcon_ctlr *ctlr = hid_get_drvdata(hdev);
> > +
> > +       hid_dbg(hdev, "remove\n");
> > +       hid_hw_close(hdev);
> > +       hid_hw_stop(hdev);
> > +       switchcon_ctlr_deinit(ctlr);
> > +}
> > +
> > +static const struct hid_device_id switchcon_hid_devices[] = {
> > +       { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> > +                        USB_DEVICE_ID_NINTENDO_PROCON) },
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO,
> > +                        USB_DEVICE_ID_NINTENDO_PROCON) },
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO,
> > +                        USB_DEVICE_ID_NINTENDO_JOYCONL) },
> > +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO,
> > +                        USB_DEVICE_ID_NINTENDO_JOYCONR) },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(hid, switchcon_hid_devices);
> > +
> > +static struct hid_driver switchcon_hid_driver = {
> > +       .name           = "switchcon",
> > +       .id_table       = switchcon_hid_devices,
> > +       .probe          = switchcon_hid_probe,
> > +       .remove         = switchcon_hid_remove,
> > +       .raw_event      = switchcon_hid_event,
> > +};
> > +module_hid_driver(switchcon_hid_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Daniel J. Ogorchock <djogorchock@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Driver for Nintendo Switch Controllers");
> > --
> > 2.20.1
> >
>
> Cheers,
> Benjamin

I'll continue working on version 2 in the coming days.

Thanks,
Daniel




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux