[PATCH v2 0/3] new driver for Valve Steam Controller

[PATCH v2 0/3] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

This patchset implements a driver for Valve Steam Controller, based on a
reverse analysis by myself.

Notable changes from patchset v1:
 * 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 (3):
  HID: add driver for Valve Steam Controller
  HID: steam: add serial number information.
  HID: steam: add battery device.

 drivers/hid/Kconfig      |   8 +
 drivers/hid/Makefile     |   1 +
 drivers/hid/hid-ids.h    |   4 +
 drivers/hid/hid-quirks.c |   4 +
 drivers/hid/hid-steam.c  | 703 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 720 insertions(+)
 create mode 100644 drivers/hid/hid-steam.c

-- 
2.16.1

[PATCH v2 1/3] HID: add driver for Valve Steam Controller

[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 direct USB access
and a creates a uinput virtual gamepad.

This driver was reverse engineered to provide direct kernel support in
case you cannot, or do not want to, use Valve Steam Client. It disables
the virtual keyboard and mouse, as they are not so useful when you have
a working gamepad.

Working: buttons, axes, pads, wireless connect/disconnect.

TO-DO: Battery, force-feedback, accelerometer/gyro, led, beeper...

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-quirks.c |   4 +
 drivers/hid/hid-steam.c  | 478 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 495 insertions(+)
 create mode 100644 drivers/hid/hid-steam.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6e80fbf04e03 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -823,6 +823,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 eb13b9e92d85..60a8abf84682 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -95,6 +95,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 43ddcdfbd0da..be31a3c20818 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -988,6 +988,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-quirks.c b/drivers/hid/hid-quirks.c
index 5f6035a5ce36..72ac972dc00b 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -629,6 +629,10 @@ static const struct hid_device_id hid_have_special_driver[] = {
 #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS, USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
 #endif
+#if IS_ENABLED(CONFIG_HID_STEAM)
+	{ HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
+#endif
 #if IS_ENABLED(CONFIG_HID_STEELSERIES)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, USB_DEVICE_ID_STEELSERIES_SRWS1) },
 #endif
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
new file mode 100644
index 000000000000..7b2f16b7bb49
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,478 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for Valve Steam Controller
+ *
+ * Supports both the wired and wireless interfaces.
+ *
+ * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include "hid-ids.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
+
+#define STEAM_QUIRK_WIRELESS		BIT(0)
+
+/* 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
+
+struct steam_device {
+	spinlock_t lock;
+	struct hid_device *hdev;
+	struct input_dev *input;
+	unsigned long quirks;
+	struct work_struct work_connect;
+	bool connected;
+};
+
+static int steam_input_open(struct input_dev *dev)
+{
+	struct steam_device *steam = input_get_drvdata(dev);
+
+	return hid_hw_open(steam->hdev);
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+	struct steam_device *steam = input_get_drvdata(dev);
+
+	hid_hw_close(steam->hdev);
+}
+
+static int steam_register(struct steam_device *steam)
+{
+	struct hid_device *hdev = steam->hdev;
+	struct input_dev *input;
+	int ret;
+
+	hid_info(hdev, "Steam Controller connected");
+
+	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 = hdev->uniq;
+	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_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_abs_params(input, ABS_Z, 0, 255, 0, 0);
+	input_set_abs_params(input, ABS_RZ, 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, 0, 0);
+	input_set_abs_params(input, ABS_RY, -32767, 32767, 0, 0);
+	input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0);
+	input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0);
+	input_abs_set_res(input, ABS_X, STEAM_PAD_RESOLUTION);
+	input_abs_set_res(input, ABS_Y, STEAM_PAD_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_Z, STEAM_TRIGGER_RESOLUTION);
+	input_abs_set_res(input, ABS_RZ, STEAM_TRIGGER_RESOLUTION);
+
+	ret = input_register_device(input);
+	if (ret)
+		goto input_register_fail;
+
+	steam->input = input;
+
+	return 0;
+
+input_register_fail:
+	input_free_device(input);
+	return ret;
+}
+
+static void steam_unregister(struct steam_device *steam)
+{
+	if (steam->input) {
+		hid_info(steam->hdev, "Steam Controller disconnected");
+		input_unregister_device(steam->input);
+		steam->input = NULL;
+	}
+}
+
+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) {
+		if (steam->input) {
+			dbg_hid("%s: already connected\n", __func__);
+			return;
+		}
+		ret = steam_register(steam);
+		if (ret) {
+			hid_err(steam->hdev,
+				"%s:steam_register failed with error %d\n",
+				__func__, ret);
+			return;
+		}
+	} else {
+		steam_unregister(steam);
+	}
+}
+
+static bool steam_is_valve_interface(struct hid_device *hdev)
+{
+	struct hid_report_enum *rep_enum;
+	struct hid_report *hreport;
+
+	/*
+	 * 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];
+	list_for_each_entry(hreport, &rep_enum->report_list, list) {
+		/* should we check hreport->id == 0? */
+		return true;
+	}
+	return false;
+}
+
+static int steam_probe(struct hid_device *hdev,
+				const struct hid_device_id *id)
+{
+	struct steam_device *steam;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev,
+			"%s:parse of hid interface failed\n", __func__);
+		return ret;
+	}
+
+	/*
+	 * Since we have a proper gamepad now, we can ignore the virtual
+	 * mouse and keyboard.
+	 */
+	if (!steam_is_valve_interface(hdev))
+		return -ENODEV;
+
+	steam = devm_kzalloc(&hdev->dev,
+			sizeof(struct steam_device), GFP_KERNEL);
+	if (!steam)
+		return -ENOMEM;
+
+	spin_lock_init(&steam->lock);
+	steam->hdev = hdev;
+	hid_set_drvdata(hdev, steam);
+	steam->quirks = id->driver_data;
+	INIT_WORK(&steam->work_connect, steam_work_connect_cb);
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret) {
+		hid_err(hdev,
+			"%s:hid_hw_start failed with error %d\n",
+			__func__, ret);
+		goto hid_hw_start_fail;
+	}
+
+	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");
+	} else {
+		ret = steam_register(steam);
+		if (ret) {
+			hid_err(hdev,
+				"%s:steam_register failed with error %d\n",
+				__func__, ret);
+			goto input_register_fail;
+		}
+	}
+
+	return 0;
+
+input_register_fail:
+hid_hw_open_fail:
+	hid_hw_stop(hdev);
+hid_hw_start_fail:
+	cancel_work_sync(&steam->work_connect);
+	hid_set_drvdata(hdev, NULL);
+	return ret;
+}
+
+static void steam_remove(struct hid_device *hdev)
+{
+	struct steam_device *steam = hid_get_drvdata(hdev);
+
+	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);
+	steam_unregister(steam);
+	hid_set_drvdata(hdev, NULL);
+}
+
+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_Z     | left trigger
+ *  12    | u8    | ABS_RZ    | right trigger
+ *  13-15 | --    | --        | always 0
+ *  16-17 | s16   | ABS_X     | X value
+ *  18-19 | s16   | ABS_Y     | 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  | -ABS_HAT0Y | lef-pad up
+ *  9.1  | +ABS_HAT0X | lef-pad right
+ *  9.2  | -ABS_HAT0X | lef-pad left
+ *  9.3  | +ABS_HAT0Y | 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  | --         | left-pad touched
+ * 10.4  | --         | 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, u8 *data)
+{
+	struct input_dev *input = steam->input;
+
+	/* 24 bits of buttons */
+	u8 b8, b9, b10;
+
+	/*
+	 * If we get input events from the wireless without a 'connected'
+	 * event, just connect it now.
+	 * This can happen if we bind the HID device with the controller
+	 * already paired.
+	 */
+	if (unlikely(!input)) {
+		dbg_hid("%s: input data without connect event\n", __func__);
+		steam_do_connect_event(steam, true);
+		return;
+	}
+
+	input_report_abs(input, ABS_Z, data[11]);
+	input_report_abs(input, ABS_RZ, data[12]);
+
+	input_report_abs(input, ABS_X,
+			(s16) le16_to_cpup((__le16 *)(data + 16)));
+	input_report_abs(input, ABS_Y,
+			-(s16) le16_to_cpup((__le16 *)(data + 18)));
+	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)));
+
+	b8 = data[8];
+	b9 = data[9];
+	b10 = data[10];
+
+	input_event(input, EV_KEY, BTN_TR2, !!(b8 & 0x01));
+	input_event(input, EV_KEY, BTN_TL2, !!(b8 & 0x02));
+	input_event(input, EV_KEY, BTN_TR, !!(b8 & 0x04));
+	input_event(input, EV_KEY, BTN_TL, !!(b8 & 0x08));
+	input_event(input, EV_KEY, BTN_Y, !!(b8 & 0x10));
+	input_event(input, EV_KEY, BTN_B, !!(b8 & 0x20));
+	input_event(input, EV_KEY, BTN_X, !!(b8 & 0x40));
+	input_event(input, EV_KEY, BTN_A, !!(b8 & 0x80));
+	input_event(input, EV_KEY, BTN_SELECT, !!(b9 & 0x10));
+	input_event(input, EV_KEY, BTN_MODE, !!(b9 & 0x20));
+	input_event(input, EV_KEY, BTN_START, !!(b9 & 0x40));
+	input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & 0x80));
+	input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & 0x01));
+	input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & 0x04));
+	input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & 0x40));
+
+	input_report_abs(input, ABS_HAT0X,
+			!!(b9 & 0x02) - !!(b9 & 0x04));
+	input_report_abs(input, ABS_HAT0Y,
+			!!(b9 & 0x08) - !!(b9 & 0x01));
+
+	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);
+
+	/*
+	 * 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:
+		steam_do_input_event(steam, data);
+		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);
-- 
2.16.1

[PATCH v2 2/3] HID: steam: add serial number information

[Rodrigo Rivas Costa]

This device has a feature report to send and receive commands.
Use it to get the serial number and set the device's uniq value.

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
 drivers/hid/hid-steam.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 7b2f16b7bb49..c9d0f909c8a0 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -12,6 +12,8 @@
 #include <linux/hid.h>
 #include <linux/module.h>
 #include <linux/workqueue.h>
+#include <linux/delay.h>
+#include <linux/power_supply.h>
 #include "hid-ids.h"
 
 MODULE_LICENSE("GPL");
@@ -31,8 +33,99 @@ struct steam_device {
 	unsigned long quirks;
 	struct work_struct work_connect;
 	bool connected;
+	char serial_no[11];
 };
 
+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;
+	int retry;
+	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.
+	 */
+	for (retry = 0; retry < 10; ++retry) {
+		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);
+	}
+	kfree(buf);
+	if (ret < 0)
+		hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
+				ret, size, cmd);
+	return ret;
+}
+
+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[14];
+
+	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[13] = 0;
+	strcpy(steam->serial_no, reply + 3);
+	return 0;
+}
+
 static int steam_input_open(struct input_dev *dev)
 {
 	struct steam_device *steam = input_get_drvdata(dev);
@@ -53,7 +146,12 @@ static int steam_register(struct steam_device *steam)
 	struct input_dev *input;
 	int ret;
 
-	hid_info(hdev, "Steam Controller connected");
+	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)
@@ -68,7 +166,7 @@ static int steam_register(struct steam_device *steam)
 		"Wireless Steam Controller" :
 		"Steam Controller";
 	input->phys = hdev->phys;
-	input->uniq = hdev->uniq;
+	input->uniq = steam->serial_no;
 	input->id.bustype = hdev->bus;
 	input->id.vendor = hdev->vendor;
 	input->id.product = hdev->product;
