() 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).
>
> 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.
> + *
> + * 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.
> + 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.
> +
> +};
> +
> +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?
> + 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)
> + 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.
> + 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?
> +
> + 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
