* [PATCH v7 0/2] hid-steam driver with user mode client dection
@ 2018-03-25 16:07 Rodrigo Rivas Costa
2018-03-25 16:07 ` [PATCH v7 1/2] HID: add driver for Valve Steam Controller Rodrigo Rivas Costa
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Rodrigo Rivas Costa @ 2018-03-25 16:07 UTC (permalink / raw)
To: Benjamin Tissoires, Pierre-Loup A. Griffais,
Clément VUCHENER, Jiri Kosina, Cameron Gutman, lkml,
linux-input
Cc: Rodrigo Rivas Costa
This is a reroll of the Steam Controller driver.
This time the client usage is detected by using exposing a custom hidraw
device (hid_ll_driver) to userland and forwarding that to the real one.
About how the lizard-mode/hidraw-client issue is handled, I have added a
module parameter (I don't know if that is the best option, but it helps me
illustrate different approaches to the problem):
Independently of the lizard_mode parameter value:
1. When a hidraw client is running:
a. I will not send any lizard-mode command to the device, so the client is
in full control.
b. The input device will not send any input message. However it will not
dissappear nor return any error message. Maybe it could return ENODEV if
they try to open the input device when hidraw client is running?
If lizard_mode == 0 ('disabled'):
2.0. When a hidraw client is not running, lizard_mode is disabled.
If lizard_mode == 1, ('auto', the default):
2.1. When a hidraw client is not running:
a. When the input device is not in use, lizard mode is enabled.
b. When the input device is in use, lizard mode is disabled.
If lizard_mode == 2 ('usermode'):
2.2. This driver does not send any lizard_mode-related command. So it is up
to user mode to configure it, with steamctrl or whatever.
Note that when Steam Client opens it always disables lizard-mode (it creates
keyboard/mouse XTest inputs with the same function, though). And when it closed
(but not when it crashes, I think) it always re-enables the lizard mode.
About the input buttons/axes mapping, as per Clément suggestion, I tried to
conform to Documentation/gamepad.rst, but with a few caveats and doubts:
* BTN_NORTH/BTN_WEST are alias of BTN_X/BTN_Y, but those buttons in this
controller have the labels changed (BTN_NORTH is actually Y). I don't know
the best option, so for now I'm mapping 'Y' to BTN_Y and 'X' to BTN_X.
* I'm mapping the lpad clicks to BTN_DPAD_{UP,RIGHT,DOWN,LEFT} but I'm not
sure if that is such a good idea: it cannot do diagonals, for example.
Maybe we could fake the whole dpad from the touch position?
* I'm mapping pressing the joystick to BTN_THUMBL and clicking the rpad to
BTN_THUMBR. Clicking the lpad is unmapped because that is used for the
dpad, depending on where it is clicked.
* Currently I'm mapping the lpad-touch to BTN_THUMB and rpad-touch to
BTN_THUMB2, but I don't know if that is so useful. lpad-touch will overlap
with any use of the dpad. And rpad-touch will overlap with rpad-click...
Changes in v7:
* All the automatic lizard_mode stuff.
* Added the lizard_mode parameter.
* The patchset is reduced to 2 commits. The separation of the
steam_get_serial command no longer makes sense, since I need the
steam_send_cmd in the first commit to implement the lizard mode.
* Change the input mapping to conform to Documentation/gamepad.rst.
(v6 was a RFC, it does not count).
Changes in v5:
* Fix license SPDX to GPL-2.0+.
* Minor stylistic changes (BIT(3) instead 0x08 and so on).
Changes in v4:
* Add command to check the wireless connection status on probe, without
waiting for a message (thanks to Clément Vuchener for the tip).
* Removed the error code on redundant connection/disconnection messages. That
was harmless but polluted dmesg.
* Added buttons for touching the left-pad and right-pad.
* Fixed a misplaced #include from 2/4 to 1/4.
Changes in v3:
* Use RCU to do the dynamic connec/disconnect of wireless devices.
* Remove entries in hid-quirks.c as they are no longer needed. This allows
this module to be blacklisted without side effects.
* Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
existing use cases (lizard mode). A user-space tool to do that is
linked.
* Fully separated axes for joystick and left-pad. As it happens.
* Add fuzz values for left/right pad axes, they are a little wiggly.
Changes in v2:
* Remove references to USB. Now the interesting interfaces are selected by
looking for the ones with feature reports.
* Feature reports buffers are allocated with hid_alloc_report_buf().
* Feature report length is checked, to avoid overflows in case of
corrupt/malicius USB devices.
* Resolution added to the ABS axes.
* A lot of minor cleanups.
Rodrigo Rivas Costa (2):
HID: add driver for Valve Steam Controller
HID: steam: add battery device.
drivers/hid/Kconfig | 8 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 4 +
drivers/hid/hid-steam.c | 978 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 1 +
5 files changed, 992 insertions(+)
create mode 100644 drivers/hid/hid-steam.c
--
2.16.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v7 1/2] HID: add driver for Valve Steam Controller
2018-03-25 16:07 [PATCH v7 0/2] hid-steam driver with user mode client dection Rodrigo Rivas Costa
@ 2018-03-25 16:07 ` Rodrigo Rivas Costa
2018-03-26 9:20 ` Benjamin Tissoires
2018-03-25 16:07 ` [PATCH v7 2/2] HID: steam: add battery device Rodrigo Rivas Costa
2018-03-26 8:12 ` [PATCH v7 0/2] hid-steam driver with user mode client dection Benjamin Tissoires
2 siblings, 1 reply; 8+ messages in thread
From: Rodrigo Rivas Costa @ 2018-03-25 16:07 UTC (permalink / raw)
To: Benjamin Tissoires, Pierre-Loup A. Griffais,
Clément VUCHENER, Jiri Kosina, Cameron Gutman, lkml,
linux-input
Cc: Rodrigo Rivas Costa
There are two ways to connect the Steam Controller: directly to the USB
or with the USB wireless adapter. Both methods are similar, but the
wireless adapter can connect up to 4 devices at the same time.
The wired device will appear as 3 interfaces: a virtual mouse, a virtual
keyboard and a custom HID device.
The wireless device will appear as 5 interfaces: a virtual keyboard and
4 custom HID devices, that will remain silent until a device is actually
connected.
The custom HID device has a report descriptor with all vendor specific
usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
Steam Client provices a software translation by using hidraw and a
creates a uinput virtual gamepad and XTest keyboard/mouse.
This driver intercepts the hidraw usage, so it can get out of the way
when the Steam Client is in use.
Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
drivers/hid/Kconfig | 8 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 4 +
drivers/hid/hid-steam.c | 840 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 1 +
5 files changed, 854 insertions(+)
create mode 100644 drivers/hid/hid-steam.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 779c5ae47f36..de5f4849bfe4 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -811,6 +811,14 @@ config HID_SPEEDLINK
---help---
Support for Speedlink Vicious and Divine Cezanne mouse.
+config HID_STEAM
+ tristate "Steam Controller support"
+ depends on HID
+ ---help---
+ Say Y here if you have a Steam Controller if you want to use it
+ without running the Steam Client. It supports both the wired and
+ the wireless adaptor.
+
config HID_STEELSERIES
tristate "Steelseries SRW-S1 steering wheel support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 235bd2a7b333..e146c257285a 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
obj-$(CONFIG_HID_SONY) += hid-sony.o
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_GREENASIA) += hid-gaff.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index a0baa5ba5b84..3014991e5d4b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -987,6 +987,10 @@
#define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
#define USB_DEVICE_ID_MTP_SITRONIX 0x5001
+#define USB_VENDOR_ID_VALVE 0x28de
+#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
+#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
+
#define USB_VENDOR_ID_STEELSERIES 0x1038
#define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
new file mode 100644
index 000000000000..3504d2e2d0e5
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,840 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * HID driver for Valve Steam Controller
+ *
+ * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
+ *
+ * Supports both the wired and wireless interfaces.
+ *
+ * This controller has a builtin emulation of mouse and keyboard: the right pad
+ * can be used as a mouse, the shoulder buttons are mouse buttons, A and B
+ * buttons are ENTER and ESCAPE, and so on. This is implemented as additional
+ * HID interfaces.
+ *
+ * This is known as the "lizard mode", because apparently lizards like to use
+ * the computer from the coach, without a proper mouse and keyboard.
+ *
+ * This driver will disable the lizard mode when the input device is opened
+ * and re-enable it when the input device is closed, so as not to break user
+ * mode behaviour. The lizard_mode parameter can be used to change that.
+ *
+ * There are a few user space applications (notably Steam Client) that use
+ * the hidraw interface directly to create input devices (XTest, uinput...).
+ * In order to avoid breaking them this driver creates a layered hidraw device,
+ * so it can detect when the client is running and then:
+ * - it will not send any command to the controller.
+ * - this input device will be disabled, to avoid double input of the same
+ * user action.
+ *
+ * For additional functions, such as changing the right-pad margin or switching
+ * the led, you can use the user-space tool at:
+ *
+ * https://github.com/rodrigorc/steamctrl
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/mutex.h>
+#include <linux/rcupdate.h>
+#include <linux/delay.h>
+#include <linux/power_supply.h>
+#include "hid-ids.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
+
+static int lizard_mode = 1;
+module_param(lizard_mode, int, 0644);
+MODULE_PARM_DESC(lizard_mode,
+ "Mouse and keyboard emulation (0 = always disabled; "
+ "1 (default): enabled when gamepad is not in use; "
+ "2: let userspace decide)");
+
+#define STEAM_QUIRK_WIRELESS BIT(0)
+
+#define STEAM_SERIAL_LEN 10
+/* Touch pads are 40 mm in diameter and 65535 units */
+#define STEAM_PAD_RESOLUTION 1638
+/* Trigger runs are about 5 mm and 256 units */
+#define STEAM_TRIGGER_RESOLUTION 51
+/* Joystick runs are about 5 mm and 256 units */
+#define STEAM_JOYSTICK_RESOLUTION 51
+
+#define STEAM_PAD_FUZZ 256
+
+struct steam_device {
+ spinlock_t lock;
+ struct hid_device *hdev, *client_hdev;
+ struct mutex mutex;
+ bool client_opened, input_opened;
+ struct input_dev __rcu *input;
+ unsigned long quirks;
+ struct work_struct work_connect;
+ bool connected;
+ char serial_no[STEAM_SERIAL_LEN + 1];
+};
+
+static int steam_recv_report(struct steam_device *steam,
+ u8 *data, int size)
+{
+ struct hid_report *r;
+ u8 *buf;
+ int ret;
+
+ r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
+ if (hid_report_len(r) < 64)
+ return -EINVAL;
+
+ buf = hid_alloc_report_buf(r, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /*
+ * The report ID is always 0, so strip the first byte from the output.
+ * hid_report_len() is not counting the report ID, so +1 to the length
+ * or else we get a EOVERFLOW. We are safe from a buffer overflow
+ * because hid_alloc_report_buf() allocates +7 bytes.
+ */
+ ret = hid_hw_raw_request(steam->hdev, 0x00,
+ buf, hid_report_len(r) + 1,
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+ if (ret > 0)
+ memcpy(data, buf + 1, min(size, ret - 1));
+ kfree(buf);
+ return ret;
+}
+
+static int steam_send_report(struct steam_device *steam,
+ u8 *cmd, int size)
+{
+ struct hid_report *r;
+ u8 *buf;
+ unsigned int retries = 10;
+ int ret;
+
+ r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
+ if (hid_report_len(r) < 64)
+ return -EINVAL;
+
+ buf = hid_alloc_report_buf(r, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* The report ID is always 0 */
+ memcpy(buf + 1, cmd, size);
+
+ /*
+ * Sometimes the wireless controller fails with EPIPE
+ * when sending a feature report.
+ * Doing a HID_REQ_GET_REPORT and waiting for a while
+ * seems to fix that.
+ */
+ do {
+ ret = hid_hw_raw_request(steam->hdev, 0,
+ buf, size + 1,
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+ if (ret != -EPIPE)
+ break;
+ steam_recv_report(steam, NULL, 0);
+ msleep(50);
+ } while (--retries);
+
+ kfree(buf);
+ if (ret < 0)
+ hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
+ ret, size, cmd);
+ return ret;
+}
+
+static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
+{
+ return steam_send_report(steam, &cmd, 1);
+}
+
+static inline int steam_write_register(struct steam_device *steam,
+ u8 reg, u16 value)
+{
+ u8 cmd[] = {0x87, 0x03, reg, value & 0xFF, value >> 8};
+
+ return steam_send_report(steam, cmd, sizeof(cmd));
+}
+
+static int steam_get_serial(struct steam_device *steam)
+{
+ /*
+ * Send: 0xae 0x15 0x01
+ * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
+ */
+ int ret;
+ u8 cmd[] = {0xae, 0x15, 0x01};
+ u8 reply[3 + STEAM_SERIAL_LEN + 1];
+
+ ret = steam_send_report(steam, cmd, sizeof(cmd));
+ if (ret < 0)
+ return ret;
+ ret = steam_recv_report(steam, reply, sizeof(reply));
+ if (ret < 0)
+ return ret;
+ reply[3 + STEAM_SERIAL_LEN] = 0;
+ strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
+ return 0;
+}
+
+/*
+ * This command requests the wireless adaptor to post an event
+ * with the connection status. Useful if this driver is loaded when
+ * the controller is already connected.
+ */
+static inline int steam_request_conn_status(struct steam_device *steam)
+{
+ return steam_send_report_byte(steam, 0xb4);
+}
+
+static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
+{
+ if (enable) {
+ /* enable esc, enter, cursors */
+ steam_send_report_byte(steam, 0x85);
+ /* enable mouse */
+ steam_send_report_byte(steam, 0x8e);
+ } else {
+ /* disable esc, enter, cursor */
+ steam_send_report_byte(steam, 0x81);
+ /* disable mouse */
+ steam_write_register(steam, 0x08, 0x07);
+ }
+}
+
+static int steam_input_open(struct input_dev *dev)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+ int ret;
+
+ ret = hid_hw_open(steam->hdev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&steam->mutex);
+ steam->input_opened = true;
+ if (!steam->client_opened && lizard_mode == 1)
+ steam_set_lizard_mode(steam, false);
+ mutex_unlock(&steam->mutex);
+ return 0;
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+
+ hid_hw_close(steam->hdev);
+
+ mutex_lock(&steam->mutex);
+ steam->input_opened = false;
+ if (!steam->client_opened && lizard_mode == 1)
+ steam_set_lizard_mode(steam, true);
+ mutex_unlock(&steam->mutex);
+}
+
+static int steam_register(struct steam_device *steam)
+{
+ struct hid_device *hdev = steam->hdev;
+ struct input_dev *input;
+ int ret;
+
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ rcu_read_unlock();
+ if (input) {
+ dbg_hid("%s: already connected\n", __func__);
+ return 0;
+ }
+
+ ret = steam_get_serial(steam);
+ if (ret)
+ return ret;
+
+ hid_info(hdev, "Steam Controller '%s' connected",
+ steam->serial_no);
+
+ input = input_allocate_device();
+ if (!input)
+ return -ENOMEM;
+
+ input_set_drvdata(input, steam);
+ input->dev.parent = &hdev->dev;
+ input->open = steam_input_open;
+ input->close = steam_input_close;
+
+ input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
+ "Wireless Steam Controller" :
+ "Steam Controller";
+ input->phys = hdev->phys;
+ input->uniq = steam->serial_no;
+ input->id.bustype = hdev->bus;
+ input->id.vendor = hdev->vendor;
+ input->id.product = hdev->product;
+ input->id.version = hdev->version;
+
+ input_set_capability(input, EV_KEY, BTN_TR2);
+ input_set_capability(input, EV_KEY, BTN_TL2);
+ input_set_capability(input, EV_KEY, BTN_TR);
+ input_set_capability(input, EV_KEY, BTN_TL);
+ input_set_capability(input, EV_KEY, BTN_Y);
+ input_set_capability(input, EV_KEY, BTN_B);
+ input_set_capability(input, EV_KEY, BTN_X);
+ input_set_capability(input, EV_KEY, BTN_A);
+ input_set_capability(input, EV_KEY, BTN_DPAD_UP);
+ input_set_capability(input, EV_KEY, BTN_DPAD_RIGHT);
+ input_set_capability(input, EV_KEY, BTN_DPAD_LEFT);
+ input_set_capability(input, EV_KEY, BTN_DPAD_DOWN);
+ input_set_capability(input, EV_KEY, BTN_SELECT);
+ input_set_capability(input, EV_KEY, BTN_MODE);
+ input_set_capability(input, EV_KEY, BTN_START);
+ input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
+ input_set_capability(input, EV_KEY, BTN_GEAR_UP);
+ input_set_capability(input, EV_KEY, BTN_THUMBR);
+ input_set_capability(input, EV_KEY, BTN_THUMBL);
+ input_set_capability(input, EV_KEY, BTN_THUMB);
+ input_set_capability(input, EV_KEY, BTN_THUMB2);
+
+ input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
+ input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
+ input_set_abs_params(input, ABS_RX, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_RY, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_HAT0X, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_HAT0Y, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
+
+ ret = input_register_device(input);
+ if (ret)
+ goto input_register_fail;
+
+ rcu_assign_pointer(steam->input, input);
+
+ return 0;
+
+input_register_fail:
+ input_free_device(input);
+ return ret;
+}
+
+static void steam_unregister(struct steam_device *steam)
+{
+ struct input_dev *input;
+
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ rcu_read_unlock();
+
+ if (input) {
+ RCU_INIT_POINTER(steam->input, NULL);
+ synchronize_rcu();
+ hid_info(steam->hdev, "Steam Controller '%s' disconnected",
+ steam->serial_no);
+ input_unregister_device(input);
+ }
+}
+
+static void steam_work_connect_cb(struct work_struct *work)
+{
+ struct steam_device *steam = container_of(work, struct steam_device,
+ work_connect);
+ unsigned long flags;
+ bool connected;
+ int ret;
+
+ spin_lock_irqsave(&steam->lock, flags);
+ connected = steam->connected;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ if (connected) {
+ ret = steam_register(steam);
+ if (ret) {
+ hid_err(steam->hdev,
+ "%s:steam_register failed with error %d\n",
+ __func__, ret);
+ }
+ } else {
+ steam_unregister(steam);
+ }
+}
+
+static bool steam_is_valve_interface(struct hid_device *hdev)
+{
+ struct hid_report_enum *rep_enum;
+
+ /*
+ * The wired device creates 3 interfaces:
+ * 0: emulated mouse.
+ * 1: emulated keyboard.
+ * 2: the real game pad.
+ * The wireless device creates 5 interfaces:
+ * 0: emulated keyboard.
+ * 1-4: slots where up to 4 real game pads will be connected to.
+ * We know which one is the real gamepad interface because they are the
+ * only ones with a feature report.
+ */
+ rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
+ return !list_empty(&rep_enum->report_list);
+}
+
+static int steam_client_ll_parse(struct hid_device *hdev)
+{
+ return 0;
+}
+
+static int steam_client_ll_start(struct hid_device *hdev)
+{
+ return 0;
+}
+
+static void steam_client_ll_stop(struct hid_device *hdev)
+{
+}
+
+static int steam_client_ll_open(struct hid_device *hdev)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+ int ret;
+
+ ret = hid_hw_open(steam->hdev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&steam->mutex);
+ steam->client_opened = true;
+ mutex_unlock(&steam->mutex);
+ return ret;
+}
+
+static void steam_client_ll_close(struct hid_device *hdev)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+
+ hid_hw_close(steam->hdev);
+
+ mutex_lock(&steam->mutex);
+ steam->client_opened = false;
+ if (lizard_mode != 2) {
+ if (steam->input_opened)
+ steam_set_lizard_mode(steam, false);
+ else
+ steam_set_lizard_mode(steam, lizard_mode);
+ }
+ mutex_unlock(&steam->mutex);
+}
+
+static int steam_client_ll_raw_request(struct hid_device *hdev,
+ unsigned char reportnum, u8 *buf,
+ size_t count, unsigned char report_type,
+ int reqtype)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+
+ return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
+ report_type, reqtype);
+}
+
+static struct hid_ll_driver steam_client_ll_driver = {
+ .parse = steam_client_ll_parse,
+ .start = steam_client_ll_start,
+ .stop = steam_client_ll_stop,
+ .open = steam_client_ll_open,
+ .close = steam_client_ll_close,
+ .raw_request = steam_client_ll_raw_request,
+};
+
+static int steam_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct steam_device *steam;
+ struct hid_device *client_hdev;
+ int ret;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev,
+ "%s:parse of hid interface failed\n", __func__);
+ return ret;
+ }
+
+ /*
+ * The non-valve interfaces (mouse and keyboard emulation) and
+ * the client_hid are connected without changes.
+ */
+ if (hdev->group == HID_GROUP_STEAM ||
+ !steam_is_valve_interface(hdev)) {
+ return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ }
+
+ /*
+ * With the real steam controller interface, do not connect hidraw.
+ * Instead, create the client_hid and connect that.
+ */
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
+ if (ret)
+ return ret;
+
+ client_hdev = hid_allocate_device();
+ if (IS_ERR(client_hdev)) {
+ ret = PTR_ERR(client_hdev);
+ goto client_hdev_fail;
+ }
+
+ client_hdev->ll_driver = &steam_client_ll_driver;
+ client_hdev->dev.parent = hdev->dev.parent;
+ client_hdev->bus = hdev->bus;
+ client_hdev->vendor = hdev->vendor;
+ client_hdev->product = hdev->product;
+ strlcpy(client_hdev->name, hdev->name,
+ sizeof(client_hdev->name));
+ strlcpy(client_hdev->phys, hdev->phys,
+ sizeof(client_hdev->phys));
+ client_hdev->dev_rdesc = kmemdup(hdev->dev_rdesc,
+ hdev->dev_rsize, GFP_KERNEL);
+ client_hdev->dev_rsize = hdev->dev_rsize;
+ /*
+ * Since we use the same device info than the real interface to
+ * trick userspace, we will be calling steam_probe recursively.
+ * We need to recognize the client interface somehow.
+ */
+ client_hdev->group = HID_GROUP_STEAM;
+
+ ret = hid_add_device(client_hdev);
+ if (ret)
+ goto client_hdev_add_fail;
+
+ steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
+ if (!steam) {
+ ret = -ENOMEM;
+ goto steam_alloc_fail;
+ }
+
+ steam->client_hdev = client_hdev;
+ hid_set_drvdata(client_hdev, steam);
+
+ spin_lock_init(&steam->lock);
+ mutex_init(&steam->mutex);
+ steam->hdev = hdev;
+ hid_set_drvdata(hdev, steam);
+ steam->quirks = id->driver_data;
+ INIT_WORK(&steam->work_connect, steam_work_connect_cb);
+
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev,
+ "%s:hid_hw_open for wireless\n",
+ __func__);
+ goto hid_hw_open_fail;
+ }
+ hid_info(hdev, "Steam wireless receiver connected");
+ steam_request_conn_status(steam);
+ } else {
+ ret = steam_register(steam);
+ if (ret) {
+ hid_err(hdev,
+ "%s:steam_register failed with error %d\n",
+ __func__, ret);
+ goto input_register_fail;
+ }
+ }
+ if (lizard_mode != 2)
+ steam_set_lizard_mode(steam, lizard_mode);
+
+ return 0;
+
+input_register_fail:
+hid_hw_open_fail:
+ cancel_work_sync(&steam->work_connect);
+ hid_set_drvdata(hdev, NULL);
+steam_alloc_fail:
+ hid_hw_stop(client_hdev);
+client_hdev_add_fail:
+ hid_destroy_device(client_hdev);
+client_hdev_fail:
+ hid_err(hdev, "%s: failed with error %d\n",
+ __func__, ret);
+ return 0;
+}
+
+static void steam_remove(struct hid_device *hdev)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+
+ if (hdev->group == HID_GROUP_STEAM)
+ return;
+
+ if (!steam) {
+ hid_hw_stop(hdev);
+ return;
+ }
+
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ hid_info(hdev, "Steam wireless receiver disconnected");
+ hid_hw_close(hdev);
+ }
+ hid_hw_stop(hdev);
+ cancel_work_sync(&steam->work_connect);
+ hid_hw_stop(steam->client_hdev);
+ hid_destroy_device(steam->client_hdev);
+ steam_unregister(steam);
+}
+
+static void steam_do_connect_event(struct steam_device *steam, bool connected)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&steam->lock, flags);
+ steam->connected = connected;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ if (schedule_work(&steam->work_connect) == 0)
+ dbg_hid("%s: connected=%d event already queued\n",
+ __func__, connected);
+}
+
+/*
+ * The size for this message payload is 60.
+ * The known values are:
+ * (* values are not sent through wireless)
+ * (* accelerator/gyro is disabled by default)
+ * Offset| Type | Mapped to |Meaning
+ * -------+-------+-----------+--------------------------
+ * 4-7 | u32 | -- | sequence number
+ * 8-10 | 24bit | see below | buttons
+ * 11 | u8 | ABS_HAT2Y | left trigger
+ * 12 | u8 | ABS_HAT2X | right trigger
+ * 13-15 | -- | -- | always 0
+ * 16-17 | s16 | ABS_X/ABS_HAT0X | X value
+ * 18-19 | s16 | ABS_Y/ABS_HAT0Y | Y value
+ * 20-21 | s16 | ABS_RX | right-pad X value
+ * 22-23 | s16 | ABS_RY | right-pad Y value
+ * 24-25 | s16 | -- | * left trigger
+ * 26-27 | s16 | -- | * right trigger
+ * 28-29 | s16 | -- | * accelerometer X value
+ * 30-31 | s16 | -- | * accelerometer Y value
+ * 32-33 | s16 | -- | * accelerometer Z value
+ * 34-35 | s16 | -- | gyro X value
+ * 36-36 | s16 | -- | gyro Y value
+ * 38-39 | s16 | -- | gyro Z value
+ * 40-41 | s16 | -- | quaternion W value
+ * 42-43 | s16 | -- | quaternion X value
+ * 44-45 | s16 | -- | quaternion Y value
+ * 46-47 | s16 | -- | quaternion Z value
+ * 48-49 | -- | -- | always 0
+ * 50-51 | s16 | -- | * left trigger (uncalibrated)
+ * 52-53 | s16 | -- | * right trigger (uncalibrated)
+ * 54-55 | s16 | -- | * joystick X value (uncalibrated)
+ * 56-57 | s16 | -- | * joystick Y value (uncalibrated)
+ * 58-59 | s16 | -- | * left-pad X value
+ * 60-61 | s16 | -- | * left-pad Y value
+ * 62-63 | u16 | -- | * battery voltage
+ *
+ * The buttons are:
+ * Bit | Mapped to | Description
+ * ------+------------+--------------------------------
+ * 8.0 | BTN_TR2 | right trigger fully pressed
+ * 8.1 | BTN_TL2 | left trigger fully pressed
+ * 8.2 | BTN_TR | right shoulder
+ * 8.3 | BTN_TL | left shoulder
+ * 8.4 | BTN_Y | button Y
+ * 8.5 | BTN_B | button B
+ * 8.6 | BTN_X | button X
+ * 8.7 | BTN_A | button A
+ * 9.0 | BTN_DPAD_UP | lef-pad up
+ * 9.1 | BTN_DPAD_RIGHT | lef-pad right
+ * 9.2 | BTN_DPAD_LEFT | lef-pad left
+ * 9.3 | BTN_DPAD_DOWN | lef-pad down
+ * 9.4 | BTN_SELECT | menu left
+ * 9.5 | BTN_MODE | steam logo
+ * 9.6 | BTN_START | menu right
+ * 9.7 | BTN_GEAR_DOWN | left back lever
+ * 10.0 | BTN_GEAR_UP | right back lever
+ * 10.1 | -- | left-pad clicked
+ * 10.2 | BTN_THUMBR | right-pad clicked
+ * 10.3 | BTN_THUMB | left-pad touched (but see explanation below)
+ * 10.4 | BTN_THUMB2 | right-pad touched
+ * 10.5 | -- | unknown
+ * 10.6 | BTN_THUMBL | joystick clicked
+ * 10.7 | -- | lpad_and_joy
+ */
+
+static void steam_do_input_event(struct steam_device *steam,
+ struct input_dev *input, u8 *data)
+{
+ /* 24 bits of buttons */
+ u8 b8, b9, b10;
+ bool lpad_touched, lpad_and_joy;
+
+ b8 = data[8];
+ b9 = data[9];
+ b10 = data[10];
+
+ input_report_abs(input, ABS_HAT2Y, data[11]);
+ input_report_abs(input, ABS_HAT2X, data[12]);
+
+ /*
+ * These two bits tells how to interpret the values X and Y.
+ * lpad_and_joy tells that the joystick and the lpad are used at the
+ * same time.
+ * lpad_touched tells whether X/Y are to be read as lpad coord or
+ * joystick values.
+ * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
+ */
+ lpad_touched = b10 & BIT(3);
+ lpad_and_joy = b10 & BIT(7);
+ input_report_abs(input, lpad_touched ? ABS_HAT0X : ABS_X,
+ (s16) le16_to_cpup((__le16 *)(data + 16)));
+ input_report_abs(input, lpad_touched ? ABS_HAT0Y : ABS_Y,
+ -(s16) le16_to_cpup((__le16 *)(data + 18)));
+ /* Check if joystick is centered */
+ if (lpad_touched && !lpad_and_joy) {
+ input_report_abs(input, ABS_X, 0);
+ input_report_abs(input, ABS_Y, 0);
+ }
+ /* Check if lpad is untouched */
+ if (!(lpad_touched || lpad_and_joy)) {
+ input_report_abs(input, ABS_HAT0X, 0);
+ input_report_abs(input, ABS_HAT0Y, 0);
+ }
+
+ input_report_abs(input, ABS_RX,
+ (s16) le16_to_cpup((__le16 *)(data + 20)));
+ input_report_abs(input, ABS_RY,
+ -(s16) le16_to_cpup((__le16 *)(data + 22)));
+
+ input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
+ input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
+ input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
+ input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
+ input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
+ input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
+ input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
+ input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
+ input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
+ input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
+ input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
+ input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
+ input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
+ input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
+ input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
+ input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
+ input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
+ input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
+ input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
+ input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
+ input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
+
+ input_sync(input);
+}
+
+static int steam_raw_event(struct hid_device *hdev,
+ struct hid_report *report, u8 *data,
+ int size)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+ struct input_dev *input;
+
+ if (!steam)
+ return 0;
+
+ if (steam->client_opened)
+ hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
+ data, size, 0);
+ /*
+ * All messages are size=64, all values little-endian.
+ * The format is:
+ * Offset| Meaning
+ * -------+--------------------------------------------
+ * 0-1 | always 0x01, 0x00, maybe protocol version?
+ * 2 | type of message
+ * 3 | length of the real payload (not checked)
+ * 4-n | payload data, depends on the type
+ *
+ * There are these known types of message:
+ * 0x01: input data (60 bytes)
+ * 0x03: wireless connect/disconnect (1 byte)
+ * 0x04: battery status (11 bytes)
+ */
+
+ if (size != 64 || data[0] != 1 || data[1] != 0)
+ return 0;
+
+ switch (data[2]) {
+ case 0x01:
+ if (steam->client_opened)
+ return 0;
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ if (likely(input)) {
+ steam_do_input_event(steam, input, data);
+ } else {
+ dbg_hid("%s: input data without connect event\n",
+ __func__);
+ steam_do_connect_event(steam, true);
+ }
+ rcu_read_unlock();
+ break;
+ case 0x03:
+ /*
+ * The payload of this event is a single byte:
+ * 0x01: disconnected.
+ * 0x02: connected.
+ */
+ switch (data[4]) {
+ case 0x01:
+ steam_do_connect_event(steam, false);
+ break;
+ case 0x02:
+ steam_do_connect_event(steam, true);
+ break;
+ }
+ break;
+ case 0x04:
+ /* TODO: battery status */
+ break;
+ }
+ return 0;
+}
+
+static const struct hid_device_id steam_controllers[] = {
+ { /* Wired Steam Controller */
+ HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+ USB_DEVICE_ID_STEAM_CONTROLLER)
+ },
+ { /* Wireless Steam Controller */
+ HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+ USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
+ .driver_data = STEAM_QUIRK_WIRELESS
+ },
+ {}
+};
+
+MODULE_DEVICE_TABLE(hid, steam_controllers);
+
+static struct hid_driver steam_controller_driver = {
+ .name = "hid-steam",
+ .id_table = steam_controllers,
+ .probe = steam_probe,
+ .remove = steam_remove,
+ .raw_event = steam_raw_event,
+};
+
+module_hid_driver(steam_controller_driver);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d491027a7c22..5e5d76589954 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -364,6 +364,7 @@ struct hid_item {
#define HID_GROUP_RMI 0x0100
#define HID_GROUP_WACOM 0x0101
#define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
+#define HID_GROUP_STEAM 0x0103
/*
* HID protocol status
--
2.16.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v7 2/2] HID: steam: add battery device.
2018-03-25 16:07 [PATCH v7 0/2] hid-steam driver with user mode client dection Rodrigo Rivas Costa
2018-03-25 16:07 ` [PATCH v7 1/2] HID: add driver for Valve Steam Controller Rodrigo Rivas Costa
@ 2018-03-25 16:07 ` Rodrigo Rivas Costa
2018-03-26 8:12 ` [PATCH v7 0/2] hid-steam driver with user mode client dection Benjamin Tissoires
2 siblings, 0 replies; 8+ messages in thread
From: Rodrigo Rivas Costa @ 2018-03-25 16:07 UTC (permalink / raw)
To: Benjamin Tissoires, Pierre-Loup A. Griffais,
Clément VUCHENER, Jiri Kosina, Cameron Gutman, lkml,
linux-input
Cc: Rodrigo Rivas Costa
The wireless Steam Controller is battery operated, so add the battery
device and power information.
---
drivers/hid/hid-steam.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 139 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 3504d2e2d0e5..28dd5ebe2b9a 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -75,6 +75,10 @@ struct steam_device {
struct work_struct work_connect;
bool connected;
char serial_no[STEAM_SERIAL_LEN + 1];
+ struct power_supply_desc battery_desc;
+ struct power_supply __rcu *battery;
+ u8 battery_charge;
+ u16 voltage;
};
static int steam_recv_report(struct steam_device *steam,
@@ -238,6 +242,85 @@ static void steam_input_close(struct input_dev *dev)
mutex_unlock(&steam->mutex);
}
+static enum power_supply_property steam_battery_props[] = {
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_SCOPE,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int steam_battery_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct steam_device *steam = power_supply_get_drvdata(psy);
+ unsigned long flags;
+ s16 volts;
+ u8 batt;
+ int ret = 0;
+
+ spin_lock_irqsave(&steam->lock, flags);
+ volts = steam->voltage;
+ batt = steam->battery_charge;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = 1;
+ break;
+ case POWER_SUPPLY_PROP_SCOPE:
+ val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+ break;
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ val->intval = volts * 1000; /* mV -> uV */
+ break;
+ case POWER_SUPPLY_PROP_CAPACITY:
+ val->intval = batt;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static int steam_battery_register(struct steam_device *steam)
+{
+ struct power_supply *battery;
+ struct power_supply_config battery_cfg = { .drv_data = steam, };
+ unsigned long flags;
+ int ret;
+
+ steam->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
+ steam->battery_desc.properties = steam_battery_props;
+ steam->battery_desc.num_properties = ARRAY_SIZE(steam_battery_props);
+ steam->battery_desc.get_property = steam_battery_get_property;
+ steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
+ GFP_KERNEL, "steam-controller-%s-battery",
+ steam->serial_no);
+ if (!steam->battery_desc.name)
+ return -ENOMEM;
+
+ /* avoid the warning of 0% battery while waiting for the first info */
+ spin_lock_irqsave(&steam->lock, flags);
+ steam->voltage = 3000;
+ steam->battery_charge = 100;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ battery = power_supply_register(&steam->hdev->dev,
+ &steam->battery_desc, &battery_cfg);
+ if (IS_ERR(battery)) {
+ ret = PTR_ERR(battery);
+ hid_err(steam->hdev,
+ "%s:power_supply_register failed with error %d\n",
+ __func__, ret);
+ return ret;
+ }
+ rcu_assign_pointer(steam->battery, battery);
+ power_supply_powers(battery, &steam->hdev->dev);
+ return 0;
+}
+
static int steam_register(struct steam_device *steam)
{
struct hid_device *hdev = steam->hdev;
@@ -327,6 +410,10 @@ static int steam_register(struct steam_device *steam)
rcu_assign_pointer(steam->input, input);
+ /* ignore battery errors, we can live without it */
+ if (steam->quirks & STEAM_QUIRK_WIRELESS)
+ steam_battery_register(steam);
+
return 0;
input_register_fail:
@@ -337,11 +424,18 @@ static int steam_register(struct steam_device *steam)
static void steam_unregister(struct steam_device *steam)
{
struct input_dev *input;
+ struct power_supply *battery;
rcu_read_lock();
input = rcu_dereference(steam->input);
+ battery = rcu_dereference(steam->battery);
rcu_read_unlock();
+ if (battery) {
+ RCU_INIT_POINTER(steam->battery, NULL);
+ synchronize_rcu();
+ power_supply_unregister(battery);
+ }
if (input) {
RCU_INIT_POINTER(steam->input, NULL);
synchronize_rcu();
@@ -745,12 +839,44 @@ static void steam_do_input_event(struct steam_device *steam,
input_sync(input);
}
+/*
+ * The size for this message payload is 11.
+ * The known values are:
+ * Offset| Type | Meaning
+ * -------+-------+---------------------------
+ * 4-7 | u32 | sequence number
+ * 8-11 | -- | always 0
+ * 12-13 | u16 | voltage (mV)
+ * 14 | u8 | battery percent
+ */
+static void steam_do_battery_event(struct steam_device *steam,
+ struct power_supply *battery, u8 *data)
+{
+ unsigned long flags;
+
+ s16 volts = le16_to_cpup((__le16 *)(data + 12));
+ u8 batt = data[14];
+
+ /* Creating the battery may have failed */
+ rcu_read_lock();
+ battery = rcu_dereference(steam->battery);
+ if (likely(battery)) {
+ spin_lock_irqsave(&steam->lock, flags);
+ steam->voltage = volts;
+ steam->battery_charge = batt;
+ spin_unlock_irqrestore(&steam->lock, flags);
+ power_supply_changed(battery);
+ }
+ rcu_read_unlock();
+}
+
static int steam_raw_event(struct hid_device *hdev,
struct hid_report *report, u8 *data,
int size)
{
struct steam_device *steam = hid_get_drvdata(hdev);
struct input_dev *input;
+ struct power_supply *battery;
if (!steam)
return 0;
@@ -808,7 +934,19 @@ static int steam_raw_event(struct hid_device *hdev,
}
break;
case 0x04:
- /* TODO: battery status */
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ rcu_read_lock();
+ battery = rcu_dereference(steam->battery);
+ if (likely(battery)) {
+ steam_do_battery_event(steam, battery, data);
+ } else {
+ dbg_hid(
+ "%s: battery data without connect event\n",
+ __func__);
+ steam_do_connect_event(steam, true);
+ }
+ rcu_read_unlock();
+ }
break;
}
return 0;
--
2.16.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v7 0/2] hid-steam driver with user mode client dection
2018-03-25 16:07 [PATCH v7 0/2] hid-steam driver with user mode client dection Rodrigo Rivas Costa
2018-03-25 16:07 ` [PATCH v7 1/2] HID: add driver for Valve Steam Controller Rodrigo Rivas Costa
2018-03-25 16:07 ` [PATCH v7 2/2] HID: steam: add battery device Rodrigo Rivas Costa
@ 2018-03-26 8:12 ` Benjamin Tissoires
2018-03-28 18:14 ` Rodrigo Rivas Costa
2 siblings, 1 reply; 8+ messages in thread
From: Benjamin Tissoires @ 2018-03-26 8:12 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
Hi Rodrigo,
On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> This is a reroll of the Steam Controller driver.
>
> This time the client usage is detected by using exposing a custom hidraw
> device (hid_ll_driver) to userland and forwarding that to the real one.
That is actually more clever than what I had in mind.
This way you reliably know if the hidraw node has been opened without
touching hid-core.c or hidraw.c, well played :)
>
> About how the lizard-mode/hidraw-client issue is handled, I have added a
> module parameter (I don't know if that is the best option, but it helps me
> illustrate different approaches to the problem):
>
> Independently of the lizard_mode parameter value:
I find the values of the lizard_mode parameter hard to understand. And
honestly, I do not think there is much a difference between 0 and 1.
AFAICT, the difference between these two values is that in the first
case you are disabling the lizard mode whether or not the joystick is
in used, and in the latter, you disable it only when the joystick is
in use. Does that gives any benefits to users?
I would think having a boolean "let the kernel handle the lizard mode
with some magic inside" would be simpler to understand. You do not
really want to support too many configurations and I think it would be
wiser to say either disable the new mode or just enable it.
Also, I think there will be races if a user changes the value of the
parameter while running the system. You might want to add an
additional patch that would trigger the mode change on module
parameter change.
As far as I can tell, I am happy with such hack. There are a few
points I'd like to raise in the patch 1, but I'll inline my comments
there.
Cheers,
Benjamin
>
> 1. When a hidraw client is running:
> a. I will not send any lizard-mode command to the device, so the client is
> in full control.
> b. The input device will not send any input message. However it will not
> dissappear nor return any error message. Maybe it could return ENODEV if
> they try to open the input device when hidraw client is running?
>
> If lizard_mode == 0 ('disabled'):
> 2.0. When a hidraw client is not running, lizard_mode is disabled.
>
> If lizard_mode == 1, ('auto', the default):
> 2.1. When a hidraw client is not running:
> a. When the input device is not in use, lizard mode is enabled.
> b. When the input device is in use, lizard mode is disabled.
>
> If lizard_mode == 2 ('usermode'):
> 2.2. This driver does not send any lizard_mode-related command. So it is up
> to user mode to configure it, with steamctrl or whatever.
>
> Note that when Steam Client opens it always disables lizard-mode (it creates
> keyboard/mouse XTest inputs with the same function, though). And when it closed
> (but not when it crashes, I think) it always re-enables the lizard mode.
>
> About the input buttons/axes mapping, as per Clément suggestion, I tried to
> conform to Documentation/gamepad.rst, but with a few caveats and doubts:
> * BTN_NORTH/BTN_WEST are alias of BTN_X/BTN_Y, but those buttons in this
> controller have the labels changed (BTN_NORTH is actually Y). I don't know
> the best option, so for now I'm mapping 'Y' to BTN_Y and 'X' to BTN_X.
> * I'm mapping the lpad clicks to BTN_DPAD_{UP,RIGHT,DOWN,LEFT} but I'm not
> sure if that is such a good idea: it cannot do diagonals, for example.
> Maybe we could fake the whole dpad from the touch position?
> * I'm mapping pressing the joystick to BTN_THUMBL and clicking the rpad to
> BTN_THUMBR. Clicking the lpad is unmapped because that is used for the
> dpad, depending on where it is clicked.
> * Currently I'm mapping the lpad-touch to BTN_THUMB and rpad-touch to
> BTN_THUMB2, but I don't know if that is so useful. lpad-touch will overlap
> with any use of the dpad. And rpad-touch will overlap with rpad-click...
>
> Changes in v7:
> * All the automatic lizard_mode stuff.
> * Added the lizard_mode parameter.
> * The patchset is reduced to 2 commits. The separation of the
> steam_get_serial command no longer makes sense, since I need the
> steam_send_cmd in the first commit to implement the lizard mode.
> * Change the input mapping to conform to Documentation/gamepad.rst.
>
> (v6 was a RFC, it does not count).
>
> Changes in v5:
> * Fix license SPDX to GPL-2.0+.
> * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>
> Changes in v4:
> * Add command to check the wireless connection status on probe, without
> waiting for a message (thanks to Clément Vuchener for the tip).
> * Removed the error code on redundant connection/disconnection messages. That
> was harmless but polluted dmesg.
> * Added buttons for touching the left-pad and right-pad.
> * Fixed a misplaced #include from 2/4 to 1/4.
>
> Changes in v3:
> * Use RCU to do the dynamic connec/disconnect of wireless devices.
> * Remove entries in hid-quirks.c as they are no longer needed. This allows
> this module to be blacklisted without side effects.
> * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> existing use cases (lizard mode). A user-space tool to do that is
> linked.
> * Fully separated axes for joystick and left-pad. As it happens.
> * Add fuzz values for left/right pad axes, they are a little wiggly.
>
> Changes in v2:
> * Remove references to USB. Now the interesting interfaces are selected by
> looking for the ones with feature reports.
> * Feature reports buffers are allocated with hid_alloc_report_buf().
> * Feature report length is checked, to avoid overflows in case of
> corrupt/malicius USB devices.
> * Resolution added to the ABS axes.
> * A lot of minor cleanups.
>
> Rodrigo Rivas Costa (2):
> HID: add driver for Valve Steam Controller
> HID: steam: add battery device.
>
> drivers/hid/Kconfig | 8 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-ids.h | 4 +
> drivers/hid/hid-steam.c | 978 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/hid.h | 1 +
> 5 files changed, 992 insertions(+)
> create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 1/2] HID: add driver for Valve Steam Controller
2018-03-25 16:07 ` [PATCH v7 1/2] HID: add driver for Valve Steam Controller Rodrigo Rivas Costa
@ 2018-03-26 9:20 ` Benjamin Tissoires
2018-03-28 21:00 ` Rodrigo Rivas Costa
0 siblings, 1 reply; 8+ messages in thread
From: Benjamin Tissoires @ 2018-03-26 9:20 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
Hi Rodrigo,
few comments inlined.
On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> There are two ways to connect the Steam Controller: directly to the USB
> or with the USB wireless adapter. Both methods are similar, but the
> wireless adapter can connect up to 4 devices at the same time.
>
> The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> keyboard and a custom HID device.
>
> The wireless device will appear as 5 interfaces: a virtual keyboard and
> 4 custom HID devices, that will remain silent until a device is actually
> connected.
>
> The custom HID device has a report descriptor with all vendor specific
> usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> Steam Client provices a software translation by using hidraw and a
> creates a uinput virtual gamepad and XTest keyboard/mouse.
>
> This driver intercepts the hidraw usage, so it can get out of the way
> when the Steam Client is in use.
>
> Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> ---
> drivers/hid/Kconfig | 8 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-ids.h | 4 +
> drivers/hid/hid-steam.c | 840 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/hid.h | 1 +
> 5 files changed, 854 insertions(+)
> create mode 100644 drivers/hid/hid-steam.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 779c5ae47f36..de5f4849bfe4 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -811,6 +811,14 @@ config HID_SPEEDLINK
> ---help---
> Support for Speedlink Vicious and Divine Cezanne mouse.
>
> +config HID_STEAM
> + tristate "Steam Controller support"
> + depends on HID
> + ---help---
> + Say Y here if you have a Steam Controller if you want to use it
> + without running the Steam Client. It supports both the wired and
> + the wireless adaptor.
> +
> config HID_STEELSERIES
> tristate "Steelseries SRW-S1 steering wheel support"
> depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 235bd2a7b333..e146c257285a 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
> obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
> obj-$(CONFIG_HID_SONY) += hid-sony.o
> 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_GREENASIA) += hid-gaff.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index a0baa5ba5b84..3014991e5d4b 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -987,6 +987,10 @@
> #define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
> #define USB_DEVICE_ID_MTP_SITRONIX 0x5001
>
> +#define USB_VENDOR_ID_VALVE 0x28de
> +#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
> +#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
> +
> #define USB_VENDOR_ID_STEELSERIES 0x1038
> #define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> new file mode 100644
> index 000000000000..3504d2e2d0e5
> --- /dev/null
> +++ b/drivers/hid/hid-steam.c
> @@ -0,0 +1,840 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * HID driver for Valve Steam Controller
> + *
> + * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> + *
> + * Supports both the wired and wireless interfaces.
> + *
> + * This controller has a builtin emulation of mouse and keyboard: the right pad
> + * can be used as a mouse, the shoulder buttons are mouse buttons, A and B
> + * buttons are ENTER and ESCAPE, and so on. This is implemented as additional
> + * HID interfaces.
> + *
> + * This is known as the "lizard mode", because apparently lizards like to use
> + * the computer from the coach, without a proper mouse and keyboard.
> + *
> + * This driver will disable the lizard mode when the input device is opened
> + * and re-enable it when the input device is closed, so as not to break user
> + * mode behaviour. The lizard_mode parameter can be used to change that.
> + *
> + * There are a few user space applications (notably Steam Client) that use
> + * the hidraw interface directly to create input devices (XTest, uinput...).
> + * In order to avoid breaking them this driver creates a layered hidraw device,
> + * so it can detect when the client is running and then:
> + * - it will not send any command to the controller.
> + * - this input device will be disabled, to avoid double input of the same
> + * user action.
> + *
> + * For additional functions, such as changing the right-pad margin or switching
> + * the led, you can use the user-space tool at:
> + *
> + * https://github.com/rodrigorc/steamctrl
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include <linux/mutex.h>
> +#include <linux/rcupdate.h>
> +#include <linux/delay.h>
> +#include <linux/power_supply.h>
> +#include "hid-ids.h"
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
> +
> +static int lizard_mode = 1;
> +module_param(lizard_mode, int, 0644);
> +MODULE_PARM_DESC(lizard_mode,
> + "Mouse and keyboard emulation (0 = always disabled; "
> + "1 (default): enabled when gamepad is not in use; "
> + "2: let userspace decide)");
As mentioned in 0/2, I think a boolean might be better. Probably
rename the parameter to something else more explicit too (like
'control_lizard_mode').
Also you might want to hook up to changes to this values so users can
control it better. But this can be added in a later patch.
> +
> +#define STEAM_QUIRK_WIRELESS BIT(0)
> +
> +#define STEAM_SERIAL_LEN 10
> +/* Touch pads are 40 mm in diameter and 65535 units */
> +#define STEAM_PAD_RESOLUTION 1638
> +/* Trigger runs are about 5 mm and 256 units */
> +#define STEAM_TRIGGER_RESOLUTION 51
> +/* Joystick runs are about 5 mm and 256 units */
> +#define STEAM_JOYSTICK_RESOLUTION 51
> +
> +#define STEAM_PAD_FUZZ 256
> +
> +struct steam_device {
> + spinlock_t lock;
> + struct hid_device *hdev, *client_hdev;
> + struct mutex mutex;
> + bool client_opened, input_opened;
> + struct input_dev __rcu *input;
> + unsigned long quirks;
> + struct work_struct work_connect;
> + bool connected;
> + char serial_no[STEAM_SERIAL_LEN + 1];
> +};
> +
> +static int steam_recv_report(struct steam_device *steam,
> + u8 *data, int size)
> +{
> + struct hid_report *r;
> + u8 *buf;
> + int ret;
> +
> + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> + if (hid_report_len(r) < 64)
> + return -EINVAL;
> +
> + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /*
> + * The report ID is always 0, so strip the first byte from the output.
> + * hid_report_len() is not counting the report ID, so +1 to the length
> + * or else we get a EOVERFLOW. We are safe from a buffer overflow
> + * because hid_alloc_report_buf() allocates +7 bytes.
> + */
> + ret = hid_hw_raw_request(steam->hdev, 0x00,
> + buf, hid_report_len(r) + 1,
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> + if (ret > 0)
> + memcpy(data, buf + 1, min(size, ret - 1));
> + kfree(buf);
> + return ret;
> +}
> +
> +static int steam_send_report(struct steam_device *steam,
> + u8 *cmd, int size)
> +{
> + struct hid_report *r;
> + u8 *buf;
> + unsigned int retries = 10;
> + int ret;
> +
> + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> + if (hid_report_len(r) < 64)
> + return -EINVAL;
> +
> + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + /* The report ID is always 0 */
> + memcpy(buf + 1, cmd, size);
> +
> + /*
> + * Sometimes the wireless controller fails with EPIPE
> + * when sending a feature report.
> + * Doing a HID_REQ_GET_REPORT and waiting for a while
> + * seems to fix that.
> + */
> + do {
> + ret = hid_hw_raw_request(steam->hdev, 0,
> + buf, size + 1,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> + if (ret != -EPIPE)
> + break;
> + steam_recv_report(steam, NULL, 0);
> + msleep(50);
> + } while (--retries);
> +
> + kfree(buf);
> + if (ret < 0)
> + hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
> + ret, size, cmd);
> + return ret;
> +}
> +
> +static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
> +{
> + return steam_send_report(steam, &cmd, 1);
> +}
> +
> +static inline int steam_write_register(struct steam_device *steam,
> + u8 reg, u16 value)
> +{
> + u8 cmd[] = {0x87, 0x03, reg, value & 0xFF, value >> 8};
> +
> + return steam_send_report(steam, cmd, sizeof(cmd));
> +}
> +
> +static int steam_get_serial(struct steam_device *steam)
> +{
> + /*
> + * Send: 0xae 0x15 0x01
> + * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> + */
> + int ret;
> + u8 cmd[] = {0xae, 0x15, 0x01};
> + u8 reply[3 + STEAM_SERIAL_LEN + 1];
> +
> + ret = steam_send_report(steam, cmd, sizeof(cmd));
> + if (ret < 0)
> + return ret;
> + ret = steam_recv_report(steam, reply, sizeof(reply));
> + if (ret < 0)
> + return ret;
> + reply[3 + STEAM_SERIAL_LEN] = 0;
> + strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
> + return 0;
> +}
> +
> +/*
> + * This command requests the wireless adaptor to post an event
> + * with the connection status. Useful if this driver is loaded when
> + * the controller is already connected.
> + */
> +static inline int steam_request_conn_status(struct steam_device *steam)
> +{
> + return steam_send_report_byte(steam, 0xb4);
> +}
> +
> +static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
> +{
> + if (enable) {
> + /* enable esc, enter, cursors */
> + steam_send_report_byte(steam, 0x85);
> + /* enable mouse */
> + steam_send_report_byte(steam, 0x8e);
> + } else {
> + /* disable esc, enter, cursor */
> + steam_send_report_byte(steam, 0x81);
> + /* disable mouse */
> + steam_write_register(steam, 0x08, 0x07);
> + }
> +}
> +
> +static int steam_input_open(struct input_dev *dev)
> +{
> + struct steam_device *steam = input_get_drvdata(dev);
> + int ret;
> +
> + ret = hid_hw_open(steam->hdev);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&steam->mutex);
> + steam->input_opened = true;
> + if (!steam->client_opened && lizard_mode == 1)
> + steam_set_lizard_mode(steam, false);
> + mutex_unlock(&steam->mutex);
> + return 0;
> +}
> +
> +static void steam_input_close(struct input_dev *dev)
> +{
> + struct steam_device *steam = input_get_drvdata(dev);
> +
> + hid_hw_close(steam->hdev);
> +
> + mutex_lock(&steam->mutex);
> + steam->input_opened = false;
> + if (!steam->client_opened && lizard_mode == 1)
> + steam_set_lizard_mode(steam, true);
> + mutex_unlock(&steam->mutex);
hid_hw_close() should be called after setting the lizard mode.
> +}
> +
> +static int steam_register(struct steam_device *steam)
> +{
> + struct hid_device *hdev = steam->hdev;
> + struct input_dev *input;
> + int ret;
> +
> + rcu_read_lock();
> + input = rcu_dereference(steam->input);
> + rcu_read_unlock();
> + if (input) {
> + dbg_hid("%s: already connected\n", __func__);
> + return 0;
> + }
> +
> + ret = steam_get_serial(steam);
> + if (ret)
> + return ret;
> +
> + hid_info(hdev, "Steam Controller '%s' connected",
> + steam->serial_no);
> +
> + input = input_allocate_device();
> + if (!input)
> + return -ENOMEM;
> +
> + input_set_drvdata(input, steam);
> + input->dev.parent = &hdev->dev;
> + input->open = steam_input_open;
> + input->close = steam_input_close;
> +
> + input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
> + "Wireless Steam Controller" :
> + "Steam Controller";
> + input->phys = hdev->phys;
> + input->uniq = steam->serial_no;
> + input->id.bustype = hdev->bus;
> + input->id.vendor = hdev->vendor;
> + input->id.product = hdev->product;
> + input->id.version = hdev->version;
> +
> + input_set_capability(input, EV_KEY, BTN_TR2);
> + input_set_capability(input, EV_KEY, BTN_TL2);
> + input_set_capability(input, EV_KEY, BTN_TR);
> + input_set_capability(input, EV_KEY, BTN_TL);
> + input_set_capability(input, EV_KEY, BTN_Y);
> + input_set_capability(input, EV_KEY, BTN_B);
> + input_set_capability(input, EV_KEY, BTN_X);
> + input_set_capability(input, EV_KEY, BTN_A);
> + input_set_capability(input, EV_KEY, BTN_DPAD_UP);
> + input_set_capability(input, EV_KEY, BTN_DPAD_RIGHT);
> + input_set_capability(input, EV_KEY, BTN_DPAD_LEFT);
> + input_set_capability(input, EV_KEY, BTN_DPAD_DOWN);
> + input_set_capability(input, EV_KEY, BTN_SELECT);
> + input_set_capability(input, EV_KEY, BTN_MODE);
> + input_set_capability(input, EV_KEY, BTN_START);
> + input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
> + input_set_capability(input, EV_KEY, BTN_GEAR_UP);
> + input_set_capability(input, EV_KEY, BTN_THUMBR);
> + input_set_capability(input, EV_KEY, BTN_THUMBL);
> + input_set_capability(input, EV_KEY, BTN_THUMB);
> + input_set_capability(input, EV_KEY, BTN_THUMB2);
> +
> + input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
> + input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
> + input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
> + input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
> + input_set_abs_params(input, ABS_RX, -32767, 32767,
> + STEAM_PAD_FUZZ, 0);
> + input_set_abs_params(input, ABS_RY, -32767, 32767,
> + STEAM_PAD_FUZZ, 0);
> + input_set_abs_params(input, ABS_HAT0X, -32767, 32767,
> + STEAM_PAD_FUZZ, 0);
> + input_set_abs_params(input, ABS_HAT0Y, -32767, 32767,
> + STEAM_PAD_FUZZ, 0);
> + input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
> + input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
> + input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
> + input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
> + input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
> + input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
> + input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
> + input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
> +
> + ret = input_register_device(input);
> + if (ret)
> + goto input_register_fail;
> +
> + rcu_assign_pointer(steam->input, input);
> +
> + return 0;
> +
> +input_register_fail:
> + input_free_device(input);
> + return ret;
> +}
> +
> +static void steam_unregister(struct steam_device *steam)
> +{
> + struct input_dev *input;
> +
> + rcu_read_lock();
> + input = rcu_dereference(steam->input);
> + rcu_read_unlock();
> +
> + if (input) {
> + RCU_INIT_POINTER(steam->input, NULL);
> + synchronize_rcu();
> + hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> + steam->serial_no);
> + input_unregister_device(input);
> + }
> +}
> +
> +static void steam_work_connect_cb(struct work_struct *work)
> +{
> + struct steam_device *steam = container_of(work, struct steam_device,
> + work_connect);
> + unsigned long flags;
> + bool connected;
> + int ret;
> +
> + spin_lock_irqsave(&steam->lock, flags);
> + connected = steam->connected;
> + spin_unlock_irqrestore(&steam->lock, flags);
> +
> + if (connected) {
> + ret = steam_register(steam);
> + if (ret) {
> + hid_err(steam->hdev,
> + "%s:steam_register failed with error %d\n",
> + __func__, ret);
> + }
> + } else {
> + steam_unregister(steam);
> + }
> +}
> +
> +static bool steam_is_valve_interface(struct hid_device *hdev)
> +{
> + struct hid_report_enum *rep_enum;
> +
> + /*
> + * The wired device creates 3 interfaces:
> + * 0: emulated mouse.
> + * 1: emulated keyboard.
> + * 2: the real game pad.
> + * The wireless device creates 5 interfaces:
> + * 0: emulated keyboard.
> + * 1-4: slots where up to 4 real game pads will be connected to.
> + * We know which one is the real gamepad interface because they are the
> + * only ones with a feature report.
> + */
> + rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
> + return !list_empty(&rep_enum->report_list);
> +}
> +
> +static int steam_client_ll_parse(struct hid_device *hdev)
> +{
> + return 0;
Instead of returning 0 here, you should probably call
hid_parse_report() on the report descriptors from the parent node.
Some clients might want to have a look at them or might even rely on
them.
> +}
> +
> +static int steam_client_ll_start(struct hid_device *hdev)
> +{
> + return 0;
> +}
> +
> +static void steam_client_ll_stop(struct hid_device *hdev)
> +{
> +}
> +
> +static int steam_client_ll_open(struct hid_device *hdev)
> +{
> + struct steam_device *steam = hid_get_drvdata(hdev);
You probably want to check if steam is not null here. Given the
ordering of the initialization, you might have someone attempting to
open the hidraw node before steam is created.
> + int ret;
> +
> + ret = hid_hw_open(steam->hdev);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&steam->mutex);
> + steam->client_opened = true;
> + mutex_unlock(&steam->mutex);
> + return ret;
> +}
> +
> +static void steam_client_ll_close(struct hid_device *hdev)
> +{
> + struct steam_device *steam = hid_get_drvdata(hdev);
Same here, you might want to check on the validity of steam.
> +
> + hid_hw_close(steam->hdev);
> +
> + mutex_lock(&steam->mutex);
> + steam->client_opened = false;
> + if (lizard_mode != 2) {
> + if (steam->input_opened)
> + steam_set_lizard_mode(steam, false);
> + else
> + steam_set_lizard_mode(steam, lizard_mode);
> + }
> + mutex_unlock(&steam->mutex);
You should call hid_hw_close(steam->hdev); after sending the commands
and not before.
> +}
> +
> +static int steam_client_ll_raw_request(struct hid_device *hdev,
> + unsigned char reportnum, u8 *buf,
> + size_t count, unsigned char report_type,
> + int reqtype)
> +{
> + struct steam_device *steam = hid_get_drvdata(hdev);
> +
> + return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
> + report_type, reqtype);
> +}
I wish we could reuse directly the pointer in
hdev->ll_driver->raw_request to avoid adding an indirection.
OTOH, the raw_requests are not happening that often, so we should be good.
> +
> +static struct hid_ll_driver steam_client_ll_driver = {
> + .parse = steam_client_ll_parse,
> + .start = steam_client_ll_start,
> + .stop = steam_client_ll_stop,
> + .open = steam_client_ll_open,
> + .close = steam_client_ll_close,
> + .raw_request = steam_client_ll_raw_request,
> +};
> +
> +static int steam_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct steam_device *steam;
> + struct hid_device *client_hdev;
> + int ret;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev,
> + "%s:parse of hid interface failed\n", __func__);
> + return ret;
> + }
> +
> + /*
> + * The non-valve interfaces (mouse and keyboard emulation) and
> + * the client_hid are connected without changes.
> + */
> + if (hdev->group == HID_GROUP_STEAM ||
> + !steam_is_valve_interface(hdev)) {
> + return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> + }
To make thinks clearer, I would split these calls in 2:
- if (!steam_is_valve_interface(hdev)) return hid_hw_start(hdev,
HID_CONNECT_DEFAULT);
and
- if (hdev->group == HID_GROUP_STEAM) return hid_hw_start(hdev,
HID_CONNECT_HIDRAW);
Explicitly having HID_CONNECT_HIDRAW would also make it clearer you
are just exporting the hidraw interface here. It'll prevent any other
interface to mess your detection of the hidraw usage.
> +
> + /*
> + * With the real steam controller interface, do not connect hidraw.
> + * Instead, create the client_hid and connect that.
> + */
> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> + if (ret)
> + return ret;
this should likely be at the end of the probe, when you are done
allocating your data.
> +
> + client_hdev = hid_allocate_device();
> + if (IS_ERR(client_hdev)) {
> + ret = PTR_ERR(client_hdev);
> + goto client_hdev_fail;
> + }
> +
> + client_hdev->ll_driver = &steam_client_ll_driver;
> + client_hdev->dev.parent = hdev->dev.parent;
> + client_hdev->bus = hdev->bus;
> + client_hdev->vendor = hdev->vendor;
> + client_hdev->product = hdev->product;
> + strlcpy(client_hdev->name, hdev->name,
> + sizeof(client_hdev->name));
> + strlcpy(client_hdev->phys, hdev->phys,
> + sizeof(client_hdev->phys));
> + client_hdev->dev_rdesc = kmemdup(hdev->dev_rdesc,
> + hdev->dev_rsize, GFP_KERNEL);
> + client_hdev->dev_rsize = hdev->dev_rsize;
> + /*
> + * Since we use the same device info than the real interface to
> + * trick userspace, we will be calling steam_probe recursively.
> + * We need to recognize the client interface somehow.
> + */
> + client_hdev->group = HID_GROUP_STEAM;
I'd extract out the client_hdev initialization in its own function to
keep .probe() clean.
> +
> + ret = hid_add_device(client_hdev);
> + if (ret)
> + goto client_hdev_add_fail;
This should likely be called after initializing steam. It'll keep the
cleanup path cleaner and make sure all fields are properly initialized
before they are used.
> +
> + steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
> + if (!steam) {
> + ret = -ENOMEM;
> + goto steam_alloc_fail;
> + }
> +
> + steam->client_hdev = client_hdev;
> + hid_set_drvdata(client_hdev, steam);
> +
> + spin_lock_init(&steam->lock);
> + mutex_init(&steam->mutex);
> + steam->hdev = hdev;
> + hid_set_drvdata(hdev, steam);
> + steam->quirks = id->driver_data;
> + INIT_WORK(&steam->work_connect, steam_work_connect_cb);
I'd call hid_hw_start(hdev, ...) here, and then hid_add_device(client_hdev);
To have a better cleanup path, you porbably should allocate
client_hdev here too (before hid_add_device, of course)
> +
> + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> + ret = hid_hw_open(hdev);
> + if (ret) {
> + hid_err(hdev,
> + "%s:hid_hw_open for wireless\n",
> + __func__);
> + goto hid_hw_open_fail;
> + }
> + hid_info(hdev, "Steam wireless receiver connected");
> + steam_request_conn_status(steam);
> + } else {
> + ret = steam_register(steam);
> + if (ret) {
> + hid_err(hdev,
> + "%s:steam_register failed with error %d\n",
> + __func__, ret);
> + goto input_register_fail;
> + }
> + }
> + if (lizard_mode != 2)
> + steam_set_lizard_mode(steam, lizard_mode);
> +
> + return 0;
> +
> +input_register_fail:
> +hid_hw_open_fail:
> + cancel_work_sync(&steam->work_connect);
> + hid_set_drvdata(hdev, NULL);
> +steam_alloc_fail:
> + hid_hw_stop(client_hdev);
> +client_hdev_add_fail:
> + hid_destroy_device(client_hdev);
> +client_hdev_fail:
> + hid_err(hdev, "%s: failed with error %d\n",
> + __func__, ret);
> + return 0;
> +}
> +
> +static void steam_remove(struct hid_device *hdev)
> +{
> + struct steam_device *steam = hid_get_drvdata(hdev);
> +
> + if (hdev->group == HID_GROUP_STEAM)
Why don't you call hid_hw_stop() here instead of calling it later in
the parent device?
> + return;
> +
> + if (!steam) {
> + hid_hw_stop(hdev);
> + return;
> + }
> +
You should reorder these cleanup calls:
- you first call hid_destroy_device(), this will clean up properly
everything related to the hidraw node and should set client_opened to
false (just to be on the safe side, you might want to overwrite it
after to be sure not to forward events to the destoryed hid node).
- then you take care of the rest of the hid device:
- cancel_work_sync should happen before calling hid_hw_close() and
hid_hw_stop() on the hdev.
- steam_unregister(steam);
- if needed call hid_hw_close()
- hid_hw_stop()
> + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> + hid_info(hdev, "Steam wireless receiver disconnected");
> + hid_hw_close(hdev);
> + }
> + hid_hw_stop(hdev);
> + cancel_work_sync(&steam->work_connect);
> + hid_hw_stop(steam->client_hdev);
> + hid_destroy_device(steam->client_hdev);
> +
> +}
> +
> +static void steam_do_connect_event(struct steam_device *steam, bool connected)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&steam->lock, flags);
> + steam->connected = connected;
> + spin_unlock_irqrestore(&steam->lock, flags);
> +
> + if (schedule_work(&steam->work_connect) == 0)
> + dbg_hid("%s: connected=%d event already queued\n",
> + __func__, connected);
> +}
> +
> +/*
> + * The size for this message payload is 60.
> + * The known values are:
> + * (* values are not sent through wireless)
> + * (* accelerator/gyro is disabled by default)
> + * Offset| Type | Mapped to |Meaning
> + * -------+-------+-----------+--------------------------
> + * 4-7 | u32 | -- | sequence number
> + * 8-10 | 24bit | see below | buttons
> + * 11 | u8 | ABS_HAT2Y | left trigger
> + * 12 | u8 | ABS_HAT2X | right trigger
> + * 13-15 | -- | -- | always 0
> + * 16-17 | s16 | ABS_X/ABS_HAT0X | X value
> + * 18-19 | s16 | ABS_Y/ABS_HAT0Y | Y value
> + * 20-21 | s16 | ABS_RX | right-pad X value
> + * 22-23 | s16 | ABS_RY | right-pad Y value
> + * 24-25 | s16 | -- | * left trigger
> + * 26-27 | s16 | -- | * right trigger
> + * 28-29 | s16 | -- | * accelerometer X value
> + * 30-31 | s16 | -- | * accelerometer Y value
> + * 32-33 | s16 | -- | * accelerometer Z value
> + * 34-35 | s16 | -- | gyro X value
> + * 36-36 | s16 | -- | gyro Y value
> + * 38-39 | s16 | -- | gyro Z value
> + * 40-41 | s16 | -- | quaternion W value
> + * 42-43 | s16 | -- | quaternion X value
> + * 44-45 | s16 | -- | quaternion Y value
> + * 46-47 | s16 | -- | quaternion Z value
> + * 48-49 | -- | -- | always 0
> + * 50-51 | s16 | -- | * left trigger (uncalibrated)
> + * 52-53 | s16 | -- | * right trigger (uncalibrated)
> + * 54-55 | s16 | -- | * joystick X value (uncalibrated)
> + * 56-57 | s16 | -- | * joystick Y value (uncalibrated)
> + * 58-59 | s16 | -- | * left-pad X value
> + * 60-61 | s16 | -- | * left-pad Y value
> + * 62-63 | u16 | -- | * battery voltage
> + *
> + * The buttons are:
> + * Bit | Mapped to | Description
> + * ------+------------+--------------------------------
> + * 8.0 | BTN_TR2 | right trigger fully pressed
> + * 8.1 | BTN_TL2 | left trigger fully pressed
> + * 8.2 | BTN_TR | right shoulder
> + * 8.3 | BTN_TL | left shoulder
> + * 8.4 | BTN_Y | button Y
> + * 8.5 | BTN_B | button B
> + * 8.6 | BTN_X | button X
> + * 8.7 | BTN_A | button A
> + * 9.0 | BTN_DPAD_UP | lef-pad up
> + * 9.1 | BTN_DPAD_RIGHT | lef-pad right
> + * 9.2 | BTN_DPAD_LEFT | lef-pad left
> + * 9.3 | BTN_DPAD_DOWN | lef-pad down
> + * 9.4 | BTN_SELECT | menu left
> + * 9.5 | BTN_MODE | steam logo
> + * 9.6 | BTN_START | menu right
> + * 9.7 | BTN_GEAR_DOWN | left back lever
> + * 10.0 | BTN_GEAR_UP | right back lever
> + * 10.1 | -- | left-pad clicked
> + * 10.2 | BTN_THUMBR | right-pad clicked
> + * 10.3 | BTN_THUMB | left-pad touched (but see explanation below)
> + * 10.4 | BTN_THUMB2 | right-pad touched
> + * 10.5 | -- | unknown
> + * 10.6 | BTN_THUMBL | joystick clicked
> + * 10.7 | -- | lpad_and_joy
> + */
> +
> +static void steam_do_input_event(struct steam_device *steam,
> + struct input_dev *input, u8 *data)
> +{
> + /* 24 bits of buttons */
> + u8 b8, b9, b10;
> + bool lpad_touched, lpad_and_joy;
> +
> + b8 = data[8];
> + b9 = data[9];
> + b10 = data[10];
> +
> + input_report_abs(input, ABS_HAT2Y, data[11]);
> + input_report_abs(input, ABS_HAT2X, data[12]);
> +
> + /*
> + * These two bits tells how to interpret the values X and Y.
> + * lpad_and_joy tells that the joystick and the lpad are used at the
> + * same time.
> + * lpad_touched tells whether X/Y are to be read as lpad coord or
> + * joystick values.
> + * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> + */
> + lpad_touched = b10 & BIT(3);
> + lpad_and_joy = b10 & BIT(7);
> + input_report_abs(input, lpad_touched ? ABS_HAT0X : ABS_X,
> + (s16) le16_to_cpup((__le16 *)(data + 16)));
> + input_report_abs(input, lpad_touched ? ABS_HAT0Y : ABS_Y,
> + -(s16) le16_to_cpup((__le16 *)(data + 18)));
> + /* Check if joystick is centered */
> + if (lpad_touched && !lpad_and_joy) {
> + input_report_abs(input, ABS_X, 0);
> + input_report_abs(input, ABS_Y, 0);
> + }
> + /* Check if lpad is untouched */
> + if (!(lpad_touched || lpad_and_joy)) {
> + input_report_abs(input, ABS_HAT0X, 0);
> + input_report_abs(input, ABS_HAT0Y, 0);
> + }
> +
> + input_report_abs(input, ABS_RX,
> + (s16) le16_to_cpup((__le16 *)(data + 20)));
> + input_report_abs(input, ABS_RY,
> + -(s16) le16_to_cpup((__le16 *)(data + 22)));
> +
> + input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
> + input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
> + input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
> + input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
> + input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
> + input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
> + input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
> + input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
> + input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
> + input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
> + input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
> + input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
> + input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
> + input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
> + input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
> + input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
> + input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
> + input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
> + input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
> + input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
> + input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
> +
> + input_sync(input);
> +}
> +
> +static int steam_raw_event(struct hid_device *hdev,
> + struct hid_report *report, u8 *data,
> + int size)
> +{
> + struct steam_device *steam = hid_get_drvdata(hdev);
> + struct input_dev *input;
> +
> + if (!steam)
> + return 0;
> +
> + if (steam->client_opened)
> + hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
> + data, size, 0);
> + /*
> + * All messages are size=64, all values little-endian.
> + * The format is:
> + * Offset| Meaning
> + * -------+--------------------------------------------
> + * 0-1 | always 0x01, 0x00, maybe protocol version?
> + * 2 | type of message
> + * 3 | length of the real payload (not checked)
> + * 4-n | payload data, depends on the type
> + *
> + * There are these known types of message:
> + * 0x01: input data (60 bytes)
> + * 0x03: wireless connect/disconnect (1 byte)
> + * 0x04: battery status (11 bytes)
> + */
> +
> + if (size != 64 || data[0] != 1 || data[1] != 0)
> + return 0;
> +
> + switch (data[2]) {
> + case 0x01:
> + if (steam->client_opened)
> + return 0;
> + rcu_read_lock();
> + input = rcu_dereference(steam->input);
> + if (likely(input)) {
> + steam_do_input_event(steam, input, data);
> + } else {
> + dbg_hid("%s: input data without connect event\n",
> + __func__);
> + steam_do_connect_event(steam, true);
> + }
> + rcu_read_unlock();
> + break;
> + case 0x03:
> + /*
> + * The payload of this event is a single byte:
> + * 0x01: disconnected.
> + * 0x02: connected.
> + */
> + switch (data[4]) {
> + case 0x01:
> + steam_do_connect_event(steam, false);
> + break;
> + case 0x02:
> + steam_do_connect_event(steam, true);
> + break;
> + }
> + break;
> + case 0x04:
> + /* TODO: battery status */
> + break;
> + }
> + return 0;
> +}
> +
> +static const struct hid_device_id steam_controllers[] = {
> + { /* Wired Steam Controller */
> + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> + USB_DEVICE_ID_STEAM_CONTROLLER)
> + },
> + { /* Wireless Steam Controller */
> + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> + USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
> + .driver_data = STEAM_QUIRK_WIRELESS
> + },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(hid, steam_controllers);
> +
> +static struct hid_driver steam_controller_driver = {
> + .name = "hid-steam",
> + .id_table = steam_controllers,
> + .probe = steam_probe,
> + .remove = steam_remove,
> + .raw_event = steam_raw_event,
> +};
> +
> +module_hid_driver(steam_controller_driver);
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index d491027a7c22..5e5d76589954 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -364,6 +364,7 @@ struct hid_item {
> #define HID_GROUP_RMI 0x0100
> #define HID_GROUP_WACOM 0x0101
> #define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
> +#define HID_GROUP_STEAM 0x0103
>
> /*
> * HID protocol status
> --
> 2.16.2
>
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 0/2] hid-steam driver with user mode client dection
2018-03-26 8:12 ` [PATCH v7 0/2] hid-steam driver with user mode client dection Benjamin Tissoires
@ 2018-03-28 18:14 ` Rodrigo Rivas Costa
2018-03-28 21:44 ` Rodrigo Rivas Costa
0 siblings, 1 reply; 8+ messages in thread
From: Rodrigo Rivas Costa @ 2018-03-28 18:14 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
On Mon, Mar 26, 2018 at 10:12:19AM +0200, Benjamin Tissoires wrote:
> Hi Rodrigo,
>
> On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > This is a reroll of the Steam Controller driver.
> >
> > This time the client usage is detected by using exposing a custom hidraw
> > device (hid_ll_driver) to userland and forwarding that to the real one.
>
> That is actually more clever than what I had in mind.
> This way you reliably know if the hidraw node has been opened without
> touching hid-core.c or hidraw.c, well played :)
8-)
> > About how the lizard-mode/hidraw-client issue is handled, I have added a
> > module parameter (I don't know if that is the best option, but it helps me
> > illustrate different approaches to the problem):
> >
> > Independently of the lizard_mode parameter value:
>
> I find the values of the lizard_mode parameter hard to understand. And
> honestly, I do not think there is much a difference between 0 and 1.
> AFAICT, the difference between these two values is that in the first
> case you are disabling the lizard mode whether or not the joystick is
> in used, and in the latter, you disable it only when the joystick is
> in use. Does that gives any benefits to users?
Well, the rules I'm trying to apply are, in order of priority:
1. Never send a lizard command when hidraw is in use.
2. Disable the lizard mode when the input node is in use.
Now, the freedom is in what to do when the input node and hidraw are
both unused, hence the parameters. The two easy options are:
* lizard_mode=1: lizard mode is on, same experience as without
hid-steam, so it is the default.
* lizard_mode=0: lizard mode is off. I always use a mouse and
keyboard together with the controller, so I like this one.
But that leaves a marginal use case uncovered. What if I want to manage
the lizard mode using a user mode tool, such as my steamctrl from
github? This tool opens hidraw and sends a lizard_mode command (enable
or disable), but it does not matter, because as soon as hidraw is closed
hid-steam will reset the lizard mode to the value of the lizard_mode
parameter.
That is why I added lizard_mode=2: it instructs hid-steam not to send
any lizard mode command at all. You do not get any magic (I even break
rule 2 above) but you can use steamctrl freely.
Summing up:
* lizard_mode = 0: disabled.
* lizard_mode = 1: enabled (except when input is in use).
* lizard_mode = 2: no magic (use a user mode tool or expect weirdness).
I don't think anybody is actually using a user-mode tool for that so
maybe mode 2 can be discarded.
Note that Steam disables the lizard_mode when it starts and restores it
when it closes, unconditionally, so default option lizard_mode=1 will be
indistinguishable from hid-generic for the average user... except if
Steam crashes, then you will have lizard mode reenabled automatically,
which is nice.
The other two lizard_mode={0,1} I think they are quite useful, and I'd
like to keep them.
> I would think having a boolean "let the kernel handle the lizard mode
> with some magic inside" would be simpler to understand. You do not
> really want to support too many configurations and I think it would be
> wiser to say either disable the new mode or just enable it.
That would be the lizard_mode={0,1} I talked about? Both of them have
magic. The magicless option was the 2.
> Also, I think there will be races if a user changes the value of the
> parameter while running the system. You might want to add an
> additional patch that would trigger the mode change on module
> parameter change.
True, but the races should be easy to remove, with a simple local
variable. About the trigger, I've never seen a trigger on module
parameter change. Can you give me a hint or example of how to do that.
> As far as I can tell, I am happy with such hack. There are a few
> points I'd like to raise in the patch 1, but I'll inline my comments
> there.
I'll address them promptly.
Thanks.
Rodrigo
> Cheers,
> Benjamin
>
> >
> > 1. When a hidraw client is running:
> > a. I will not send any lizard-mode command to the device, so the client is
> > in full control.
> > b. The input device will not send any input message. However it will not
> > dissappear nor return any error message. Maybe it could return ENODEV if
> > they try to open the input device when hidraw client is running?
> >
> > If lizard_mode == 0 ('disabled'):
> > 2.0. When a hidraw client is not running, lizard_mode is disabled.
> >
> > If lizard_mode == 1, ('auto', the default):
> > 2.1. When a hidraw client is not running:
> > a. When the input device is not in use, lizard mode is enabled.
> > b. When the input device is in use, lizard mode is disabled.
> >
> > If lizard_mode == 2 ('usermode'):
> > 2.2. This driver does not send any lizard_mode-related command. So it is up
> > to user mode to configure it, with steamctrl or whatever.
> >
> > Note that when Steam Client opens it always disables lizard-mode (it creates
> > keyboard/mouse XTest inputs with the same function, though). And when it closed
> > (but not when it crashes, I think) it always re-enables the lizard mode.
> >
> > About the input buttons/axes mapping, as per Clément suggestion, I tried to
> > conform to Documentation/gamepad.rst, but with a few caveats and doubts:
> > * BTN_NORTH/BTN_WEST are alias of BTN_X/BTN_Y, but those buttons in this
> > controller have the labels changed (BTN_NORTH is actually Y). I don't know
> > the best option, so for now I'm mapping 'Y' to BTN_Y and 'X' to BTN_X.
> > * I'm mapping the lpad clicks to BTN_DPAD_{UP,RIGHT,DOWN,LEFT} but I'm not
> > sure if that is such a good idea: it cannot do diagonals, for example.
> > Maybe we could fake the whole dpad from the touch position?
> > * I'm mapping pressing the joystick to BTN_THUMBL and clicking the rpad to
> > BTN_THUMBR. Clicking the lpad is unmapped because that is used for the
> > dpad, depending on where it is clicked.
> > * Currently I'm mapping the lpad-touch to BTN_THUMB and rpad-touch to
> > BTN_THUMB2, but I don't know if that is so useful. lpad-touch will overlap
> > with any use of the dpad. And rpad-touch will overlap with rpad-click...
> >
> > Changes in v7:
> > * All the automatic lizard_mode stuff.
> > * Added the lizard_mode parameter.
> > * The patchset is reduced to 2 commits. The separation of the
> > steam_get_serial command no longer makes sense, since I need the
> > steam_send_cmd in the first commit to implement the lizard mode.
> > * Change the input mapping to conform to Documentation/gamepad.rst.
> >
> > (v6 was a RFC, it does not count).
> >
> > Changes in v5:
> > * Fix license SPDX to GPL-2.0+.
> > * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> >
> > Changes in v4:
> > * Add command to check the wireless connection status on probe, without
> > waiting for a message (thanks to Clément Vuchener for the tip).
> > * Removed the error code on redundant connection/disconnection messages. That
> > was harmless but polluted dmesg.
> > * Added buttons for touching the left-pad and right-pad.
> > * Fixed a misplaced #include from 2/4 to 1/4.
> >
> > Changes in v3:
> > * Use RCU to do the dynamic connec/disconnect of wireless devices.
> > * Remove entries in hid-quirks.c as they are no longer needed. This allows
> > this module to be blacklisted without side effects.
> > * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> > existing use cases (lizard mode). A user-space tool to do that is
> > linked.
> > * Fully separated axes for joystick and left-pad. As it happens.
> > * Add fuzz values for left/right pad axes, they are a little wiggly.
> >
> > Changes in v2:
> > * Remove references to USB. Now the interesting interfaces are selected by
> > looking for the ones with feature reports.
> > * Feature reports buffers are allocated with hid_alloc_report_buf().
> > * Feature report length is checked, to avoid overflows in case of
> > corrupt/malicius USB devices.
> > * Resolution added to the ABS axes.
> > * A lot of minor cleanups.
> >
> > Rodrigo Rivas Costa (2):
> > HID: add driver for Valve Steam Controller
> > HID: steam: add battery device.
> >
> > drivers/hid/Kconfig | 8 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 4 +
> > drivers/hid/hid-steam.c | 978 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/hid.h | 1 +
> > 5 files changed, 992 insertions(+)
> > create mode 100644 drivers/hid/hid-steam.c
> >
> > --
> > 2.16.2
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 1/2] HID: add driver for Valve Steam Controller
2018-03-26 9:20 ` Benjamin Tissoires
@ 2018-03-28 21:00 ` Rodrigo Rivas Costa
0 siblings, 0 replies; 8+ messages in thread
From: Rodrigo Rivas Costa @ 2018-03-28 21:00 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
On Mon, Mar 26, 2018 at 11:20:30AM +0200, Benjamin Tissoires wrote:
> Hi Rodrigo,
>
> few comments inlined.
>
> On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > There are two ways to connect the Steam Controller: directly to the USB
> > or with the USB wireless adapter. Both methods are similar, but the
> > wireless adapter can connect up to 4 devices at the same time.
> >
> > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > keyboard and a custom HID device.
> >
> > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > 4 custom HID devices, that will remain silent until a device is actually
> > connected.
> >
> > The custom HID device has a report descriptor with all vendor specific
> > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > Steam Client provices a software translation by using hidraw and a
> > creates a uinput virtual gamepad and XTest keyboard/mouse.
> >
> > This driver intercepts the hidraw usage, so it can get out of the way
> > when the Steam Client is in use.
> >
> > Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> > ---
> > drivers/hid/Kconfig | 8 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 4 +
> > drivers/hid/hid-steam.c | 840 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/hid.h | 1 +
> > 5 files changed, 854 insertions(+)
> > create mode 100644 drivers/hid/hid-steam.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 779c5ae47f36..de5f4849bfe4 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -811,6 +811,14 @@ config HID_SPEEDLINK
> > ---help---
> > Support for Speedlink Vicious and Divine Cezanne mouse.
> >
> > +config HID_STEAM
> > + tristate "Steam Controller support"
> > + depends on HID
> > + ---help---
> > + Say Y here if you have a Steam Controller if you want to use it
> > + without running the Steam Client. It supports both the wired and
> > + the wireless adaptor.
> > +
> > config HID_STEELSERIES
> > tristate "Steelseries SRW-S1 steering wheel support"
> > depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 235bd2a7b333..e146c257285a 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -94,6 +94,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
> > obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
> > obj-$(CONFIG_HID_SONY) += hid-sony.o
> > 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_GREENASIA) += hid-gaff.o
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index a0baa5ba5b84..3014991e5d4b 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -987,6 +987,10 @@
> > #define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
> > #define USB_DEVICE_ID_MTP_SITRONIX 0x5001
> >
> > +#define USB_VENDOR_ID_VALVE 0x28de
> > +#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
> > +#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
> > +
> > #define USB_VENDOR_ID_STEELSERIES 0x1038
> > #define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
> >
> > diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> > new file mode 100644
> > index 000000000000..3504d2e2d0e5
> > --- /dev/null
> > +++ b/drivers/hid/hid-steam.c
> > @@ -0,0 +1,840 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * HID driver for Valve Steam Controller
> > + *
> > + * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> > + *
> > + * Supports both the wired and wireless interfaces.
> > + *
> > + * This controller has a builtin emulation of mouse and keyboard: the right pad
> > + * can be used as a mouse, the shoulder buttons are mouse buttons, A and B
> > + * buttons are ENTER and ESCAPE, and so on. This is implemented as additional
> > + * HID interfaces.
> > + *
> > + * This is known as the "lizard mode", because apparently lizards like to use
> > + * the computer from the coach, without a proper mouse and keyboard.
> > + *
> > + * This driver will disable the lizard mode when the input device is opened
> > + * and re-enable it when the input device is closed, so as not to break user
> > + * mode behaviour. The lizard_mode parameter can be used to change that.
> > + *
> > + * There are a few user space applications (notably Steam Client) that use
> > + * the hidraw interface directly to create input devices (XTest, uinput...).
> > + * In order to avoid breaking them this driver creates a layered hidraw device,
> > + * so it can detect when the client is running and then:
> > + * - it will not send any command to the controller.
> > + * - this input device will be disabled, to avoid double input of the same
> > + * user action.
> > + *
> > + * For additional functions, such as changing the right-pad margin or switching
> > + * the led, you can use the user-space tool at:
> > + *
> > + * https://github.com/rodrigorc/steamctrl
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/mutex.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/delay.h>
> > +#include <linux/power_supply.h>
> > +#include "hid-ids.h"
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
> > +
> > +static int lizard_mode = 1;
> > +module_param(lizard_mode, int, 0644);
> > +MODULE_PARM_DESC(lizard_mode,
> > + "Mouse and keyboard emulation (0 = always disabled; "
> > + "1 (default): enabled when gamepad is not in use; "
> > + "2: let userspace decide)");
>
> As mentioned in 0/2, I think a boolean might be better. Probably
> rename the parameter to something else more explicit too (like
> 'control_lizard_mode').
> Also you might want to hook up to changes to this values so users can
> control it better. But this can be added in a later patch.
As I replied to your previous comment, I really like the options
enable_lizard_mode=true/false. If you add to the mix the no_magic,
then you have the current situation.
I admit that it can be confusing to the user, and that the no_magic may
not have a practical use case. So I'd stick to the enable/disable for
now, if you agree.
> > +
> > +#define STEAM_QUIRK_WIRELESS BIT(0)
> > +
> > +#define STEAM_SERIAL_LEN 10
> > +/* Touch pads are 40 mm in diameter and 65535 units */
> > +#define STEAM_PAD_RESOLUTION 1638
> > +/* Trigger runs are about 5 mm and 256 units */
> > +#define STEAM_TRIGGER_RESOLUTION 51
> > +/* Joystick runs are about 5 mm and 256 units */
> > +#define STEAM_JOYSTICK_RESOLUTION 51
> > +
> > +#define STEAM_PAD_FUZZ 256
> > +
> > +struct steam_device {
> > + spinlock_t lock;
> > + struct hid_device *hdev, *client_hdev;
> > + struct mutex mutex;
> > + bool client_opened, input_opened;
> > + struct input_dev __rcu *input;
> > + unsigned long quirks;
> > + struct work_struct work_connect;
> > + bool connected;
> > + char serial_no[STEAM_SERIAL_LEN + 1];
> > +};
> > +
> > +static int steam_recv_report(struct steam_device *steam,
> > + u8 *data, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
> > +
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /*
> > + * The report ID is always 0, so strip the first byte from the output.
> > + * hid_report_len() is not counting the report ID, so +1 to the length
> > + * or else we get a EOVERFLOW. We are safe from a buffer overflow
> > + * because hid_alloc_report_buf() allocates +7 bytes.
> > + */
> > + ret = hid_hw_raw_request(steam->hdev, 0x00,
> > + buf, hid_report_len(r) + 1,
> > + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > + if (ret > 0)
> > + memcpy(data, buf + 1, min(size, ret - 1));
> > + kfree(buf);
> > + return ret;
> > +}
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > + u8 *cmd, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + unsigned int retries = 10;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
> > +
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /* The report ID is always 0 */
> > + memcpy(buf + 1, cmd, size);
> > +
> > + /*
> > + * Sometimes the wireless controller fails with EPIPE
> > + * when sending a feature report.
> > + * Doing a HID_REQ_GET_REPORT and waiting for a while
> > + * seems to fix that.
> > + */
> > + do {
> > + ret = hid_hw_raw_request(steam->hdev, 0,
> > + buf, size + 1,
> > + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > + if (ret != -EPIPE)
> > + break;
> > + steam_recv_report(steam, NULL, 0);
> > + msleep(50);
> > + } while (--retries);
> > +
> > + kfree(buf);
> > + if (ret < 0)
> > + hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
> > + ret, size, cmd);
> > + return ret;
> > +}
> > +
> > +static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
> > +{
> > + return steam_send_report(steam, &cmd, 1);
> > +}
> > +
> > +static inline int steam_write_register(struct steam_device *steam,
> > + u8 reg, u16 value)
> > +{
> > + u8 cmd[] = {0x87, 0x03, reg, value & 0xFF, value >> 8};
> > +
> > + return steam_send_report(steam, cmd, sizeof(cmd));
> > +}
> > +
> > +static int steam_get_serial(struct steam_device *steam)
> > +{
> > + /*
> > + * Send: 0xae 0x15 0x01
> > + * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> > + */
> > + int ret;
> > + u8 cmd[] = {0xae, 0x15, 0x01};
> > + u8 reply[3 + STEAM_SERIAL_LEN + 1];
> > +
> > + ret = steam_send_report(steam, cmd, sizeof(cmd));
> > + if (ret < 0)
> > + return ret;
> > + ret = steam_recv_report(steam, reply, sizeof(reply));
> > + if (ret < 0)
> > + return ret;
> > + reply[3 + STEAM_SERIAL_LEN] = 0;
> > + strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
> > + return 0;
> > +}
> > +
> > +/*
> > + * This command requests the wireless adaptor to post an event
> > + * with the connection status. Useful if this driver is loaded when
> > + * the controller is already connected.
> > + */
> > +static inline int steam_request_conn_status(struct steam_device *steam)
> > +{
> > + return steam_send_report_byte(steam, 0xb4);
> > +}
> > +
> > +static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
> > +{
> > + if (enable) {
> > + /* enable esc, enter, cursors */
> > + steam_send_report_byte(steam, 0x85);
> > + /* enable mouse */
> > + steam_send_report_byte(steam, 0x8e);
> > + } else {
> > + /* disable esc, enter, cursor */
> > + steam_send_report_byte(steam, 0x81);
> > + /* disable mouse */
> > + steam_write_register(steam, 0x08, 0x07);
> > + }
> > +}
> > +
> > +static int steam_input_open(struct input_dev *dev)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = hid_hw_open(steam->hdev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->input_opened = true;
> > + if (!steam->client_opened && lizard_mode == 1)
> > + steam_set_lizard_mode(steam, false);
> > + mutex_unlock(&steam->mutex);
> > + return 0;
> > +}
> > +
> > +static void steam_input_close(struct input_dev *dev)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > +
> > + hid_hw_close(steam->hdev);
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->input_opened = false;
> > + if (!steam->client_opened && lizard_mode == 1)
> > + steam_set_lizard_mode(steam, true);
> > + mutex_unlock(&steam->mutex);
>
> hid_hw_close() should be called after setting the lizard mode.
Done.
> > +}
> > +
> > +static int steam_register(struct steam_device *steam)
> > +{
> > + struct hid_device *hdev = steam->hdev;
> > + struct input_dev *input;
> > + int ret;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > + if (input) {
> > + dbg_hid("%s: already connected\n", __func__);
> > + return 0;
> > + }
> > +
> > + ret = steam_get_serial(steam);
> > + if (ret)
> > + return ret;
> > +
> > + hid_info(hdev, "Steam Controller '%s' connected",
> > + steam->serial_no);
> > +
> > + input = input_allocate_device();
> > + if (!input)
> > + return -ENOMEM;
> > +
> > + input_set_drvdata(input, steam);
> > + input->dev.parent = &hdev->dev;
> > + input->open = steam_input_open;
> > + input->close = steam_input_close;
> > +
> > + input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
> > + "Wireless Steam Controller" :
> > + "Steam Controller";
> > + input->phys = hdev->phys;
> > + input->uniq = steam->serial_no;
> > + input->id.bustype = hdev->bus;
> > + input->id.vendor = hdev->vendor;
> > + input->id.product = hdev->product;
> > + input->id.version = hdev->version;
> > +
> > + input_set_capability(input, EV_KEY, BTN_TR2);
> > + input_set_capability(input, EV_KEY, BTN_TL2);
> > + input_set_capability(input, EV_KEY, BTN_TR);
> > + input_set_capability(input, EV_KEY, BTN_TL);
> > + input_set_capability(input, EV_KEY, BTN_Y);
> > + input_set_capability(input, EV_KEY, BTN_B);
> > + input_set_capability(input, EV_KEY, BTN_X);
> > + input_set_capability(input, EV_KEY, BTN_A);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_UP);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_RIGHT);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_LEFT);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_DOWN);
> > + input_set_capability(input, EV_KEY, BTN_SELECT);
> > + input_set_capability(input, EV_KEY, BTN_MODE);
> > + input_set_capability(input, EV_KEY, BTN_START);
> > + input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
> > + input_set_capability(input, EV_KEY, BTN_GEAR_UP);
> > + input_set_capability(input, EV_KEY, BTN_THUMBR);
> > + input_set_capability(input, EV_KEY, BTN_THUMBL);
> > + input_set_capability(input, EV_KEY, BTN_THUMB);
> > + input_set_capability(input, EV_KEY, BTN_THUMB2);
> > +
> > + input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_RX, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_RY, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_HAT0X, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_HAT0Y, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
> > + input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
> > + input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
> > +
> > + ret = input_register_device(input);
> > + if (ret)
> > + goto input_register_fail;
> > +
> > + rcu_assign_pointer(steam->input, input);
> > +
> > + return 0;
> > +
> > +input_register_fail:
> > + input_free_device(input);
> > + return ret;
> > +}
> > +
> > +static void steam_unregister(struct steam_device *steam)
> > +{
> > + struct input_dev *input;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > +
> > + if (input) {
> > + RCU_INIT_POINTER(steam->input, NULL);
> > + synchronize_rcu();
> > + hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> > + steam->serial_no);
> > + input_unregister_device(input);
> > + }
> > +}
> > +
> > +static void steam_work_connect_cb(struct work_struct *work)
> > +{
> > + struct steam_device *steam = container_of(work, struct steam_device,
> > + work_connect);
> > + unsigned long flags;
> > + bool connected;
> > + int ret;
> > +
> > + spin_lock_irqsave(&steam->lock, flags);
> > + connected = steam->connected;
> > + spin_unlock_irqrestore(&steam->lock, flags);
> > +
> > + if (connected) {
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(steam->hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + }
> > + } else {
> > + steam_unregister(steam);
> > + }
> > +}
> > +
> > +static bool steam_is_valve_interface(struct hid_device *hdev)
> > +{
> > + struct hid_report_enum *rep_enum;
> > +
> > + /*
> > + * The wired device creates 3 interfaces:
> > + * 0: emulated mouse.
> > + * 1: emulated keyboard.
> > + * 2: the real game pad.
> > + * The wireless device creates 5 interfaces:
> > + * 0: emulated keyboard.
> > + * 1-4: slots where up to 4 real game pads will be connected to.
> > + * We know which one is the real gamepad interface because they are the
> > + * only ones with a feature report.
> > + */
> > + rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
> > + return !list_empty(&rep_enum->report_list);
> > +}
> > +
> > +static int steam_client_ll_parse(struct hid_device *hdev)
> > +{
> > + return 0;
>
> Instead of returning 0 here, you should probably call
> hid_parse_report() on the report descriptors from the parent node.
> Some clients might want to have a look at them or might even rely on
> them.
Ah, but hid_parse_report() just copies the descriptor from the parent,
and I am already doing that from steam_probe(). I'll move the code to
here and change to call hid_parse_report().
> > +}
> > +
> > +static int steam_client_ll_start(struct hid_device *hdev)
> > +{
> > + return 0;
> > +}
> > +
> > +static void steam_client_ll_stop(struct hid_device *hdev)
> > +{
> > +}
> > +
> > +static int steam_client_ll_open(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
>
> You probably want to check if steam is not null here. Given the
> ordering of the initialization, you might have someone attempting to
> open the hidraw node before steam is created.
Tricky. Particularly since I've moved hid_parse_report() to
steam_client_ll_parse(), if steam is null there it all will break
apart. And parse is called from hid_add_device(), so I have to reorder
things a bit. Maybe calling hid_add_device() later.
> > + int ret;
> > +
> > + ret = hid_hw_open(steam->hdev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->client_opened = true;
> > + mutex_unlock(&steam->mutex);
> > + return ret;
> > +}
> > +
> > +static void steam_client_ll_close(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
>
> Same here, you might want to check on the validity of steam.
Now that I've reordered the probe, steam cannot be NULL.
> > +
> > + hid_hw_close(steam->hdev);
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->client_opened = false;
> > + if (lizard_mode != 2) {
> > + if (steam->input_opened)
> > + steam_set_lizard_mode(steam, false);
> > + else
> > + steam_set_lizard_mode(steam, lizard_mode);
> > + }
> > + mutex_unlock(&steam->mutex);
>
> You should call hid_hw_close(steam->hdev); after sending the commands
> and not before.
Done.
> > +}
> > +
> > +static int steam_client_ll_raw_request(struct hid_device *hdev,
> > + unsigned char reportnum, u8 *buf,
> > + size_t count, unsigned char report_type,
> > + int reqtype)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
> > + report_type, reqtype);
> > +}
>
> I wish we could reuse directly the pointer in
> hdev->ll_driver->raw_request to avoid adding an indirection.
> OTOH, the raw_requests are not happening that often, so we should be good.
hid_hw_raw_request() is an inline function that does exactly that, so I
would not worry about that.
However the call to hid_input_report() from steam_raw_event()... I wanted
to call client_hid->driver->raw_event() directly, but I didn't dare.
> > +static struct hid_ll_driver steam_client_ll_driver = {
> > + .parse = steam_client_ll_parse,
> > + .start = steam_client_ll_start,
> > + .stop = steam_client_ll_stop,
> > + .open = steam_client_ll_open,
> > + .close = steam_client_ll_close,
> > + .raw_request = steam_client_ll_raw_request,
> > +};
> > +
> > +static int steam_probe(struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct steam_device *steam;
> > + struct hid_device *client_hdev;
> > + int ret;
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:parse of hid interface failed\n", __func__);
> > + return ret;
> > + }
> > +
> > + /*
> > + * The non-valve interfaces (mouse and keyboard emulation) and
> > + * the client_hid are connected without changes.
> > + */
> > + if (hdev->group == HID_GROUP_STEAM ||
> > + !steam_is_valve_interface(hdev)) {
> > + return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > + }
>
> To make thinks clearer, I would split these calls in 2:
> - if (!steam_is_valve_interface(hdev)) return hid_hw_start(hdev,
> HID_CONNECT_DEFAULT);
> and
> - if (hdev->group == HID_GROUP_STEAM) return hid_hw_start(hdev,
> HID_CONNECT_HIDRAW);
>
> Explicitly having HID_CONNECT_HIDRAW would also make it clearer you
> are just exporting the hidraw interface here. It'll prevent any other
> interface to mess your detection of the hidraw usage.
Ok, done.
> > + /*
> > + * With the real steam controller interface, do not connect hidraw.
> > + * Instead, create the client_hid and connect that.
> > + */
> > + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> > + if (ret)
> > + return ret;
>
> this should likely be at the end of the probe, when you are done
> allocating your data.
Changed.
> > + client_hdev = hid_allocate_device();
> > + if (IS_ERR(client_hdev)) {
> > + ret = PTR_ERR(client_hdev);
> > + goto client_hdev_fail;
> > + }
> > +
> > + client_hdev->ll_driver = &steam_client_ll_driver;
> > + client_hdev->dev.parent = hdev->dev.parent;
> > + client_hdev->bus = hdev->bus;
> > + client_hdev->vendor = hdev->vendor;
> > + client_hdev->product = hdev->product;
> > + strlcpy(client_hdev->name, hdev->name,
> > + sizeof(client_hdev->name));
> > + strlcpy(client_hdev->phys, hdev->phys,
> > + sizeof(client_hdev->phys));
> > + client_hdev->dev_rdesc = kmemdup(hdev->dev_rdesc,
> > + hdev->dev_rsize, GFP_KERNEL);
> > + client_hdev->dev_rsize = hdev->dev_rsize;
> > + /*
> > + * Since we use the same device info than the real interface to
> > + * trick userspace, we will be calling steam_probe recursively.
> > + * We need to recognize the client interface somehow.
> > + */
> > + client_hdev->group = HID_GROUP_STEAM;
>
> I'd extract out the client_hdev initialization in its own function to
> keep .probe() clean.
Yeah, better. Done.
> > + ret = hid_add_device(client_hdev);
> > + if (ret)
> > + goto client_hdev_add_fail;
>
> This should likely be called after initializing steam. It'll keep the
> cleanup path cleaner and make sure all fields are properly initialized
> before they are used.
I've rewritten the probe so much that this does not apply anymore.
>
> > +
> > + steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
> > + if (!steam) {
> > + ret = -ENOMEM;
> > + goto steam_alloc_fail;
> > + }
> > +
> > + steam->client_hdev = client_hdev;
> > + hid_set_drvdata(client_hdev, steam);
> > +
> > + spin_lock_init(&steam->lock);
> > + mutex_init(&steam->mutex);
> > + steam->hdev = hdev;
> > + hid_set_drvdata(hdev, steam);
> > + steam->quirks = id->driver_data;
> > + INIT_WORK(&steam->work_connect, steam_work_connect_cb);
>
> I'd call hid_hw_start(hdev, ...) here, and then hid_add_device(client_hdev);
Ok.
> To have a better cleanup path, you porbably should allocate
> client_hdev here too (before hid_add_device, of course)
> > +
> > + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > + ret = hid_hw_open(hdev);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:hid_hw_open for wireless\n",
> > + __func__);
> > + goto hid_hw_open_fail;
> > + }
> > + hid_info(hdev, "Steam wireless receiver connected");
> > + steam_request_conn_status(steam);
> > + } else {
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + goto input_register_fail;
> > + }
> > + }
> > + if (lizard_mode != 2)
> > + steam_set_lizard_mode(steam, lizard_mode);
> > +
> > + return 0;
> > +
> > +input_register_fail:
> > +hid_hw_open_fail:
> > + cancel_work_sync(&steam->work_connect);
> > + hid_set_drvdata(hdev, NULL);
> > +steam_alloc_fail:
> > + hid_hw_stop(client_hdev);
> > +client_hdev_add_fail:
> > + hid_destroy_device(client_hdev);
> > +client_hdev_fail:
> > + hid_err(hdev, "%s: failed with error %d\n",
> > + __func__, ret);
> > + return 0;
> > +}
> > +
> > +static void steam_remove(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + if (hdev->group == HID_GROUP_STEAM)
>
> Why don't you call hid_hw_stop() here instead of calling it later in
> the parent device?
I tried to keep together these two lines:
hid_hw_stop(steam->client_hdev);
hid_destroy_device(steam->client_hdev);
but I guess that it is better to the first one here and the last one
there.
> > + return;
> > +
> > + if (!steam) {
> > + hid_hw_stop(hdev);
> > + return;
> > + }
> > +
>
> You should reorder these cleanup calls:
> - you first call hid_destroy_device(), this will clean up properly
> everything related to the hidraw node and should set client_opened to
> false (just to be on the safe side, you might want to overwrite it
> after to be sure not to forward events to the destoryed hid node).
> - then you take care of the rest of the hid device:
> - cancel_work_sync should happen before calling hid_hw_close() and
> hid_hw_stop() on the hdev.
> - steam_unregister(steam);
> - if needed call hid_hw_close()
> - hid_hw_stop()
That was a bit messy. Fixed.
That's all for now... I'll wait a couple of days more, in case I get any
more feedback and reroll.
Best regards.
Rodrigo
> > + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > + hid_info(hdev, "Steam wireless receiver disconnected");
> > + hid_hw_close(hdev);
> > + }
> > + hid_hw_stop(hdev);
> > + cancel_work_sync(&steam->work_connect);
> > + hid_hw_stop(steam->client_hdev);
> > + hid_destroy_device(steam->client_hdev);
> > +
> > +}
> > +
> > +static void steam_do_connect_event(struct steam_device *steam, bool connected)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&steam->lock, flags);
> > + steam->connected = connected;
> > + spin_unlock_irqrestore(&steam->lock, flags);
> > +
> > + if (schedule_work(&steam->work_connect) == 0)
> > + dbg_hid("%s: connected=%d event already queued\n",
> > + __func__, connected);
> > +}
> > +
> > +/*
> > + * The size for this message payload is 60.
> > + * The known values are:
> > + * (* values are not sent through wireless)
> > + * (* accelerator/gyro is disabled by default)
> > + * Offset| Type | Mapped to |Meaning
> > + * -------+-------+-----------+--------------------------
> > + * 4-7 | u32 | -- | sequence number
> > + * 8-10 | 24bit | see below | buttons
> > + * 11 | u8 | ABS_HAT2Y | left trigger
> > + * 12 | u8 | ABS_HAT2X | right trigger
> > + * 13-15 | -- | -- | always 0
> > + * 16-17 | s16 | ABS_X/ABS_HAT0X | X value
> > + * 18-19 | s16 | ABS_Y/ABS_HAT0Y | Y value
> > + * 20-21 | s16 | ABS_RX | right-pad X value
> > + * 22-23 | s16 | ABS_RY | right-pad Y value
> > + * 24-25 | s16 | -- | * left trigger
> > + * 26-27 | s16 | -- | * right trigger
> > + * 28-29 | s16 | -- | * accelerometer X value
> > + * 30-31 | s16 | -- | * accelerometer Y value
> > + * 32-33 | s16 | -- | * accelerometer Z value
> > + * 34-35 | s16 | -- | gyro X value
> > + * 36-36 | s16 | -- | gyro Y value
> > + * 38-39 | s16 | -- | gyro Z value
> > + * 40-41 | s16 | -- | quaternion W value
> > + * 42-43 | s16 | -- | quaternion X value
> > + * 44-45 | s16 | -- | quaternion Y value
> > + * 46-47 | s16 | -- | quaternion Z value
> > + * 48-49 | -- | -- | always 0
> > + * 50-51 | s16 | -- | * left trigger (uncalibrated)
> > + * 52-53 | s16 | -- | * right trigger (uncalibrated)
> > + * 54-55 | s16 | -- | * joystick X value (uncalibrated)
> > + * 56-57 | s16 | -- | * joystick Y value (uncalibrated)
> > + * 58-59 | s16 | -- | * left-pad X value
> > + * 60-61 | s16 | -- | * left-pad Y value
> > + * 62-63 | u16 | -- | * battery voltage
> > + *
> > + * The buttons are:
> > + * Bit | Mapped to | Description
> > + * ------+------------+--------------------------------
> > + * 8.0 | BTN_TR2 | right trigger fully pressed
> > + * 8.1 | BTN_TL2 | left trigger fully pressed
> > + * 8.2 | BTN_TR | right shoulder
> > + * 8.3 | BTN_TL | left shoulder
> > + * 8.4 | BTN_Y | button Y
> > + * 8.5 | BTN_B | button B
> > + * 8.6 | BTN_X | button X
> > + * 8.7 | BTN_A | button A
> > + * 9.0 | BTN_DPAD_UP | lef-pad up
> > + * 9.1 | BTN_DPAD_RIGHT | lef-pad right
> > + * 9.2 | BTN_DPAD_LEFT | lef-pad left
> > + * 9.3 | BTN_DPAD_DOWN | lef-pad down
> > + * 9.4 | BTN_SELECT | menu left
> > + * 9.5 | BTN_MODE | steam logo
> > + * 9.6 | BTN_START | menu right
> > + * 9.7 | BTN_GEAR_DOWN | left back lever
> > + * 10.0 | BTN_GEAR_UP | right back lever
> > + * 10.1 | -- | left-pad clicked
> > + * 10.2 | BTN_THUMBR | right-pad clicked
> > + * 10.3 | BTN_THUMB | left-pad touched (but see explanation below)
> > + * 10.4 | BTN_THUMB2 | right-pad touched
> > + * 10.5 | -- | unknown
> > + * 10.6 | BTN_THUMBL | joystick clicked
> > + * 10.7 | -- | lpad_and_joy
> > + */
> > +
> > +static void steam_do_input_event(struct steam_device *steam,
> > + struct input_dev *input, u8 *data)
> > +{
> > + /* 24 bits of buttons */
> > + u8 b8, b9, b10;
> > + bool lpad_touched, lpad_and_joy;
> > +
> > + b8 = data[8];
> > + b9 = data[9];
> > + b10 = data[10];
> > +
> > + input_report_abs(input, ABS_HAT2Y, data[11]);
> > + input_report_abs(input, ABS_HAT2X, data[12]);
> > +
> > + /*
> > + * These two bits tells how to interpret the values X and Y.
> > + * lpad_and_joy tells that the joystick and the lpad are used at the
> > + * same time.
> > + * lpad_touched tells whether X/Y are to be read as lpad coord or
> > + * joystick values.
> > + * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> > + */
> > + lpad_touched = b10 & BIT(3);
> > + lpad_and_joy = b10 & BIT(7);
> > + input_report_abs(input, lpad_touched ? ABS_HAT0X : ABS_X,
> > + (s16) le16_to_cpup((__le16 *)(data + 16)));
> > + input_report_abs(input, lpad_touched ? ABS_HAT0Y : ABS_Y,
> > + -(s16) le16_to_cpup((__le16 *)(data + 18)));
> > + /* Check if joystick is centered */
> > + if (lpad_touched && !lpad_and_joy) {
> > + input_report_abs(input, ABS_X, 0);
> > + input_report_abs(input, ABS_Y, 0);
> > + }
> > + /* Check if lpad is untouched */
> > + if (!(lpad_touched || lpad_and_joy)) {
> > + input_report_abs(input, ABS_HAT0X, 0);
> > + input_report_abs(input, ABS_HAT0Y, 0);
> > + }
> > +
> > + input_report_abs(input, ABS_RX,
> > + (s16) le16_to_cpup((__le16 *)(data + 20)));
> > + input_report_abs(input, ABS_RY,
> > + -(s16) le16_to_cpup((__le16 *)(data + 22)));
> > +
> > + input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
> > + input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
> > + input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
> > + input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
> > + input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
> > + input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
> > + input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
> > + input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
> > + input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
> > + input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
> > + input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
> > + input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
> > +
> > + input_sync(input);
> > +}
> > +
> > +static int steam_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data,
> > + int size)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > + struct input_dev *input;
> > +
> > + if (!steam)
> > + return 0;
> > +
> > + if (steam->client_opened)
> > + hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
> > + data, size, 0);
> > + /*
> > + * All messages are size=64, all values little-endian.
> > + * The format is:
> > + * Offset| Meaning
> > + * -------+--------------------------------------------
> > + * 0-1 | always 0x01, 0x00, maybe protocol version?
> > + * 2 | type of message
> > + * 3 | length of the real payload (not checked)
> > + * 4-n | payload data, depends on the type
> > + *
> > + * There are these known types of message:
> > + * 0x01: input data (60 bytes)
> > + * 0x03: wireless connect/disconnect (1 byte)
> > + * 0x04: battery status (11 bytes)
> > + */
> > +
> > + if (size != 64 || data[0] != 1 || data[1] != 0)
> > + return 0;
> > +
> > + switch (data[2]) {
> > + case 0x01:
> > + if (steam->client_opened)
> > + return 0;
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + if (likely(input)) {
> > + steam_do_input_event(steam, input, data);
> > + } else {
> > + dbg_hid("%s: input data without connect event\n",
> > + __func__);
> > + steam_do_connect_event(steam, true);
> > + }
> > + rcu_read_unlock();
> > + break;
> > + case 0x03:
> > + /*
> > + * The payload of this event is a single byte:
> > + * 0x01: disconnected.
> > + * 0x02: connected.
> > + */
> > + switch (data[4]) {
> > + case 0x01:
> > + steam_do_connect_event(steam, false);
> > + break;
> > + case 0x02:
> > + steam_do_connect_event(steam, true);
> > + break;
> > + }
> > + break;
> > + case 0x04:
> > + /* TODO: battery status */
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > +static const struct hid_device_id steam_controllers[] = {
> > + { /* Wired Steam Controller */
> > + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> > + USB_DEVICE_ID_STEAM_CONTROLLER)
> > + },
> > + { /* Wireless Steam Controller */
> > + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> > + USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
> > + .driver_data = STEAM_QUIRK_WIRELESS
> > + },
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, steam_controllers);
> > +
> > +static struct hid_driver steam_controller_driver = {
> > + .name = "hid-steam",
> > + .id_table = steam_controllers,
> > + .probe = steam_probe,
> > + .remove = steam_remove,
> > + .raw_event = steam_raw_event,
> > +};
> > +
> > +module_hid_driver(steam_controller_driver);
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index d491027a7c22..5e5d76589954 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -364,6 +364,7 @@ struct hid_item {
> > #define HID_GROUP_RMI 0x0100
> > #define HID_GROUP_WACOM 0x0101
> > #define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
> > +#define HID_GROUP_STEAM 0x0103
> >
> > /*
> > * HID protocol status
> > --
> > 2.16.2
> >
>
> Cheers,
> Benjamin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v7 0/2] hid-steam driver with user mode client dection
2018-03-28 18:14 ` Rodrigo Rivas Costa
@ 2018-03-28 21:44 ` Rodrigo Rivas Costa
0 siblings, 0 replies; 8+ messages in thread
From: Rodrigo Rivas Costa @ 2018-03-28 21:44 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
On Wed, Mar 28, 2018 at 08:14:48PM +0200, Rodrigo Rivas Costa wrote:
> On Mon, Mar 26, 2018 at 10:12:19AM +0200, Benjamin Tissoires wrote:
> > Also, I think there will be races if a user changes the value of the
> > parameter while running the system. You might want to add an
> > additional patch that would trigger the mode change on module
> > parameter change.
>
> True, but the races should be easy to remove, with a simple local
> variable. About the trigger, I've never seen a trigger on module
> parameter change. Can you give me a hint or example of how to do that?
Never mind, I've found it: module_param_cb().
Moreover, I'll have to keep a global list of steam_devices, just in case
the parameter is changed...
Regards.
Rodrigo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-28 21:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-25 16:07 [PATCH v7 0/2] hid-steam driver with user mode client dection Rodrigo Rivas Costa
2018-03-25 16:07 ` [PATCH v7 1/2] HID: add driver for Valve Steam Controller Rodrigo Rivas Costa
2018-03-26 9:20 ` Benjamin Tissoires
2018-03-28 21:00 ` Rodrigo Rivas Costa
2018-03-25 16:07 ` [PATCH v7 2/2] HID: steam: add battery device Rodrigo Rivas Costa
2018-03-26 8:12 ` [PATCH v7 0/2] hid-steam driver with user mode client dection Benjamin Tissoires
2018-03-28 18:14 ` Rodrigo Rivas Costa
2018-03-28 21:44 ` Rodrigo Rivas Costa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.