@@ -121,7 +219,8 @@ static int steam_register(struct steam_device *steam)
 static void steam_unregister(struct steam_device *steam)
 {
 	if (steam->input) {
-		hid_info(steam->hdev, "Steam Controller disconnected");
+		hid_info(steam->hdev, "Steam Controller '%s' disconnected",
+				steam->serial_no);
 		input_unregister_device(steam->input);
 		steam->input = NULL;
 	}
-- 
2.16.1

[PATCH v2 3/3] HID: steam: add battery device

[Rodrigo Rivas Costa]

The wireless Steam Controller is battery operated, so add the battery
device and power information.

Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
 drivers/hid/hid-steam.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index c9d0f909c8a0..d6926e86d664 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -34,6 +34,10 @@ struct steam_device {
 	struct work_struct work_connect;
 	bool connected;
 	char serial_no[11];
+	struct power_supply_desc battery_desc;
+	struct power_supply *battery;
+	u8 battery_charge;
+	u16 voltage;
 };
 
 static int steam_recv_report(struct steam_device *steam,
@@ -140,6 +144,85 @@ static void steam_input_close(struct input_dev *dev)
 	hid_hw_close(steam->hdev);
 }
 
+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;
+	}
+	steam->battery = battery;
+	power_supply_powers(steam->battery, &steam->hdev->dev);
+	return 0;
+}
+
 static int steam_register(struct steam_device *steam)
 {
 	struct hid_device *hdev = steam->hdev;
@@ -209,6 +292,10 @@ static int steam_register(struct steam_device *steam)
 
 	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:
@@ -218,6 +305,10 @@ static int steam_register(struct steam_device *steam)
 
 static void steam_unregister(struct steam_device *steam)
 {
+	if (steam->battery) {
+		power_supply_unregister(steam->battery);
+		steam->battery = NULL;
+	}
 	if (steam->input) {
 		hid_info(steam->hdev, "Steam Controller '%s' disconnected",
 				steam->serial_no);
@@ -500,6 +591,40 @@ static void steam_do_input_event(struct steam_device *steam, u8 *data)
 	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, u8 *data)
+{
+	unsigned long flags;
+	s16 volts = le16_to_cpup((__le16 *)(data + 12));
+	u8 batt = data[14];
+
+	if (unlikely(!steam->input)) {
+		dbg_hid("%s: battery data without connect event\n", __func__);
+		steam_do_connect_event(steam, true);
+		return;
+	}
+
+	/* Creating the battery may have failed */
+	if (unlikely(!steam->battery))
+		return;
+
+	spin_lock_irqsave(&steam->lock, flags);
+	steam->voltage = volts;
+	steam->battery_charge = batt;
+	spin_unlock_irqrestore(&steam->lock, flags);
+
+	power_supply_changed(steam->battery);
+}
+
 static int steam_raw_event(struct hid_device *hdev,
 			struct hid_report *report, u8 *data,
 			int size)
@@ -545,7 +670,8 @@ static int steam_raw_event(struct hid_device *hdev,
 		}
 		break;
 	case 0x04:
-		/* TODO battery status */
+		if (steam->quirks & STEAM_QUIRK_WIRELESS)
+			steam_do_battery_event(steam, data);
 		break;
 	}
 	return 0;
-- 
2.16.1

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Pierre-Loup A. Griffais]

Hi Rodrigo,

Thanks for working on that! I have a few questions and remarks.

For the reverse-engineering part, there's a lot of existing reference in 
existing (user-space) drivers for the controllers like sc-controller, 
but feel free to reach out if you have any questions. It's overall 
pretty simple and there's nothing secret about how it functions; there 
are some quirks, however. Nothing secret about it, but also no 
documentation, so might as well be... Have you tried deflecting the 
analog stick while touching the left trackpad? You'll most likely need 
special handling there. How are you planning to expose 
enabling/disabling auxiliary data like gyro over wireless?

Will this driver being loaded affect functionality of existing 
applications that talk to it through HID directly, like Steam or 
sc-controller? Will they be able to keep getting the same HID data they 
do today? If so, the extent of the work needed to support it in Steam 
might just be to ignore the controller device it's exposing, since Steam 
will expose that itself through its own means.

Thanks,
  - Pierre-Loup

On 02/20/2018 11:33 AM, Rodrigo Rivas Costa wrote:
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
> 
> Notable changes from patchset v1:
>   * 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 (3):
>    HID: add driver for Valve Steam Controller
>    HID: steam: add serial number information.
>    HID: steam: add battery device.
> 
>   drivers/hid/Kconfig      |   8 +
>   drivers/hid/Makefile     |   1 +
>   drivers/hid/hid-ids.h    |   4 +
>   drivers/hid/hid-quirks.c |   4 +
>   drivers/hid/hid-steam.c  | 703 +++++++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 720 insertions(+)
>   create mode 100644 drivers/hid/hid-steam.c
> 

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
> Hi Rodrigo,
> 
> Thanks for working on that! I have a few questions and remarks.
> 
> For the reverse-engineering part, there's a lot of existing reference in
> existing (user-space) drivers for the controllers like sc-controller, but
> feel free to reach out if you have any questions. It's overall pretty simple
> and there's nothing secret about how it functions; there are some quirks,
> however. Nothing secret about it, but also no documentation, so might as
> well be... Have you tried deflecting the analog stick while touching the
> left trackpad? You'll most likely need special handling there. How are you
> planning to expose enabling/disabling auxiliary data like gyro over
> wireless?

Yeah, I look for information about a year when I started on this. I read
Ynsta's user-mode driver [1] (I didn't find sc-controller), but I found
that a bit incomplete, so I wrote my own named `inputmap` [2]. 

About the left trackpad/joystick, currently I'm not treating them
different. I'm out of ABS axes, and anyway, it is not likely that the
left pad and the joystick be used at the same time (I only have one left
thumb). Nevertheless, if we really want to make them apart, we can use
bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
details in [2], but I'm not currently doing that in this driver.

About the gyroscope/acceleration/quaternion, you know the issue
that the wireless gives gyro plus quat but no accel, while the wired
gives all three. My general idea is to create an additional input device
with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
the wireless gives no accel, maybe there is some command to enable it?

Also, ideally, we could expose the quat. data directly to userspace, but
I see no clear way to do that. Maybe defining INPUT_PROP_QUATERNION? I
also thought of computing yaw/pitch/roll from the quat. and exposing
those, but doing that requires atan2() and sin() (16-bit precision), and
that would be tricky. Any thoughts on that?

> Will this driver being loaded affect functionality of existing applications
> that talk to it through HID directly, like Steam or sc-controller? Will they
> be able to keep getting the same HID data they do today? If so, the extent
> of the work needed to support it in Steam might just be to ignore the
> controller device it's exposing, since Steam will expose that itself through
> its own means.

As it is, this patchset hould be pretty safe. The only command sent to
the controller is the get-serial-number, and that seems to do no harm to
Steam Client. Other than that, it only reads. Moreover, the hidraw
device is still created, so user-mode driver should keep working
(but AFAIK, Steam Client uses direct USB access, not hidraw).

Curiously, Steam Client does not show the new native Steam Controller
gamepad. That is, if I connect the Steam controller with hid-steam.ko
and an Acme HID gamepad, then in Steam Client controller settings I
see only two controllers, the managed Steam Controller (not HID) and the
Acme HID gamepad. I reckon that Steam Client recognizes that the
new HID device is a Steam Controller and ignores it in favor of its own
managed controller.

Regards.
Rodrigo

[1]: https://github.com/ynsta/steamcontroller
[2]: https://github.com/rodrigorc/inputmap

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Pierre-Loup A. Griffais]

On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
>> Hi Rodrigo,
>>
>> Thanks for working on that! I have a few questions and remarks.
>>
>> For the reverse-engineering part, there's a lot of existing reference in
>> existing (user-space) drivers for the controllers like sc-controller, but
>> feel free to reach out if you have any questions. It's overall pretty simple
>> and there's nothing secret about how it functions; there are some quirks,
>> however. Nothing secret about it, but also no documentation, so might as
>> well be... Have you tried deflecting the analog stick while touching the
>> left trackpad? You'll most likely need special handling there. How are you
>> planning to expose enabling/disabling auxiliary data like gyro over
>> wireless?
> 
> Yeah, I look for information about a year when I started on this. I read
> Ynsta's user-mode driver [1] (I didn't find sc-controller), but I found
> that a bit incomplete, so I wrote my own named `inputmap` [2].
> 
> About the left trackpad/joystick, currently I'm not treating them
> different. I'm out of ABS axes, and anyway, it is not likely that the
> left pad and the joystick be used at the same time (I only have one left
> thumb). Nevertheless, if we really want to make them apart, we can use
> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
> details in [2], but I'm not currently doing that in this driver.

I didn't necessarily mean exposing it, but in the event a user is using 
both at the same time (it happens, using claw grip with some games is 
necessary to use the D-pad while deflecting the stick), then you'll most 
likely run into issues unless you demux the inbound data, since we were 
also out of left analog fields and mux them together if used at the same 
time. Not sure if you're already handling that case, but not doing it 
would result in the stick appearing to fully deflect every other input 
report if the user is doing both at the same time.

> About the gyroscope/acceleration/quaternion, you know the issue
> that the wireless gives gyro plus quat but no accel, while the wired
> gives all three. My general idea is to create an additional input device
> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
> the wireless gives no accel, maybe there is some command to enable it?

It should be three neighboring bits for that setting; does this not work 
for you?

// Bitmask that define which IMU features to enable.
typedef enum
{
	SETTING_GYRO_MODE_OFF				= 0x0000,
	SETTING_GYRO_MODE_STEERING			= 0x0001,
	SETTING_GYRO_MODE_TILT				= 0x0002,
	SETTING_GYRO_MODE_SEND_ORIENTATION	= 0x0004,
	SETTING_GYRO_MODE_SEND_RAW_ACCEL	= 0x0008,
	SETTING_GYRO_MODE_SEND_RAW_GYRO		= 0x0010,
} SettingGyroMode;

> 
> Also, ideally, we could expose the quat. data directly to userspace, but
> I see no clear way to do that. Maybe defining INPUT_PROP_QUATERNION? I
> also thought of computing yaw/pitch/roll from the quat. and exposing
> those, but doing that requires atan2() and sin() (16-bit precision), and
> that would be tricky. Any thoughts on that?

I have no useful feedback on the actual shape of the data, but you'll 
want to give careful thought to sending the setting above. The battery 
life of the controller is going to get worse the more data you're 
enabling to be transmitted over the radio. For this reason, Steam only 
selectively enables the data that the current configuration demands, 
dynamically based on what client application has focus. It might make 
sense to expose the quaternion like you describe, but would it be doable 
to have it exposed through a separate device node? This way, I assume 
the driver could properly send the appropriate radio setting to the 
device based on whether that gyro-specific device node is open by an 
interested user or not. It would be unfortunate to enable these data 
channels by default if they're not needed.
>> Will this driver being loaded affect functionality of existing applications
>> that talk to it through HID directly, like Steam or sc-controller? Will they
>> be able to keep getting the same HID data they do today? If so, the extent
>> of the work needed to support it in Steam might just be to ignore the
>> controller device it's exposing, since Steam will expose that itself through
>> its own means.
> 
> As it is, this patchset hould be pretty safe. The only command sent to
> the controller is the get-serial-number, and that seems to do no harm to
> Steam Client. Other than that, it only reads. Moreover, the hidraw
> device is still created, so user-mode driver should keep working
> (but AFAIK, Steam Client uses direct USB access, not hidraw).
> 
> Curiously, Steam Client does not show the new native Steam Controller
> gamepad. That is, if I connect the Steam controller with hid-steam.ko
> and an Acme HID gamepad, then in Steam Client controller settings I
> see only two controllers, the managed Steam Controller (not HID) and the
> Acme HID gamepad. I reckon that Steam Client recognizes that the
> new HID device is a Steam Controller and ignores it in favor of its own
> managed controller.

OK, I can give it a spin to see what Steam thinks about that device. 
Maybe the proper VID/PID being exposed is enough for it to ignore it.

In general I'm concerned about sending of the gyro-enable message 
potentially impairing Steam functionality if it's running at the same 
time (eg. if gyro gets disabled over the radio while Steam still needs 
it), or sending any stateful messages to the device. For instance, are 
you ever sending the "blank configuration" setting? I assume you must be 
or users would still get keyboard/mouse input over these USB endpoints 
while trying to interact with the controller. But sending this after 
Steam programmed a setting, or instructed the controller to go back to 
lizard mode because it was requested by a configuration, would be 
problematic.

Thanks,
  - Pierre-Loup

> Regards.
> Rodrigo
> 
> [1]: https://github.com/ynsta/steamcontroller
> [2]: https://github.com/rodrigorc/inputmap
> 
> 

Re: [PATCH v2 1/3] HID: add driver for Valve Steam Controller

[Cameron Gutman]

On 02/20/2018 11:33 AM, Rodrigo Rivas Costa wrote:
> +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) {
> +		if (steam->input) {
> +			dbg_hid("%s: already connected\n", __func__);
> +			return;
> +		}
> +		ret = steam_register(steam);
> +		if (ret) {
> +			hid_err(steam->hdev,
> +				"%s:steam_register failed with error %d\n",
> +				__func__, ret);
> +			return;
> +		}
> +	} else {
> +		steam_unregister(steam);

I think you need synchronization here. You don't want to be in the middle of
processing a HID event or power supply update and have your device freed out
from underneath you.

xpad uses RCU to avoid this race.

> +	}
> +}
> +

