Hi Roderick,
On Mon, Jan 28, 2019 at 6:11 AM 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.
>
> 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.
Actually, I think you are missing `hid_device_io_start(hdev);` in hid-sony.c
What this driver does and which is race free (in pseudo code):
probe() {
hid_parse();
hid_hw_start(CONNECT_HIDRAW);
hid_hw_open(hdev);
hid_device_io_start(hdev);
// now you can do whatever IO you want, .raw_event() will also be called
hid_hw_close(hdev);
switchcon_input_create(ctlr);
}
If you want to rely on HIDINPUT to create the inputs for you and not
prematurely expose a hidraw node, then you can first hid_hwstart with
CONNECT_DRIVER, do your init, then stop the hid device and finally
start with CONNECT_DEFAULT.
Cheers,
Benjamin
>
> 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).
>
> 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.
>
> > +
> > +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
> >
