Hi Daniel,
On Tue, Jan 15, 2019 at 6:43 AM Daniel Ogorchock <djogorchock@xxxxxxxxx> wrote:
>
> 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?
That is correct. But given that they are not putting the CPU to sleep
while they are waiting, we should limit their usage.
> 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.
That or the mutexes were always available. Given the structure of the
v1, I think there was a low chance of having conflicts between the
workqueues and the incoming IRQs.
>
> 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.
Yep, that looks right.
> 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.
It's not receiving a constant stream of unsollicited data ona regular
mouse. However, for devices with accelerometers, we could have the
same situation.
You have 2 solutions:
- either until the device hasn't finished the probe, you filter all of
the incoming events (not answers)
- either you have a flag that says that you are expecting an answer of
a specific type, and you only release the waiting probe when you get
this particular event (this is what we do in hidpp).
The second option is the more reliable, but the first might be easier
to implement if the device plays nicely (in hidpp, we can have more
than one request at a time, and the requessts can be emitted after the
probe, so we need 2.)
>
> 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.
You are probably correct fort the input events sides. I am not
entirely sure why you need to combine the controller states given that
they are separate devices.
> >
> > >
> > > 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.
OK. Starting from scratch is perfectly fine.
> >
> > > + *
> > > + * 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?
yep. Well, it depends :)
if you can afford the wait between 2 incoming IRQs from the 2
different devices, then yes, you can ditch the kfifos and only rely on
spinlocks or on down_trylock(mutex).
If there is a chance both JoyCons sends data at the same time, then
you might want to keep the fifo. You take a spinlock, put the raw data
in the fifo (no processing), then release the spinlock and schedule a
worker. Then you have a worker that gets the incoming data in a proper
order and can process them without locking mechanism.
> >
> > > + 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.
Oh, OK. Does the switch has an UI to actually explain what needs to be
done for the pairing?
And can you unpair/repair the controllers on the Switch?
If so, then we might probably do what was suggested in the other
thread and by Roderick here: let the userspace do the pairing for us.
> >
> > > + 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.
Yeah, that is historical. We can't easily clean up drivers without the
hardware or thoroughly checking that there is nothing wrong. I
couldn't suppress the quirk entirely because there is at least one
user that relies on it, so we just kept those but removed the
(usb)hid-core logic behind.
> >
> > > +
> > > + 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.
I see that Roderick gave also a review and I have been told others
would like to chime in too :)
Your v2 doesn't need to be perfect as there will probably be a v3
(every new driver gets its shared amount of revisions).
Please try to take into account Roderick's suggestions. The LED one is
very important and will need to be in the final version, but I think
the most important part for now is to make a driver we can clearly
review and understand.
Cheers,
Benjamin