Regards,
Cameron

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

Hi Rodrigo,

I have written a kernel driver [1], some time ago. I did not submit it
for merging in the main-line because I thought that would mess with
user-space drivers. If your driver create an input device, a
user-space driver will have to disable it before creating its own. I
guess libusb based drivers will detach the kernel driver and will be
fine. But it is not as simple if you use hidraw. And there may be some
state problem as Pierre-Loup already said.

A few more tips about your discussion with Pierre-Loup (sorry I am not
replying to the answers directly, it's hard to insert myself in this
long discussion):

Beside Ynsta's driver, you may have a look at my own user-space driver
[2] or Dennis Hamester's library scraw [3].

You can enable and disable the accel and gyro when the separate input
device is opened or closed to save battery. My kernel driver does it,
IIRC I have taken the idea from the wiimote driver. Note that there is
an issue with joydev using the accel/gyro input device. hid-sony
solved it by adding an exception in joydev driver (joydev_blacklist in
drivers/input/joydev.c).

In my driver, I split the left pad and left stick events by looking at
the left pad touch bit. It works well as long as they are not used at
the same time. When they both being used the data alternate between
left pad and left stick data, the touch bit is set accordingly so axis
data is easy to use, but it looks like the left touch pad is being
tapped very fast.

[1]: https://github.com/cvuchener/steamcontroller-linux-kernel
[2]: https://github.com/cvuchener/input-scripts/tree/master/src/daemon/steamcontroller
[3]: https://gitlab.com/dennis-hamester/scraw)

2018-02-20 20:33 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> This patchset implements a driver for Valve Steam Controller, based on a
> reverse analysis by myself.
>
> Notable changes from patchset v1:
>  * 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 (3):
>   HID: add driver for Valve Steam Controller
>   HID: steam: add serial number information.
>   HID: steam: add battery device.
>
>  drivers/hid/Kconfig      |   8 +
>  drivers/hid/Makefile     |   1 +
>  drivers/hid/hid-ids.h    |   4 +
>  drivers/hid/hid-quirks.c |   4 +
>  drivers/hid/hid-steam.c  | 703 +++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 720 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Wed, Feb 21, 2018 at 11:39:49AM +0100, Clément VUCHENER wrote:
> Hi Rodrigo,

Hello, Clément!

> I have written a kernel driver [1], some time ago. I did not submit it
> for merging in the main-line because I thought that would mess with
> user-space drivers. If your driver create an input device, a
> user-space driver will have to disable it before creating its own. I
> guess libusb based drivers will detach the kernel driver and will be
> fine. But it is not as simple if you use hidraw. And there may be some
> state problem as Pierre-Loup already said.


I not think that is a real problem. You can run this driver and a
user-space driver and the Steam Client all at the same time. You will
have several input devices that represet the same physical device. But
as long as the user client (game) uses only one of them, all will be
fine. That is, as long as the different drivers do not send any stateful
commands, of course.

> A few more tips about your discussion with Pierre-Loup (sorry I am not
> replying to the answers directly, it's hard to insert myself in this
> long discussion):
> 
> Beside Ynsta's driver, you may have a look at my own user-space driver
> [2] or Dennis Hamester's library scraw [3].
> 
> You can enable and disable the accel and gyro when the separate input
> device is opened or closed to save battery. My kernel driver does it,
> IIRC I have taken the idea from the wiimote driver.

Yeah, my future accel/gyro patch does exactly that. I copied the wii
too.

Also, my intention is to add a module variable so that the separate
input device is disabled by default and Steam Client will work properly.

> Note that there is
> an issue with joydev using the accel/gyro input device. hid-sony
> solved it by adding an exception in joydev driver (joydev_blacklist in
> drivers/input/joydev.c).

I don't know about that, I will look into it, thanks.

> In my driver, I split the left pad and left stick events by looking at
> the left pad touch bit. It works well as long as they are not used at
> the same time. When they both being used the data alternate between
> left pad and left stick data, the touch bit is set accordingly so axis
> data is easy to use, but it looks like the left touch pad is being
> tapped very fast.

Actually, there is a proper way to do that, that involves using the
lpad_touch and the lpand_and_joy bits at the same time. I think that I
will split the joystick and left-pad axes definitely. Few problems that
way. Maybe I'll map the left-pad to ABS_HAT1{X,Y}, that will play nicely
with the dpad being ABS_HAT0{X,Y].

Regards.
Rodrigo

> [1]: https://github.com/cvuchener/steamcontroller-linux-kernel
> [2]: https://github.com/cvuchener/input-scripts/tree/master/src/daemon/steamcontroller
> [3]: https://gitlab.com/dennis-hamester/scraw)

Re: [PATCH v2 1/3] HID: add driver for Valve Steam Controller

[Benjamin Tissoires]

On Tue, Feb 20, 2018 at 8:33 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 direct USB access
> and a creates a uinput virtual gamepad.
>
> This driver was reverse engineered to provide direct kernel support in
> case you cannot, or do not want to, use Valve Steam Client. It disables
> the virtual keyboard and mouse, as they are not so useful when you have
> a working gamepad.
>
> Working: buttons, axes, pads, wireless connect/disconnect.
>
> TO-DO: Battery, force-feedback, accelerometer/gyro, led, beeper...
>
> 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-quirks.c |   4 +
>  drivers/hid/hid-steam.c  | 478 +++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 495 insertions(+)
>  create mode 100644 drivers/hid/hid-steam.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 19c499f5623d..6e80fbf04e03 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -823,6 +823,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 eb13b9e92d85..60a8abf84682 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -95,6 +95,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 43ddcdfbd0da..be31a3c20818 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -988,6 +988,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-quirks.c b/drivers/hid/hid-quirks.c
> index 5f6035a5ce36..72ac972dc00b 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -629,6 +629,10 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>         { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS, USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>  #endif
> +#if IS_ENABLED(CONFIG_HID_STEAM)
> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER) },
> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
> +#endif

In addition to the discussion in 0/3, I wonder if you should not
remove this hunk. Unless having hid-generic binding the device before
your hid-steam driver, it would be better not force the Steam boxes to
use your driver.

I'll sneak in the discussion in 0/3.

Cheers,
Benjamin

