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 '>' 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 && 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 && 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 && 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(&ctlr->output_lock);
> > +
> > + ctlr->output_buf[ctlr->output_head++] = output;
> > + if (ctlr->output_head == SC_OUTPUT_BUF_SIZE)
> > + ctlr->output_head = 0;
> > + schedule_work(&ctlr->output_worker);
> > +
> > + spin_unlock(&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(&ctlr->output_lock);
> > +
> > + ctlr->output_buf[ctlr->output_head++] = output;
> > + if (ctlr->output_head == SC_OUTPUT_BUF_SIZE)
> > + ctlr->output_head = 0;
> > + schedule_work(&ctlr->output_worker);
> > +
> > + spin_unlock(&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 << 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 & SC_CAL_DATA_START,
> > + 0xFF & (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 < -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 < -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(&sc_input->mutex);
> > + mutex_lock(&ctlr_l->mutex);
> > + if (sc_input->ctlr_right)
> > + mutex_lock(&sc_input->ctlr_right->mutex);
> > +
> > + /* map the stick values */
> > + s_l_x = switchcon_map_x_stick_val(&ctlr_l->left_stick_cal,
> > + ctlr_l->left_stick_x);
> > + s_l_y = switchcon_map_y_stick_val(&ctlr_l->left_stick_cal,
> > + ctlr_l->left_stick_y);
> > + s_r_x = switchcon_map_x_stick_val(&ctlr_r->right_stick_cal,
> > + ctlr_r->right_stick_x);
> > + s_r_y = switchcon_map_y_stick_val(&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(&ctlr_l->mutex);
> > + if (sc_input->ctlr_right)
> > + mutex_unlock(&sc_input->ctlr_right->mutex);
> > + mutex_unlock(&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(&sc_input->mutex);
> > +
> > + sc_input->ctlr_left->switchcon_in = NULL;
> > + mutex_lock(&sc_input->ctlr_left->mutex);
> > + if (sc_input->ctlr_right) {
> > + mutex_lock(&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(&sc_input->ctlr_left->mutex);
> > + if (sc_input->ctlr_right)
> > + mutex_unlock(&sc_input->ctlr_right->mutex);
> > + mutex_unlock(&sc_input->mutex);
> > + mutex_destroy(&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 = &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
> > + && 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(&sc_input->mutex);
> > + INIT_WORK(&sc_input->input_worker, switchcon_update_input_handler);
> > +
> > + /* Set the controller player leds based on input number */
> > + mutex_lock(&switchcon_input_num_mutex);
> > + /* Need to send multiple times to make sure it's not ignored */
> > + for (i = 0; i < 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(&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(&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(&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(&switchcon_detect_pair_mutex);
> > + mutex_lock(&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 && ctlr_l == NULL)
> > + ctlr_l = ctlr;
> > + if (ctlr->is_right_joycon && ctlr_r == NULL)
> > + ctlr_r = ctlr;
> > +
> > + /* see if there's a pair */
> > + if (ctlr_l && ctlr_r) {
> > + if (ctlr == ctlr_l)
> > + mutex_lock(&ctlr_r->mutex);
> > + else
> > + mutex_lock(&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(&ctlr_r->mutex);
> > + else
> > + mutex_unlock(&ctlr_l->mutex);
> > + ctlr_l = NULL;
> > + ctlr_r = NULL;
> > + }
> > + }
> > +
> > + mutex_unlock(&ctlr->mutex);
> > + mutex_unlock(&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(&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(&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(&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(&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 < 0)
> > + hid_warn(hdev, "failed output report ret=%d",
> > + ret);
> > + kfree(output.data);
> > +
> > + }
> > + spin_lock(&ctlr->output_lock);
> > + }
> > + spin_unlock(&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 < 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] << 8) & 0xF00) | data[0];
> > + y_max_above = (data[2] << 4) | (data[1] >> 4);
> > + x_min_below = ((data[7] << 8) & 0xF00) | data[6];
> > + y_min_below = (data[8] << 4) | (data[7] >> 4);
> > + cal->x_center = ((data[4] << 8) & 0xF00) | data[3];
> > + cal->y_center = (data[5] << 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] << 8) & 0xF00) | data[0];
> > + cal->y_center = (data[2] << 4) | (data[1] >> 4);
> > + x_max_above = ((data[7] << 8) & 0xF00) | data[6];
> > + y_max_above = (data[8] << 4) | (data[7] >> 4);
> > + x_min_below = ((data[4] << 8) & 0xF00) | data[3];
> > + y_min_below = (data[5] << 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 < 12) /* make sure it contains the input report */
> > + break;
> > +
> > + /* Parse analog sticks */
> > + ctlr->left_stick_x = data[6] | ((data[7] & 0xF) << 8);
> > + ctlr->left_stick_y = (data[7] >> 4) | (data[8] << 4);
> > + ctlr->right_stick_x = data[9] | ((data[10] & 0xF) << 8);
> > + ctlr->right_stick_y = (data[10] >> 4) | (data[11] << 4);
> > +
> > + /* Parse digital buttons */
> > + ctlr->but_y = 0x01 & data[3];
> > + ctlr->but_x = 0x02 & data[3];
> > + ctlr->but_b = 0x04 & data[3];
> > + ctlr->but_a = 0x08 & data[3];
> > + ctlr->but_sr_right_jc = 0x10 & data[3];
> > + ctlr->but_sl_right_jc = 0x20 & data[3];
> > + ctlr->but_r = 0x40 & data[3];
> > + ctlr->but_zr = 0x80 & data[3];
> > + ctlr->but_l = 0x40 & data[5];
> > + ctlr->but_zl = 0x80 & data[5];
> > + ctlr->but_minus = 0x01 & data[4];
> > + ctlr->but_plus = 0x02 & data[4];
> > + ctlr->but_rstick = 0x04 & data[4];
> > + ctlr->but_lstick = 0x08 & data[4];
> > + ctlr->but_home = 0x10 & data[4];
> > + ctlr->but_capture = 0x20 & data[4];
> > + ctlr->but_down = 0x01 & data[5];
> > + ctlr->but_up = 0x02 & data[5];
> > + ctlr->but_right = 0x04 & data[5];
> > + ctlr->but_left = 0x08 & data[5];
> > + ctlr->but_sr_left_jc = 0x10 & data[5];
> > + ctlr->but_sl_left_jc = 0x20 & 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(&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(&ctlr->mutex);
> > + return ret;
> > +}
> > +
> > +static int switchcon_ctlr_joyconl_init(struct switchcon_ctlr *ctlr)
> > +{
> > + mutex_lock(&ctlr->mutex);
> > + switchcon_request_calibration(ctlr);
> > + ctlr->ctlr_state = SWITCHCON_CTLR_STATE_CALIBRATION;
> > + ctlr->is_right_joycon = false;
> > + mutex_unlock(&ctlr->mutex);
> > + return 0;
> > +}
> > +
> > +static int switchcon_ctlr_joyconr_init(struct switchcon_ctlr *ctlr)
> > +{
> > + mutex_lock(&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(&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(&ctlr->mutex);
> > + flush_work(&ctlr->output_worker);
> > + mutex_unlock(&ctlr->mutex);
> > + mutex_destroy(&ctlr->mutex);
> > +}
> > +
> > +static int switchcon_ctlr_general_handle_event(struct switchcon_ctlr *ctlr,
> > + u8 *data, int size)
> > +{
> > + mutex_lock(&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 < 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,
> > + &ctlr->left_stick_cal);
> > + switchcon_parse_rstick_calibration(data + 29,
> > + &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(&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(&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(&ctlr->mutex);
> > + switch (ctlr->ctlr_state) {
> > + case SWITCHCON_CTLR_STATE_USB_SET_BAUD:
> > + if (size < 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 < 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(&ctlr->create_input_worker);
> > + ctlr->ctlr_state = SWITCHCON_CTLR_STATE_READ;
> > + break;
> > + default:
> > + mutex_unlock(&ctlr->mutex);
> > + return switchcon_ctlr_general_handle_event(ctlr, data, size);
> > + }
> > + mutex_unlock(&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(&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 < 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(&ctlr->detect_pair_worker);
> > + } else if ((ctlr->but_sr_left_jc && ctlr->but_sl_left_jc)
> > + || (ctlr->but_sr_right_jc && ctlr->but_sl_right_jc)) {
> > + schedule_work(&ctlr->create_horiz_worker);
> > + ctlr->ctlr_state = SWITCHCON_CTLR_STATE_READ;
> > + }
> > + break;
> > + default:
> > + mutex_unlock(&ctlr->mutex);
> > + return switchcon_ctlr_general_handle_event(ctlr, data, size);
> > + }
> > + mutex_unlock(&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(&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 = &switchcon_impl_procon;
> > + break;
> > + case USB_DEVICE_ID_NINTENDO_JOYCONL:
> > + hid_info(hdev, "detected left joy-con\n");
> > + ctlr->impl = &switchcon_impl_joycon_l;
> > + break;
> > + case USB_DEVICE_ID_NINTENDO_JOYCONR:
> > + hid_info(hdev, "detected right joy-con\n");
> > + ctlr->impl = &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(&ctlr->output_lock);
> > + mutex_init(&ctlr->mutex);
> > + INIT_WORK(&ctlr->output_worker, switchcon_send_work_handler);
> > + INIT_WORK(&ctlr->create_input_worker,
> > + switchcon_create_procon_input_handler);
> > + INIT_WORK(&ctlr->detect_pair_worker, switchcon_detect_pair_handler);
> > + INIT_WORK(&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
