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 <[email protected]>
---
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 | 480 +++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 497 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..03f912ab5484
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,480 @@
+// 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 <[email protected]>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/usb.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 <[email protected]>");
+
+#define STEAM_QUIRK_WIRELESS BIT(0)
+
+struct steam_device {
+ spinlock_t lock;
+ struct hid_device *hid_dev;
+ struct input_dev *input_dev;
+ unsigned long quirks;
+ struct work_struct work_connect;
+ bool connected;
+};
+
+static int steam_register(struct steam_device *steam);
+static void steam_unregister(struct steam_device *steam);
+static void steam_do_connect_event(struct steam_device *steam, bool connected);
+static void steam_do_input_event(struct steam_device *steam, u8 *data);
+
+static int steam_input_open(struct input_dev *dev)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+
+ return hid_hw_open(steam->hid_dev);
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+
+ hid_hw_close(steam->hid_dev);
+}
+
+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;
+
+ dbg_hid("%s\n", __func__);
+
+ spin_lock_irqsave(&steam->lock, flags);
+ connected = steam->connected;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ if (connected) {
+ if (steam->input_dev) {
+ dbg_hid("%s: already connected\n", __func__);
+ return;
+ }
+ ret = steam_register(steam);
+ if (ret) {
+ hid_err(steam->hid_dev,
+ "%s:steam_register returned error %d\n",
+ __func__, ret);
+ return;
+ }
+ } else {
+ steam_unregister(steam);
+ }
+}
+
+static int steam_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+ struct steam_device *steam;
+ int ret;
+
+ dbg_hid("%s called for ifnum %d protocol %d\n", __func__,
+ intf->cur_altsetting->desc.bInterfaceNumber,
+ intf->cur_altsetting->desc.bInterfaceProtocol
+ );
+
+ /*
+ * 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.
+ * Instead of the interface index we use the protocol, it is 0
+ * for the real game pad.
+ * Since we have a real game pad now, we can ignore the virtual
+ * mouse and keyboard.
+ */
+ if (intf->cur_altsetting->desc.bInterfaceProtocol != 0) {
+ dbg_hid("%s: interface ignored\n", __func__);
+ return -ENODEV;
+ }
+
+ steam = kzalloc(sizeof(struct steam_device), GFP_KERNEL);
+ if (!steam)
+ return -ENOMEM;
+
+ spin_lock_init(&steam->lock);
+ steam->hid_dev = hdev;
+ hid_set_drvdata(hdev, steam);
+ steam->quirks = id->driver_data;
+ INIT_WORK(&steam->work_connect, steam_work_connect_cb);
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev,
+ "%s:parse of hid interface failed\n", __func__);
+ goto hid_parse_fail;
+ }
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hdev,
+ "%s:hid_hw_start returned error\n", __func__);
+ goto hid_hw_start_fail;
+ }
+
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ steam->input_dev = NULL;
+ 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 returned error\n",
+ __func__);
+ goto input_register_fail;
+ }
+ }
+
+ return 0;
+
+input_register_fail:
+hid_hw_open_fail:
+ hid_hw_stop(hdev);
+hid_hw_start_fail:
+hid_parse_fail:
+ cancel_work_sync(&steam->work_connect);
+ kfree(steam);
+ hid_set_drvdata(hdev, NULL);
+ return ret;
+}
+
+static void steam_remove(struct hid_device *hdev)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+
+ dbg_hid("%s\n", __func__);
+
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ hid_info(hdev, "Steam wireless receiver disconnected");
+ hid_hw_close(hdev);
+ }
+ hid_hw_stop(hdev);
+ steam_unregister(steam);
+ cancel_work_sync(&steam->work_connect);
+ kfree(steam);
+ hid_set_drvdata(hdev, NULL);
+}
+
+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 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_dev;
+
+ /* 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, for example, 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_register(struct steam_device *steam)
+{
+ struct hid_device *hdev = steam->hid_dev;
+ struct input_dev *input;
+ int ret;
+
+ dbg_hid("%s\n", __func__);
+
+ 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 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);
+
+ ret = input_register_device(input);
+ if (ret)
+ goto input_register_fail;
+
+ steam->input_dev = input;
+
+ return 0;
+
+input_register_fail:
+ input_free_device(input);
+ return ret;
+}
+
+static void steam_unregister(struct steam_device *steam)
+{
+ dbg_hid("%s\n", __func__);
+
+ if (steam->input_dev) {
+ hid_info(steam->hid_dev, "Steam Controller disconnected");
+ input_unregister_device(steam->input_dev);
+ steam->input_dev = NULL;
+ }
+}
+
+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);
+/* vi: set softtabstop=8 shiftwidth=8 noexpandtab tabstop=8: */
--
2.16.1
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 <[email protected]>
---
drivers/hid/hid-steam.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 86 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 03f912ab5484..f269a986e2be 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -34,12 +34,17 @@ struct steam_device {
unsigned long quirks;
struct work_struct work_connect;
bool connected;
+ char serial_no[11];
};
static int steam_register(struct steam_device *steam);
static void steam_unregister(struct steam_device *steam);
static void steam_do_connect_event(struct steam_device *steam, bool connected);
static void steam_do_input_event(struct steam_device *steam, u8 *data);
+static int steam_send_report(struct steam_device *steam,
+ u8 *cmd, int size);
+static int steam_recv_report(struct steam_device *steam,
+ u8 *data, int size);
static int steam_input_open(struct input_dev *dev)
{
@@ -55,6 +60,78 @@ static void steam_input_close(struct input_dev *dev)
hid_hw_close(steam->hid_dev);
}
+#define STEAM_FEATURE_REPORT_SIZE 65
+
+static int steam_send_report(struct steam_device *steam,
+ u8 *cmd, int size)
+{
+ int retry;
+ int ret;
+ u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, 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->hid_dev, 0,
+ buf, size + 1,
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+ if (ret != -EPIPE)
+ break;
+ dbg_hid("%s: failed, retrying (%d times)\n", __func__, retry+1);
+ steam_recv_report(steam, NULL, 0);
+ msleep(50);
+ }
+ kfree(buf);
+ return ret;
+}
+
+static int steam_recv_report(struct steam_device *steam,
+ u8 *data, int size)
+{
+ int ret;
+ u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
+
+ if (!buf)
+ return -ENOMEM;
+
+ /* The report ID is always 0 */
+ ret = hid_hw_raw_request(steam->hid_dev, 0x00,
+ buf, STEAM_FEATURE_REPORT_SIZE,
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+ memcpy(data, buf + 1, size);
+ kfree(buf);
+ 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 void steam_work_connect_cb(struct work_struct *work)
{
struct steam_device *steam = container_of(work, struct steam_device,
@@ -385,7 +462,12 @@ static int steam_register(struct steam_device *steam)
dbg_hid("%s\n", __func__);
- hid_info(hdev, "Steam Controller connected");
+ ret = steam_get_serial(steam);
+ if (ret)
+ return ret;
+
+ hid_info(hdev, "Steam Controller SN: '%s' connected",
+ steam->serial_no);
input = input_allocate_device();
if (!input)
@@ -398,7 +480,7 @@ static int steam_register(struct steam_device *steam)
input->name = "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;
@@ -447,7 +529,8 @@ static void steam_unregister(struct steam_device *steam)
dbg_hid("%s\n", __func__);
if (steam->input_dev) {
- hid_info(steam->hid_dev, "Steam Controller disconnected");
+ hid_info(steam->hid_dev, "Steam Controller SN: '%s' disconnected",
+ steam->serial_no);
input_unregister_device(steam->input_dev);
steam->input_dev = NULL;
}
--
2.16.1
The wireless Steam Controller is battery operated, so add the battery
device and power information.
Signed-off-by: Rodrigo Rivas Costa <[email protected]>
---
drivers/hid/hid-steam.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 138 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index f269a986e2be..eb2424688092 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -20,6 +20,7 @@
#include <linux/hid.h>
#include <linux/module.h>
#include <linux/workqueue.h>
+#include <linux/power_supply.h>
#include "hid-ids.h"
MODULE_LICENSE("GPL");
@@ -35,6 +36,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_register(struct steam_device *steam);
@@ -45,6 +50,8 @@ static int steam_send_report(struct steam_device *steam,
u8 *cmd, int size);
static int steam_recv_report(struct steam_device *steam,
u8 *data, int size);
+static int steam_battery_register(struct steam_device *steam);
+static void steam_do_battery_event(struct steam_device *steam, u8 *data);
static int steam_input_open(struct input_dev *dev)
{
@@ -311,7 +318,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;
@@ -517,6 +525,10 @@ static int steam_register(struct steam_device *steam)
steam->input_dev = input;
+ /* ignore battery errors, we can live without it */
+ if (steam->quirks & STEAM_QUIRK_WIRELESS)
+ steam_battery_register(steam);
+
return 0;
input_register_fail:
@@ -528,6 +540,12 @@ static void steam_unregister(struct steam_device *steam)
{
dbg_hid("%s\n", __func__);
+ if (steam->battery) {
+ power_supply_unregister(steam->battery);
+ steam->battery = NULL;
+ kfree(steam->battery_desc.name);
+ steam->battery_desc.name = NULL;
+ }
if (steam->input_dev) {
hid_info(steam->hid_dev, "Steam Controller SN: '%s' disconnected",
steam->serial_no);
@@ -536,6 +554,125 @@ static void steam_unregister(struct steam_device *steam)
}
}
+/* 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];
+
+ dbg_hid("%s: %d %d\n", __func__, volts, batt);
+
+ if (unlikely(!steam->battery)) {
+ dbg_hid("%s: battery data without connect event\n", __func__);
+ steam_do_connect_event(steam, true);
+ 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 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;
+
+ dbg_hid("%s\n", __func__);
+
+ 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 = kasprintf(GFP_KERNEL,
+ "steam-controller-%s-battery", steam->serial_no);
+ if (!steam->battery_desc.name) {
+ ret = -ENOMEM;
+ goto print_name_fail;
+ }
+
+ /* 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->hid_dev->dev,
+ &steam->battery_desc, &battery_cfg);
+ if (IS_ERR(battery)) {
+ ret = PTR_ERR(battery);
+ hid_err(steam->hid_dev,
+ "%s:power_supply_register returned error %d\n",
+ __func__, ret);
+ goto power_supply_reg_fail;
+ }
+ steam->battery = battery;
+ power_supply_powers(steam->battery, &steam->hid_dev->dev);
+ return 0;
+
+power_supply_reg_fail:
+ kfree(steam->battery_desc.name);
+ steam->battery_desc.name = NULL;
+print_name_fail:
+ return ret;
+}
+
static const struct hid_device_id steam_controllers[] = {
{ /* Wired Steam Controller */
HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
--
2.16.1
Hi Rodrigo,
On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
<[email protected]> 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.
I think I had a look at this a while ago, and didn't want to interfere
with SteamOS regarding this. I think your patch should be fine in that
regard, but have you tried SteamOS on a kernel patched with your
series? Does it behave properly or will it break?
See more comments inlined.
>
> 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 <[email protected]>
> ---
> 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 | 480 +++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 497 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..03f912ab5484
> --- /dev/null
> +++ b/drivers/hid/hid-steam.c
> @@ -0,0 +1,480 @@
> +// SPDX-License-Identifier: GPL-2.0
Non standard header
> +/*
> + * HID driver for Valve Steam Controller
> + *
> + * Supports both the wired and wireless interfaces.
> + *
> + * Copyright (c) 2018 Rodrigo Rivas Costa <[email protected]>
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/usb.h>
Generally, this raises a red flag from my side. HID should be
transport agnostic and depending on usb.h kills this. The advantage of
not depending on USB is that we can replay the devices with uhid in a
way it still works.
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include "hid-ids.h"
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Rodrigo Rivas Costa <[email protected]>");
> +
> +#define STEAM_QUIRK_WIRELESS BIT(0)
> +
> +struct steam_device {
> + spinlock_t lock;
> + struct hid_device *hid_dev;
> + struct input_dev *input_dev;
> + unsigned long quirks;
> + struct work_struct work_connect;
> + bool connected;
> +};
> +
> +static int steam_register(struct steam_device *steam);
> +static void steam_unregister(struct steam_device *steam);
> +static void steam_do_connect_event(struct steam_device *steam, bool connected);
> +static void steam_do_input_event(struct steam_device *steam, u8 *data);
We tend to generally not do forward declaration of functions in the
kernel. Unless really necessary.
> +
> +static int steam_input_open(struct input_dev *dev)
> +{
> + struct steam_device *steam = input_get_drvdata(dev);
> +
> + return hid_hw_open(steam->hid_dev);
> +}
> +
> +static void steam_input_close(struct input_dev *dev)
> +{
> + struct steam_device *steam = input_get_drvdata(dev);
> +
> + hid_hw_close(steam->hid_dev);
> +}
> +
> +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;
> +
> + dbg_hid("%s\n", __func__);
This is pure debugging, and should be stripped out of the code (you
can use ftrace to see if your code is called BTW).
> +
> + spin_lock_irqsave(&steam->lock, flags);
> + connected = steam->connected;
> + spin_unlock_irqrestore(&steam->lock, flags);
> +
> + if (connected) {
> + if (steam->input_dev) {
> + dbg_hid("%s: already connected\n", __func__);
> + return;
> + }
> + ret = steam_register(steam);
> + if (ret) {
> + hid_err(steam->hid_dev,
> + "%s:steam_register returned error %d\n",
> + __func__, ret);
> + return;
> + }
> + } else {
> + steam_unregister(steam);
> + }
> +}
> +
> +static int steam_probe(struct hid_device *hdev,
> + const struct hid_device_id *id)
> +{
> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> + struct steam_device *steam;
> + int ret;
> +
> + dbg_hid("%s called for ifnum %d protocol %d\n", __func__,
> + intf->cur_altsetting->desc.bInterfaceNumber,
> + intf->cur_altsetting->desc.bInterfaceProtocol
> + );
> +
> + /*
> + * 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.
> + * Instead of the interface index we use the protocol, it is 0
> + * for the real game pad.
> + * Since we have a real game pad now, we can ignore the virtual
> + * mouse and keyboard.
> + */
> + if (intf->cur_altsetting->desc.bInterfaceProtocol != 0) {
> + dbg_hid("%s: interface ignored\n", __func__);
> + return -ENODEV;
As mentioned above, I'd rather you to decide on whether ignoring or
not the interface based on the actual reports, not the place in the
USB device. Valve can also decide to change their USB stack design,
and you'll have to amend this.
If you check that the reports ID you are expecting are not in the HID
device, you should be fine (hopefully)
> + }
> +
> + steam = kzalloc(sizeof(struct steam_device), GFP_KERNEL);
Please use devres to not have to free the memory on fail or on
disconnect (look for devm_kzalloc in the hid tree for examples).
> + if (!steam)
> + return -ENOMEM;
> +
> + spin_lock_init(&steam->lock);
> + steam->hid_dev = hdev;
> + hid_set_drvdata(hdev, steam);
> + steam->quirks = id->driver_data;
> + INIT_WORK(&steam->work_connect, steam_work_connect_cb);
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev,
> + "%s:parse of hid interface failed\n", __func__);
> + goto hid_parse_fail;
> + }
> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> + if (ret) {
> + hid_err(hdev,
> + "%s:hid_hw_start returned error\n", __func__);
> + goto hid_hw_start_fail;
> + }
> +
> + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> + steam->input_dev = NULL;
This seems superfluous
> + 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 returned error\n",
> + __func__);
> + goto input_register_fail;
> + }
> + }
> +
> + return 0;
> +
> +input_register_fail:
> +hid_hw_open_fail:
> + hid_hw_stop(hdev);
> +hid_hw_start_fail:
> +hid_parse_fail:
> + cancel_work_sync(&steam->work_connect);
> + kfree(steam);
> + hid_set_drvdata(hdev, NULL);
> + return ret;
> +}
> +
> +static void steam_remove(struct hid_device *hdev)
> +{
> + struct steam_device *steam = hid_get_drvdata(hdev);
> +
> + dbg_hid("%s\n", __func__);
please remove
> +
> + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> + hid_info(hdev, "Steam wireless receiver disconnected");
> + hid_hw_close(hdev);
> + }
> + hid_hw_stop(hdev);
> + steam_unregister(steam);
shouldn't you unregister the input nodes *after* cancelling the work
that might use it (haven't fully read the code, so it might be fine)
> + cancel_work_sync(&steam->work_connect);
> + kfree(steam);
> + hid_set_drvdata(hdev, NULL);
> +}
> +
> +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?
Offset 0 is usually the report ID.
> + * 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 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 beginning of the comment should be on its own line:
/*
* The size ...
> + * 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_dev;
> +
> + /* 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, for example, 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_register(struct steam_device *steam)
> +{
> + struct hid_device *hdev = steam->hid_dev;
> + struct input_dev *input;
> + int ret;
> +
> + dbg_hid("%s\n", __func__);
please remove
> +
> + hid_info(hdev, "Steam Controller connected");
> +
> + input = input_allocate_device();
Don't you need one input node per wireless gamepad too?
> + 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 Controller";
In case of the wireless controllers, you might want to personalize this a bit.
> + 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);
You are also probably missing the resolution bits. We need to
accurately report the physical dimensions to the user space (thanks to
the resolution)
> + input_set_abs_params(input, ABS_RX, -32767, 32767, 0, 0);
> + input_set_abs_params(input, ABS_RY, -32767, 32767, 0, 0);
What do RX/RY correspond to?
> + input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0);
> + input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0);
> +
> + ret = input_register_device(input);
> + if (ret)
> + goto input_register_fail;
> +
> + steam->input_dev = input;
> +
> + return 0;
> +
> +input_register_fail:
> + input_free_device(input);
> + return ret;
> +}
> +
> +static void steam_unregister(struct steam_device *steam)
> +{
> + dbg_hid("%s\n", __func__);
please remove
> +
> + if (steam->input_dev) {
> + hid_info(steam->hid_dev, "Steam Controller disconnected");
> + input_unregister_device(steam->input_dev);
> + steam->input_dev = NULL;
> + }
> +}
> +
> +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);
> +/* vi: set softtabstop=8 shiftwidth=8 noexpandtab tabstop=8: */
Non standard comment, useful, but we strip out these in upstream
Anyway. Thanks for the driver, there are a few bits to fix, nothing
scary though.
Cheers,
Benjamin
> --
> 2.16.1
>
On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
<[email protected]> wrote:
> 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 <[email protected]>
> ---
> drivers/hid/hid-steam.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 86 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 03f912ab5484..f269a986e2be 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -34,12 +34,17 @@ struct steam_device {
> unsigned long quirks;
> struct work_struct work_connect;
> bool connected;
> + char serial_no[11];
> };
>
> static int steam_register(struct steam_device *steam);
> static void steam_unregister(struct steam_device *steam);
> static void steam_do_connect_event(struct steam_device *steam, bool connected);
> static void steam_do_input_event(struct steam_device *steam, u8 *data);
> +static int steam_send_report(struct steam_device *steam,
> + u8 *cmd, int size);
> +static int steam_recv_report(struct steam_device *steam,
> + u8 *data, int size);
>
> static int steam_input_open(struct input_dev *dev)
> {
> @@ -55,6 +60,78 @@ static void steam_input_close(struct input_dev *dev)
> hid_hw_close(steam->hid_dev);
> }
>
> +#define STEAM_FEATURE_REPORT_SIZE 65
> +
> +static int steam_send_report(struct steam_device *steam,
> + u8 *cmd, int size)
> +{
> + int retry;
> + int ret;
> + u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
Please use hid_alloc_report_buf() as sometimes we need to allocate a
slightly bigger report.
> +
> + 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->hid_dev, 0,
> + buf, size + 1,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> + if (ret != -EPIPE)
> + break;
> + dbg_hid("%s: failed, retrying (%d times)\n", __func__, retry+1);
> + steam_recv_report(steam, NULL, 0);
> + msleep(50);
> + }
> + kfree(buf);
> + return ret;
> +}
> +
> +static int steam_recv_report(struct steam_device *steam,
> + u8 *data, int size)
> +{
> + int ret;
> + u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
same her hid_alloc_report_buf should be used
> +
> + if (!buf)
> + return -ENOMEM;
> +
> + /* The report ID is always 0 */
> + ret = hid_hw_raw_request(steam->hid_dev, 0x00,
> + buf, STEAM_FEATURE_REPORT_SIZE,
> + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> + memcpy(data, buf + 1, size);
> + kfree(buf);
> + 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 void steam_work_connect_cb(struct work_struct *work)
> {
> struct steam_device *steam = container_of(work, struct steam_device,
> @@ -385,7 +462,12 @@ static int steam_register(struct steam_device *steam)
>
> dbg_hid("%s\n", __func__);
>
> - hid_info(hdev, "Steam Controller connected");
> + ret = steam_get_serial(steam);
> + if (ret)
> + return ret;
> +
> + hid_info(hdev, "Steam Controller SN: '%s' connected",
> + steam->serial_no);
>
> input = input_allocate_device();
> if (!input)
> @@ -398,7 +480,7 @@ static int steam_register(struct steam_device *steam)
>
> input->name = "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;
> @@ -447,7 +529,8 @@ static void steam_unregister(struct steam_device *steam)
> dbg_hid("%s\n", __func__);
>
> if (steam->input_dev) {
> - hid_info(steam->hid_dev, "Steam Controller disconnected");
> + hid_info(steam->hid_dev, "Steam Controller SN: '%s' disconnected",
> + steam->serial_no);
> input_unregister_device(steam->input_dev);
> steam->input_dev = NULL;
> }
> --
> 2.16.1
>
Rest is OK (minus the multi lines comments as pointed in 1/3)
Cheers,
Benjamin
On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
<[email protected]> wrote:
> The wireless Steam Controller is battery operated, so add the battery
> device and power information.
>
> Signed-off-by: Rodrigo Rivas Costa <[email protected]>
> ---
> drivers/hid/hid-steam.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index f269a986e2be..eb2424688092 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -20,6 +20,7 @@
> #include <linux/hid.h>
> #include <linux/module.h>
> #include <linux/workqueue.h>
> +#include <linux/power_supply.h>
> #include "hid-ids.h"
>
> MODULE_LICENSE("GPL");
> @@ -35,6 +36,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_register(struct steam_device *steam);
> @@ -45,6 +50,8 @@ static int steam_send_report(struct steam_device *steam,
> u8 *cmd, int size);
> static int steam_recv_report(struct steam_device *steam,
> u8 *data, int size);
> +static int steam_battery_register(struct steam_device *steam);
> +static void steam_do_battery_event(struct steam_device *steam, u8 *data);
>
> static int steam_input_open(struct input_dev *dev)
> {
> @@ -311,7 +318,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;
> @@ -517,6 +525,10 @@ static int steam_register(struct steam_device *steam)
>
> steam->input_dev = input;
>
> + /* ignore battery errors, we can live without it */
> + if (steam->quirks & STEAM_QUIRK_WIRELESS)
> + steam_battery_register(steam);
> +
> return 0;
>
> input_register_fail:
> @@ -528,6 +540,12 @@ static void steam_unregister(struct steam_device *steam)
> {
> dbg_hid("%s\n", __func__);
>
> + if (steam->battery) {
> + power_supply_unregister(steam->battery);
> + steam->battery = NULL;
> + kfree(steam->battery_desc.name);
> + steam->battery_desc.name = NULL;
> + }
> if (steam->input_dev) {
> hid_info(steam->hid_dev, "Steam Controller SN: '%s' disconnected",
> steam->serial_no);
> @@ -536,6 +554,125 @@ static void steam_unregister(struct steam_device *steam)
> }
> }
>
> +/* 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];
> +
> + dbg_hid("%s: %d %d\n", __func__, volts, batt);
> +
> + if (unlikely(!steam->battery)) {
> + dbg_hid("%s: battery data without connect event\n", __func__);
> + steam_do_connect_event(steam, true);
> + 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 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;
> +
> + dbg_hid("%s\n", __func__);
> +
> + 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 = kasprintf(GFP_KERNEL,
> + "steam-controller-%s-battery", steam->serial_no);
Please devm_kasprintf() so you don't have to free the memory later.
Cheers,
Benjamin
> + if (!steam->battery_desc.name) {
> + ret = -ENOMEM;
> + goto print_name_fail;
> + }
> +
> + /* 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->hid_dev->dev,
> + &steam->battery_desc, &battery_cfg);
> + if (IS_ERR(battery)) {
> + ret = PTR_ERR(battery);
> + hid_err(steam->hid_dev,
> + "%s:power_supply_register returned error %d\n",
> + __func__, ret);
> + goto power_supply_reg_fail;
> + }
> + steam->battery = battery;
> + power_supply_powers(steam->battery, &steam->hid_dev->dev);
> + return 0;
> +
> +power_supply_reg_fail:
> + kfree(steam->battery_desc.name);
> + steam->battery_desc.name = NULL;
> +print_name_fail:
> + return ret;
> +}
> +
> static const struct hid_device_id steam_controllers[] = {
> { /* Wired Steam Controller */
> HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> --
> 2.16.1
>
Benjamin, Rodrigo,
On Wed, Feb 14, 2018 at 3:45 PM, Benjamin Tissoires
<[email protected]> wrote:
> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa <[email protected]> wrote:
<snip>
>> --- /dev/null
>> +++ b/drivers/hid/hid-steam.c
>> @@ -0,0 +1,480 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> Non standard header
Benjamin:
What do you mean by this?
This is following the proper style for this line as documented (and
discussed on list at great length) [1]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
>> +/*
>> + * HID driver for Valve Steam Controller
>> + *
>> + * Supports both the wired and wireless interfaces.
>> + *
>> + * Copyright (c) 2018 Rodrigo Rivas Costa <[email protected]>
>> + */
>> +
>> +/*
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the Free
>> + * Software Foundation; either version 2 of the License, or (at your option)
>> + * any later version.
>> + */
Rodrigo,
Since you used the proper SPDX tag (in the proper style as explained
in the doc), you can remove this boilerplate alright as it does double
duty with the tag.
--
Cordially
Philippe Ombredanne
On Wed, Feb 14, 2018 at 03:45:14PM +0100, Benjamin Tissoires wrote:
> I think I had a look at this a while ago, and didn't want to interfere
> with SteamOS regarding this. I think your patch should be fine in that
> regard, but have you tried SteamOS on a kernel patched with your
> series? Does it behave properly or will it break?
Well, SteamOS is just a modified Debian with the Steam Client running in
Big Picture mode (fullscreen). I tried the Steam Client with this driver
and it works just fine:
- The first thing the Steam Client (SC) does is to disable the virtual
keyboard and mouse, so not creating those input devices make no
difference.
- Input events are sent both to the libusb (or whatever SC uses) and
this driver.
- The only source of conflict would be in sending commands to the
controller. But currently the only command sent is the get_serial, and
that seems to do no harm.
> > +
> > + hid_info(hdev, "Steam Controller connected");
> > +
> > + input = input_allocate_device();
>
> Don't you need one input node per wireless gamepad too?
>
No, the wired and wireless methods are two different USB devices.
The wireless adaptor is a small usb device that creates 4 HID interfaces
for the 4 to-be-connected controllers. When the 'connected' event
arrives on one of those interfaces we will create one input device.
Another controller connected will use a different interface so another
unrelated input device will be created.
The wired adaptor is the controller connected directly to USB: it
creates one HID inteface and one input device directly.
Anyway, one steam_device will contain 0 or 1 input_device.
> > + input->name = "Steam Controller";
>
> In case of the wireless controllers, you might want to personalize this a bit.
Do you mean e.g. "Wireless Steam Controller"? Would be wise to add the serial
number here (from PATCH 2/3?)? Or is the 'uniq' enough for that?
> > + 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);
>
> You are also probably missing the resolution bits. We need to
> accurately report the physical dimensions to the user space (thanks to
> the resolution)
>
> > + input_set_abs_params(input, ABS_RX, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_RY, -32767, 32767, 0, 0);
>
> What do RX/RY correspond to?
This gamepad has 1 joystick and 2 touchpads (lpad and rpad).
I'm mapping the joystick/lpad to ABS_X/ABS_Y and the rpad to
ABS_RX/ABS_RY.
About the resolution, I looked around other drivers and many have 0 as
resolution... is it necessary only in the pads or the joystick too?
And what are the units of that value?
For reference, the pads are round and exactly 40 mm in diameter.
Why the lpad and the joystick are mapped to the same axes? Well, because
the device returns them at the same offset. We could make them
apart by using bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) but I
don't think it is worth it... you use the joystick and the lpad with the
same thumb!
> Anyway. Thanks for the driver, there are a few bits to fix, nothing
> scary though.
Great, I'll correct all those issues and resubmit the patches in a few
days. It is my very first driver and I'm still a bit slow...
Thank you for your attention!
Rodrigo.
On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:
> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
> > +#define STEAM_FEATURE_REPORT_SIZE 65
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > + u8 *cmd, int size)
> > +{
> > + int retry;
> > + int ret;
> > + u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
>
> Please use hid_alloc_report_buf() as sometimes we need to allocate a
> slightly bigger report.
I have an issue with this one. The problem is that using
hid_report_len() on the feature report returns 64. But I must call
hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
Currently I'm allocating a buffer of 65 bytes and all is well.
If I change to hid_alloc_report_buf(), the current implementation
allocates (64+7), so I'm still safe. But I'm worried that the extra
bytes are not guaranteed and a future implementation could return
exactly 64 bytes, leaving me 1 byte short.
About why an array of 65 is required for a report of size 64, I think it
is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
So what would be the proper solution?
Thanks.
Rodrigo.
On Thu, Feb 15, 2018 at 11:16 PM, Rodrigo Rivas Costa
<[email protected]> wrote:
> On Wed, Feb 14, 2018 at 03:51:31PM +0100, Benjamin Tissoires wrote:
>> On Tue, Feb 13, 2018 at 1:03 PM, Rodrigo Rivas Costa
>> > +#define STEAM_FEATURE_REPORT_SIZE 65
>> > +
>> > +static int steam_send_report(struct steam_device *steam,
>> > + u8 *cmd, int size)
>> > +{
>> > + int retry;
>> > + int ret;
>> > + u8 *buf = kzalloc(STEAM_FEATURE_REPORT_SIZE, GFP_KERNEL);
>>
>> Please use hid_alloc_report_buf() as sometimes we need to allocate a
>> slightly bigger report.
>
> I have an issue with this one. The problem is that using
> hid_report_len() on the feature report returns 64. But I must call
> hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
>
> Currently I'm allocating a buffer of 65 bytes and all is well.
> If I change to hid_alloc_report_buf(), the current implementation
> allocates (64+7), so I'm still safe. But I'm worried that the extra
> bytes are not guaranteed and a future implementation could return
> exactly 64 bytes, leaving me 1 byte short.
>
> About why an array of 65 is required for a report of size 64, I think it
> is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
That's the other way around actually. If you are just using the output
of hid_report_len(), it will take into account the extra byte for the
report ID.
*But*, given the way implement() is working (see the comment in the
implementation of hid_alloc_report()), you need to have up to 7 extra
bytes to not have the EOVERFLOW.
So if we ever change the implement() function (which is *really*
unlikely), we will have to make sure hid_alloc_report() still works,
so you are on the safe side if you use hid_alloc_report().
>
> So what would be the proper solution?
hid_alloc_report() is the one you want :)
And generally speaking, using the internal API to deal with reports
and others is the safe bet, as you are guaranteed to not being broken
if the API changes (or it'll be a regression and we will have to take
this as high priority).
Cheers,
Benjamin
>
> Thanks.
> Rodrigo.
On Fri, Feb 16, 2018 at 09:44:34AM +0100, Benjamin Tissoires wrote:
> > I have an issue with this one. The problem is that using
> > hid_report_len() on the feature report returns 64. But I must call
> > hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
> >
> > Currently I'm allocating a buffer of 65 bytes and all is well.
> > If I change to hid_alloc_report_buf(), the current implementation
> > allocates (64+7), so I'm still safe. But I'm worried that the extra
> > bytes are not guaranteed and a future implementation could return
> > exactly 64 bytes, leaving me 1 byte short.
> >
> > About why an array of 65 is required for a report of size 64, I think it
> > is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
>
> That's the other way around actually. If you are just using the output
> of hid_report_len(), it will take into account the extra byte for the
> report ID.
> *But*, given the way implement() is working (see the comment in the
> implementation of hid_alloc_report()), you need to have up to 7 extra
> bytes to not have the EOVERFLOW.
>
> So if we ever change the implement() function (which is *really*
> unlikely), we will have to make sure hid_alloc_report() still works,
> so you are on the safe side if you use hid_alloc_report().
Ok, I'll do that. The weird thing, however, is that:
hid_hw_raw_request(steam->hid_dev, 0x00,
buf, hid_report_len(r), /* 64 */
HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
fails with EOVERFLOW. I have to use:
hid_hw_raw_request(steam->hid_dev, 0x00,
buf, 65
HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
which just feels wrong to me.
And looking around drivers/hid/*.c I see that most calls to
hid_hw_raw_request(..., HID_REQ_GET_REPORT) use a buffer allocated with
{devm_,}kzalloc() and a constant length, never using
hid_alloc_report_buf() or hid_report_len().
Maybe there is a bug in hid_hw_raw_request() and it should add 1 to the
given buffer len? But then, custom buffer allocations will overflow by
one!
Rodrigo.
On Fri, Feb 16, 2018 at 10:02 AM, Rodrigo Rivas Costa
<[email protected]> wrote:
> On Fri, Feb 16, 2018 at 09:44:34AM +0100, Benjamin Tissoires wrote:
>> > I have an issue with this one. The problem is that using
>> > hid_report_len() on the feature report returns 64. But I must call
>> > hid_hw_raw_request() with 65 or it will fail with EOVERFLOW.
>> >
>> > Currently I'm allocating a buffer of 65 bytes and all is well.
>> > If I change to hid_alloc_report_buf(), the current implementation
>> > allocates (64+7), so I'm still safe. But I'm worried that the extra
>> > bytes are not guaranteed and a future implementation could return
>> > exactly 64 bytes, leaving me 1 byte short.
>> >
>> > About why an array of 65 is required for a report of size 64, I think it
>> > is related to hid_report->id == 0 (so hid_report_enum->numbered == 0).
>>
>> That's the other way around actually. If you are just using the output
>> of hid_report_len(), it will take into account the extra byte for the
>> report ID.
>> *But*, given the way implement() is working (see the comment in the
>> implementation of hid_alloc_report()), you need to have up to 7 extra
>> bytes to not have the EOVERFLOW.
>>
>> So if we ever change the implement() function (which is *really*
>> unlikely), we will have to make sure hid_alloc_report() still works,
>> so you are on the safe side if you use hid_alloc_report().
>
> Ok, I'll do that. The weird thing, however, is that:
>
> hid_hw_raw_request(steam->hid_dev, 0x00,
> buf, hid_report_len(r), /* 64 */
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> fails with EOVERFLOW. I have to use:
>
> hid_hw_raw_request(steam->hid_dev, 0x00,
> buf, 65
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> which just feels wrong to me.
Indeed
>
> And looking around drivers/hid/*.c I see that most calls to
> hid_hw_raw_request(..., HID_REQ_GET_REPORT) use a buffer allocated with
> {devm_,}kzalloc() and a constant length, never using
> hid_alloc_report_buf() or hid_report_len().
well, hid-input.c and hid-multitouch.c are using
hid_alloc_report_buf() and these two are the most generic ones. We
haven't converted everybody to use hid_alloc_report_buf(), but it's
not a reason to not use it for new drivers :)
>
> Maybe there is a bug in hid_hw_raw_request() and it should add 1 to the
> given buffer len? But then, custom buffer allocations will overflow by
> one!
No, it's unlikely that there is a bug. Can you forward to me the
report descriptors of the Steam controller (ideally with the tool
hid-recorder from http://bentiss.github.io/hid-replay-docs/, so I can
get a few events too)?
Cheers,
Benjamin
>
> Rodrigo.
On Fri, Feb 16, 2018 at 10:31:35AM +0100, Benjamin Tissoires wrote:
> > Ok, I'll do that. The weird thing, however, is that:
> >
> > hid_hw_raw_request(steam->hid_dev, 0x00,
> > buf, hid_report_len(r), /* 64 */
> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >
> > fails with EOVERFLOW. I have to use:
> >
> > hid_hw_raw_request(steam->hid_dev, 0x00,
> > buf, 65
> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >
> > which just feels wrong to me.
>
> Indeed
>
> >
> > And looking around drivers/hid/*.c I see that most calls to
> > hid_hw_raw_request(..., HID_REQ_GET_REPORT) use a buffer allocated with
> > {devm_,}kzalloc() and a constant length, never using
> > hid_alloc_report_buf() or hid_report_len().
>
> well, hid-input.c and hid-multitouch.c are using
> hid_alloc_report_buf() and these two are the most generic ones. We
> haven't converted everybody to use hid_alloc_report_buf(), but it's
> not a reason to not use it for new drivers :)
Agreed. And they use hid_report_len() on the hid_hw_raw_request().
Now my guess is that hid_report_len() should return 65 instead of 64.
And it would do that if the report.id would be >0, but, alas, it is =0.
Maybe feature reports should add 1 without checking id>0? Or maybe
the Steam report descriptor is wrong and I should use report_fixup or
something?
> No, it's unlikely that there is a bug. Can you forward to me the
> report descriptors of the Steam controller (ideally with the tool
> hid-recorder from http://bentiss.github.io/hid-replay-docs/, so I can
> get a few events too)?
Sure, see the attached file. It is the wired one, I opened jstest and
moved the joystick around to get a few events. Unfortunately the
HID_REQ_GET_REPORT are not recorded.
For the casual reader, here is the decoded report descriptor [1]:
0x06, 0x00, 0xFF, // Usage Page (Vendor Defined 0xFF00)
0x09, 0x01, // Usage (0x01)
0xA1, 0x01, // Collection (Application)
0x15, 0x00, // Logical Minimum (0)
0x26, 0xFF, 0x00, // Logical Maximum (255)
0x75, 0x08, // Report Size (8)
0x95, 0x40, // Report Count (64)
0x09, 0x01, // Usage (0x01)
0x81, 0x02, // Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0x95, 0x40, // Report Count (64)
0x09, 0x01, // Usage (0x01)
0x91, 0x02, // Output (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
0x95, 0x40, // Report Count (64)
0x09, 0x01, // Usage (0x01)
0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
0xC0, // End Collection
Regards.
Rodrigo
[1]: http://eleccelerator.com/usbdescreqparser/
On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
<[email protected]> wrote:
> On Fri, Feb 16, 2018 at 10:31:35AM +0100, Benjamin Tissoires wrote:
>> > Ok, I'll do that. The weird thing, however, is that:
>> >
>> > hid_hw_raw_request(steam->hid_dev, 0x00,
>> > buf, hid_report_len(r), /* 64 */
>> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>> >
>> > fails with EOVERFLOW. I have to use:
>> >
>> > hid_hw_raw_request(steam->hid_dev, 0x00,
>> > buf, 65
>> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>> >
>> > which just feels wrong to me.
>>
>> Indeed
>>
>> >
>> > And looking around drivers/hid/*.c I see that most calls to
>> > hid_hw_raw_request(..., HID_REQ_GET_REPORT) use a buffer allocated with
>> > {devm_,}kzalloc() and a constant length, never using
>> > hid_alloc_report_buf() or hid_report_len().
>>
>> well, hid-input.c and hid-multitouch.c are using
>> hid_alloc_report_buf() and these two are the most generic ones. We
>> haven't converted everybody to use hid_alloc_report_buf(), but it's
>> not a reason to not use it for new drivers :)
>
> Agreed. And they use hid_report_len() on the hid_hw_raw_request().
>
> Now my guess is that hid_report_len() should return 65 instead of 64.
> And it would do that if the report.id would be >0, but, alas, it is =0.
> Maybe feature reports should add 1 without checking id>0? Or maybe
> the Steam report descriptor is wrong and I should use report_fixup or
> something?
>
>> No, it's unlikely that there is a bug. Can you forward to me the
>> report descriptors of the Steam controller (ideally with the tool
>> hid-recorder from http://bentiss.github.io/hid-replay-docs/, so I can
>> get a few events too)?
>
> Sure, see the attached file. It is the wired one, I opened jstest and
> moved the joystick around to get a few events. Unfortunately the
> HID_REQ_GET_REPORT are not recorded.
>
> For the casual reader, here is the decoded report descriptor [1]:
>
> 0x06, 0x00, 0xFF, // Usage Page (Vendor Defined 0xFF00)
> 0x09, 0x01, // Usage (0x01)
> 0xA1, 0x01, // Collection (Application)
> 0x15, 0x00, // Logical Minimum (0)
> 0x26, 0xFF, 0x00, // Logical Maximum (255)
> 0x75, 0x08, // Report Size (8)
> 0x95, 0x40, // Report Count (64)
> 0x09, 0x01, // Usage (0x01)
> 0x81, 0x02, // Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
> 0x95, 0x40, // Report Count (64)
> 0x09, 0x01, // Usage (0x01)
> 0x91, 0x02, // Output (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> 0x95, 0x40, // Report Count (64)
> 0x09, 0x01, // Usage (0x01)
> 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> 0xC0, // End Collection
>
>
> [1]: http://eleccelerator.com/usbdescreqparser/
Thanks.
Arf, looks like usbhid expects the buffer to start with 0x00 when the
report is not numbered, thus adding one to the report length.
I guess that nobody tried to recently set/get reports on a device
without a report ID. And hidraw matches this behavior too, which means
it's hard to change.
One thing I'd like to try to change is the result of hid_report_len.
If everybody expects the size to be of one more when the report is
unnumbered, maybe we could simply add this placeholder in the report
size from the beginning.
We'd need more testing though that just my gut feeling that it's the
right thing to do.
Cheers,
Benjamin
On Fri, Feb 16, 2018 at 11:38:11AM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
> <[email protected]> wrote:
> > On Fri, Feb 16, 2018 at 10:31:35AM +0100, Benjamin Tissoires wrote:
> >> > Ok, I'll do that. The weird thing, however, is that:
> >> >
> >> > hid_hw_raw_request(steam->hid_dev, 0x00,
> >> > buf, hid_report_len(r), /* 64 */
> >> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >> >
> >> > fails with EOVERFLOW. I have to use:
> >> >
> >> > hid_hw_raw_request(steam->hid_dev, 0x00,
> >> > buf, 65
> >> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >> >
> >> > which just feels wrong to me.
> >>
> >> Indeed
> >>
>
> Arf, looks like usbhid expects the buffer to start with 0x00 when the
> report is not numbered, thus adding one to the report length.
>
> I guess that nobody tried to recently set/get reports on a device
> without a report ID. And hidraw matches this behavior too, which means
> it's hard to change.
>
> One thing I'd like to try to change is the result of hid_report_len.
> If everybody expects the size to be of one more when the report is
> unnumbered, maybe we could simply add this placeholder in the report
> size from the beginning.
I've been trying to make sense of all this numbered/buf++/count-- stuff,
and I admit I don't understand half of it...
But about that +7 in hid_alloc_report_buf(), isn't it to make room for
the implement()/extract() functions? And IIUIC those are not used for
raw_requests... they are instead passed directly to usb_control_msg()
(or whatever the ll driver does). That's the point of being raw, isn't
it?
If I'm right with that, would it make sense to go back to kzalloc(65)?
If I'm wrong, then if you agree, I'll default to:
hid_hw_raw_request(steam->hid_dev, 0x00,
buf, hid_report_len(r) + 1, /* count the request number */
HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
Then, if hid_report_len() is ever updated to return the +1, this one
should be removed. And even if it is not, we still have +6 extra bytes
from hid_alloc_report_buf(), so no real harm done.
Regards.
Rodrigo
On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
<[email protected]> wrote:
> On Fri, Feb 16, 2018 at 11:38:11AM +0100, Benjamin Tissoires wrote:
>> On Fri, Feb 16, 2018 at 10:57 AM, Rodrigo Rivas Costa
>> <[email protected]> wrote:
>> > On Fri, Feb 16, 2018 at 10:31:35AM +0100, Benjamin Tissoires wrote:
>> >> > Ok, I'll do that. The weird thing, however, is that:
>> >> >
>> >> > hid_hw_raw_request(steam->hid_dev, 0x00,
>> >> > buf, hid_report_len(r), /* 64 */
>> >> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>> >> >
>> >> > fails with EOVERFLOW. I have to use:
>> >> >
>> >> > hid_hw_raw_request(steam->hid_dev, 0x00,
>> >> > buf, 65
>> >> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>> >> >
>> >> > which just feels wrong to me.
>> >>
>> >> Indeed
>> >>
>>
>> Arf, looks like usbhid expects the buffer to start with 0x00 when the
>> report is not numbered, thus adding one to the report length.
>>
>> I guess that nobody tried to recently set/get reports on a device
>> without a report ID. And hidraw matches this behavior too, which means
>> it's hard to change.
>>
>> One thing I'd like to try to change is the result of hid_report_len.
>> If everybody expects the size to be of one more when the report is
>> unnumbered, maybe we could simply add this placeholder in the report
>> size from the beginning.
>
> I've been trying to make sense of all this numbered/buf++/count-- stuff,
> and I admit I don't understand half of it...
>
> But about that +7 in hid_alloc_report_buf(), isn't it to make room for
> the implement()/extract() functions? And IIUIC those are not used for
> raw_requests... they are instead passed directly to usb_control_msg()
> (or whatever the ll driver does). That's the point of being raw, isn't
> it?
>
> If I'm right with that, would it make sense to go back to kzalloc(65)?
>
> If I'm wrong, then if you agree, I'll default to:
>
> hid_hw_raw_request(steam->hid_dev, 0x00,
> buf, hid_report_len(r) + 1, /* count the request number */
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> Then, if hid_report_len() is ever updated to return the +1, this one
> should be removed. And even if it is not, we still have +6 extra bytes
> from hid_alloc_report_buf(), so no real harm done.
I am fine with your analysis except for the last point :)
We need all 7 extra bytes, not 6 (in implement()). However, as you
said, implement() is not used in the low_level transport functions, so
there is no point bike shedding for ages.
I'd say please stick to hid_report_alloc_buf (maybe add a comment
about the missing report ID added by usb), and use hid_report_len(r) +
1 while calling hid_hw_raw_request(). This way, we can always fix the
behavior later and have something which will not break.
How does that sound?
Cheers,
Benjamin
>
> Regards.
> Rodrigo
On Tue, Feb 20, 2018 at 05:49:02PM +0100, Benjamin Tissoires wrote:
> On Fri, Feb 16, 2018 at 9:59 PM, Rodrigo Rivas Costa
> > But about that +7 in hid_alloc_report_buf(), isn't it to make room for
> > the implement()/extract() functions? And IIUIC those are not used for
> > raw_requests... they are instead passed directly to usb_control_msg()
> > (or whatever the ll driver does). That's the point of being raw, isn't
> > it?
> >
> > If I'm right with that, would it make sense to go back to kzalloc(65)?
> >
> > If I'm wrong, then if you agree, I'll default to:
> >
> > hid_hw_raw_request(steam->hid_dev, 0x00,
> > buf, hid_report_len(r) + 1, /* count the request number */
> > HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >
> > Then, if hid_report_len() is ever updated to return the +1, this one
> > should be removed. And even if it is not, we still have +6 extra bytes
> > from hid_alloc_report_buf(), so no real harm done.
>
> I am fine with your analysis except for the last point :)
> We need all 7 extra bytes, not 6 (in implement()). However, as you
> said, implement() is not used in the low_level transport functions, so
> there is no point bike shedding for ages.
>
> I'd say please stick to hid_report_alloc_buf (maybe add a comment
> about the missing report ID added by usb), and use hid_report_len(r) +
> 1 while calling hid_hw_raw_request(). This way, we can always fix the
> behavior later and have something which will not break.
>
> How does that sound?
It sound fine to me. I'll try to write a small comment explaining the
+1.
Thanks
Rodrigo
On 02/14/2018 03:29 PM, Rodrigo Rivas Costa wrote:
> On Wed, Feb 14, 2018 at 03:45:14PM +0100, Benjamin Tissoires wrote:
>> I think I had a look at this a while ago, and didn't want to interfere
>> with SteamOS regarding this. I think your patch should be fine in that
>> regard, but have you tried SteamOS on a kernel patched with your
>> series? Does it behave properly or will it break?
>
> Well, SteamOS is just a modified Debian with the Steam Client running in
> Big Picture mode (fullscreen). I tried the Steam Client with this driver
> and it works just fine:
> - The first thing the Steam Client (SC) does is to disable the virtual
> keyboard and mouse, so not creating those input devices make no
> difference.
There's a bit more to it than that; for example, on SteamOS, the virtual
keyboard/mouse will be re-enabled when switching to Desktop Mode.
In addition, chord configurations can include a bindable action that
temporarily programs the controller to re-enable the virtual
keyboard/mouse (what we call "Lizard Mode"). People use this to navigate
around cases where our software input doesn't cut it, like a launcher
pop-up window that Steam isn't able to hook into for whatever reason.
That behavior will also change over time since the client is often
updated with support for new controller features.
Thanks,
- Pierre-Loup
> - Input events are sent both to the libusb (or whatever SC uses) and
> this driver.
> - The only source of conflict would be in sending commands to the
> controller. But currently the only command sent is the get_serial, and
> that seems to do no harm.
>
>>> +
>>> + hid_info(hdev, "Steam Controller connected");
>>> +
>>> + input = input_allocate_device();
>>
>> Don't you need one input node per wireless gamepad too?
>>
> No, the wired and wireless methods are two different USB devices.
> The wireless adaptor is a small usb device that creates 4 HID interfaces
> for the 4 to-be-connected controllers. When the 'connected' event
> arrives on one of those interfaces we will create one input device.
> Another controller connected will use a different interface so another
> unrelated input device will be created.
>
> The wired adaptor is the controller connected directly to USB: it
> creates one HID inteface and one input device directly.
>
> Anyway, one steam_device will contain 0 or 1 input_device.
>
>>> + input->name = "Steam Controller";
>>
>> In case of the wireless controllers, you might want to personalize this a bit.
>
> Do you mean e.g. "Wireless Steam Controller"? Would be wise to add the serial
> number here (from PATCH 2/3?)? Or is the 'uniq' enough for that?
>
>>> + 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);
>>
>> You are also probably missing the resolution bits. We need to
>> accurately report the physical dimensions to the user space (thanks to
>> the resolution)
>>
>>> + input_set_abs_params(input, ABS_RX, -32767, 32767, 0, 0);
>>> + input_set_abs_params(input, ABS_RY, -32767, 32767, 0, 0);
>>
>> What do RX/RY correspond to?
>
> This gamepad has 1 joystick and 2 touchpads (lpad and rpad).
> I'm mapping the joystick/lpad to ABS_X/ABS_Y and the rpad to
> ABS_RX/ABS_RY.
>
> About the resolution, I looked around other drivers and many have 0 as
> resolution... is it necessary only in the pads or the joystick too?
> And what are the units of that value?
> For reference, the pads are round and exactly 40 mm in diameter.
>
> Why the lpad and the joystick are mapped to the same axes? Well, because
> the device returns them at the same offset. We could make them
> apart by using bits 10.3 (lpad_touch) and 10.7 (lpad_and_joy) but I
> don't think it is worth it... you use the joystick and the lpad with the
> same thumb!
>
>> Anyway. Thanks for the driver, there are a few bits to fix, nothing
>> scary though.
>
> Great, I'll correct all those issues and resubmit the patches in a few
> days. It is my very first driver and I'm still a bit slow...
>
> Thank you for your attention!
> Rodrigo.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>