Re: [PATCH v3 1/3] HID: switchcon: add nintendo switch controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux