Hi Roderick, Thanks for the again prompt response. On Sun, Jan 27, 2019 at 11:11 PM Roderick Colenbrander <thunderbird2k@xxxxxxxxx> wrote: > > Hi Daniel, > > Thanks for updating the patch series. It is definitely improved and > easier to read. See some of my comments below. > > One other thing I noticed, see the comment in v2 review about breaking > userspace. I think since you change the button / stick mapping > compared to what SDL2 supports for Pro controller. You need to do > something e.g. different version like hid-sony does. > Ah yes, I had read your initial email but forgot about it entirely when making the v3 changes. I'll address that in v4. > Out of curiosity did you test the driver on Bluetooth? The reason I'm > asking is, because I suspect you would have hit a deadlock issue, > which we hit in hid-sony before. I'm not sure how you got around that > one.. > > Let me explain the issue. When using Bluetooth, many Bluetooth stacks > rely on uhid (either Bluez on desktop and even Android's one). I > recall when the BT stack requests uhid to "create" an input device, > this ultimately triggers "_probe" in your driver. hid-sony at the time > and similar for hid-switchcon triggers various requests to the > hardware during probe. However uhid is single threaded and it is > holding a lock. This caused a deadlock and caused hardware request > timeouts. I don't see how this is not happening for you. > > The solution in hid-sony at the time was to move logic to > "_input_configured", which had slightly different timing. (It is > triggered if I recall by hid_hw_start or a call like that and avoided > the deadlock). > Interesting. I've been using bluetooth extensively and haven't run into this issue. I've found a 2016 patch from you which reports to fix this problem here https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=8442b8809cd81d9d7e99ee58f44967b94b7c4168 Is this referring to the same issue? If so, looks like what you did is working for me. > Thanks, > Roderick > > On Sun, Jan 27, 2019 at 8:05 PM 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 each create their own, independent input devices, so it is > > up to userspace to combine them if desired. > > > > Signed-off-by: Daniel J. Ogorchock <djogorchock@xxxxxxxxx> > > --- > > MAINTAINERS | 6 + > > drivers/hid/Kconfig | 11 + > > drivers/hid/Makefile | 1 + > > drivers/hid/hid-ids.h | 3 + > > drivers/hid/hid-switchcon.c | 827 ++++++++++++++++++++++++++++++++++++ > > 5 files changed, 848 insertions(+) > > create mode 100644 drivers/hid/hid-switchcon.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index f3a5c97e3419..001817e2e08a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -14397,6 +14397,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..9702c07802a5 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -943,6 +943,17 @@ 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 > > + 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 27519eb8ee63..7924f7d502a8 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -846,6 +846,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-switchcon.c b/drivers/hid/hid-switchcon.c > > new file mode 100644 > > index 000000000000..f70b7cf95021 > > --- /dev/null > > +++ b/drivers/hid/hid-switchcon.c > > @@ -0,0 +1,827 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * HID driver for Nintendo Switch Joy-Cons and Pro Controllers > > + * > > + * Copyright (c) 2019 Daniel J. Ogorchock <djogorchock@xxxxxxxxx> > > + * > > + * 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 > > + * hid-logitech-hidpp driver > > + * > > + * This driver supports the Nintendo Switch Joy-Cons and Pro Controllers. The > > + * Pro Controllers can either be used over USB or Bluetooth. > > + * > > + * 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: > > + * 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 > > +#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 > > +#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 > > + > > +/* States for controller state machine */ > > +enum switchcon_ctlr_state { > > + SWITCHCON_CTLR_STATE_INIT, > > + SWITCHCON_CTLR_STATE_READ, > > +}; > > + > > +struct switchcon_stick_cal { > > + s32 max; > > + s32 min; > > + s32 center; > > +}; > > + > > +/* > > + * All the controller's button values are stored in a u32. > > + * They can be accessed with bitwise ANDs. > > + */ > > +#define SC_BTN_Y BIT(0) > > +#define SC_BTN_X BIT(1) > > +#define SC_BTN_B BIT(2) > > +#define SC_BTN_A BIT(3) > > +#define SC_BTN_SR_R BIT(4) > > +#define SC_BTN_SL_R BIT(5) > > +#define SC_BTN_R BIT(6) > > +#define SC_BTN_ZR BIT(7) > > +#define SC_BTN_MINUS BIT(8) > > +#define SC_BTN_PLUS BIT(9) > > +#define SC_BTN_RSTICK BIT(10) > > +#define SC_BTN_LSTICK BIT(11) > > +#define SC_BTN_HOME BIT(12) > > +#define SC_BTN_CAP BIT(13) /* capture button */ > > +#define SC_BTN_DOWN BIT(16) > > +#define SC_BTN_UP BIT(17) > > +#define SC_BTN_RIGHT BIT(18) > > +#define SC_BTN_LEFT BIT(19) > > +#define SC_BTN_SR_L BIT(20) > > +#define SC_BTN_SL_L BIT(21) > > +#define SC_BTN_L BIT(22) > > +#define SC_BTN_ZL BIT(23) > > + > > +enum switchcon_ctlr_type { > > + SWITCHCON_CTLR_TYPE_PROCON, > > + SWITCHCON_CTLR_TYPE_JOYCON_L, > > + SWITCHCON_CTLR_TYPE_JOYCON_R, > > +}; > > I think there is little use in having this enum. See further comments > below, hdev seems to be sufficient for this driver. > I hadn't changed it due to Benjamin's comments on v2 about the potential for future 3rd party controllers. That said, the enum could always be re-added if that comes to pass. I guess everyone can weigh in opinions. > > + > > +static const char * const switchcon_input_names[] = { > > + "Nintendo Switch Pro Controller", > > + "Nintendo Switch Left Joy-Con", > > + "Nintendo Switch Right Joy-Con", > > +}; > > + > > +enum switchcon_msg_type { > > + SWITCHCON_MSG_TYPE_NONE, > > + SWITCHCON_MSG_TYPE_USB, > > + SWITCHCON_MSG_TYPE_SUBCMD, > > +}; > > + > > +struct switchcon_subcmd_request { > > + u8 output_id; /* must be 0x01 for subcommand, 0x10 for rumble only */ > > + u8 packet_num; /* incremented every send */ > > + u8 rumble_data[8]; > > + u8 subcmd_id; > > + /* data is here */ > > +} __packed; > > + > > +/* should pass in pointer to a struct switchcon_subcmd_request */ > > +#define SC_SUBCMD_REQ_GET_DATA(req) \ > > + ((u8 *)(req) + sizeof(struct switchcon_subcmd_request)) > > + > > +struct switchcon_subcmd_reply { > > + u8 ack; /* MSB 1 for ACK, 0 for NACK */ > > + u8 id; /* id of requested subcmd */ > > + /* data is here, can be up to 35 bytes */ > > +} __packed; > > + > > +/* should pass in pointer to a struct switchcon_subcmd_reply */ > > +#define SC_SUBCMD_REPLY_GET_DATA(reply) \ > > + ((u8 *)(reply) + sizeof(struct switchcon_subcmd_reply)) > > + > > +struct switchcon_input_report { > > + u8 id; > > + u8 timer; > > + u8 bat_con; /* battery and connection info */ > > + u8 button_status[3]; > > + u8 left_stick[3]; > > + u8 right_stick[3]; > > + u8 vibrator_report; > > + > > + /* > > + * If support for firmware updates, gyroscope data, and/or NFC/IR > > + * are added in the future, this can be swapped for a union. > > + */ > > + struct switchcon_subcmd_reply reply; > > +} __packed; > > + > > +#define SC_MAX_RESP_SIZE (sizeof(struct switchcon_input_report) + 35) > > + > > +/* Each physical controller is associated with a switchcon_ctlr struct */ > > +struct switchcon_ctlr { > > + struct hid_device *hdev; > > + struct input_dev *input; > > + enum switchcon_ctlr_type type; > > + enum switchcon_ctlr_state ctlr_state; > > + > > + /* The following members are used for synchronous sends/receives */ > > + enum switchcon_msg_type msg_type; > > + u8 subcmd_num; > > + struct mutex output_mutex; > > + u8 input_buf[SC_MAX_RESP_SIZE]; > > + wait_queue_head_t wait; > > + bool received_resp; > > + u8 usb_ack_match; > > + u8 subcmd_ack_match; > > + > > + /* factory calibration data */ > > + struct switchcon_stick_cal left_stick_cal_x; > > + struct switchcon_stick_cal left_stick_cal_y; > > + struct switchcon_stick_cal right_stick_cal_x; > > + struct switchcon_stick_cal right_stick_cal_y; > > + > > +}; > > + > > +static int __switchcon_hid_send(struct hid_device *hdev, u8 *data, size_t len) > > +{ > > + u8 *buf; > > + int ret; > > + > > + buf = kmemdup(data, len, GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + ret = hid_hw_output_report(hdev, buf, len); > > + kfree(buf); > > + if (ret < 0) > > + hid_dbg(hdev, "Failed to send output report ret=%d\n", ret); > > + return ret; > > +} > > + > > +static int switchcon_hid_send_sync(struct switchcon_ctlr *ctlr, u8 *data, > > + size_t len) > > +{ > > + int ret; > > + > > + ret = __switchcon_hid_send(ctlr->hdev, data, len); > > + if (ret < 0) { > > + memset(ctlr->input_buf, 0, SC_MAX_RESP_SIZE); > > + return ret; > > + } > > + > > + if (!wait_event_timeout(ctlr->wait, ctlr->received_resp, HZ)) { > > + hid_dbg(ctlr->hdev, "syncronous send/receive timed out\n"); > > + memset(ctlr->input_buf, 0, SC_MAX_RESP_SIZE); > > + return -ETIMEDOUT; > > + } > > + > > + ctlr->received_resp = false; > > + return 0; > > +} > > + > > +static int switchcon_send_usb(struct switchcon_ctlr *ctlr, u8 cmd) > > +{ > > + int ret; > > + u8 buf[2] = {SC_OUTPUT_USB_CMD}; > > + > > + buf[1] = cmd; > > + ctlr->usb_ack_match = cmd; > > + ctlr->msg_type = SWITCHCON_MSG_TYPE_USB; > > + ret = switchcon_hid_send_sync(ctlr, buf, sizeof(buf)); > > + if (ret) > > + hid_dbg(ctlr->hdev, "send usb command failed; ret=%d\n", ret); > > + return ret; > > +} > > + > > +static int switchcon_send_subcmd(struct switchcon_ctlr *ctlr, > > + struct switchcon_subcmd_request *subcmd, > > + size_t data_len) > > +{ > > + int ret; > > + > > + subcmd->output_id = SC_OUTPUT_RUMBLE_AND_SUBCMD; > > + subcmd->packet_num = ctlr->subcmd_num; > > + if (++ctlr->subcmd_num > 0xF) > > + ctlr->subcmd_num = 0; > > + ctlr->subcmd_ack_match = subcmd->subcmd_id; > > + ctlr->msg_type = SWITCHCON_MSG_TYPE_SUBCMD; > > + > > + ret = switchcon_hid_send_sync(ctlr, (u8 *)subcmd, > > + sizeof(*subcmd) + data_len); > > + if (ret) > > + hid_dbg(ctlr->hdev, "send subcommand failed; ret=%d\n", ret); > > + return ret; > > +} > > + > > +/* Supply nibbles for flash and on. Ones correspond to active */ > > +static int switchcon_set_player_leds(struct switchcon_ctlr *ctlr, > > + u8 flash, u8 on) > > +{ > > + struct switchcon_subcmd_request *req; > > + u8 buffer[sizeof(*req) + 1] = { 0 }; > > + > > + req = (struct switchcon_subcmd_request *)buffer; > > + req->subcmd_id = SC_SUBCMD_SET_PLAYER_LIGHTS; > > + SC_SUBCMD_REQ_GET_DATA(req)[0] = (flash << 4) | on; > > + > > + hid_dbg(ctlr->hdev, "setting player leds\n"); > > + return switchcon_send_subcmd(ctlr, req, 1); > > +} > > + > > +static int switchcon_request_calibration(struct switchcon_ctlr *ctlr) > > +{ > > + struct switchcon_subcmd_request *req; > > + u8 buffer[sizeof(*req) + 5] = { 0 }; > > + u8 *data; > > + > > + req = (struct switchcon_subcmd_request *)buffer; > > + req->subcmd_id = SC_SUBCMD_SPI_FLASH_READ; > > + data = SC_SUBCMD_REQ_GET_DATA(req); > > + data[0] = 0xFF & SC_CAL_DATA_START; > > + data[1] = 0xFF & (SC_CAL_DATA_START >> 8); > > + data[2] = 0xFF & (SC_CAL_DATA_START >> 16); > > + data[3] = 0xFF & (SC_CAL_DATA_START >> 24); > > + data[4] = SC_CAL_DATA_SIZE; > > + > > + hid_dbg(ctlr->hdev, "requesting cal data\n"); > > + return switchcon_send_subcmd(ctlr, req, 5); > > +} > > + > > +static int switchcon_set_report_mode(struct switchcon_ctlr *ctlr) > > +{ > > + struct switchcon_subcmd_request *req; > > + u8 buffer[sizeof(*req) + 1] = { 0 }; > > + > > + req = (struct switchcon_subcmd_request *)buffer; > > + req->subcmd_id = SC_SUBCMD_SET_REPORT_MODE; > > + SC_SUBCMD_REQ_GET_DATA(req)[0] = 0x30; /* standard, full report mode */ > > + > > + hid_dbg(ctlr->hdev, "setting controller report mode\n"); > > + return switchcon_send_subcmd(ctlr, req, 1); > > +} > > + > > +static int switchcon_map_stick_val(struct switchcon_stick_cal *cal, s32 val) > > +{ > > + s32 center = cal->center; > > + s32 min = cal->min; > > + s32 max = cal->max; > > + int new_val; > > + > > + if (val > center) { > > + new_val = (val - center) * SC_MAX_STICK_MAG; > > + new_val /= (max - center); > > + } else { > > + new_val = (center - val) * -SC_MAX_STICK_MAG; > > + new_val /= (center - min); > > + } > > + new_val = clamp(new_val, -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG); > > + return new_val; > > +} > > + > > +static void switchcon_parse_report(struct switchcon_ctlr *ctlr, u8 *data) > > +{ > > + struct input_dev *dev = ctlr->input; > > + enum switchcon_ctlr_type type = ctlr->type; > > + u32 btns; > > + > > + btns = hid_field_extract(ctlr->hdev, data + 3, 0, 24); > > + > > + if (type == SWITCHCON_CTLR_TYPE_PROCON || > > + type == SWITCHCON_CTLR_TYPE_JOYCON_L) { > > + u16 raw_l_x; > > + u16 raw_l_y; > > + int s_l_x; > > + int s_l_y; > > Naming wise, I would probably rename "raw_l_x" to "raw_x" (abs_x later > implies left a bit). Similar for "s_l_x" probably just call it "x". > Similar later for the right stick. > > > + > > + /* get raw stick values */ > > + raw_l_x = hid_field_extract(ctlr->hdev, (data + 6), 0, 12); > > + raw_l_y = hid_field_extract(ctlr->hdev, (data + 7), 4, 12); > > + /* map the stick values */ > > + s_l_x = switchcon_map_stick_val(&ctlr->left_stick_cal_x, > > + raw_l_x); > > + s_l_y = -switchcon_map_stick_val(&ctlr->left_stick_cal_y, > > + raw_l_y); > > + /* report sticks */ > > + input_report_abs(dev, ABS_X, s_l_x); > > + input_report_abs(dev, ABS_Y, s_l_y); > > + > > + /* report buttons */ > > + input_report_key(dev, BTN_TL, btns & SC_BTN_L || > > + btns & SC_BTN_SL_L); > > + input_report_key(dev, BTN_TR, btns & SC_BTN_R || > > + btns & SC_BTN_SR_L); > > + input_report_key(dev, BTN_TL2, btns & SC_BTN_ZL); > > + input_report_key(dev, BTN_SELECT, btns & SC_BTN_MINUS); > > + input_report_key(dev, BTN_THUMBL, btns & SC_BTN_LSTICK); > > + input_report_key(dev, BTN_Z, btns & SC_BTN_CAP); > > + input_report_key(dev, BTN_DPAD_DOWN, btns & SC_BTN_DOWN); > > + input_report_key(dev, BTN_DPAD_UP, btns & SC_BTN_UP); > > + input_report_key(dev, BTN_DPAD_RIGHT, btns & SC_BTN_RIGHT); > > + input_report_key(dev, BTN_DPAD_LEFT, btns & SC_BTN_LEFT); > > + } > > + if (type == SWITCHCON_CTLR_TYPE_PROCON || > > + type == SWITCHCON_CTLR_TYPE_JOYCON_R) { > > + u16 raw_r_x; > > + u16 raw_r_y; > > + int s_r_x; > > + int s_r_y; > > "raw_rx" and "rx" perhaps? > > > + > > + /* get raw stick values */ > > + raw_r_x = hid_field_extract(ctlr->hdev, (data + 9), 0, 12); > > + raw_r_y = hid_field_extract(ctlr->hdev, (data + 10), 4, 12); > > + /* map stick values */ > > + s_r_x = switchcon_map_stick_val(&ctlr->right_stick_cal_x, > > + raw_r_x); > > + s_r_y = -switchcon_map_stick_val(&ctlr->right_stick_cal_y, > > + raw_r_y); > > + /* report sticks */ > > + 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_TL, btns & SC_BTN_L || > > + btns & SC_BTN_SL_R); > > + input_report_key(dev, BTN_TR, btns & SC_BTN_R || > > + btns & SC_BTN_SR_R); > > + input_report_key(dev, BTN_TR2, btns & SC_BTN_ZR); > > + input_report_key(dev, BTN_START, btns & SC_BTN_PLUS); > > + input_report_key(dev, BTN_THUMBR, btns & SC_BTN_RSTICK); > > + input_report_key(dev, BTN_MODE, btns & SC_BTN_HOME); > > + input_report_key(dev, BTN_WEST, btns & SC_BTN_Y); > > + input_report_key(dev, BTN_NORTH, btns & SC_BTN_X); > > + input_report_key(dev, BTN_EAST, btns & SC_BTN_A); > > + input_report_key(dev, BTN_SOUTH, btns & SC_BTN_B); > > + } > > + > > + input_sync(dev); > > +} > > + > > + > > +static const unsigned int switchcon_button_inputs[] = { > > + BTN_SELECT, BTN_Z, BTN_THUMBL, > > + BTN_START, BTN_MODE, BTN_THUMBR, > > + BTN_SOUTH, BTN_EAST, BTN_NORTH, BTN_WEST, > > + BTN_DPAD_UP, BTN_DPAD_DOWN, BTN_DPAD_LEFT, BTN_DPAD_RIGHT, > > + BTN_TL, BTN_TR, BTN_TL2, BTN_TR2, > > + 0 /* 0 signals end of array */ > > +}; > > + > > +static DEFINE_MUTEX(switchcon_input_num_mutex); > > +static int switchcon_input_create(struct switchcon_ctlr *ctlr) > > +{ > > + struct hid_device *hdev; > > + static int input_num = 1; > > + int ret; > > + int i; > > + > > + hdev = ctlr->hdev; > > + ctlr->input = input_allocate_device(); > > + if (!ctlr->input) { > > + ret = -ENOMEM; > > + goto err; > > + } > > + ctlr->input->dev.parent = &hdev->dev; > > + ctlr->input->id.bustype = hdev->bus; > > + ctlr->input->id.vendor = hdev->vendor; > > + ctlr->input->id.product = hdev->product; > > + ctlr->input->id.version = hdev->version; > > + ctlr->input->name = switchcon_input_names[ctlr->type]; > > + input_set_drvdata(ctlr->input, ctlr); > > + > > + > > + /* set up sticks */ > > + input_set_abs_params(ctlr->input, ABS_X, > > + -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG, > > + SC_STICK_FUZZ, SC_STICK_FLAT); > > + input_set_abs_params(ctlr->input, ABS_Y, > > + -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG, > > + SC_STICK_FUZZ, SC_STICK_FLAT); > > + input_set_abs_params(ctlr->input, ABS_RX, > > + -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG, > > + SC_STICK_FUZZ, SC_STICK_FLAT); > > + input_set_abs_params(ctlr->input, ABS_RY, > > + -SC_MAX_STICK_MAG, SC_MAX_STICK_MAG, > > + SC_STICK_FUZZ, SC_STICK_FLAT); > > + /* set up buttons */ > > + for (i = 0; switchcon_button_inputs[i] > 0; i++) > > + input_set_capability(ctlr->input, EV_KEY, > > + switchcon_button_inputs[i]); > > + > > + ret = input_register_device(ctlr->input); > > + if (ret) > > + goto err_input; > > + > > + /* Set the default controller player leds based on controller number */ > > + mutex_lock(&switchcon_input_num_mutex); > > + mutex_lock(&ctlr->output_mutex); > > + ret = switchcon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num)); > > + if (ret) > > + hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret); > > + mutex_unlock(&ctlr->output_mutex); > > + if (++input_num > 4) > > + input_num = 1; > > + mutex_unlock(&switchcon_input_num_mutex); > > + > > + return 0; > > + > > +err_input: > > + input_free_device(ctlr->input); > > +err: > > + return ret; > > +} > > + > > +/* data input must have at least 9 bytes */ > > +static void switchcon_parse_lstick_calibration(u8 *data, > > + struct switchcon_ctlr *ctlr) > > +{ > > + struct switchcon_stick_cal *cal_x = &ctlr->left_stick_cal_x; > > + struct switchcon_stick_cal *cal_y = &ctlr->left_stick_cal_y; > > + s32 x_max_above; > > + s32 x_min_below; > > + s32 y_max_above; > > + s32 y_min_below; > > + > > + x_max_above = hid_field_extract(ctlr->hdev, (data + 0), 0, 12); > > + y_max_above = hid_field_extract(ctlr->hdev, (data + 1), 4, 12); > > + cal_x->center = hid_field_extract(ctlr->hdev, (data + 3), 0, 12); > > + cal_y->center = hid_field_extract(ctlr->hdev, (data + 4), 4, 12); > > + x_min_below = hid_field_extract(ctlr->hdev, (data + 6), 0, 12); > > + y_min_below = hid_field_extract(ctlr->hdev, (data + 7), 4, 12); > > + 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_ctlr *ctlr) > > +{ > > + struct switchcon_stick_cal *cal_x = &ctlr->right_stick_cal_x; > > + struct switchcon_stick_cal *cal_y = &ctlr->right_stick_cal_y; > > + s32 x_max_above; > > + s32 x_min_below; > > + s32 y_max_above; > > + s32 y_min_below; > > + > > + cal_x->center = hid_field_extract(ctlr->hdev, (data + 0), 0, 12); > > + cal_y->center = hid_field_extract(ctlr->hdev, (data + 1), 4, 12); > > + x_min_below = hid_field_extract(ctlr->hdev, (data + 3), 0, 12); > > + y_min_below = hid_field_extract(ctlr->hdev, (data + 4), 4, 12); > > + x_max_above = hid_field_extract(ctlr->hdev, (data + 6), 0, 12); > > + y_max_above = hid_field_extract(ctlr->hdev, (data + 7), 4, 12); > > + 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 */ > > + switchcon_parse_report(ctlr, data); > > + break; > > + default: > > + break; > > + } > > + > > + return ret; > > +} > > + > > +static int switchcon_ctlr_handle_event(struct switchcon_ctlr *ctlr, u8 *data, > > + int size) > > +{ > > + int ret = 0; > > + bool match = false; > > + int copy_size; > > + struct switchcon_input_report *report; > > + > > + if (unlikely(mutex_is_locked(&ctlr->output_mutex)) && > > + ctlr->msg_type != SWITCHCON_MSG_TYPE_NONE) { > > + switch (ctlr->msg_type) { > > + case SWITCHCON_MSG_TYPE_USB: > > + if (size < 2) > > + break; > > + if (data[0] == SC_INPUT_USB_RESPONSE && > > + data[1] == ctlr->usb_ack_match) > > + match = true; > > + break; > > + case SWITCHCON_MSG_TYPE_SUBCMD: > > + if (size < sizeof(struct switchcon_input_report) || > > + data[0] != SC_INPUT_SUBCMD_REPLY) > > + break; > > + report = (struct switchcon_input_report *)data; > > + if (report->reply.id == ctlr->subcmd_ack_match) > > + match = true; > > + break; > > + default: > > + break; > > + } > > + > > + if (match) { > > + if (size > SC_MAX_RESP_SIZE) > > + copy_size = SC_MAX_RESP_SIZE; > > + else > > + copy_size = size; > > + memcpy(ctlr->input_buf, data, copy_size); > > + ctlr->msg_type = SWITCHCON_MSG_TYPE_NONE; > > + ctlr->received_resp = true; > > + wake_up(&ctlr->wait); > > + > > + /* This message has been handled */ > > + return 1; > > + } > > + } > > + > > + if (ctlr->ctlr_state == SWITCHCON_CTLR_STATE_READ) > > + ret = switchcon_ctlr_read_handler(ctlr, data, size); > > + > > + return ret; > > +} > > + > > +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); > > +} > > + > > +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: > > + ctlr->type = SWITCHCON_CTLR_TYPE_PROCON; > > + break; > > + case USB_DEVICE_ID_NINTENDO_JOYCONL: > > + ctlr->type = SWITCHCON_CTLR_TYPE_JOYCON_L; > > + break; > > + case USB_DEVICE_ID_NINTENDO_JOYCONR: > > + ctlr->type = SWITCHCON_CTLR_TYPE_JOYCON_R; > > + break; > > + default: > > + return ERR_PTR(-EINVAL); > > + } > > There is no need to store "ctlr->type". It is checked in one or two > places. Just use hdev in those. > > > + ctlr->hdev = hdev; > > + ctlr->ctlr_state = SWITCHCON_CTLR_STATE_INIT; > > + hid_set_drvdata(hdev, ctlr); > > + mutex_init(&ctlr->output_mutex); > > + init_waitqueue_head(&ctlr->wait); > > + return ctlr; > > +} > > With removal of the "ctrl->type" logic and some of the other > refactoring of calibration as suggested below in "_probe", there is > probably no good reason to keep a separate create function. > > > + > > +static void switchcon_ctlr_destroy(struct switchcon_ctlr *ctlr) > > +{ > > + if (ctlr->input) > > + input_unregister_device(ctlr->input); > > + mutex_destroy(&ctlr->output_mutex); > > +} > > + > > +static int switchcon_hid_probe(struct hid_device *hdev, > > + const struct hid_device_id *id) > > +{ > > + int ret; > > + struct switchcon_ctlr *ctlr; > > + struct switchcon_input_report *report; > > + u8 *raw_cal; > > + > > + hid_dbg(hdev, "probe - start\n"); > > + > > + 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; > > + } > > + > > + hid_device_io_start(hdev); > > + > > + /* Initialize the controller */ > > + mutex_lock(&ctlr->output_mutex); > > + /* if baudrate command fails, assume ble pro controller */ > > + if (ctlr->type == SWITCHCON_CTLR_TYPE_PROCON && > > + !switchcon_send_usb(ctlr, SC_USB_CMD_BAUDRATE_3M)) { > > + /* handshake */ > > + ret = switchcon_send_usb(ctlr, SC_USB_CMD_HANDSHAKE); > > + if (ret) { > > + hid_err(hdev, "Failed handshake; ret=%d\n", ret); > > + goto err_mutex; > > + } > > + /* > > + * Set no timeout (to keep controller in USB mode). > > + * This doesn't send a response, so ignore the timeout. > > + */ > > + switchcon_send_usb(ctlr, SC_USB_CMD_NO_TIMEOUT); > > + } > > + > > + /* get controller calibration data, and parse it */ > > + ret = switchcon_request_calibration(ctlr); > > + if (ret) { > > + hid_err(hdev, "Failed to retrieve calibration; ret=%d\n", ret); > > + goto err_mutex; > > + } > > + report = (struct switchcon_input_report *)ctlr->input_buf; > > + raw_cal = SC_SUBCMD_REPLY_GET_DATA(&report->reply) + 5; > > + switchcon_parse_lstick_calibration(raw_cal, ctlr); > > + switchcon_parse_rstick_calibration(raw_cal + 9, ctlr); > > Move the lstick/rstick code into the calibration function. I think > there is also little reason to keep the lstick/rstick calibration > helper functions. They are only used once, so make a slightly bigger > overall calibration function hiding all this detail. > > > + hid_info(ctlr->hdev, "calibration:\n" > > + "l_x_c=%d l_x_max=%d l_x_min=%d\n" > > + "l_y_c=%d l_y_max=%d l_y_min=%d\n" > > + "r_x_c=%d r_x_max=%d r_x_min=%d\n" > > + "r_y_c=%d r_y_max=%d r_y_min=%d\n", > > + ctlr->left_stick_cal_x.center, > > + ctlr->left_stick_cal_x.max, > > + ctlr->left_stick_cal_x.min, > > + ctlr->left_stick_cal_y.center, > > + ctlr->left_stick_cal_y.max, > > + ctlr->left_stick_cal_y.min, > > + ctlr->right_stick_cal_x.center, > > + ctlr->right_stick_cal_x.max, > > + ctlr->right_stick_cal_x.min, > > + ctlr->right_stick_cal_y.center, > > + ctlr->right_stick_cal_y.max, > > + ctlr->right_stick_cal_y.min); > > I would move more of this logic into your calibration function. Not > sure how much sense it makes to print the calibrarion data. Probably > print it using hid_dbg, so it is only printed on debug builds. > > > + > > + /* Set the reporting mode to 0x30, which is the full report mode */ > > + ret = switchcon_set_report_mode(ctlr); > > + if (ret) { > > + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret); > > + goto err_mutex; > > + } > > + > > + mutex_unlock(&ctlr->output_mutex); > > + > > + ret = switchcon_input_create(ctlr); > > + if (ret) { > > + hid_err(hdev, "Failed to create input device; ret=%d\n", ret); > > + goto err_close; > > + } > > + > > + ctlr->ctlr_state = SWITCHCON_CTLR_STATE_READ; > > + > > + hid_dbg(hdev, "probe - success\n"); > > + return 0; > > + > > +err_mutex: > > + mutex_unlock(&ctlr->output_mutex); > > +err_close: > > + switchcon_ctlr_destroy(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_destroy(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 > > Thanks, Daniel