>  #if IS_ENABLED(CONFIG_HID_STEELSERIES)
>         { HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, USB_DEVICE_ID_STEELSERIES_SRWS1) },
>  #endif
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> new file mode 100644
> index 000000000000..7b2f16b7bb49
> --- /dev/null
> +++ b/drivers/hid/hid-steam.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HID driver for Valve Steam Controller
> + *
> + * Supports both the wired and wireless interfaces.
> + *
> + * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include "hid-ids.h"
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
> +
> +#define STEAM_QUIRK_WIRELESS           BIT(0)
> +
> +/* 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
> +
> +struct steam_device {
> +       spinlock_t lock;
> +       struct hid_device *hdev;
> +       struct input_dev *input;
> +       unsigned long quirks;
> +       struct work_struct work_connect;
> +       bool connected;
> +};
> +
> +static int steam_input_open(struct input_dev *dev)
> +{
> +       struct steam_device *steam = input_get_drvdata(dev);
> +
> +       return hid_hw_open(steam->hdev);
> +}
> +
> +static void steam_input_close(struct input_dev *dev)
> +{
> +       struct steam_device *steam = input_get_drvdata(dev);
> +
> +       hid_hw_close(steam->hdev);
> +}
> +
> +static int steam_register(struct steam_device *steam)
> +{
> +       struct hid_device *hdev = steam->hdev;
> +       struct input_dev *input;
> +       int ret;
> +
> +       hid_info(hdev, "Steam Controller connected");
> +
> +       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 = hdev->uniq;
> +       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_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_abs_params(input, ABS_Z, 0, 255, 0, 0);
> +       input_set_abs_params(input, ABS_RZ, 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, 0, 0);
> +       input_set_abs_params(input, ABS_RY, -32767, 32767, 0, 0);
> +       input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0);
> +       input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0);
> +       input_abs_set_res(input, ABS_X, STEAM_PAD_RESOLUTION);
> +       input_abs_set_res(input, ABS_Y, STEAM_PAD_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_Z, STEAM_TRIGGER_RESOLUTION);
> +       input_abs_set_res(input, ABS_RZ, STEAM_TRIGGER_RESOLUTION);
> +
> +       ret = input_register_device(input);
> +       if (ret)
> +               goto input_register_fail;
> +
> +       steam->input = input;
> +
> +       return 0;
> +
> +input_register_fail:
> +       input_free_device(input);
> +       return ret;
> +}
> +
> +static void steam_unregister(struct steam_device *steam)
> +{
> +       if (steam->input) {
> +               hid_info(steam->hdev, "Steam Controller disconnected");
> +               input_unregister_device(steam->input);
> +               steam->input = NULL;
> +       }
> +}
> +
> +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) {
> +               if (steam->input) {
> +                       dbg_hid("%s: already connected\n", __func__);
> +                       return;
> +               }
> +               ret = steam_register(steam);
> +               if (ret) {
> +                       hid_err(steam->hdev,
> +                               "%s:steam_register failed with error %d\n",
> +                               __func__, ret);
> +                       return;
> +               }
> +       } else {
> +               steam_unregister(steam);
> +       }
> +}
> +
> +static bool steam_is_valve_interface(struct hid_device *hdev)
> +{
> +       struct hid_report_enum *rep_enum;
> +       struct hid_report *hreport;
> +
> +       /*
> +        * 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];
> +       list_for_each_entry(hreport, &rep_enum->report_list, list) {
> +               /* should we check hreport->id == 0? */
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static int steam_probe(struct hid_device *hdev,
> +                               const struct hid_device_id *id)
> +{
> +       struct steam_device *steam;
> +       int ret;
> +
> +       ret = hid_parse(hdev);
> +       if (ret) {
> +               hid_err(hdev,
> +                       "%s:parse of hid interface failed\n", __func__);
> +               return ret;
> +       }
> +
> +       /*
> +        * Since we have a proper gamepad now, we can ignore the virtual
> +        * mouse and keyboard.
> +        */
> +       if (!steam_is_valve_interface(hdev))
> +               return -ENODEV;
> +
> +       steam = devm_kzalloc(&hdev->dev,
> +                       sizeof(struct steam_device), GFP_KERNEL);
> +       if (!steam)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&steam->lock);
> +       steam->hdev = hdev;
> +       hid_set_drvdata(hdev, steam);
> +       steam->quirks = id->driver_data;
> +       INIT_WORK(&steam->work_connect, steam_work_connect_cb);
> +
> +       ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +       if (ret) {
> +               hid_err(hdev,
> +                       "%s:hid_hw_start failed with error %d\n",
> +                       __func__, ret);
> +               goto hid_hw_start_fail;
> +       }
> +
> +       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");
> +       } else {
> +               ret = steam_register(steam);
> +               if (ret) {
> +                       hid_err(hdev,
> +                               "%s:steam_register failed with error %d\n",
> +                               __func__, ret);
> +                       goto input_register_fail;
> +               }
> +       }
> +
> +       return 0;
> +
> +input_register_fail:
> +hid_hw_open_fail:
> +       hid_hw_stop(hdev);
> +hid_hw_start_fail:
> +       cancel_work_sync(&steam->work_connect);
> +       hid_set_drvdata(hdev, NULL);
> +       return ret;
> +}
> +
> +static void steam_remove(struct hid_device *hdev)
> +{
> +       struct steam_device *steam = hid_get_drvdata(hdev);
> +
> +       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);
> +       steam_unregister(steam);
> +       hid_set_drvdata(hdev, NULL);
> +}
> +
> +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_Z     | left trigger
> + *  12    | u8    | ABS_RZ    | right trigger
> + *  13-15 | --    | --        | always 0
> + *  16-17 | s16   | ABS_X     | X value
> + *  18-19 | s16   | ABS_Y     | 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  | -ABS_HAT0Y | lef-pad up
> + *  9.1  | +ABS_HAT0X | lef-pad right
> + *  9.2  | -ABS_HAT0X | lef-pad left
> + *  9.3  | +ABS_HAT0Y | 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  | --         | left-pad touched
> + * 10.4  | --         | 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, u8 *data)
> +{
> +       struct input_dev *input = steam->input;
> +
> +       /* 24 bits of buttons */
> +       u8 b8, b9, b10;
> +
> +       /*
> +        * If we get input events from the wireless without a 'connected'
> +        * event, just connect it now.
> +        * This can happen if we bind the HID device with the controller
> +        * already paired.
> +        */
> +       if (unlikely(!input)) {
> +               dbg_hid("%s: input data without connect event\n", __func__);
> +               steam_do_connect_event(steam, true);
> +               return;
> +       }
> +
> +       input_report_abs(input, ABS_Z, data[11]);
> +       input_report_abs(input, ABS_RZ, data[12]);
> +
> +       input_report_abs(input, ABS_X,
> +                       (s16) le16_to_cpup((__le16 *)(data + 16)));
> +       input_report_abs(input, ABS_Y,
> +                       -(s16) le16_to_cpup((__le16 *)(data + 18)));
> +       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)));
> +
> +       b8 = data[8];
> +       b9 = data[9];
> +       b10 = data[10];
> +
> +       input_event(input, EV_KEY, BTN_TR2, !!(b8 & 0x01));
> +       input_event(input, EV_KEY, BTN_TL2, !!(b8 & 0x02));
> +       input_event(input, EV_KEY, BTN_TR, !!(b8 & 0x04));
> +       input_event(input, EV_KEY, BTN_TL, !!(b8 & 0x08));
> +       input_event(input, EV_KEY, BTN_Y, !!(b8 & 0x10));
> +       input_event(input, EV_KEY, BTN_B, !!(b8 & 0x20));
> +       input_event(input, EV_KEY, BTN_X, !!(b8 & 0x40));
> +       input_event(input, EV_KEY, BTN_A, !!(b8 & 0x80));
> +       input_event(input, EV_KEY, BTN_SELECT, !!(b9 & 0x10));
> +       input_event(input, EV_KEY, BTN_MODE, !!(b9 & 0x20));
> +       input_event(input, EV_KEY, BTN_START, !!(b9 & 0x40));
> +       input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & 0x80));
> +       input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & 0x01));
> +       input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & 0x04));
> +       input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & 0x40));
> +
> +       input_report_abs(input, ABS_HAT0X,
> +                       !!(b9 & 0x02) - !!(b9 & 0x04));
> +       input_report_abs(input, ABS_HAT0Y,
> +                       !!(b9 & 0x08) - !!(b9 & 0x01));
> +
> +       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);
> +
> +       /*
> +        * 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:
> +               steam_do_input_event(steam, data);
> +               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);
> --
> 2.16.1
>

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
> > On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
> > About the left trackpad/joystick, currently I'm not treating them
> > different. I'm out of ABS axes, and anyway, it is not likely that the
> > left pad and the joystick be used at the same time (I only have one left
> > thumb). Nevertheless, if we really want to make them apart, we can use
> > bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
> > details in [2], but I'm not currently doing that in this driver.
> 
> I didn't necessarily mean exposing it, but in the event a user is using both
> at the same time (it happens, using claw grip with some games is necessary
> to use the D-pad while deflecting the stick), then you'll most likely run
> into issues unless you demux the inbound data, since we were also out of
> left analog fields and mux them together if used at the same time. Not sure
> if you're already handling that case, but not doing it would result in the
> stick appearing to fully deflect every other input report if the user is
> doing both at the same time.

"Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
right. I think that it will be best to fully separate the left-pad and
the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...

> > About the gyroscope/acceleration/quaternion, you know the issue
> > that the wireless gives gyro plus quat but no accel, while the wired
> > gives all three. My general idea is to create an additional input device
> > with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
> > the wireless gives no accel, maybe there is some command to enable it?
> 
> It should be three neighboring bits for that setting; does this not work for
> you?
> 
> // Bitmask that define which IMU features to enable.
> typedef enum
> {
> 	SETTING_GYRO_MODE_OFF				= 0x0000,
> 	SETTING_GYRO_MODE_STEERING			= 0x0001,
> 	SETTING_GYRO_MODE_TILT				= 0x0002,
> 	SETTING_GYRO_MODE_SEND_ORIENTATION	= 0x0004,
> 	SETTING_GYRO_MODE_SEND_RAW_ACCEL	= 0x0008,
> 	SETTING_GYRO_MODE_SEND_RAW_GYRO		= 0x0010,
> } SettingGyroMode;

Thanks for that, it will be useful! Those are the values to be written
to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
bit-field...

> In general I'm concerned about sending of the gyro-enable message
> potentially impairing Steam functionality if it's running at the same time
> (eg. if gyro gets disabled over the radio while Steam still needs it), or
> sending any stateful messages to the device. For instance, are you ever
> sending the "blank configuration" setting? I assume you must be or users
> would still get keyboard/mouse input over these USB endpoints while trying
> to interact with the controller. But sending this after Steam programmed a
> setting, or instructed the controller to go back to lizard mode because it
> was requested by a configuration, would be problematic.

Sure, but this patchset should be safe, shouldn't it?
Maube future patches that play with the gyro/force-feedback could be
disabled by default, and could need a module parameter to be enabled.
That way Steam Client will work out-of-the-box anywhere but proficient
users could use the extra functionality easily.

Related to that, Benjamin Tissoires replied to 1/3:
> > --- a/drivers/hid/hid-quirks.c
> > +++ b/drivers/hid/hid-quirks.c
> > @@ -629,6 +629,10 @@ static const struct hid_device_id
> > hid_have_special_driver[] = {
> >  #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
> >         { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
> >         USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
> >  #endif
> > +#if IS_ENABLED(CONFIG_HID_STEAM)
> > +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER) },
> > +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
> > +#endif
> 
> In addition to the discussion in 0/3, I wonder if you should not
> remove this hunk. Unless having hid-generic binding the device before
> your hid-steam driver, it would be better not force the Steam boxes to
> use your driver.

I don't really see the point on that. If we do that I'll have to unbind
and bind the device manually, and that is a pain, and impossible without
root (my ultimate goal is to use this controller with my Tizen TV ;-P).

And anyway this driver is mostly harmless, the only visible changes from
userspace are the new input and power devices, and that the virtual
keyboard/mouse are no more. If the virtual devices are really missed we
could use:

	hid_hw_start(hdev, HID_CONNECT_DEFAULT);

on them, maybe with a module parameter? Note that one of the first
things the Steam Client does is to disable the virtual devices (with a
command to the device).
(TBH I always had the impression that those virtual devices are there
to move the Grub menu...)

If Valve people really feel that this driver is not needed for SteamOS,
because the Steam Client is always running, then SteamOS can always do
CONFIG_HID_STEAM=n.

Regards.
Rodrigo

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Pierre-Loup A. Griffais]

On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
>>> About the left trackpad/joystick, currently I'm not treating them
>>> different. I'm out of ABS axes, and anyway, it is not likely that the
>>> left pad and the joystick be used at the same time (I only have one left
>>> thumb). Nevertheless, if we really want to make them apart, we can use
>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
>>> details in [2], but I'm not currently doing that in this driver.
>>
>> I didn't necessarily mean exposing it, but in the event a user is using both
>> at the same time (it happens, using claw grip with some games is necessary
>> to use the D-pad while deflecting the stick), then you'll most likely run
>> into issues unless you demux the inbound data, since we were also out of
>> left analog fields and mux them together if used at the same time. Not sure
>> if you're already handling that case, but not doing it would result in the
>> stick appearing to fully deflect every other input report if the user is
>> doing both at the same time.
> 
> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
> right. I think that it will be best to fully separate the left-pad and
> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
> 
>>> About the gyroscope/acceleration/quaternion, you know the issue
>>> that the wireless gives gyro plus quat but no accel, while the wired
>>> gives all three. My general idea is to create an additional input device
>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
>>> the wireless gives no accel, maybe there is some command to enable it?
>>
>> It should be three neighboring bits for that setting; does this not work for
>> you?
>>
>> // Bitmask that define which IMU features to enable.
>> typedef enum
>> {
>> 	SETTING_GYRO_MODE_OFF				= 0x0000,
>> 	SETTING_GYRO_MODE_STEERING			= 0x0001,
>> 	SETTING_GYRO_MODE_TILT				= 0x0002,
>> 	SETTING_GYRO_MODE_SEND_ORIENTATION	= 0x0004,
>> 	SETTING_GYRO_MODE_SEND_RAW_ACCEL	= 0x0008,
>> 	SETTING_GYRO_MODE_SEND_RAW_GYRO		= 0x0010,
>> } SettingGyroMode;
> 
> Thanks for that, it will be useful! Those are the values to be written
> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
> bit-field...
> 
>> In general I'm concerned about sending of the gyro-enable message
>> potentially impairing Steam functionality if it's running at the same time
>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
>> sending any stateful messages to the device. For instance, are you ever
>> sending the "blank configuration" setting? I assume you must be or users
>> would still get keyboard/mouse input over these USB endpoints while trying
>> to interact with the controller. But sending this after Steam programmed a
>> setting, or instructed the controller to go back to lizard mode because it
>> was requested by a configuration, would be problematic.
> 
> Sure, but this patchset should be safe, shouldn't it?
> Maube future patches that play with the gyro/force-feedback could be
> disabled by default, and could need a module parameter to be enabled.
> That way Steam Client will work out-of-the-box anywhere but proficient
> users could use the extra functionality easily.
> 
> Related to that, Benjamin Tissoires replied to 1/3:
>>> --- a/drivers/hid/hid-quirks.c
>>> +++ b/drivers/hid/hid-quirks.c
>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
>>> hid_have_special_driver[] = {
>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>>>   #endif
>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER) },
>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE, USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
>>> +#endif
>>
>> In addition to the discussion in 0/3, I wonder if you should not
>> remove this hunk. Unless having hid-generic binding the device before
>> your hid-steam driver, it would be better not force the Steam boxes to
>> use your driver.
> 
> I don't really see the point on that. If we do that I'll have to unbind
> and bind the device manually, and that is a pain, and impossible without
> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
> 
> And anyway this driver is mostly harmless, the only visible changes from
> userspace are the new input and power devices, and that the virtual
> keyboard/mouse are no more. If the virtual devices are really missed we
> could use:
> 
> 	hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> 
> on them, maybe with a module parameter? Note that one of the first
> things the Steam Client does is to disable the virtual devices (with a
> command to the device).
> (TBH I always had the impression that those virtual devices are there
> to move the Grub menu...)
> 
> If Valve people really feel that this driver is not needed for SteamOS,
> because the Steam Client is always running, then SteamOS can always do
> CONFIG_HID_STEAM=n.

Yes, certainly; that isn't really the usecase I'm worried about. What 
I'm worried about is behavior changing for existing users of the 
controller on normal desktop distributions. Currently the Steam client 
will program these pieces of state (enable/disable direct keyboard/mouse 
HID functionality, enable/disable gyro/accel) based on the user's 
configuration, and a user getting a kernel update that includes a driver 
that also programs that without their intervention is bound to affect 
the behavior. If there was a way to make it so the states won't be 
programmed until it's pretty clear the user is trying to use that 
driver's functionality, that would feel safer.

Thanks,
  - Pierre-Loup

> 
> Regards.
> Rodrigo
> 

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Clément VUCHENER]

2018-02-22 1:13 GMT+01:00 Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>:
>
> On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>>
>> Related to that, Benjamin Tissoires replied to 1/3:
>>>>
>>>> --- a/drivers/hid/hid-quirks.c
>>>> +++ b/drivers/hid/hid-quirks.c
>>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
>>>> hid_have_special_driver[] = {
>>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
>>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>>>>   #endif
>>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
>>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
>>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
>>>> +#endif
>>>
>>>
>>> In addition to the discussion in 0/3, I wonder if you should not
>>> remove this hunk. Unless having hid-generic binding the device before
>>> your hid-steam driver, it would be better not force the Steam boxes to
>>> use your driver.
>>
>>
>> I don't really see the point on that. If we do that I'll have to unbind
>> and bind the device manually, and that is a pain, and impossible without
>> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
>>
>> And anyway this driver is mostly harmless, the only visible changes from
>> userspace are the new input and power devices, and that the virtual
>> keyboard/mouse are no more. If the virtual devices are really missed we
>> could use:
>>
>>         hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>
>> on them, maybe with a module parameter? Note that one of the first
>> things the Steam Client does is to disable the virtual devices (with a
>> command to the device).
>> (TBH I always had the impression that those virtual devices are there
>> to move the Grub menu...)
>>
>> If Valve people really feel that this driver is not needed for SteamOS,
>> because the Steam Client is always running, then SteamOS can always do
>> CONFIG_HID_STEAM=n.
>
>
> Yes, certainly; that isn't really the usecase I'm worried about. What I'm
> worried about is behavior changing for existing users of the controller on
> normal desktop distributions. Currently the Steam client will program these
> pieces of state (enable/disable direct keyboard/mouse HID functionality,
> enable/disable gyro/accel) based on the user's configuration, and a user
> getting a kernel update that includes a driver that also programs that
> without their intervention is bound to affect the behavior. If there was a
> way to make it so the states won't be programmed until it's pretty clear the
> user is trying to use that driver's functionality, that would feel safer.

hid_have_special_driver is no longer required (see patch "HID: core:
remove the absolute need of hid_have_special_driver[]" [1]). If I
understand Benjamin's intent correctly, the driver will still be used,
but if you have a conflict you can simply unload/blacklist the special
driver module and hid-generic will handle the device. This way there
is no need to recompile the kernel to disable the special driver while
keeping standard HID features working. User-space driver packagers
(e.g. steam packagers) could simply add a modprobe conf file to
blacklist the module and avoid conflicts.

[1]: https://patchwork.kernel.org/patch/10066303/

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Benjamin Tissoires]

On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais
<pgriffais@valvesoftware.com> wrote:
>
>
> On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>>
>> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
>>>
>>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
>>>>
>>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
>>>> About the left trackpad/joystick, currently I'm not treating them
>>>> different. I'm out of ABS axes, and anyway, it is not likely that the
>>>> left pad and the joystick be used at the same time (I only have one left
>>>> thumb). Nevertheless, if we really want to make them apart, we can use
>>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
>>>> details in [2], but I'm not currently doing that in this driver.
>>>
>>>
>>> I didn't necessarily mean exposing it, but in the event a user is using
>>> both
>>> at the same time (it happens, using claw grip with some games is
>>> necessary
>>> to use the D-pad while deflecting the stick), then you'll most likely run
>>> into issues unless you demux the inbound data, since we were also out of
>>> left analog fields and mux them together if used at the same time. Not
>>> sure
>>> if you're already handling that case, but not doing it would result in
>>> the
>>> stick appearing to fully deflect every other input report if the user is
>>> doing both at the same time.
>>
>>
>> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
>> right. I think that it will be best to fully separate the left-pad and
>> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
>>
>>>> About the gyroscope/acceleration/quaternion, you know the issue
>>>> that the wireless gives gyro plus quat but no accel, while the wired
>>>> gives all three. My general idea is to create an additional input device
>>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
>>>> the wireless gives no accel, maybe there is some command to enable it?
>>>
>>>
>>> It should be three neighboring bits for that setting; does this not work
>>> for
>>> you?
>>>
>>> // Bitmask that define which IMU features to enable.
>>> typedef enum
>>> {
>>>         SETTING_GYRO_MODE_OFF                           = 0x0000,
>>>         SETTING_GYRO_MODE_STEERING                      = 0x0001,
>>>         SETTING_GYRO_MODE_TILT                          = 0x0002,
>>>         SETTING_GYRO_MODE_SEND_ORIENTATION      = 0x0004,
>>>         SETTING_GYRO_MODE_SEND_RAW_ACCEL        = 0x0008,
>>>         SETTING_GYRO_MODE_SEND_RAW_GYRO         = 0x0010,
>>> } SettingGyroMode;
>>
>>
>> Thanks for that, it will be useful! Those are the values to be written
>> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
>> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
>> bit-field...
>>
>>> In general I'm concerned about sending of the gyro-enable message
>>> potentially impairing Steam functionality if it's running at the same
>>> time
>>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
>>> sending any stateful messages to the device. For instance, are you ever
>>> sending the "blank configuration" setting? I assume you must be or users
>>> would still get keyboard/mouse input over these USB endpoints while
>>> trying
>>> to interact with the controller. But sending this after Steam programmed
>>> a
>>> setting, or instructed the controller to go back to lizard mode because
>>> it
>>> was requested by a configuration, would be problematic.
>>
>>
>> Sure, but this patchset should be safe, shouldn't it?
>> Maube future patches that play with the gyro/force-feedback could be
>> disabled by default, and could need a module parameter to be enabled.
>> That way Steam Client will work out-of-the-box anywhere but proficient
>> users could use the extra functionality easily.
>>
>> Related to that, Benjamin Tissoires replied to 1/3:
>>>>
>>>> --- a/drivers/hid/hid-quirks.c
>>>> +++ b/drivers/hid/hid-quirks.c
>>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
>>>> hid_have_special_driver[] = {
>>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
>>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>>>>   #endif
>>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
>>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
>>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
>>>> +#endif
>>>
>>>
>>> In addition to the discussion in 0/3, I wonder if you should not
>>> remove this hunk. Unless having hid-generic binding the device before
>>> your hid-steam driver, it would be better not force the Steam boxes to
>>> use your driver.
>>
>>
>> I don't really see the point on that. If we do that I'll have to unbind
>> and bind the device manually, and that is a pain, and impossible without
>> root (my ultimate goal is to use this controller with my Tizen TV ;-P).

I guess you are not running v4.16-rc2 :) (see Clement answers too)

Since v4.16, there is no need to absolutely blacklist the devices in
this list. hid-generic will bind them first, but as soon as an other
driver claims the device, hid-generic unbinds itself and let the other
driver to handle the device. Without any user input!

>>
>> And anyway this driver is mostly harmless, the only visible changes from
>> userspace are the new input and power devices, and that the virtual
>> keyboard/mouse are no more.

Pierre-Loup mentioned that sometime the Steam client needs those
interfaces for the Desktop mode. I have the feeling things are going
to be too intricated for us to gracefully accept the driver unless
there is a clear ACK from Valve that they are happy with the new
driver.

>> If the virtual devices are really missed we
>> could use:
>>
>>         hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>>
>> on them, maybe with a module parameter? Note that one of the first
>> things the Steam Client does is to disable the virtual devices (with a
>> command to the device).
>> (TBH I always had the impression that those virtual devices are there
>> to move the Grub menu...)
>>
>> If Valve people really feel that this driver is not needed for SteamOS,
>> because the Steam Client is always running, then SteamOS can always do
>> CONFIG_HID_STEAM=n.

That is what I was expecting, but if the driver also breaks the
regular Steam client, we have a situation :)

>
>
> Yes, certainly; that isn't really the usecase I'm worried about. What I'm
> worried about is behavior changing for existing users of the controller on
> normal desktop distributions. Currently the Steam client will program these
> pieces of state (enable/disable direct keyboard/mouse HID functionality,
> enable/disable gyro/accel) based on the user's configuration, and a user
> getting a kernel update that includes a driver that also programs that
> without their intervention is bound to affect the behavior. If there was a
> way to make it so the states won't be programmed until it's pretty clear the
> user is trying to use that driver's functionality, that would feel safer.

I do not think there is a way to know beforehand, given that the
kernel module will be loaded first, and sometimes even without the
root file system if the driver has been included in the initramfs
(which is the point of not having the device blacklisted in
hid-quirks.c)

I guess the only reasonable solution would be for the kernel HID
driver to expose all the interfaces (keyboard, touchpad, gyro, etc...)
but not take any action on whether which mode it switches into. Then,
(and unfortunatelly) though a custom sysfs API, we could teach SDL or
any other configurator to change the device into the desired mode.
I would prefer not having a custom sysfs API, but this would allow us
to change the owner to a non-root user while hidraw needs to have the
root access.

An other solution than the sysfs API, would be to have a small driver
in libratbag (or a similar daemon) that sends the mode switch command
as if the joystick where a special gaming mouse.

Cheers,
Benjamin

>
>
> Thanks,
>  - Pierre-Loup
>
>>
>> Regards.
>> Rodrigo
>>
>

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Thu, Feb 22, 2018 at 10:10:40AM +0100, Benjamin Tissoires wrote:
> On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais
> <pgriffais@valvesoftware.com> wrote:
> >
> >
> > On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
> >>
> >> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
> >>>
> >>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
> >>>>
> >>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
> >>>> About the left trackpad/joystick, currently I'm not treating them
> >>>> different. I'm out of ABS axes, and anyway, it is not likely that the
> >>>> left pad and the joystick be used at the same time (I only have one left
> >>>> thumb). Nevertheless, if we really want to make them apart, we can use
> >>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
> >>>> details in [2], but I'm not currently doing that in this driver.
> >>>
> >>>
> >>> I didn't necessarily mean exposing it, but in the event a user is using
> >>> both
> >>> at the same time (it happens, using claw grip with some games is
> >>> necessary
> >>> to use the D-pad while deflecting the stick), then you'll most likely run
> >>> into issues unless you demux the inbound data, since we were also out of
> >>> left analog fields and mux them together if used at the same time. Not
> >>> sure
> >>> if you're already handling that case, but not doing it would result in
> >>> the
> >>> stick appearing to fully deflect every other input report if the user is
> >>> doing both at the same time.
> >>
> >>
> >> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
> >> right. I think that it will be best to fully separate the left-pad and
> >> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
> >>
> >>>> About the gyroscope/acceleration/quaternion, you know the issue
> >>>> that the wireless gives gyro plus quat but no accel, while the wired
> >>>> gives all three. My general idea is to create an additional input device
> >>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
> >>>> the wireless gives no accel, maybe there is some command to enable it?
> >>>
> >>>
> >>> It should be three neighboring bits for that setting; does this not work
> >>> for
> >>> you?
> >>>
> >>> // Bitmask that define which IMU features to enable.
> >>> typedef enum
> >>> {
> >>>         SETTING_GYRO_MODE_OFF                           = 0x0000,
> >>>         SETTING_GYRO_MODE_STEERING                      = 0x0001,
> >>>         SETTING_GYRO_MODE_TILT                          = 0x0002,
> >>>         SETTING_GYRO_MODE_SEND_ORIENTATION      = 0x0004,
> >>>         SETTING_GYRO_MODE_SEND_RAW_ACCEL        = 0x0008,
> >>>         SETTING_GYRO_MODE_SEND_RAW_GYRO         = 0x0010,
> >>> } SettingGyroMode;
> >>
> >>
> >> Thanks for that, it will be useful! Those are the values to be written
> >> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
> >> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
> >> bit-field...
> >>
> >>> In general I'm concerned about sending of the gyro-enable message
> >>> potentially impairing Steam functionality if it's running at the same
> >>> time
> >>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
> >>> sending any stateful messages to the device. For instance, are you ever
> >>> sending the "blank configuration" setting? I assume you must be or users
> >>> would still get keyboard/mouse input over these USB endpoints while
> >>> trying
> >>> to interact with the controller. But sending this after Steam programmed
> >>> a
> >>> setting, or instructed the controller to go back to lizard mode because
> >>> it
> >>> was requested by a configuration, would be problematic.
> >>
> >>
> >> Sure, but this patchset should be safe, shouldn't it?
> >> Maube future patches that play with the gyro/force-feedback could be
> >> disabled by default, and could need a module parameter to be enabled.
> >> That way Steam Client will work out-of-the-box anywhere but proficient
> >> users could use the extra functionality easily.
> >>
> >> Related to that, Benjamin Tissoires replied to 1/3:
> >>>>
> >>>> --- a/drivers/hid/hid-quirks.c
> >>>> +++ b/drivers/hid/hid-quirks.c
> >>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
> >>>> hid_have_special_driver[] = {
> >>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
> >>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
> >>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
> >>>>   #endif
> >>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> >>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> >>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
> >>>> +#endif
> >>>
> >>>
> >>> In addition to the discussion in 0/3, I wonder if you should not
> >>> remove this hunk. Unless having hid-generic binding the device before
> >>> your hid-steam driver, it would be better not force the Steam boxes to
> >>> use your driver.
> >>
> >>
> >> I don't really see the point on that. If we do that I'll have to unbind
> >> and bind the device manually, and that is a pain, and impossible without
> >> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
> 
> I guess you are not running v4.16-rc2 :) (see Clement answers too)
> 
> Since v4.16, there is no need to absolutely blacklist the devices in
> this list. hid-generic will bind them first, but as soon as an other
> driver claims the device, hid-generic unbinds itself and let the other
> driver to handle the device. Without any user input!

No, I'm still in v4.15.3, and I thought I was bleeding edge...
I've seen those patches in the hid git tree, but I thought it was
experimental. Good to know it is upstream, that quirks array was
weird... I'll try to upgrade and see what happens.
> 
> >> And anyway this driver is mostly harmless, the only visible changes from
> >> userspace are the new input and power devices, and that the virtual
> >> keyboard/mouse are no more.
> 
> Pierre-Loup mentioned that sometime the Steam client needs those
> interfaces for the Desktop mode. I have the feeling things are going
> to be too intricated for us to gracefully accept the driver unless
> there is a clear ACK from Valve that they are happy with the new
> driver.
> 
> >> If the virtual devices are really missed we
> >> could use:
> >>
> >>         hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> >>
> >> on them, maybe with a module parameter? Note that one of the first
> >> things the Steam Client does is to disable the virtual devices (with a
> >> command to the device).
> >> (TBH I always had the impression that those virtual devices are there
> >> to move the Grub menu...)
> >>
> >> If Valve people really feel that this driver is not needed for SteamOS,
> >> because the Steam Client is always running, then SteamOS can always do
> >> CONFIG_HID_STEAM=n.
> 
> That is what I was expecting, but if the driver also breaks the
> regular Steam client, we have a situation :)
> 
> >
> >
> > Yes, certainly; that isn't really the usecase I'm worried about. What I'm
> > worried about is behavior changing for existing users of the controller on
> > normal desktop distributions. Currently the Steam client will program these
> > pieces of state (enable/disable direct keyboard/mouse HID functionality,
> > enable/disable gyro/accel) based on the user's configuration, and a user
> > getting a kernel update that includes a driver that also programs that
> > without their intervention is bound to affect the behavior. If there was a
> > way to make it so the states won't be programmed until it's pretty clear the
> > user is trying to use that driver's functionality, that would feel safer.
> 
> I do not think there is a way to know beforehand, given that the
> kernel module will be loaded first, and sometimes even without the
> root file system if the driver has been included in the initramfs
> (which is the point of not having the device blacklisted in
> hid-quirks.c)
> 
> I guess the only reasonable solution would be for the kernel HID
> driver to expose all the interfaces (keyboard, touchpad, gyro, etc...)
> but not take any action on whether which mode it switches into. Then,
> (and unfortunatelly) though a custom sysfs API, we could teach SDL or
> any other configurator to change the device into the desired mode.
> I would prefer not having a custom sysfs API, but this would allow us
> to change the owner to a non-root user while hidraw needs to have the
> root access.
> 
> An other solution than the sysfs API, would be to have a small driver
> in libratbag (or a similar daemon) that sends the mode switch command
> as if the joystick where a special gaming mouse.

Ok, I agree that we cannot remove the keyboard/mouse interfaces. What
about a module parameter? Something like `disable_lizard_mode` or
`disable_hid_devices`. That would be 0 by default, so no user-mode
breakage. BTW, are module parameters considered userspace API stable?

Note that when this parameter is set, the driver will not send any
command to the controller, (as Steam Client does). Instead, it will
simply not call hid_hw_start() on those interfaces. I'm not sure how the
no-quirks hid-generic will behave in this case... I'll try and see...

In the future, we could add a few other modules paramteres to enable the
gyro and the force-feedback, as these functions could very well break
Steam. But these functions will be welcomed by DIY enthusiasts.

Regards.
Rodrigo

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Benjamin Tissoires]

On Thu, Feb 22, 2018 at 5:31 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Thu, Feb 22, 2018 at 10:10:40AM +0100, Benjamin Tissoires wrote:
>> On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais
>> <pgriffais@valvesoftware.com> wrote:
>> >
>> >
>> > On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>> >>
>> >> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
>> >>>
>> >>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
>> >>>>
>> >>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
>> >>>> About the left trackpad/joystick, currently I'm not treating them
>> >>>> different. I'm out of ABS axes, and anyway, it is not likely that the
>> >>>> left pad and the joystick be used at the same time (I only have one left
>> >>>> thumb). Nevertheless, if we really want to make them apart, we can use
>> >>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
>> >>>> details in [2], but I'm not currently doing that in this driver.
>> >>>
>> >>>
>> >>> I didn't necessarily mean exposing it, but in the event a user is using
>> >>> both
>> >>> at the same time (it happens, using claw grip with some games is
>> >>> necessary
>> >>> to use the D-pad while deflecting the stick), then you'll most likely run
>> >>> into issues unless you demux the inbound data, since we were also out of
>> >>> left analog fields and mux them together if used at the same time. Not
>> >>> sure
>> >>> if you're already handling that case, but not doing it would result in
>> >>> the
>> >>> stick appearing to fully deflect every other input report if the user is
>> >>> doing both at the same time.
>> >>
>> >>
>> >> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
>> >> right. I think that it will be best to fully separate the left-pad and
>> >> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
>> >>
>> >>>> About the gyroscope/acceleration/quaternion, you know the issue
>> >>>> that the wireless gives gyro plus quat but no accel, while the wired
>> >>>> gives all three. My general idea is to create an additional input device
>> >>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
>> >>>> the wireless gives no accel, maybe there is some command to enable it?
>> >>>
>> >>>
>> >>> It should be three neighboring bits for that setting; does this not work
>> >>> for
>> >>> you?
>> >>>
>> >>> // Bitmask that define which IMU features to enable.
>> >>> typedef enum
>> >>> {
>> >>>         SETTING_GYRO_MODE_OFF                           = 0x0000,
>> >>>         SETTING_GYRO_MODE_STEERING                      = 0x0001,
>> >>>         SETTING_GYRO_MODE_TILT                          = 0x0002,
>> >>>         SETTING_GYRO_MODE_SEND_ORIENTATION      = 0x0004,
>> >>>         SETTING_GYRO_MODE_SEND_RAW_ACCEL        = 0x0008,
>> >>>         SETTING_GYRO_MODE_SEND_RAW_GYRO         = 0x0010,
>> >>> } SettingGyroMode;
>> >>
>> >>
>> >> Thanks for that, it will be useful! Those are the values to be written
>> >> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
>> >> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
>> >> bit-field...
>> >>
>> >>> In general I'm concerned about sending of the gyro-enable message
>> >>> potentially impairing Steam functionality if it's running at the same
>> >>> time
>> >>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
>> >>> sending any stateful messages to the device. For instance, are you ever
>> >>> sending the "blank configuration" setting? I assume you must be or users
>> >>> would still get keyboard/mouse input over these USB endpoints while
>> >>> trying
>> >>> to interact with the controller. But sending this after Steam programmed
>> >>> a
>> >>> setting, or instructed the controller to go back to lizard mode because
>> >>> it
>> >>> was requested by a configuration, would be problematic.
>> >>
>> >>
>> >> Sure, but this patchset should be safe, shouldn't it?
>> >> Maube future patches that play with the gyro/force-feedback could be
>> >> disabled by default, and could need a module parameter to be enabled.
>> >> That way Steam Client will work out-of-the-box anywhere but proficient
>> >> users could use the extra functionality easily.
>> >>
>> >> Related to that, Benjamin Tissoires replied to 1/3:
>> >>>>
>> >>>> --- a/drivers/hid/hid-quirks.c
>> >>>> +++ b/drivers/hid/hid-quirks.c
>> >>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
>> >>>> hid_have_special_driver[] = {
>> >>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>> >>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
>> >>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>> >>>>   #endif
>> >>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
>> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>> >>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
>> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>> >>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
>> >>>> +#endif
>> >>>
>> >>>
>> >>> In addition to the discussion in 0/3, I wonder if you should not
>> >>> remove this hunk. Unless having hid-generic binding the device before
>> >>> your hid-steam driver, it would be better not force the Steam boxes to
>> >>> use your driver.
>> >>
>> >>
>> >> I don't really see the point on that. If we do that I'll have to unbind
>> >> and bind the device manually, and that is a pain, and impossible without
>> >> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
>>
>> I guess you are not running v4.16-rc2 :) (see Clement answers too)
>>
>> Since v4.16, there is no need to absolutely blacklist the devices in
>> this list. hid-generic will bind them first, but as soon as an other
>> driver claims the device, hid-generic unbinds itself and let the other
>> driver to handle the device. Without any user input!
>
> No, I'm still in v4.15.3, and I thought I was bleeding edge...
> I've seen those patches in the hid git tree, but I thought it was
> experimental. Good to know it is upstream, that quirks array was
> weird... I'll try to upgrade and see what happens.
>>
>> >> And anyway this driver is mostly harmless, the only visible changes from
>> >> userspace are the new input and power devices, and that the virtual
>> >> keyboard/mouse are no more.
>>
>> Pierre-Loup mentioned that sometime the Steam client needs those
>> interfaces for the Desktop mode. I have the feeling things are going
>> to be too intricated for us to gracefully accept the driver unless
>> there is a clear ACK from Valve that they are happy with the new
>> driver.
>>
>> >> If the virtual devices are really missed we
>> >> could use:
>> >>
>> >>         hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> >>
>> >> on them, maybe with a module parameter? Note that one of the first
>> >> things the Steam Client does is to disable the virtual devices (with a
>> >> command to the device).
>> >> (TBH I always had the impression that those virtual devices are there
>> >> to move the Grub menu...)
>> >>
>> >> If Valve people really feel that this driver is not needed for SteamOS,
>> >> because the Steam Client is always running, then SteamOS can always do
>> >> CONFIG_HID_STEAM=n.
>>
>> That is what I was expecting, but if the driver also breaks the
>> regular Steam client, we have a situation :)
>>
>> >
>> >
>> > Yes, certainly; that isn't really the usecase I'm worried about. What I'm
>> > worried about is behavior changing for existing users of the controller on
>> > normal desktop distributions. Currently the Steam client will program these
>> > pieces of state (enable/disable direct keyboard/mouse HID functionality,
>> > enable/disable gyro/accel) based on the user's configuration, and a user
>> > getting a kernel update that includes a driver that also programs that
>> > without their intervention is bound to affect the behavior. If there was a
>> > way to make it so the states won't be programmed until it's pretty clear the
>> > user is trying to use that driver's functionality, that would feel safer.
>>
>> I do not think there is a way to know beforehand, given that the
>> kernel module will be loaded first, and sometimes even without the
>> root file system if the driver has been included in the initramfs
>> (which is the point of not having the device blacklisted in
>> hid-quirks.c)
>>
>> I guess the only reasonable solution would be for the kernel HID
>> driver to expose all the interfaces (keyboard, touchpad, gyro, etc...)
>> but not take any action on whether which mode it switches into. Then,
>> (and unfortunatelly) though a custom sysfs API, we could teach SDL or
>> any other configurator to change the device into the desired mode.
>> I would prefer not having a custom sysfs API, but this would allow us
>> to change the owner to a non-root user while hidraw needs to have the
>> root access.
>>
>> An other solution than the sysfs API, would be to have a small driver
>> in libratbag (or a similar daemon) that sends the mode switch command
>> as if the joystick where a special gaming mouse.
>
> Ok, I agree that we cannot remove the keyboard/mouse interfaces. What
> about a module parameter? Something like `disable_lizard_mode` or
> `disable_hid_devices`. That would be 0 by default, so no user-mode
> breakage. BTW, are module parameters considered userspace API stable?

There are 2 issues with parameters modules:
- yes, it's kernel API (if you change it and one user in the South
Pole has it in the grub config and it now breaks the device, it will
be considered as a regression)
- they are usually taken into account at loading time, and you need
root to change them

>
> Note that when this parameter is set, the driver will not send any
> command to the controller, (as Steam Client does). Instead, it will
> simply not call hid_hw_start() on those interfaces. I'm not sure how the
> no-quirks hid-generic will behave in this case... I'll try and see...
>
> In the future, we could add a few other modules paramteres to enable the
> gyro and the force-feedback, as these functions could very well break
> Steam. But these functions will be welcomed by DIY enthusiasts.

The more I think of it, the more I think you need to split the "driver" in 2:
- the kernel part that will be able to understand the raw values from
the device and inject the correct events in the correct input nodes
- the config part that will control how the device behaves. This
config part is the one interfering with Steam, so you might simply
want to have a standalone tool (or in libratbag, and integrate it in
SDL) to send the commands to switch the device into the desired mode.

Note that we all tried to use custom sysfs API for configuring our
fancy input devices, and we all came down to the conclusion that it
was better to leave the kernel to the minimum and have external tool
to talk to the hardware for the config. This way, you can break the
API as you want (it's internal to your tool), and you can also adapt
much quicker to new devices.

Cheers,
Benjamin

>
> Regards.
> Rodrigo
>
>

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Thu, Feb 22, 2018 at 06:06:30PM +0100, Benjamin Tissoires wrote:
> On Thu, Feb 22, 2018 at 5:31 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > On Thu, Feb 22, 2018 at 10:10:40AM +0100, Benjamin Tissoires wrote:
> >> On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais
> >> <pgriffais@valvesoftware.com> wrote:
> >> >
> >> >
> >> > On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
> >> >>
> >> >> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
> >> >>>
> >> >>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
> >> >>>>
> >> >>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
> >> >>>> About the left trackpad/joystick, currently I'm not treating them
> >> >>>> different. I'm out of ABS axes, and anyway, it is not likely that the
> >> >>>> left pad and the joystick be used at the same time (I only have one left
> >> >>>> thumb). Nevertheless, if we really want to make them apart, we can use
> >> >>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
> >> >>>> details in [2], but I'm not currently doing that in this driver.
> >> >>>
> >> >>>
> >> >>> I didn't necessarily mean exposing it, but in the event a user is using
> >> >>> both
> >> >>> at the same time (it happens, using claw grip with some games is
> >> >>> necessary
> >> >>> to use the D-pad while deflecting the stick), then you'll most likely run
> >> >>> into issues unless you demux the inbound data, since we were also out of
> >> >>> left analog fields and mux them together if used at the same time. Not
> >> >>> sure
> >> >>> if you're already handling that case, but not doing it would result in
> >> >>> the
> >> >>> stick appearing to fully deflect every other input report if the user is
> >> >>> doing both at the same time.
> >> >>
> >> >>
> >> >> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
> >> >> right. I think that it will be best to fully separate the left-pad and
> >> >> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
> >> >>
> >> >>>> About the gyroscope/acceleration/quaternion, you know the issue
> >> >>>> that the wireless gives gyro plus quat but no accel, while the wired
> >> >>>> gives all three. My general idea is to create an additional input device
> >> >>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
> >> >>>> the wireless gives no accel, maybe there is some command to enable it?
> >> >>>
> >> >>>
> >> >>> It should be three neighboring bits for that setting; does this not work
> >> >>> for
> >> >>> you?
> >> >>>
> >> >>> // Bitmask that define which IMU features to enable.
> >> >>> typedef enum
> >> >>> {
> >> >>>         SETTING_GYRO_MODE_OFF                           = 0x0000,
> >> >>>         SETTING_GYRO_MODE_STEERING                      = 0x0001,
> >> >>>         SETTING_GYRO_MODE_TILT                          = 0x0002,
> >> >>>         SETTING_GYRO_MODE_SEND_ORIENTATION      = 0x0004,
> >> >>>         SETTING_GYRO_MODE_SEND_RAW_ACCEL        = 0x0008,
> >> >>>         SETTING_GYRO_MODE_SEND_RAW_GYRO         = 0x0010,
> >> >>> } SettingGyroMode;
> >> >>
> >> >>
> >> >> Thanks for that, it will be useful! Those are the values to be written
> >> >> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
> >> >> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
> >> >> bit-field...
> >> >>
> >> >>> In general I'm concerned about sending of the gyro-enable message
> >> >>> potentially impairing Steam functionality if it's running at the same
> >> >>> time
> >> >>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
> >> >>> sending any stateful messages to the device. For instance, are you ever
> >> >>> sending the "blank configuration" setting? I assume you must be or users
> >> >>> would still get keyboard/mouse input over these USB endpoints while
> >> >>> trying
> >> >>> to interact with the controller. But sending this after Steam programmed
> >> >>> a
> >> >>> setting, or instructed the controller to go back to lizard mode because
> >> >>> it
> >> >>> was requested by a configuration, would be problematic.
> >> >>
> >> >>
> >> >> Sure, but this patchset should be safe, shouldn't it?
> >> >> Maube future patches that play with the gyro/force-feedback could be
> >> >> disabled by default, and could need a module parameter to be enabled.
> >> >> That way Steam Client will work out-of-the-box anywhere but proficient
> >> >> users could use the extra functionality easily.
> >> >>
> >> >> Related to that, Benjamin Tissoires replied to 1/3:
> >> >>>>
> >> >>>> --- a/drivers/hid/hid-quirks.c
> >> >>>> +++ b/drivers/hid/hid-quirks.c
> >> >>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
> >> >>>> hid_have_special_driver[] = {
> >> >>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
> >> >>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
> >> >>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
> >> >>>>   #endif
> >> >>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
> >> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
> >> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
> >> >>>> +#endif
> >> >>>
> >> >>>
> >> >>> In addition to the discussion in 0/3, I wonder if you should not
> >> >>> remove this hunk. Unless having hid-generic binding the device before
> >> >>> your hid-steam driver, it would be better not force the Steam boxes to
> >> >>> use your driver.
> >> >>
> >> >>
> >> >> I don't really see the point on that. If we do that I'll have to unbind
> >> >> and bind the device manually, and that is a pain, and impossible without
> >> >> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
> >>
> >> I guess you are not running v4.16-rc2 :) (see Clement answers too)
> >>
> >> Since v4.16, there is no need to absolutely blacklist the devices in
> >> this list. hid-generic will bind them first, but as soon as an other
> >> driver claims the device, hid-generic unbinds itself and let the other
> >> driver to handle the device. Without any user input!
> >
> > No, I'm still in v4.15.3, and I thought I was bleeding edge...
> > I've seen those patches in the hid git tree, but I thought it was
> > experimental. Good to know it is upstream, that quirks array was
> > weird... I'll try to upgrade and see what happens.
> >>
> >> >> And anyway this driver is mostly harmless, the only visible changes from
> >> >> userspace are the new input and power devices, and that the virtual
> >> >> keyboard/mouse are no more.
> >>
> >> Pierre-Loup mentioned that sometime the Steam client needs those
> >> interfaces for the Desktop mode. I have the feeling things are going
> >> to be too intricated for us to gracefully accept the driver unless
> >> there is a clear ACK from Valve that they are happy with the new
> >> driver.
> >>
> >> >> If the virtual devices are really missed we
> >> >> could use:
> >> >>
> >> >>         hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> >> >>
> >> >> on them, maybe with a module parameter? Note that one of the first
> >> >> things the Steam Client does is to disable the virtual devices (with a
> >> >> command to the device).
> >> >> (TBH I always had the impression that those virtual devices are there
> >> >> to move the Grub menu...)
> >> >>
> >> >> If Valve people really feel that this driver is not needed for SteamOS,
> >> >> because the Steam Client is always running, then SteamOS can always do
> >> >> CONFIG_HID_STEAM=n.
> >>
> >> That is what I was expecting, but if the driver also breaks the
> >> regular Steam client, we have a situation :)
> >>
> >> >
> >> >
> >> > Yes, certainly; that isn't really the usecase I'm worried about. What I'm
> >> > worried about is behavior changing for existing users of the controller on
> >> > normal desktop distributions. Currently the Steam client will program these
> >> > pieces of state (enable/disable direct keyboard/mouse HID functionality,
> >> > enable/disable gyro/accel) based on the user's configuration, and a user
> >> > getting a kernel update that includes a driver that also programs that
> >> > without their intervention is bound to affect the behavior. If there was a
> >> > way to make it so the states won't be programmed until it's pretty clear the
> >> > user is trying to use that driver's functionality, that would feel safer.
> >>
> >> I do not think there is a way to know beforehand, given that the
> >> kernel module will be loaded first, and sometimes even without the
> >> root file system if the driver has been included in the initramfs
> >> (which is the point of not having the device blacklisted in
> >> hid-quirks.c)
> >>
> >> I guess the only reasonable solution would be for the kernel HID
> >> driver to expose all the interfaces (keyboard, touchpad, gyro, etc...)
> >> but not take any action on whether which mode it switches into. Then,
> >> (and unfortunatelly) though a custom sysfs API, we could teach SDL or
> >> any other configurator to change the device into the desired mode.
> >> I would prefer not having a custom sysfs API, but this would allow us
> >> to change the owner to a non-root user while hidraw needs to have the
> >> root access.
> >>
> >> An other solution than the sysfs API, would be to have a small driver
> >> in libratbag (or a similar daemon) that sends the mode switch command
> >> as if the joystick where a special gaming mouse.
> >
> > Ok, I agree that we cannot remove the keyboard/mouse interfaces. What
> > about a module parameter? Something like `disable_lizard_mode` or
> > `disable_hid_devices`. That would be 0 by default, so no user-mode
> > breakage. BTW, are module parameters considered userspace API stable?
> 
> There are 2 issues with parameters modules:
> - yes, it's kernel API (if you change it and one user in the South
> Pole has it in the grub config and it now breaks the device, it will
> be considered as a regression)
> - they are usually taken into account at loading time, and you need
> root to change them

Well, that parameter would be using at probe time. So  you will need to
unplug and replug the controller to take effect.

> >
> > Note that when this parameter is set, the driver will not send any
> > command to the controller, (as Steam Client does). Instead, it will
> > simply not call hid_hw_start() on those interfaces. I'm not sure how the
> > no-quirks hid-generic will behave in this case... I'll try and see...
> >
> > In the future, we could add a few other modules paramteres to enable the
> > gyro and the force-feedback, as these functions could very well break
> > Steam. But these functions will be welcomed by DIY enthusiasts.
> 
> The more I think of it, the more I think you need to split the "driver" in 2:
> - the kernel part that will be able to understand the raw values from
> the device and inject the correct events in the correct input nodes
> - the config part that will control how the device behaves. This
> config part is the one interfering with Steam, so you might simply
> want to have a standalone tool (or in libratbag, and integrate it in
> SDL) to send the commands to switch the device into the desired mode.
> 
> Note that we all tried to use custom sysfs API for configuring our
> fancy input devices, and we all came down to the conclusion that it
> was better to leave the kernel to the minimum and have external tool
> to talk to the hardware for the config. This way, you can break the
> API as you want (it's internal to your tool), and you can also adapt
> much quicker to new devices.

Nice idea. If we are relying in user-mode external tools, no need for
sysfs, we already can send commands using hidraw. Maybe I can write a
simple command-line tool to do that. Would adding a link to my github
page with that tool be considered too much self-promotion?

The future situation with the gyro will be trikier, though. Because the
driver should send the enable/disable gyro commands when the
corresponding input-gyro device is opened/closed. And that cannot be
done with a user-mode tool.

But one problem at a time... I'll modify the driver to leave the lizard
mode alone, and see what you all think, ok?

Thank you.
Rodrigo

Re: [PATCH v2 1/3] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Tue, Feb 20, 2018 at 09:32:08PM -0800, Cameron Gutman wrote:
> On 02/20/2018 11:33 AM, Rodrigo Rivas Costa wrote:
> > +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) {
> > +		if (steam->input) {
> > +			dbg_hid("%s: already connected\n", __func__);
> > +			return;
> > +		}
> > +		ret = steam_register(steam);
> > +		if (ret) {
> > +			hid_err(steam->hdev,
> > +				"%s:steam_register failed with error %d\n",
> > +				__func__, ret);
> > +			return;
> > +		}
> > +	} else {
> > +		steam_unregister(steam);
> 
> I think you need synchronization here. You don't want to be in the middle of
> processing a HID event or power supply update and have your device freed out
> from underneath you.
> 
> xpad uses RCU to avoid this race.

Ah, I see, if we get an input message just after the "disconnect"
packet, (unlikely) it could file. I'll don't know RCU very will but I'll
try and do my best.

Please, stay tuned for v3.
Thanks.
Rodrigo

Re: [PATCH v2 0/3] new driver for Valve Steam Controller

[Benjamin Tissoires]

On Thu, Feb 22, 2018 at 6:48 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Thu, Feb 22, 2018 at 06:06:30PM +0100, Benjamin Tissoires wrote:
>> On Thu, Feb 22, 2018 at 5:31 PM, Rodrigo Rivas Costa
>> <rodrigorivascosta@gmail.com> wrote:
>> > On Thu, Feb 22, 2018 at 10:10:40AM +0100, Benjamin Tissoires wrote:
>> >> On Thu, Feb 22, 2018 at 1:13 AM, Pierre-Loup A. Griffais
>> >> <pgriffais@valvesoftware.com> wrote:
>> >> >
>> >> >
>> >> > On 02/21/2018 12:21 PM, Rodrigo Rivas Costa wrote:
>> >> >>
>> >> >> On Tue, Feb 20, 2018 at 04:09:39PM -0800, Pierre-Loup A. Griffais wrote:
>> >> >>>
>> >> >>> On 02/20/2018 03:20 PM, Rodrigo Rivas Costa wrote:
>> >> >>>>
>> >> >>>> On Tue, Feb 20, 2018 at 02:29:48PM -0800, Pierre-Loup A. Griffais wrote:
>> >> >>>> About the left trackpad/joystick, currently I'm not treating them
>> >> >>>> different. I'm out of ABS axes, and anyway, it is not likely that the
>> >> >>>> left pad and the joystick be used at the same time (I only have one left
>> >> >>>> thumb). Nevertheless, if we really want to make them apart, we can use
>> >> >>>> bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) together. I described the
>> >> >>>> details in [2], but I'm not currently doing that in this driver.
>> >> >>>
>> >> >>>
>> >> >>> I didn't necessarily mean exposing it, but in the event a user is using
>> >> >>> both
>> >> >>> at the same time (it happens, using claw grip with some games is
>> >> >>> necessary
>> >> >>> to use the D-pad while deflecting the stick), then you'll most likely run
>> >> >>> into issues unless you demux the inbound data, since we were also out of
>> >> >>> left analog fields and mux them together if used at the same time. Not
>> >> >>> sure
>> >> >>> if you're already handling that case, but not doing it would result in
>> >> >>> the
>> >> >>> stick appearing to fully deflect every other input report if the user is
>> >> >>> doing both at the same time.
>> >> >>
>> >> >>
>> >> >> "Claw grip"? Is that a real thing? Yes, it is! Well, you're totally
>> >> >> right. I think that it will be best to fully separate the left-pad and
>> >> >> the joystick. Maybe the left-pad can be mapped to ABS_HAT1{X,Y]...
>> >> >>
>> >> >>>> About the gyroscope/acceleration/quaternion, you know the issue
>> >> >>>> that the wireless gives gyro plus quat but no accel, while the wired
>> >> >>>> gives all three. My general idea is to create an additional input device
>> >> >>>> with INPUT_PROP_ACCELEROMETER to expose what is available. Pity is that
>> >> >>>> the wireless gives no accel, maybe there is some command to enable it?
>> >> >>>
>> >> >>>
>> >> >>> It should be three neighboring bits for that setting; does this not work
>> >> >>> for
>> >> >>> you?
>> >> >>>
>> >> >>> // Bitmask that define which IMU features to enable.
>> >> >>> typedef enum
>> >> >>> {
>> >> >>>         SETTING_GYRO_MODE_OFF                           = 0x0000,
>> >> >>>         SETTING_GYRO_MODE_STEERING                      = 0x0001,
>> >> >>>         SETTING_GYRO_MODE_TILT                          = 0x0002,
>> >> >>>         SETTING_GYRO_MODE_SEND_ORIENTATION      = 0x0004,
>> >> >>>         SETTING_GYRO_MODE_SEND_RAW_ACCEL        = 0x0008,
>> >> >>>         SETTING_GYRO_MODE_SEND_RAW_GYRO         = 0x0010,
>> >> >>> } SettingGyroMode;
>> >> >>
>> >> >>
>> >> >> Thanks for that, it will be useful! Those are the values to be written
>> >> >> to what I called "register 0x30" with {0x87 0x03 0x30 X 0x00}. Currently
>> >> >> I am writing 0x14 to enable and 0x00 to disable. I suspected it was a
>> >> >> bit-field...
>> >> >>
>> >> >>> In general I'm concerned about sending of the gyro-enable message
>> >> >>> potentially impairing Steam functionality if it's running at the same
>> >> >>> time
>> >> >>> (eg. if gyro gets disabled over the radio while Steam still needs it), or
>> >> >>> sending any stateful messages to the device. For instance, are you ever
>> >> >>> sending the "blank configuration" setting? I assume you must be or users
>> >> >>> would still get keyboard/mouse input over these USB endpoints while
>> >> >>> trying
>> >> >>> to interact with the controller. But sending this after Steam programmed
>> >> >>> a
>> >> >>> setting, or instructed the controller to go back to lizard mode because
>> >> >>> it
>> >> >>> was requested by a configuration, would be problematic.
>> >> >>
>> >> >>
>> >> >> Sure, but this patchset should be safe, shouldn't it?
>> >> >> Maube future patches that play with the gyro/force-feedback could be
>> >> >> disabled by default, and could need a module parameter to be enabled.
>> >> >> That way Steam Client will work out-of-the-box anywhere but proficient
>> >> >> users could use the extra functionality easily.
>> >> >>
>> >> >> Related to that, Benjamin Tissoires replied to 1/3:
>> >> >>>>
>> >> >>>> --- a/drivers/hid/hid-quirks.c
>> >> >>>> +++ b/drivers/hid/hid-quirks.c
>> >> >>>> @@ -629,6 +629,10 @@ static const struct hid_device_id
>> >> >>>> hid_have_special_driver[] = {
>> >> >>>>   #if IS_ENABLED(CONFIG_HID_SPEEDLINK)
>> >> >>>>          { HID_USB_DEVICE(USB_VENDOR_ID_X_TENSIONS,
>> >> >>>>          USB_DEVICE_ID_SPEEDLINK_VAD_CEZANNE) },
>> >> >>>>   #endif
>> >> >>>> +#if IS_ENABLED(CONFIG_HID_STEAM)
>> >> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>> >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER) },
>> >> >>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
>> >> >>>> USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS) },
>> >> >>>> +#endif
>> >> >>>
>> >> >>>
>> >> >>> In addition to the discussion in 0/3, I wonder if you should not
>> >> >>> remove this hunk. Unless having hid-generic binding the device before
>> >> >>> your hid-steam driver, it would be better not force the Steam boxes to
>> >> >>> use your driver.
>> >> >>
>> >> >>
>> >> >> I don't really see the point on that. If we do that I'll have to unbind
>> >> >> and bind the device manually, and that is a pain, and impossible without
>> >> >> root (my ultimate goal is to use this controller with my Tizen TV ;-P).
>> >>
>> >> I guess you are not running v4.16-rc2 :) (see Clement answers too)
>> >>
>> >> Since v4.16, there is no need to absolutely blacklist the devices in
>> >> this list. hid-generic will bind them first, but as soon as an other
>> >> driver claims the device, hid-generic unbinds itself and let the other
>> >> driver to handle the device. Without any user input!
>> >
>> > No, I'm still in v4.15.3, and I thought I was bleeding edge...
>> > I've seen those patches in the hid git tree, but I thought it was
>> > experimental. Good to know it is upstream, that quirks array was
>> > weird... I'll try to upgrade and see what happens.
>> >>
>> >> >> And anyway this driver is mostly harmless, the only visible changes from
>> >> >> userspace are the new input and power devices, and that the virtual
>> >> >> keyboard/mouse are no more.
>> >>
>> >> Pierre-Loup mentioned that sometime the Steam client needs those
>> >> interfaces for the Desktop mode. I have the feeling things are going
>> >> to be too intricated for us to gracefully accept the driver unless
>> >> there is a clear ACK from Valve that they are happy with the new
>> >> driver.
>> >>
>> >> >> If the virtual devices are really missed we
>> >> >> could use:
>> >> >>
>> >> >>         hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>> >> >>
>> >> >> on them, maybe with a module parameter? Note that one of the first
>> >> >> things the Steam Client does is to disable the virtual devices (with a
>> >> >> command to the device).
>> >> >> (TBH I always had the impression that those virtual devices are there
>> >> >> to move the Grub menu...)
>> >> >>
>> >> >> If Valve people really feel that this driver is not needed for SteamOS,
>> >> >> because the Steam Client is always running, then SteamOS can always do
>> >> >> CONFIG_HID_STEAM=n.
>> >>
>> >> That is what I was expecting, but if the driver also breaks the
>> >> regular Steam client, we have a situation :)
>> >>
>> >> >
>> >> >
>> >> > Yes, certainly; that isn't really the usecase I'm worried about. What I'm
>> >> > worried about is behavior changing for existing users of the controller on
>> >> > normal desktop distributions. Currently the Steam client will program these
>> >> > pieces of state (enable/disable direct keyboard/mouse HID functionality,
>> >> > enable/disable gyro/accel) based on the user's configuration, and a user
>> >> > getting a kernel update that includes a driver that also programs that
>> >> > without their intervention is bound to affect the behavior. If there was a
>> >> > way to make it so the states won't be programmed until it's pretty clear the
>> >> > user is trying to use that driver's functionality, that would feel safer.
>> >>
>> >> I do not think there is a way to know beforehand, given that the
>> >> kernel module will be loaded first, and sometimes even without the
>> >> root file system if the driver has been included in the initramfs
>> >> (which is the point of not having the device blacklisted in
>> >> hid-quirks.c)
>> >>
>> >> I guess the only reasonable solution would be for the kernel HID
>> >> driver to expose all the interfaces (keyboard, touchpad, gyro, etc...)
>> >> but not take any action on whether which mode it switches into. Then,
>> >> (and unfortunatelly) though a custom sysfs API, we could teach SDL or
>> >> any other configurator to change the device into the desired mode.
>> >> I would prefer not having a custom sysfs API, but this would allow us
>> >> to change the owner to a non-root user while hidraw needs to have the
>> >> root access.
>> >>
>> >> An other solution than the sysfs API, would be to have a small driver
>> >> in libratbag (or a similar daemon) that sends the mode switch command
>> >> as if the joystick where a special gaming mouse.
>> >
>> > Ok, I agree that we cannot remove the keyboard/mouse interfaces. What
>> > about a module parameter? Something like `disable_lizard_mode` or
>> > `disable_hid_devices`. That would be 0 by default, so no user-mode
>> > breakage. BTW, are module parameters considered userspace API stable?
>>
>> There are 2 issues with parameters modules:
>> - yes, it's kernel API (if you change it and one user in the South
>> Pole has it in the grub config and it now breaks the device, it will
>> be considered as a regression)
>> - they are usually taken into account at loading time, and you need
>> root to change them
>
> Well, that parameter would be using at probe time. So  you will need to
> unplug and replug the controller to take effect.

And this is not an acceptable solution for end users :)
Note that you can also get notified when a parameter changes while the
module is loaded and a device is already bound. But this is the same
than using sysfs with all the cons we already discussed about.

>
>> >
>> > Note that when this parameter is set, the driver will not send any
>> > command to the controller, (as Steam Client does). Instead, it will
>> > simply not call hid_hw_start() on those interfaces. I'm not sure how the
>> > no-quirks hid-generic will behave in this case... I'll try and see...
>> >
>> > In the future, we could add a few other modules paramteres to enable the
>> > gyro and the force-feedback, as these functions could very well break
>> > Steam. But these functions will be welcomed by DIY enthusiasts.
>>
>> The more I think of it, the more I think you need to split the "driver" in 2:
>> - the kernel part that will be able to understand the raw values from
>> the device and inject the correct events in the correct input nodes
>> - the config part that will control how the device behaves. This
>> config part is the one interfering with Steam, so you might simply
>> want to have a standalone tool (or in libratbag, and integrate it in
>> SDL) to send the commands to switch the device into the desired mode.
>>
>> Note that we all tried to use custom sysfs API for configuring our
>> fancy input devices, and we all came down to the conclusion that it
>> was better to leave the kernel to the minimum and have external tool
>> to talk to the hardware for the config. This way, you can break the
>> API as you want (it's internal to your tool), and you can also adapt
>> much quicker to new devices.
>
> Nice idea. If we are relying in user-mode external tools, no need for
> sysfs, we already can send commands using hidraw. Maybe I can write a
> simple command-line tool to do that. Would adding a link to my github
> page with that tool be considered too much self-promotion?

Please go ahead. It's not like we are writing the things in stone, and
on top of that, you are the author of the driver. 2 reasons to not
consider this as self-promotion :)

>
> The future situation with the gyro will be trikier, though. Because the
> driver should send the enable/disable gyro commands when the
> corresponding input-gyro device is opened/closed. And that cannot be
> done with a user-mode tool.

But it can be done directly in the kernel. If user space opens the
gyro, you are called in the .open() callback and you can trigger the
correct modes.
One other solution is to use IIO, but I do not think we like this for
gaming devices.

>
> But one problem at a time... I'll modify the driver to leave the lizard
> mode alone, and see what you all think, ok?

Sounds good.

Cheers,
Benjamin