From 740329d7120f8608ead64b0f3417c02ca1d6b32f Mon Sep 17 00:00:00 2001 From: Johannes Roith Date: Thu, 21 Sep 2023 18:49:28 +0200 Subject: [PATCH 01/53] HID: mcp2200: added driver for GPIOs of MCP2200 Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200 by using the HID interface. Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG, GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by default). The driver was tested while also using the UART of the chip. Setting and reading the GPIOs has no effect on the UART communication. However, a reset is triggered after the CONFIGURE command. If the GPIO Direction is constantly changed, this will affect the communication at low baud rates. This is a hardware problem of the MCP2200 and is not caused by the driver. Signed-off-by: Johannes Roith Reviewed-by: Rahul Rameshbabu Signed-off-by: Jiri Kosina --- drivers/hid/Kconfig | 9 + drivers/hid/Makefile | 1 + drivers/hid/hid-ids.h | 1 + drivers/hid/hid-mcp2200.c | 392 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 403 insertions(+) create mode 100644 drivers/hid/hid-mcp2200.c diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 4ce74af796579..03e7fc3d05a51 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -1296,6 +1296,15 @@ config HID_ALPS Say Y here if you have a Alps touchpads over i2c-hid or usbhid and want support for its special functionalities. +config HID_MCP2200 + tristate "Microchip MCP2200 HID USB-to-GPIO bridge" + depends on USB_HID && GPIOLIB + help + Provides GPIO functionality over USB-HID through MCP2200 device. + + To compile this driver as a module, choose M here: the module + will be called hid-mcp2200.ko. + config HID_MCP2221 tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" depends on USB_HID && I2C diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index 8a06d0f840bcb..082a728eac600 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -79,6 +79,7 @@ obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o obj-$(CONFIG_HID_MACALLY) += hid-macally.o obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o obj-$(CONFIG_HID_MALTRON) += hid-maltron.o +obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index f7973ccd84a28..f63e16095c108 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -917,6 +917,7 @@ #define USB_DEVICE_ID_PICK16F1454 0x0042 #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7 #define USB_DEVICE_ID_LUXAFOR 0xf372 +#define USB_DEVICE_ID_MCP2200 0x00df #define USB_DEVICE_ID_MCP2221 0x00dd #define USB_VENDOR_ID_MICROSOFT 0x045e diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c new file mode 100644 index 0000000000000..bf57f7f6caa08 --- /dev/null +++ b/drivers/hid/hid-mcp2200.c @@ -0,0 +1,392 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * MCP2200 - Microchip USB to GPIO bridge + * + * Copyright (c) 2023, Johannes Roith + * + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include "hid-ids.h" + +/* Commands codes in a raw output report */ +#define SET_CLEAR_OUTPUTS 0x08 +#define CONFIGURE 0x10 +#define READ_EE 0x20 +#define WRITE_EE 0x40 +#define READ_ALL 0x80 + +/* MCP GPIO direction encoding */ +enum MCP_IO_DIR { + MCP2200_DIR_OUT = 0x00, + MCP2200_DIR_IN = 0x01, +}; + +/* Altternative pin assignments */ +#define TXLED 2 +#define RXLED 3 +#define USBCFG 6 +#define SSPND 7 +#define MCP_NGPIO 8 + +/* CMD to set or clear a GPIO output */ +struct mcp_set_clear_outputs { + u8 cmd; + u8 dummys1[10]; + u8 set_bmap; + u8 clear_bmap; + u8 dummys2[3]; +} __packed; + +/* CMD to configure the IOs */ +struct mcp_configure { + u8 cmd; + u8 dummys1[3]; + u8 io_bmap; + u8 config_alt_pins; + u8 io_default_val_bmap; + u8 config_alt_options; + u8 baud_h; + u8 baud_l; + u8 dummys2[6]; +} __packed; + +/* CMD to read all parameters */ +struct mcp_read_all { + u8 cmd; + u8 dummys[15]; +} __packed; + +/* Response to the read all cmd */ +struct mcp_read_all_resp { + u8 cmd; + u8 eep_addr; + u8 dummy; + u8 eep_val; + u8 io_bmap; + u8 config_alt_pins; + u8 io_default_val_bmap; + u8 config_alt_options; + u8 baud_h; + u8 baud_l; + u8 io_port_val_bmap; + u8 dummys[5]; +} __packed; + +struct mcp2200 { + struct hid_device *hdev; + struct mutex lock; + struct completion wait_in_report; + u8 gpio_dir; + u8 gpio_val; + u8 gpio_inval; + u8 baud_h; + u8 baud_l; + u8 config_alt_pins; + u8 gpio_reset_val; + u8 config_alt_options; + int status; + struct gpio_chip gc; + u8 hid_report[16]; +}; + +/* this executes the READ_ALL cmd */ +static int mcp_cmd_read_all(struct mcp2200 *mcp) +{ + struct mcp_read_all *read_all; + int len, t; + + reinit_completion(&mcp->wait_in_report); + + mutex_lock(&mcp->lock); + + read_all = (struct mcp_read_all *) mcp->hid_report; + read_all->cmd = READ_ALL; + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all, + sizeof(struct mcp_read_all)); + + mutex_unlock(&mcp->lock); + + if (len != sizeof(struct mcp_read_all)) + return -EINVAL; + + t = wait_for_completion_timeout(&mcp->wait_in_report, + msecs_to_jiffies(4000)); + if (!t) + return -ETIMEDOUT; + + /* return status, negative value if wrong response was received */ + return mcp->status; +} + +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) +{ + struct mcp2200 *mcp = gpiochip_get_data(gc); + u8 value; + int status; + struct mcp_set_clear_outputs *cmd; + + mutex_lock(&mcp->lock); + cmd = (struct mcp_set_clear_outputs *) mcp->hid_report; + + value = mcp->gpio_val & ~*mask; + value |= (*mask & *bits); + + cmd->cmd = SET_CLEAR_OUTPUTS; + cmd->set_bmap = value; + cmd->clear_bmap = ~(value); + + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd, + sizeof(struct mcp_set_clear_outputs)); + + if (status == sizeof(struct mcp_set_clear_outputs)) + mcp->gpio_val = value; + + mutex_unlock(&mcp->lock); +} + +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value) +{ + unsigned long mask = 1 << gpio_nr; + unsigned long bmap_value = value << gpio_nr; + + mcp_set_multiple(gc, &mask, &bmap_value); +} + +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask, + unsigned long *bits) +{ + u32 val; + struct mcp2200 *mcp = gpiochip_get_data(gc); + int status; + + status = mcp_cmd_read_all(mcp); + if (status) + return status; + + val = mcp->gpio_inval; + *bits = (val & *mask); + return 0; +} + +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr) +{ + unsigned long mask = 0, bits = 0; + + mask = (1 << gpio_nr); + mcp_get_multiple(gc, &mask, &bits); + return bits > 0; +} + +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr) +{ + struct mcp2200 *mcp = gpiochip_get_data(gc); + + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr)) + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT; +} + +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr, + enum MCP_IO_DIR io_direction) +{ + struct mcp2200 *mcp = gpiochip_get_data(gc); + struct mcp_configure *conf; + int status; + /* after the configure cmd we will need to set the outputs again */ + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */ + unsigned long bits = mcp->gpio_val; + /* Offsets of alternative pins in config_alt_pins, 0 is not used */ + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED}; + u8 config_alt_pins = mcp->config_alt_pins; + + /* Read in the reset baudrate first, we need it later */ + status = mcp_cmd_read_all(mcp); + if (status != 0) + return status; + + mutex_lock(&mcp->lock); + conf = (struct mcp_configure *) mcp->hid_report; + + /* configure will reset the chip! */ + conf->cmd = CONFIGURE; + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr)) + | (io_direction << gpio_nr); + /* Don't overwrite the reset parameters */ + conf->baud_h = mcp->baud_h; + conf->baud_l = mcp->baud_l; + conf->config_alt_options = mcp->config_alt_options; + conf->io_default_val_bmap = mcp->gpio_reset_val; + /* Adjust alt. func if necessary */ + if (alt_pin_conf[gpio_nr]) + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]); + conf->config_alt_pins = config_alt_pins; + + status = hid_hw_output_report(mcp->hdev, (u8 *) conf, + sizeof(struct mcp_set_clear_outputs)); + + if (status == sizeof(struct mcp_set_clear_outputs)) { + mcp->gpio_dir = conf->io_bmap; + mcp->config_alt_pins = config_alt_pins; + } else { + mutex_unlock(&mcp->lock); + return -EIO; + } + + mutex_unlock(&mcp->lock); + + /* Configure CMD will clear all IOs -> rewrite them */ + mcp_set_multiple(gc, &mask, &bits); + return 0; +} + +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr) +{ + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN); +} + +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr, + int value) +{ + int ret; + unsigned long mask, bmap_value; + + mask = 1 << gpio_nr; + bmap_value = value << gpio_nr; + + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT); + if (!ret) + mcp_set_multiple(gc, &mask, &bmap_value); + return ret; +} + +static const struct gpio_chip template_chip = { + .label = "mcp2200", + .owner = THIS_MODULE, + .get_direction = mcp_get_direction, + .direction_input = mcp_direction_input, + .direction_output = mcp_direction_output, + .set = mcp_set, + .set_multiple = mcp_set_multiple, + .get = mcp_get, + .get_multiple = mcp_get_multiple, + .base = -1, + .ngpio = MCP_NGPIO, + .can_sleep = true, +}; + +/* + * MCP2200 uses interrupt endpoint for input reports. This function + * is called by HID layer when it receives i/p report from mcp2200, + * which is actually a response to the previously sent command. + */ +static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report, + u8 *data, int size) +{ + struct mcp2200 *mcp = hid_get_drvdata(hdev); + struct mcp_read_all_resp *all_resp; + + switch (data[0]) { + case READ_ALL: + all_resp = (struct mcp_read_all_resp *) data; + mcp->status = 0; + mcp->gpio_inval = all_resp->io_port_val_bmap; + mcp->baud_h = all_resp->baud_h; + mcp->baud_l = all_resp->baud_l; + mcp->gpio_reset_val = all_resp->io_default_val_bmap; + mcp->config_alt_pins = all_resp->config_alt_pins; + mcp->config_alt_options = all_resp->config_alt_options; + break; + default: + mcp->status = -EIO; + break; + } + + complete(&mcp->wait_in_report); + return 0; +} + +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id) +{ + int ret; + struct mcp2200 *mcp; + + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL); + if (!mcp) + return -ENOMEM; + + ret = hid_parse(hdev); + if (ret) { + hid_err(hdev, "can't parse reports\n"); + return ret; + } + + ret = hid_hw_start(hdev, 0); + if (ret) { + hid_err(hdev, "can't start hardware\n"); + return ret; + } + + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8, + hdev->version & 0xff, hdev->name, hdev->phys); + + ret = hid_hw_open(hdev); + if (ret) { + hid_err(hdev, "can't open device\n"); + hid_hw_stop(hdev); + return ret; + } + + mutex_init(&mcp->lock); + init_completion(&mcp->wait_in_report); + hid_set_drvdata(hdev, mcp); + mcp->hdev = hdev; + + mcp->gc = template_chip; + mcp->gc.parent = &hdev->dev; + + ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp); + if (ret < 0) { + hid_err(hdev, "Unable to register gpiochip\n"); + hid_hw_close(hdev); + hid_hw_stop(hdev); + return ret; + } + + return 0; +} + +static void mcp2200_remove(struct hid_device *hdev) +{ + hid_hw_close(hdev); + hid_hw_stop(hdev); +} + +static const struct hid_device_id mcp2200_devices[] = { + { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) }, + { } +}; +MODULE_DEVICE_TABLE(hid, mcp2200_devices); + +static struct hid_driver mcp2200_driver = { + .name = "mcp2200", + .id_table = mcp2200_devices, + .probe = mcp2200_probe, + .remove = mcp2200_remove, + .raw_event = mcp2200_raw_event, +}; + +/* Register with HID core */ +module_hid_driver(mcp2200_driver); + +MODULE_AUTHOR("Johannes Roith "); +MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge"); +MODULE_LICENSE("GPL"); From d9786159d229cdc1f579f7cf3abf464efb453e40 Mon Sep 17 00:00:00 2001 From: Hamish Martin Date: Wed, 25 Oct 2023 16:55:12 +1300 Subject: [PATCH 02/53] HID: mcp2221: Set ACPI companion In scenarios where an I2C device tree is defined in ACPI and exists off the MCP2221 I2C bus, the devices could not be instantiated. Mark the USB port that the MCP2221 is connected to as its ACPI companion so that the USB device can be bound to the ACPI tree when enumerated. With this change the downstream I2C tree devices can be instantiated on ACPI systems. Signed-off-by: Hamish Martin Signed-off-by: Jiri Kosina --- drivers/hid/hid-mcp2221.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c index 72883e0ce7575..90e23bba21306 100644 --- a/drivers/hid/hid-mcp2221.c +++ b/drivers/hid/hid-mcp2221.c @@ -1154,6 +1154,7 @@ static int mcp2221_probe(struct hid_device *hdev, mcp->adapter.algo = &mcp_i2c_algo; mcp->adapter.retries = 1; mcp->adapter.dev.parent = &hdev->dev; + ACPI_COMPANION_SET(&mcp->adapter.dev, ACPI_COMPANION(hdev->dev.parent)); snprintf(mcp->adapter.name, sizeof(mcp->adapter.name), "MCP2221 usb-i2c bridge"); From 02a46753601a24e1673d9c28173121055e8e6cc9 Mon Sep 17 00:00:00 2001 From: Hamish Martin Date: Wed, 25 Oct 2023 16:55:13 +1300 Subject: [PATCH 03/53] HID: mcp2221: Don't set bus speed on every transfer Since the initial commit of this driver the I2C bus speed has been reconfigured for every single transfer. This is despite the fact that we never change the speed and it is never "lost" by the chip. Upon investigation we find that what was really happening was that the setting of the bus speed had the side effect of cancelling a previous failed command if there was one, thereby freeing the bus. This is the part that was actually required to keep the bus operational in the face of failed commands. Instead of always setting the speed, we now correctly cancel any failed commands as they are detected. This means we can just set the bus speed at probe time and remove the previous speed sets on each transfer. This has the effect of improving performance and reducing the number of commands required to complete transfers. Signed-off-by: Hamish Martin Signed-off-by: Jiri Kosina --- drivers/hid/hid-mcp2221.c | 41 ++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c index 90e23bba21306..ecd830ce0acff 100644 --- a/drivers/hid/hid-mcp2221.c +++ b/drivers/hid/hid-mcp2221.c @@ -187,6 +187,25 @@ static int mcp_cancel_last_cmd(struct mcp2221 *mcp) return mcp_send_data_req_status(mcp, mcp->txbuf, 8); } +/* Check if the last command succeeded or failed and return the result. + * If the command did fail, cancel that command which will free the i2c bus. + */ +static int mcp_chk_last_cmd_status_free_bus(struct mcp2221 *mcp) +{ + int ret; + + ret = mcp_chk_last_cmd_status(mcp); + if (ret) { + /* The last command was a failure. + * Send a cancel which will also free the bus. + */ + usleep_range(980, 1000); + mcp_cancel_last_cmd(mcp); + } + + return ret; +} + static int mcp_set_i2c_speed(struct mcp2221 *mcp) { int ret; @@ -241,7 +260,7 @@ static int mcp_i2c_write(struct mcp2221 *mcp, usleep_range(980, 1000); if (last_status) { - ret = mcp_chk_last_cmd_status(mcp); + ret = mcp_chk_last_cmd_status_free_bus(mcp); if (ret) return ret; } @@ -308,7 +327,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp, if (ret) return ret; - ret = mcp_chk_last_cmd_status(mcp); + ret = mcp_chk_last_cmd_status_free_bus(mcp); if (ret) return ret; @@ -328,11 +347,6 @@ static int mcp_i2c_xfer(struct i2c_adapter *adapter, mutex_lock(&mcp->lock); - /* Setting speed before every transaction is required for mcp2221 */ - ret = mcp_set_i2c_speed(mcp); - if (ret) - goto exit; - if (num == 1) { if (msgs->flags & I2C_M_RD) { ret = mcp_i2c_smbus_read(mcp, msgs, MCP2221_I2C_RD_DATA, @@ -417,9 +431,7 @@ static int mcp_smbus_write(struct mcp2221 *mcp, u16 addr, if (last_status) { usleep_range(980, 1000); - ret = mcp_chk_last_cmd_status(mcp); - if (ret) - return ret; + ret = mcp_chk_last_cmd_status_free_bus(mcp); } return ret; @@ -437,10 +449,6 @@ static int mcp_smbus_xfer(struct i2c_adapter *adapter, u16 addr, mutex_lock(&mcp->lock); - ret = mcp_set_i2c_speed(mcp); - if (ret) - goto exit; - switch (size) { case I2C_SMBUS_QUICK: @@ -1148,6 +1156,11 @@ static int mcp2221_probe(struct hid_device *hdev, if (i2c_clk_freq < 50) i2c_clk_freq = 50; mcp->cur_i2c_clk_div = (12000000 / (i2c_clk_freq * 1000)) - 3; + ret = mcp_set_i2c_speed(mcp); + if (ret) { + hid_err(hdev, "can't set i2c speed: %d\n", ret); + return ret; + } mcp->adapter.owner = THIS_MODULE; mcp->adapter.class = I2C_CLASS_HWMON; From 2682468671aa0b4d52ae09779b48212a80d71b91 Mon Sep 17 00:00:00 2001 From: Hamish Martin Date: Wed, 25 Oct 2023 16:55:14 +1300 Subject: [PATCH 04/53] HID: mcp2221: Handle reads greater than 60 bytes When a user requests more than 60 bytes of data the MCP2221 must chunk the data in chunks up to 60 bytes long (see command/response code 0x40 in the datasheet). In order to signal that the device has more data the (undocumented) byte at byte index 2 of the Get I2C Data response uses the value 0x54. This contrasts with the case for the final data chunk where the value returned is 0x55 (MCP2221_I2C_READ_COMPL). The fact that 0x55 was not returned in the response was interpreted by the driver as a failure meaning that all reads of more than 60 bytes would fail. Add support for reads that are split over multiple chunks by looking for the response code indicating that more data is expected and continuing the read as the code intended. Some timing delays are required to ensure the chip has time to refill its FIFO as data is read in from the I2C bus. This timing has been tested in my system when configured for bus speeds of 50KHz, 100KHz, and 400KHz and operates well. Signed-off-by: Hamish Martin Signed-off-by: Jiri Kosina --- drivers/hid/hid-mcp2221.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c index ecd830ce0acff..b242a312c377f 100644 --- a/drivers/hid/hid-mcp2221.c +++ b/drivers/hid/hid-mcp2221.c @@ -49,6 +49,7 @@ enum { MCP2221_I2C_MASK_ADDR_NACK = 0x40, MCP2221_I2C_WRADDRL_SEND = 0x21, MCP2221_I2C_ADDR_NACK = 0x25, + MCP2221_I2C_READ_PARTIAL = 0x54, MCP2221_I2C_READ_COMPL = 0x55, MCP2221_ALT_F_NOT_GPIOV = 0xEE, MCP2221_ALT_F_NOT_GPIOD = 0xEF, @@ -297,6 +298,7 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp, { int ret; u16 total_len; + int retries = 0; mcp->txbuf[0] = type; if (msg) { @@ -320,20 +322,31 @@ static int mcp_i2c_smbus_read(struct mcp2221 *mcp, mcp->rxbuf_idx = 0; do { + /* Wait for the data to be read by the device */ + usleep_range(980, 1000); + memset(mcp->txbuf, 0, 4); mcp->txbuf[0] = MCP2221_I2C_GET_DATA; ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); - if (ret) - return ret; - - ret = mcp_chk_last_cmd_status_free_bus(mcp); - if (ret) - return ret; - - usleep_range(980, 1000); + if (ret) { + if (retries < 5) { + /* The data wasn't ready to read. + * Wait a bit longer and try again. + */ + usleep_range(90, 100); + retries++; + } else { + return ret; + } + } else { + retries = 0; + } } while (mcp->rxbuf_idx < total_len); + usleep_range(980, 1000); + ret = mcp_chk_last_cmd_status_free_bus(mcp); + return ret; } @@ -799,7 +812,8 @@ static int mcp2221_raw_event(struct hid_device *hdev, mcp->status = -EIO; break; } - if (data[2] == MCP2221_I2C_READ_COMPL) { + if (data[2] == MCP2221_I2C_READ_COMPL || + data[2] == MCP2221_I2C_READ_PARTIAL) { buf = mcp->rxbuf; memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]); mcp->rxbuf_idx = mcp->rxbuf_idx + data[3]; From fd2a9b29dc9c4c35def91d5d1c5b470843539de6 Mon Sep 17 00:00:00 2001 From: Tatsunosuke Tobita Date: Wed, 15 Nov 2023 08:57:29 +0900 Subject: [PATCH 05/53] HID: wacom: Remove AES power_supply after extended inactivity Even if a user does not use their AES pen for an extended period, the battery power supply attributes continue to exist. This results in the desktop showing battery status for a pen that is no longer in use and which may in fact be in a different state (e.g. the user may be charging the pen). To avoid confusion and ensure userspace has an accurate view of the battery state, this patch automatically removes the power_supply after 30 minutes of inactivity. Signed-off-by: Tatsunosuke Tobita Reviewed-by: Jason Gerecke Reviewed-by: Aaron Skomra Reviewed-by: Josh Dickens Link: https://lore.kernel.org/r/20231114235729.6867-1-tatsunosuke.wacom@gmail.com Signed-off-by: Benjamin Tissoires --- drivers/hid/wacom.h | 1 + drivers/hid/wacom_sys.c | 8 ++++++++ drivers/hid/wacom_wac.c | 12 +++++++++++- drivers/hid/wacom_wac.h | 1 + 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/drivers/hid/wacom.h b/drivers/hid/wacom.h index 166a76c9bcad3..77c5fb26cd14c 100644 --- a/drivers/hid/wacom.h +++ b/drivers/hid/wacom.h @@ -164,6 +164,7 @@ struct wacom { struct work_struct battery_work; struct work_struct remote_work; struct delayed_work init_work; + struct delayed_work aes_battery_work; struct wacom_remote *remote; struct work_struct mode_change_work; struct timer_list idleprox_timer; diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 3f704b8072e8a..b613f11ed9498 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -1813,6 +1813,13 @@ static void wacom_destroy_battery(struct wacom *wacom) } } +static void wacom_aes_battery_handler(struct work_struct *work) +{ + struct wacom *wacom = container_of(work, struct wacom, aes_battery_work.work); + + wacom_destroy_battery(wacom); +} + static ssize_t wacom_show_speed(struct device *dev, struct device_attribute *attr, char *buf) @@ -2794,6 +2801,7 @@ static int wacom_probe(struct hid_device *hdev, mutex_init(&wacom->lock); INIT_DELAYED_WORK(&wacom->init_work, wacom_init_work); + INIT_DELAYED_WORK(&wacom->aes_battery_work, wacom_aes_battery_handler); INIT_WORK(&wacom->wireless_work, wacom_wireless_work); INIT_WORK(&wacom->battery_work, wacom_battery_work); INIT_WORK(&wacom->remote_work, wacom_remote_work); diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index 471db78dbbf02..c205198ded118 100644 --- a/drivers/hid/wacom_wac.c +++ b/drivers/hid/wacom_wac.c @@ -2528,11 +2528,12 @@ static void wacom_wac_pen_report(struct hid_device *hdev, struct input_dev *input = wacom_wac->pen_input; bool range = wacom_wac->hid_data.inrange_state; bool sense = wacom_wac->hid_data.sense_state; + bool entering_range = !wacom_wac->tool[0] && range; if (wacom_wac->is_invalid_bt_frame) return; - if (!wacom_wac->tool[0] && range) { /* first in range */ + if (entering_range) { /* first in range */ /* Going into range select tool */ if (wacom_wac->hid_data.invert_state) wacom_wac->tool[0] = BTN_TOOL_RUBBER; @@ -2583,6 +2584,15 @@ static void wacom_wac_pen_report(struct hid_device *hdev, input_sync(input); } + /* Handle AES battery timeout behavior */ + if (wacom_wac->features.quirks & WACOM_QUIRK_AESPEN) { + if (entering_range) + cancel_delayed_work(&wacom->aes_battery_work); + if (!sense) + schedule_delayed_work(&wacom->aes_battery_work, + msecs_to_jiffies(WACOM_AES_BATTERY_TIMEOUT)); + } + if (!sense) { wacom_wac->tool[0] = 0; wacom_wac->id[0] = 0; diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h index 57e185f18d53d..e63b1e806e347 100644 --- a/drivers/hid/wacom_wac.h +++ b/drivers/hid/wacom_wac.h @@ -14,6 +14,7 @@ #define WACOM_MAX_REMOTES 5 #define WACOM_STATUS_UNKNOWN 255 #define WACOM_REMOTE_BATTERY_TIMEOUT 21000000000ll +#define WACOM_AES_BATTERY_TIMEOUT 1800000 /* packet length for individual models */ #define WACOM_PKGLEN_BBFUN 9 From a3a44d2d3a5c5ff6e73c711db5b1911b5a676bb0 Mon Sep 17 00:00:00 2001 From: Even Xu Date: Tue, 5 Dec 2023 09:50:30 +0800 Subject: [PATCH 06/53] HID: Intel-ish-hid: Ishtp: Add helper functions for client connection For every ishtp client driver during initialization state, the flow is: 1 - Allocate an ISHTP client instance 2 - Reserve a host id and link the client instance 3 - Search a firmware client using UUID and get related client information 4 - Bind firmware client id to the ISHTP client instance 5 - Set the state the ISHTP client instance to CONNECTING 6 - Send connect request to firmware 7 - Register event callback for messages from the firmware During deinitizalization state, the flow is: 9 - Set the state the ISHTP client instance to ISHTP_CL_DISCONNECTING 10 - Issue disconnect request to firmware 11 - Unlike the client instance 12 - Flush message queue 13 - Free ISHTP client instance Step 2-7 are identical to the steps of client driver initialization and driver reset flow, but reallocation of the RX/TX ring buffers can be avoided in reset flow. Also for step 9-12, they are identical to the steps of client driver failure handling after connect request, driver reset flow and driver removing. So, add two helper functions to simplify client driver code. ishtp_cl_establish_connection() ishtp_cl_destroy_connection() No functional changes are expected. Signed-off-by: Even Xu Acked-by: Srinivas Pandruvada Signed-off-by: Jiri Kosina --- drivers/hid/intel-ish-hid/ishtp/client.c | 185 +++++++++++++++++++++-- include/linux/intel-ish-client-if.h | 3 + 2 files changed, 177 insertions(+), 11 deletions(-) diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c index 2d92fc129ce4d..82c907f01bd3b 100644 --- a/drivers/hid/intel-ish-hid/ishtp/client.c +++ b/drivers/hid/intel-ish-hid/ishtp/client.c @@ -339,16 +339,17 @@ static bool ishtp_cl_is_other_connecting(struct ishtp_cl *cl) } /** - * ishtp_cl_connect() - Send connect request to firmware + * ishtp_cl_connect_to_fw() - Send connect request to firmware * @cl: client device instance * - * Send a connect request for a client to firmware. If successful it will - * RX and TX ring buffers + * Send a connect request to the firmware and wait for firmware response. + * If there is successful connection response from the firmware, change + * client state to ISHTP_CL_CONNECTED, and bind client to related + * firmware client_id. * - * Return: 0 if successful connect response from the firmware and able - * to bind and allocate ring buffers or error code on failure + * Return: 0 for success and error code on failure */ -int ishtp_cl_connect(struct ishtp_cl *cl) +static int ishtp_cl_connect_to_fw(struct ishtp_cl *cl) { struct ishtp_device *dev; int rets; @@ -358,8 +359,6 @@ int ishtp_cl_connect(struct ishtp_cl *cl) dev = cl->dev; - dev->print_log(dev, "%s() current_state = %d\n", __func__, cl->state); - if (ishtp_cl_is_other_connecting(cl)) { dev->print_log(dev, "%s() Busy\n", __func__); return -EBUSY; @@ -405,6 +404,38 @@ int ishtp_cl_connect(struct ishtp_cl *cl) return rets; } + return rets; +} + +/** + * ishtp_cl_connect() - Build connection with firmware + * @cl: client device instance + * + * Call ishtp_cl_connect_to_fw() to connect and bind to firmware. If successful, + * allocate RX and TX ring buffers, and start flow control with firmware to + * start communication. + * + * Return: 0 if there is successful connection to the firmware, allocate + * ring buffers. + */ +int ishtp_cl_connect(struct ishtp_cl *cl) +{ + struct ishtp_device *dev; + int rets; + + if (!cl || !cl->dev) + return -ENODEV; + + dev = cl->dev; + + dev->print_log(dev, "%s() current_state = %d\n", __func__, cl->state); + + rets = ishtp_cl_connect_to_fw(cl); + if (rets) { + dev->print_log(dev, "%s() Connect to fw failed\n", __func__); + return rets; + } + rets = ishtp_cl_alloc_rx_ring(cl); if (rets) { dev->print_log(dev, "%s() Alloc RX ring failed\n", __func__); @@ -422,15 +453,147 @@ int ishtp_cl_connect(struct ishtp_cl *cl) return rets; } - /* Upon successful connection and allocation, emit flow-control */ + /* + * Upon successful connection and allocation, start flow-control. + */ rets = ishtp_cl_read_start(cl); - dev->print_log(dev, "%s() successful\n", __func__); - return rets; } EXPORT_SYMBOL(ishtp_cl_connect); +/** + * ishtp_cl_establish_connection() - Establish connection with the firmware + * @cl: client device instance + * @uuid: uuid of the client to search + * @tx_size: TX ring buffer size + * @rx_size: RX ring buffer size + * @reset: true if called for reset connection, otherwise for first connection + * + * This is a helper function for client driver to build connection with firmware. + * If it's first time connecting to the firmware, set reset to false, this + * function will link client to bus, find client id and send connect request to + * the firmware. + * + * If it's called for reset handler where client lost connection after + * firmware reset, set reset to true, this function will reinit client state and + * establish connection again. In this case, this function reuses current client + * structure and ring buffers to avoid allocation failure and memory fragments. + * + * Return: 0 for successful connection with the firmware, + * or error code on failure + */ +int ishtp_cl_establish_connection(struct ishtp_cl *cl, const guid_t *uuid, + int tx_size, int rx_size, bool reset) +{ + struct ishtp_device *dev; + struct ishtp_fw_client *fw_client; + int rets; + + if (!cl || !cl->dev) + return -ENODEV; + + dev = cl->dev; + + ishtp_set_connection_state(cl, ISHTP_CL_INITIALIZING); + + /* reinit ishtp_cl structure if call for reset */ + if (reset) { + cl->host_client_id = 0; + cl->fw_client_id = 0; + cl->ishtp_flow_ctrl_creds = 0; + cl->out_flow_ctrl_creds = 0; + + cl->last_tx_path = CL_TX_PATH_IPC; + cl->last_dma_acked = 1; + cl->last_dma_addr = NULL; + cl->last_ipc_acked = 1; + + cl->sending = 0; + cl->err_send_msg = 0; + cl->err_send_fc = 0; + + cl->send_msg_cnt_ipc = 0; + cl->send_msg_cnt_dma = 0; + cl->recv_msg_cnt_ipc = 0; + cl->recv_msg_cnt_dma = 0; + cl->recv_msg_num_frags = 0; + cl->ishtp_flow_ctrl_cnt = 0; + cl->out_flow_ctrl_cnt = 0; + } + + /* link to bus */ + rets = ishtp_cl_link(cl); + if (rets) { + dev->print_log(dev, "%s() ishtp_cl_link failed\n", __func__); + return rets; + } + + /* find firmware client */ + fw_client = ishtp_fw_cl_get_client(dev, uuid); + if (!fw_client) { + dev->print_log(dev, + "%s() ish client uuid not found\n", __func__); + return -ENOENT; + } + + ishtp_set_tx_ring_size(cl, tx_size); + ishtp_set_rx_ring_size(cl, rx_size); + + ishtp_cl_set_fw_client_id(cl, ishtp_get_fw_client_id(fw_client)); + + ishtp_set_connection_state(cl, ISHTP_CL_CONNECTING); + + /* + * For reset case, not allocate tx/rx ring buffer which are already + * done in ishtp_cl_connect() during first connection. + */ + if (reset) { + rets = ishtp_cl_connect_to_fw(cl); + if (!rets) + rets = ishtp_cl_read_start(cl); + else + dev->print_log(dev, + "%s() connect to fw failed\n", __func__); + } else { + rets = ishtp_cl_connect(cl); + } + + return rets; +} +EXPORT_SYMBOL(ishtp_cl_establish_connection); + +/** + * ishtp_cl_destroy_connection() - Disconnect with the firmware + * @cl: client device instance + * @reset: true if called for firmware reset, false for normal disconnection + * + * This is a helper function for client driver to disconnect with firmware, + * unlink to bus and flush message queue. + */ +void ishtp_cl_destroy_connection(struct ishtp_cl *cl, bool reset) +{ + if (!cl) + return; + + if (reset) { + /* + * For reset case, connection is already lost during fw reset. + * Just set state to DISCONNECTED is enough. + */ + ishtp_set_connection_state(cl, ISHTP_CL_DISCONNECTED); + } else { + if (cl->state != ISHTP_CL_DISCONNECTED) { + ishtp_set_connection_state(cl, ISHTP_CL_DISCONNECTING); + ishtp_cl_disconnect(cl); + } + } + + ishtp_cl_unlink(cl); + ishtp_cl_flush_queues(cl); +} +EXPORT_SYMBOL(ishtp_cl_destroy_connection); + /** * ishtp_cl_read_start() - Prepare to read client message * @cl: client device instance diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h index f45f13304addd..771622650247a 100644 --- a/include/linux/intel-ish-client-if.h +++ b/include/linux/intel-ish-client-if.h @@ -94,6 +94,9 @@ int ishtp_cl_link(struct ishtp_cl *cl); void ishtp_cl_unlink(struct ishtp_cl *cl); int ishtp_cl_disconnect(struct ishtp_cl *cl); int ishtp_cl_connect(struct ishtp_cl *cl); +int ishtp_cl_establish_connection(struct ishtp_cl *cl, const guid_t *uuid, + int tx_size, int rx_size, bool reset); +void ishtp_cl_destroy_connection(struct ishtp_cl *cl, bool reset); int ishtp_cl_send(struct ishtp_cl *cl, uint8_t *buf, size_t length); int ishtp_cl_flush_queues(struct ishtp_cl *cl); int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb); From f645a90e8ff732c48dd9f18815baef08c44ac8a0 Mon Sep 17 00:00:00 2001 From: Even Xu Date: Tue, 5 Dec 2023 09:50:31 +0800 Subject: [PATCH 07/53] HID: intel-ish-hid: ishtp-hid-client: use helper functions for connection Use helper functions ishtp_cl_establish_connection() and ishtp_cl_destroy_connection() to establish and destroy connection respectively. These functions are used during initialization, reset and deinitialization flows. No functional changes are expected. Signed-off-by: Even Xu Acked-by: Srinivas Pandruvada Signed-off-by: Jiri Kosina --- drivers/hid/intel-ish-hid/ishtp-hid-client.c | 63 ++++---------------- 1 file changed, 13 insertions(+), 50 deletions(-) diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c index e3d70c5460e96..fbd4f8ea1951b 100644 --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c @@ -639,47 +639,26 @@ static int ishtp_get_report_descriptor(struct ishtp_cl *hid_ishtp_cl, * * Return: 0 on success, non zero on error */ -static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset) +static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, bool reset) { - struct ishtp_device *dev; struct ishtp_cl_data *client_data = ishtp_get_client_data(hid_ishtp_cl); - struct ishtp_fw_client *fw_client; int i; int rv; dev_dbg(cl_data_to_dev(client_data), "%s\n", __func__); hid_ishtp_trace(client_data, "%s reset flag: %d\n", __func__, reset); - rv = ishtp_cl_link(hid_ishtp_cl); - if (rv) { - dev_err(cl_data_to_dev(client_data), - "ishtp_cl_link failed\n"); - return -ENOMEM; - } - client_data->init_done = 0; - dev = ishtp_get_ishtp_device(hid_ishtp_cl); - - /* Connect to FW client */ - ishtp_set_tx_ring_size(hid_ishtp_cl, HID_CL_TX_RING_SIZE); - ishtp_set_rx_ring_size(hid_ishtp_cl, HID_CL_RX_RING_SIZE); - - fw_client = ishtp_fw_cl_get_client(dev, &hid_ishtp_id_table[0].guid); - if (!fw_client) { - dev_err(cl_data_to_dev(client_data), - "ish client uuid not found\n"); - return -ENOENT; - } - ishtp_cl_set_fw_client_id(hid_ishtp_cl, - ishtp_get_fw_client_id(fw_client)); - ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_CONNECTING); - - rv = ishtp_cl_connect(hid_ishtp_cl); + rv = ishtp_cl_establish_connection(hid_ishtp_cl, + &hid_ishtp_id_table[0].guid, + HID_CL_TX_RING_SIZE, + HID_CL_RX_RING_SIZE, + reset); if (rv) { dev_err(cl_data_to_dev(client_data), "client connect fail\n"); - goto err_cl_unlink; + goto err_cl_disconnect; } hid_ishtp_trace(client_data, "%s client connected\n", __func__); @@ -723,10 +702,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset) return 0; err_cl_disconnect: - ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_DISCONNECTING); - ishtp_cl_disconnect(hid_ishtp_cl); -err_cl_unlink: - ishtp_cl_unlink(hid_ishtp_cl); + ishtp_cl_destroy_connection(hid_ishtp_cl, reset); return rv; } @@ -738,8 +714,7 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset) */ static void hid_ishtp_cl_deinit(struct ishtp_cl *hid_ishtp_cl) { - ishtp_cl_unlink(hid_ishtp_cl); - ishtp_cl_flush_queues(hid_ishtp_cl); + ishtp_cl_destroy_connection(hid_ishtp_cl, false); /* disband and free all Tx and Rx client-level rings */ ishtp_cl_free(hid_ishtp_cl); @@ -749,33 +724,23 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work) { struct ishtp_cl_data *client_data; struct ishtp_cl *hid_ishtp_cl; - struct ishtp_cl_device *cl_device; int retry; int rv; client_data = container_of(work, struct ishtp_cl_data, work); hid_ishtp_cl = client_data->hid_ishtp_cl; - cl_device = client_data->cl_device; hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__, hid_ishtp_cl); dev_dbg(ishtp_device(client_data->cl_device), "%s\n", __func__); - hid_ishtp_cl_deinit(hid_ishtp_cl); - - hid_ishtp_cl = ishtp_cl_allocate(cl_device); - if (!hid_ishtp_cl) - return; - - ishtp_set_drvdata(cl_device, hid_ishtp_cl); - ishtp_set_client_data(hid_ishtp_cl, client_data); - client_data->hid_ishtp_cl = hid_ishtp_cl; + ishtp_cl_destroy_connection(hid_ishtp_cl, true); client_data->num_hid_devices = 0; for (retry = 0; retry < 3; ++retry) { - rv = hid_ishtp_cl_init(hid_ishtp_cl, 1); + rv = hid_ishtp_cl_init(hid_ishtp_cl, true); if (!rv) break; dev_err(cl_data_to_dev(client_data), "Retry reset init\n"); @@ -841,7 +806,7 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device) ishtp_hid_print_trace = ishtp_trace_callback(cl_device); - rv = hid_ishtp_cl_init(hid_ishtp_cl, 0); + rv = hid_ishtp_cl_init(hid_ishtp_cl, false); if (rv) { ishtp_cl_free(hid_ishtp_cl); return rv; @@ -868,11 +833,9 @@ static void hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device) hid_ishtp_cl); dev_dbg(ishtp_device(cl_device), "%s\n", __func__); - ishtp_set_connection_state(hid_ishtp_cl, ISHTP_CL_DISCONNECTING); - ishtp_cl_disconnect(hid_ishtp_cl); + hid_ishtp_cl_deinit(hid_ishtp_cl); ishtp_put_device(cl_device); ishtp_hid_remove(client_data); - hid_ishtp_cl_deinit(hid_ishtp_cl); hid_ishtp_cl = NULL; From 09b57d983e0d85d223e11e1e69de4d3f0b675bf1 Mon Sep 17 00:00:00 2001 From: Even Xu Date: Tue, 5 Dec 2023 09:50:32 +0800 Subject: [PATCH 08/53] HID: intel-ish-hid: ishtp-fw-loader: use helper functions for connection Use helper functions ishtp_cl_establish_connection() and ishtp_cl_destroy_connection() to establish and destroy connection respectively. These functions are used during initialization, reset and deinitialization flows. No functional changes are expected. Signed-off-by: Even Xu Acked-by: Srinivas Pandruvada Signed-off-by: Jiri Kosina --- drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 60 +++++---------------- 1 file changed, 12 insertions(+), 48 deletions(-) diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c index 16aa030af8453..e157863a8b250 100644 --- a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c @@ -840,43 +840,22 @@ static void load_fw_from_host_handler(struct work_struct *work) * * Return: 0 for success, negative error code for failure */ -static int loader_init(struct ishtp_cl *loader_ishtp_cl, int reset) +static int loader_init(struct ishtp_cl *loader_ishtp_cl, bool reset) { int rv; - struct ishtp_fw_client *fw_client; struct ishtp_cl_data *client_data = ishtp_get_client_data(loader_ishtp_cl); dev_dbg(cl_data_to_dev(client_data), "reset flag: %d\n", reset); - rv = ishtp_cl_link(loader_ishtp_cl); - if (rv < 0) { - dev_err(cl_data_to_dev(client_data), "ishtp_cl_link failed\n"); - return rv; - } - - /* Connect to firmware client */ - ishtp_set_tx_ring_size(loader_ishtp_cl, LOADER_CL_TX_RING_SIZE); - ishtp_set_rx_ring_size(loader_ishtp_cl, LOADER_CL_RX_RING_SIZE); - - fw_client = - ishtp_fw_cl_get_client(ishtp_get_ishtp_device(loader_ishtp_cl), - &loader_ishtp_id_table[0].guid); - if (!fw_client) { - dev_err(cl_data_to_dev(client_data), - "ISH client uuid not found\n"); - rv = -ENOENT; - goto err_cl_unlink; - } - - ishtp_cl_set_fw_client_id(loader_ishtp_cl, - ishtp_get_fw_client_id(fw_client)); - ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_CONNECTING); - - rv = ishtp_cl_connect(loader_ishtp_cl); + rv = ishtp_cl_establish_connection(loader_ishtp_cl, + &loader_ishtp_id_table[0].guid, + LOADER_CL_TX_RING_SIZE, + LOADER_CL_RX_RING_SIZE, + reset); if (rv < 0) { dev_err(cl_data_to_dev(client_data), "Client connect fail\n"); - goto err_cl_unlink; + goto err_cl_disconnect; } dev_dbg(cl_data_to_dev(client_data), "Client connected\n"); @@ -885,17 +864,14 @@ static int loader_init(struct ishtp_cl *loader_ishtp_cl, int reset) return 0; -err_cl_unlink: - ishtp_cl_unlink(loader_ishtp_cl); +err_cl_disconnect: + ishtp_cl_destroy_connection(loader_ishtp_cl, reset); return rv; } static void loader_deinit(struct ishtp_cl *loader_ishtp_cl) { - ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_DISCONNECTING); - ishtp_cl_disconnect(loader_ishtp_cl); - ishtp_cl_unlink(loader_ishtp_cl); - ishtp_cl_flush_queues(loader_ishtp_cl); + ishtp_cl_destroy_connection(loader_ishtp_cl, false); /* Disband and free all Tx and Rx client-level rings */ ishtp_cl_free(loader_ishtp_cl); @@ -914,19 +890,7 @@ static void reset_handler(struct work_struct *work) loader_ishtp_cl = client_data->loader_ishtp_cl; cl_device = client_data->cl_device; - /* Unlink, flush queues & start again */ - ishtp_cl_unlink(loader_ishtp_cl); - ishtp_cl_flush_queues(loader_ishtp_cl); - ishtp_cl_free(loader_ishtp_cl); - - loader_ishtp_cl = ishtp_cl_allocate(cl_device); - if (!loader_ishtp_cl) - return; - - ishtp_set_drvdata(cl_device, loader_ishtp_cl); - ishtp_set_client_data(loader_ishtp_cl, client_data); - client_data->loader_ishtp_cl = loader_ishtp_cl; - client_data->cl_device = cl_device; + ishtp_cl_destroy_connection(loader_ishtp_cl, true); rv = loader_init(loader_ishtp_cl, 1); if (rv < 0) { @@ -974,7 +938,7 @@ static int loader_ishtp_cl_probe(struct ishtp_cl_device *cl_device) INIT_WORK(&client_data->work_fw_load, load_fw_from_host_handler); - rv = loader_init(loader_ishtp_cl, 0); + rv = loader_init(loader_ishtp_cl, false); if (rv < 0) { ishtp_cl_free(loader_ishtp_cl); return rv; From 42a244be36cda2da571c72feb5d1d2b45866735f Mon Sep 17 00:00:00 2001 From: Even Xu Date: Tue, 5 Dec 2023 09:50:33 +0800 Subject: [PATCH 09/53] platform/chrome: cros_ec_ishtp: use helper functions for connection Use helper functions ishtp_cl_establish_connection() and ishtp_cl_destroy_connection() to establish and destroy connection respectively. These functions are used during initialization, reset and deinitialization flows. No functional changes are expected. Signed-off-by: Even Xu Acked-by: Srinivas Pandruvada Acked-by: Tzung-Bi Shih Signed-off-by: Jiri Kosina --- drivers/platform/chrome/cros_ec_ishtp.c | 74 +++++-------------------- 1 file changed, 15 insertions(+), 59 deletions(-) diff --git a/drivers/platform/chrome/cros_ec_ishtp.c b/drivers/platform/chrome/cros_ec_ishtp.c index cb2031cf7106f..5ac37bd024c88 100644 --- a/drivers/platform/chrome/cros_ec_ishtp.c +++ b/drivers/platform/chrome/cros_ec_ishtp.c @@ -367,55 +367,33 @@ static void ish_event_cb(struct ishtp_cl_device *cl_device) /** * cros_ish_init() - Init function for ISHTP client * @cros_ish_cl: ISHTP client instance + * @reset: true if called from reset handler * * This function complete the initializtion of the client. * * Return: 0 for success, negative error code for failure. */ -static int cros_ish_init(struct ishtp_cl *cros_ish_cl) +static int cros_ish_init(struct ishtp_cl *cros_ish_cl, bool reset) { int rv; - struct ishtp_device *dev; - struct ishtp_fw_client *fw_client; struct ishtp_cl_data *client_data = ishtp_get_client_data(cros_ish_cl); - rv = ishtp_cl_link(cros_ish_cl); - if (rv) { - dev_err(cl_data_to_dev(client_data), - "ishtp_cl_link failed\n"); - return rv; - } - - dev = ishtp_get_ishtp_device(cros_ish_cl); - - /* Connect to firmware client */ - ishtp_set_tx_ring_size(cros_ish_cl, CROS_ISH_CL_TX_RING_SIZE); - ishtp_set_rx_ring_size(cros_ish_cl, CROS_ISH_CL_RX_RING_SIZE); - - fw_client = ishtp_fw_cl_get_client(dev, &cros_ec_ishtp_id_table[0].guid); - if (!fw_client) { - dev_err(cl_data_to_dev(client_data), - "ish client uuid not found\n"); - rv = -ENOENT; - goto err_cl_unlink; - } - - ishtp_cl_set_fw_client_id(cros_ish_cl, - ishtp_get_fw_client_id(fw_client)); - ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_CONNECTING); - - rv = ishtp_cl_connect(cros_ish_cl); + rv = ishtp_cl_establish_connection(cros_ish_cl, + &cros_ec_ishtp_id_table[0].guid, + CROS_ISH_CL_TX_RING_SIZE, + CROS_ISH_CL_RX_RING_SIZE, + reset); if (rv) { dev_err(cl_data_to_dev(client_data), "client connect fail\n"); - goto err_cl_unlink; + goto err_cl_disconnect; } ishtp_register_event_cb(client_data->cl_device, ish_event_cb); return 0; -err_cl_unlink: - ishtp_cl_unlink(cros_ish_cl); +err_cl_disconnect: + ishtp_cl_destroy_connection(cros_ish_cl, reset); return rv; } @@ -427,10 +405,7 @@ static int cros_ish_init(struct ishtp_cl *cros_ish_cl) */ static void cros_ish_deinit(struct ishtp_cl *cros_ish_cl) { - ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_DISCONNECTING); - ishtp_cl_disconnect(cros_ish_cl); - ishtp_cl_unlink(cros_ish_cl); - ishtp_cl_flush_queues(cros_ish_cl); + ishtp_cl_destroy_connection(cros_ish_cl, false); /* Disband and free all Tx and Rx client-level rings */ ishtp_cl_free(cros_ish_cl); @@ -592,7 +567,6 @@ static void reset_handler(struct work_struct *work) int rv; struct device *dev; struct ishtp_cl *cros_ish_cl; - struct ishtp_cl_device *cl_device; struct ishtp_cl_data *client_data = container_of(work, struct ishtp_cl_data, work_ishtp_reset); @@ -600,26 +574,11 @@ static void reset_handler(struct work_struct *work) down_write(&init_lock); cros_ish_cl = client_data->cros_ish_cl; - cl_device = client_data->cl_device; - /* Unlink, flush queues & start again */ - ishtp_cl_unlink(cros_ish_cl); - ishtp_cl_flush_queues(cros_ish_cl); - ishtp_cl_free(cros_ish_cl); - - cros_ish_cl = ishtp_cl_allocate(cl_device); - if (!cros_ish_cl) { - up_write(&init_lock); - return; - } - - ishtp_set_drvdata(cl_device, cros_ish_cl); - ishtp_set_client_data(cros_ish_cl, client_data); - client_data->cros_ish_cl = cros_ish_cl; + ishtp_cl_destroy_connection(cros_ish_cl, true); - rv = cros_ish_init(cros_ish_cl); + rv = cros_ish_init(cros_ish_cl, true); if (rv) { - ishtp_cl_free(cros_ish_cl); dev_err(cl_data_to_dev(client_data), "Reset Failed\n"); up_write(&init_lock); return; @@ -672,7 +631,7 @@ static int cros_ec_ishtp_probe(struct ishtp_cl_device *cl_device) INIT_WORK(&client_data->work_ec_evt, ish_evt_handler); - rv = cros_ish_init(cros_ish_cl); + rv = cros_ish_init(cros_ish_cl, false); if (rv) goto end_ishtp_cl_init_error; @@ -690,10 +649,7 @@ static int cros_ec_ishtp_probe(struct ishtp_cl_device *cl_device) return 0; end_cros_ec_dev_init_error: - ishtp_set_connection_state(cros_ish_cl, ISHTP_CL_DISCONNECTING); - ishtp_cl_disconnect(cros_ish_cl); - ishtp_cl_unlink(cros_ish_cl); - ishtp_cl_flush_queues(cros_ish_cl); + ishtp_cl_destroy_connection(cros_ish_cl, false); ishtp_put_device(cl_device); end_ishtp_cl_init_error: ishtp_cl_free(cros_ish_cl); From f023605d1de6f150d9266747a2630f029956041f Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:08 +0100 Subject: [PATCH 10/53] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset() i2c_hid_hwreset() is the only caller of i2c_hid_execute_reset(), fold the latter into the former. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. No functional changes intended. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 73 ++++++++++++------------------ 1 file changed, 30 insertions(+), 43 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 2735cd585af0d..ca2a4ccb9abfe 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -426,12 +426,23 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) return ret; } -static int i2c_hid_execute_reset(struct i2c_hid *ihid) +static int i2c_hid_hwreset(struct i2c_hid *ihid) { size_t length = 0; int ret; - i2c_hid_dbg(ihid, "resetting...\n"); + i2c_hid_dbg(ihid, "%s\n", __func__); + + /* + * This prevents sending feature reports while the device is + * being reset. Otherwise we may lose the reset complete + * interrupt. + */ + mutex_lock(&ihid->reset_lock); + + ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); + if (ret) + goto err_unlock; /* Prepare reset command. Command register goes first. */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; @@ -444,59 +455,35 @@ static int i2c_hid_execute_reset(struct i2c_hid *ihid) ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0); if (ret) { - dev_err(&ihid->client->dev, "failed to reset device.\n"); - goto out; + dev_err(&ihid->client->dev, + "failed to reset device: %d\n", ret); + goto err_clear_reset; } + i2c_hid_dbg(ihid, "%s: waiting...\n", __func__); + if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) { msleep(100); - goto out; - } - - i2c_hid_dbg(ihid, "%s: waiting...\n", __func__); - if (!wait_event_timeout(ihid->wait, - !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), - msecs_to_jiffies(5000))) { + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + } else if (!wait_event_timeout(ihid->wait, + !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), + msecs_to_jiffies(5000))) { ret = -ENODATA; - goto out; + goto err_clear_reset; } i2c_hid_dbg(ihid, "%s: finished.\n", __func__); -out: - clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); - return ret; -} - -static int i2c_hid_hwreset(struct i2c_hid *ihid) -{ - int ret; - - i2c_hid_dbg(ihid, "%s\n", __func__); - - /* - * This prevents sending feature reports while the device is - * being reset. Otherwise we may lose the reset complete - * interrupt. - */ - mutex_lock(&ihid->reset_lock); - - ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); - if (ret) - goto out_unlock; - - ret = i2c_hid_execute_reset(ihid); - if (ret) { - dev_err(&ihid->client->dev, - "failed to reset device: %d\n", ret); - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); - goto out_unlock; - } - /* At least some SIS devices need this after reset */ if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET)) ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); -out_unlock: + mutex_unlock(&ihid->reset_lock); + return ret; + +err_clear_reset: + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); +err_unlock: mutex_unlock(&ihid->reset_lock); return ret; } From 96d3098db835d58649b73a5788898bd7672a319b Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:09 +0100 Subject: [PATCH 11/53] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions Split i2c_hid_hwreset() into: i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and i2c_hid_finish_hwreset() which actually waits for the reset to complete. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. No functional changes intended. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 38 ++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index ca2a4ccb9abfe..21d65ca328666 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -426,7 +426,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) return ret; } -static int i2c_hid_hwreset(struct i2c_hid *ihid) +static int i2c_hid_start_hwreset(struct i2c_hid *ihid) { size_t length = 0; int ret; @@ -438,11 +438,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid) * being reset. Otherwise we may lose the reset complete * interrupt. */ - mutex_lock(&ihid->reset_lock); + lockdep_assert_held(&ihid->reset_lock); ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); if (ret) - goto err_unlock; + return ret; /* Prepare reset command. Command register goes first. */ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister; @@ -460,6 +460,18 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid) goto err_clear_reset; } + return 0; + +err_clear_reset: + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); + return ret; +} + +static int i2c_hid_finish_hwreset(struct i2c_hid *ihid) +{ + int ret = 0; + i2c_hid_dbg(ihid, "%s: waiting...\n", __func__); if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) { @@ -477,14 +489,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid) if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET)) ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); - mutex_unlock(&ihid->reset_lock); return ret; err_clear_reset: clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); -err_unlock: - mutex_unlock(&ihid->reset_lock); return ret; } @@ -731,7 +740,11 @@ static int i2c_hid_parse(struct hid_device *hid) } do { - ret = i2c_hid_hwreset(ihid); + mutex_lock(&ihid->reset_lock); + ret = i2c_hid_start_hwreset(ihid); + if (ret == 0) + ret = i2c_hid_finish_hwreset(ihid); + mutex_unlock(&ihid->reset_lock); if (ret) msleep(1000); } while (tries-- > 0 && ret); @@ -974,10 +987,15 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid) * However some ALPS touchpads generate IRQ storm without reset, so * let's still reset them here. */ - if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) - ret = i2c_hid_hwreset(ihid); - else + if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) { + mutex_lock(&ihid->reset_lock); + ret = i2c_hid_start_hwreset(ihid); + if (ret == 0) + ret = i2c_hid_finish_hwreset(ihid); + mutex_unlock(&ihid->reset_lock); + } else { ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); + } if (ret) return ret; From aa69d6974185e9f7a552ba982540a38e34f69690 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:10 +0100 Subject: [PATCH 12/53] HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling Switch i2c_hid_parse() to goto style error handling. This is a preparation patch for removing the need for I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave more like Windows. Note this changes the descriptor read error path to propagate the actual i2c_hid_read_register() error code (which is always negative) instead of hardcoding a -EIO return. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 21d65ca328666..71d742aeaf35a 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -773,23 +773,21 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc, rsize); if (ret) { hid_err(hid, "reading report descriptor failed\n"); - kfree(rdesc); - return -EIO; + goto out; } } i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc); ret = hid_parse_report(hid, rdesc, rsize); + if (ret) + dbg_hid("parsing report descriptor failed\n"); + +out: if (!use_override) kfree(rdesc); - if (ret) { - dbg_hid("parsing report descriptor failed\n"); - return ret; - } - - return 0; + return ret; } static int i2c_hid_start(struct hid_device *hid) From af93a167eda90192563bce64c4bb989836afac1f Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:11 +0100 Subject: [PATCH 13/53] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A recent bug made me look at Microsoft's i2c-hid docs again and I noticed the following: """ 4. Issue a RESET (Host Initiated Reset) to the Device. 5. Retrieve report descriptor from the device. Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C. Since report descriptors are (a) static and (b) quite long, Windows 8 may issue a request for 5 while it is waiting for a response from the device on 4. """ Which made me think that maybe on some touchpads the reset ack is delayed till after the report descriptor is read ? Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad, for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced, shows that reading the report descriptor before waiting for the reset helps with the missing reset IRQ. Now the reset does get acked properly, but the ack sometimes still does not happen unfortunately. Still moving the wait for ack to after reading the report-descriptor, is probably a good idea, both to make i2c-hid's behavior closer to Windows as well as to speed up probing i2c-hid devices. While at it drop the dbg_hid() for a malloc failure, malloc failures already get logged extensively by malloc itself. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2247751 Signed-off-by: Hans de Goede Reviewed-by: Douglas Anderson Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 71d742aeaf35a..400c15a180b57 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -725,11 +725,10 @@ static int i2c_hid_parse(struct hid_device *hid) struct i2c_client *client = hid->driver_data; struct i2c_hid *ihid = i2c_get_clientdata(client); struct i2c_hid_desc *hdesc = &ihid->hdesc; + char *rdesc = NULL, *use_override = NULL; unsigned int rsize; - char *rdesc; int ret; int tries = 3; - char *use_override; i2c_hid_dbg(ihid, "entering %s\n", __func__); @@ -739,18 +738,15 @@ static int i2c_hid_parse(struct hid_device *hid) return -EINVAL; } + mutex_lock(&ihid->reset_lock); do { - mutex_lock(&ihid->reset_lock); ret = i2c_hid_start_hwreset(ihid); - if (ret == 0) - ret = i2c_hid_finish_hwreset(ihid); - mutex_unlock(&ihid->reset_lock); if (ret) msleep(1000); } while (tries-- > 0 && ret); if (ret) - return ret; + goto abort_reset; use_override = i2c_hid_get_dmi_hid_report_desc_override(client->name, &rsize); @@ -762,8 +758,8 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc = kzalloc(rsize, GFP_KERNEL); if (!rdesc) { - dbg_hid("couldn't allocate rdesc memory\n"); - return -ENOMEM; + ret = -ENOMEM; + goto abort_reset; } i2c_hid_dbg(ihid, "asking HID report descriptor\n"); @@ -773,10 +769,23 @@ static int i2c_hid_parse(struct hid_device *hid) rdesc, rsize); if (ret) { hid_err(hid, "reading report descriptor failed\n"); - goto out; + goto abort_reset; } } + /* + * Windows directly reads the report-descriptor after sending reset + * and then waits for resets completion afterwards. Some touchpads + * actually wait for the report-descriptor to be read before signalling + * reset completion. + */ + ret = i2c_hid_finish_hwreset(ihid); +abort_reset: + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); + mutex_unlock(&ihid->reset_lock); + if (ret) + goto out; + i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc); ret = hid_parse_report(hid, rdesc, rsize); From 7bcf9ebb50f2afc84b33b06edfd20d47218af758 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:12 +0100 Subject: [PATCH 14/53] HID: i2c-hid: Turn missing reset ack into a warning On all i2c-hid devices seen sofar the reset-ack either works, or the hw is somehow buggy and does not (always) ack the reset properly, yet it still works fine. Lower the very long reset timeout to 1 second which should be plenty and change the reset not getting acked from an error into a warning. This results in a bit cleaner code and avoids the need to add more I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 400c15a180b57..88a203e920dec 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -479,9 +479,9 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid) clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); } else if (!wait_event_timeout(ihid->wait, !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), - msecs_to_jiffies(5000))) { - ret = -ENODATA; - goto err_clear_reset; + msecs_to_jiffies(1000))) { + dev_warn(&ihid->client->dev, "device did not ack reset within 1000 ms\n"); + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); } i2c_hid_dbg(ihid, "%s: finished.\n", __func__); @@ -490,11 +490,6 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid) ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON); return ret; - -err_clear_reset: - clear_bit(I2C_HID_RESET_PENDING, &ihid->flags); - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP); - return ret; } static void i2c_hid_get_input(struct i2c_hid *ihid) From bd008acdac45011f2246ec2518ef19c2da9e6008 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:13 +0100 Subject: [PATCH 15/53] HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk Re-trying the power-on command on failure on all devices should not be a problem, drop the I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk and simply retry power-on on all devices. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 88a203e920dec..0c1a7cd84e4c1 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -44,7 +44,6 @@ #include "i2c-hid.h" /* quirks to control the device */ -#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV BIT(0) #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1) #define I2C_HID_QUIRK_BOGUS_IRQ BIT(4) #define I2C_HID_QUIRK_RESET_ON_RESUME BIT(5) @@ -120,8 +119,6 @@ static const struct i2c_hid_quirks { __u16 idProduct; __u32 quirks; } i2c_hid_quirks[] = { - { USB_VENDOR_ID_WEIDA, HID_ANY_ID, - I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV }, { I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288, I2C_HID_QUIRK_NO_IRQ_AFTER_RESET }, { I2C_VENDOR_ID_ITE, I2C_DEVICE_ID_ITE_VOYO_WINPAD_A15, @@ -395,8 +392,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state) * The call will get a return value (EREMOTEIO) but device will be * triggered and activated. After that, it goes like a normal device. */ - if (power_state == I2C_HID_PWR_ON && - ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV) { + if (power_state == I2C_HID_PWR_ON) { ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON); /* Device was already activated */ From 7d7a252842ecafb9b4541dc8470907e97bc6df62 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Sat, 2 Dec 2023 23:46:14 +0100 Subject: [PATCH 16/53] HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines The quirks variable and the I2C_HID_QUIRK_ defines are never used / exported outside of the i2c-hid code renumber them to start at BIT(0) again. Reviewed-by: Douglas Anderson Signed-off-by: Hans de Goede Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 0c1a7cd84e4c1..90f316ae9819a 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -44,11 +44,11 @@ #include "i2c-hid.h" /* quirks to control the device */ -#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1) -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4) -#define I2C_HID_QUIRK_RESET_ON_RESUME BIT(5) -#define I2C_HID_QUIRK_BAD_INPUT_SIZE BIT(6) -#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET BIT(7) +#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(0) +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(1) +#define I2C_HID_QUIRK_RESET_ON_RESUME BIT(2) +#define I2C_HID_QUIRK_BAD_INPUT_SIZE BIT(3) +#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET BIT(4) /* Command opcodes */ #define I2C_HID_OPCODE_RESET 0x01 From 887f8094b3352127d1bc68f768774e97acf4e7fa Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:52 +0100 Subject: [PATCH 17/53] selftests/hid: vmtest.sh: update vm2c and container boot2container is now on an official project, so let's use that. The container image is now the same I use for the CI, so let's keep to it. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-1-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/vmtest.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/hid/vmtest.sh b/tools/testing/selftests/hid/vmtest.sh index 4da48bf6b328a..301b4e1623368 100755 --- a/tools/testing/selftests/hid/vmtest.sh +++ b/tools/testing/selftests/hid/vmtest.sh @@ -19,12 +19,12 @@ esac SCRIPT_DIR="$(dirname $(realpath $0))" OUTPUT_DIR="$SCRIPT_DIR/results" KCONFIG_REL_PATHS=("${SCRIPT_DIR}/config" "${SCRIPT_DIR}/config.common" "${SCRIPT_DIR}/config.${ARCH}") -B2C_URL="https://gitlab.freedesktop.org/mupuf/boot2container/-/raw/master/vm2c.py" +B2C_URL="https://gitlab.freedesktop.org/gfx-ci/boot2container/-/raw/main/vm2c.py" NUM_COMPILE_JOBS="$(nproc)" LOG_FILE_BASE="$(date +"hid_selftests.%Y-%m-%d_%H-%M-%S")" LOG_FILE="${LOG_FILE_BASE}.log" EXIT_STATUS_FILE="${LOG_FILE_BASE}.exit_status" -CONTAINER_IMAGE="registry.freedesktop.org/libevdev/hid-tools/fedora/37:2023-02-17.1" +CONTAINER_IMAGE="registry.freedesktop.org/bentiss/hid/fedora/39:2023-11-22.1" TARGETS="${TARGETS:=$(basename ${SCRIPT_DIR})}" DEFAULT_COMMAND="pip3 install hid-tools; make -C tools/testing/selftests TARGETS=${TARGETS} run_tests" From 46bc0277c2507338d98e166826afec330962309e Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:53 +0100 Subject: [PATCH 18/53] selftests/hid: vmtest.sh: allow finer control on the build steps vmtest.sh works great for a one shot test, but not so much for CI where I want to build (with different configs) the bzImage in a separate job than the one I am running it. Add a "build_only" option to specify whether we need to boot the currently built kernel in the vm. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-2-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/vmtest.sh | 42 +++++++++++++++------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tools/testing/selftests/hid/vmtest.sh b/tools/testing/selftests/hid/vmtest.sh index 301b4e1623368..db534e9099a8a 100755 --- a/tools/testing/selftests/hid/vmtest.sh +++ b/tools/testing/selftests/hid/vmtest.sh @@ -32,7 +32,7 @@ DEFAULT_COMMAND="pip3 install hid-tools; make -C tools/testing/selftests TARGETS usage() { cat <] -- [] +Usage: $0 [-j N] [-s] [-b] [-d ] -- [] is the command you would normally run when you are in the source kernel direcory. e.g: @@ -55,6 +55,7 @@ Options: -u) Update the boot2container script to a newer version. -d) Update the output directory (default: ${OUTPUT_DIR}) + -b) Run only the build steps for the kernel and the selftests -j) Number of jobs for compilation, similar to -j in make (default: ${NUM_COMPILE_JOBS}) -s) Instead of powering off the VM, start an interactive @@ -191,8 +192,9 @@ main() local command="${DEFAULT_COMMAND}" local update_b2c="no" local debug_shell="no" + local build_only="no" - while getopts ':hsud:j:' opt; do + while getopts ':hsud:j:b' opt; do case ${opt} in u) update_b2c="yes" @@ -207,6 +209,9 @@ main() command="/bin/sh" debug_shell="yes" ;; + b) + build_only="yes" + ;; h) usage exit 0 @@ -226,8 +231,7 @@ main() shift $((OPTIND -1)) # trap 'catch "$?"' EXIT - - if [[ "${debug_shell}" == "no" ]]; then + if [[ "${build_only}" == "no" && "${debug_shell}" == "no" ]]; then if [[ $# -eq 0 ]]; then echo "No command specified, will run ${DEFAULT_COMMAND} in the vm" else @@ -267,24 +271,26 @@ main() update_kconfig "${kernel_checkout}" "${kconfig_file}" recompile_kernel "${kernel_checkout}" "${make_command}" + update_selftests "${kernel_checkout}" "${make_command}" - if [[ "${update_b2c}" == "no" && ! -f "${b2c}" ]]; then - echo "vm2c script not found in ${b2c}" - update_b2c="yes" - fi + if [[ "${build_only}" == "no" ]]; then + if [[ "${update_b2c}" == "no" && ! -f "${b2c}" ]]; then + echo "vm2c script not found in ${b2c}" + update_b2c="yes" + fi - if [[ "${update_b2c}" == "yes" ]]; then - download $B2C_URL $b2c - chmod +x $b2c - fi + if [[ "${update_b2c}" == "yes" ]]; then + download $B2C_URL $b2c + chmod +x $b2c + fi - update_selftests "${kernel_checkout}" "${make_command}" - run_vm "${kernel_checkout}" $b2c "${kernel_bzimage}" "${command}" - if [[ "${debug_shell}" != "yes" ]]; then - echo "Logs saved in ${OUTPUT_DIR}/${LOG_FILE}" - fi + run_vm "${kernel_checkout}" $b2c "${kernel_bzimage}" "${command}" + if [[ "${debug_shell}" != "yes" ]]; then + echo "Logs saved in ${OUTPUT_DIR}/${LOG_FILE}" + fi - exit $(cat ${OUTPUT_DIR}/${EXIT_STATUS_FILE}) + exit $(cat ${OUTPUT_DIR}/${EXIT_STATUS_FILE}) + fi } main "$@" From 110292a77f7c8d11eaa472e65f6a2084b25be2db Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:54 +0100 Subject: [PATCH 19/53] selftests/hid: base: allow for multiple skip_if_uhdev We can actually have multiple occurences of `skip_if_uhdev` if we follow the information from the pytest doc[0]. This is not immediately used, but can be if we need multiple conditions on a given test. [0] https://docs.pytest.org/en/latest/historical-notes.html#update-marker-code Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-3-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/base.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/testing/selftests/hid/tests/base.py b/tools/testing/selftests/hid/tests/base.py index 1305cfc9646e0..5d9c26dfc4605 100644 --- a/tools/testing/selftests/hid/tests/base.py +++ b/tools/testing/selftests/hid/tests/base.py @@ -238,8 +238,7 @@ def context(self, new_uhdev, request): try: with HIDTestUdevRule.instance(): with new_uhdev as self.uhdev: - skip_cond = request.node.get_closest_marker("skip_if_uhdev") - if skip_cond: + for skip_cond in request.node.iter_markers("skip_if_uhdev"): test, message, *rest = skip_cond.args if test(self.uhdev): From b5edacf79c8e1990200f9392e6354583298e8765 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:55 +0100 Subject: [PATCH 20/53] selftests/hid: tablets: remove unused class Looks like this is a leftover Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-4-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/test_tablet.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index 303ffff9ee8dc..cd9c1269afa6a 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -133,10 +133,6 @@ def valid_transitions(self) -> Tuple["PenState", ...]: return tuple() -class Data(object): - pass - - class Pen(object): def __init__(self, x, y): self.x = x From d52f52069fed639113b1014cde5eff8902d3064d Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:56 +0100 Subject: [PATCH 21/53] selftests/hid: tablets: move the transitions to PenState Those transitions have nothing to do with `Pen`, so migrate them to `PenState`. The hidden agenda is to remove `Pen` and integrate it into `PenDigitizer` so that we can tweak the events in each state to emulate firmware bugs. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-5-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 215 +++++++++--------- 1 file changed, 109 insertions(+), 106 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index cd9c1269afa6a..ddf28c2450460 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -132,104 +132,8 @@ def valid_transitions(self) -> Tuple["PenState", ...]: return tuple() - -class Pen(object): - def __init__(self, x, y): - self.x = x - self.y = y - self.tipswitch = False - self.tippressure = 15 - self.azimuth = 0 - self.inrange = False - self.width = 10 - self.height = 10 - self.barrelswitch = False - self.invert = False - self.eraser = False - self.x_tilt = 0 - self.y_tilt = 0 - self.twist = 0 - self._old_values = None - self.current_state = None - - def _restore(self): - if self._old_values is not None: - for i in [ - "x", - "y", - "tippressure", - "azimuth", - "width", - "height", - "twist", - "x_tilt", - "y_tilt", - ]: - setattr(self, i, getattr(self._old_values, i)) - - def move_to(self, state): - # fill in the previous values - if self.current_state == PenState.PEN_IS_OUT_OF_RANGE: - self._restore() - - print(f"\n *** pen is moving to {state} ***") - - if state == PenState.PEN_IS_OUT_OF_RANGE: - self._old_values = copy.copy(self) - self.x = 0 - self.y = 0 - self.tipswitch = False - self.tippressure = 0 - self.azimuth = 0 - self.inrange = False - self.width = 0 - self.height = 0 - self.invert = False - self.eraser = False - self.x_tilt = 0 - self.y_tilt = 0 - self.twist = 0 - elif state == PenState.PEN_IS_IN_RANGE: - self.tipswitch = False - self.inrange = True - self.invert = False - self.eraser = False - elif state == PenState.PEN_IS_IN_CONTACT: - self.tipswitch = True - self.inrange = True - self.invert = False - self.eraser = False - elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT: - self.tipswitch = False - self.inrange = True - self.invert = True - self.eraser = False - elif state == PenState.PEN_IS_ERASING: - self.tipswitch = False - self.inrange = True - self.invert = True - self.eraser = True - - self.current_state = state - - def __assert_axis(self, evdev, axis, value): - if ( - axis == libevdev.EV_KEY.BTN_TOOL_RUBBER - and evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] is None - ): - return - - assert ( - evdev.value[axis] == value - ), f"assert evdev.value[{axis}] ({evdev.value[axis]}) != {value}" - - def assert_expected_input_events(self, evdev): - assert evdev.value[libevdev.EV_ABS.ABS_X] == self.x - assert evdev.value[libevdev.EV_ABS.ABS_Y] == self.y - assert self.current_state == PenState.from_evdev(evdev) - @staticmethod - def legal_transitions() -> Dict[str, Tuple[PenState, ...]]: + def legal_transitions() -> Dict[str, Tuple["PenState", ...]]: """This is the first half of the Windows Pen Implementation state machine: we don't have Invert nor Erase bits, so just move in/out-of-range or proximity. https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states @@ -255,7 +159,7 @@ def legal_transitions() -> Dict[str, Tuple[PenState, ...]]: } @staticmethod - def legal_transitions_with_invert() -> Dict[str, Tuple[PenState, ...]]: + def legal_transitions_with_invert() -> Dict[str, Tuple["PenState", ...]]: """This is the second half of the Windows Pen Implementation state machine: we now have Invert and Erase bits, so move in/out or proximity with the intend to erase. @@ -293,7 +197,7 @@ def legal_transitions_with_invert() -> Dict[str, Tuple[PenState, ...]]: } @staticmethod - def tolerated_transitions() -> Dict[str, Tuple[PenState, ...]]: + def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]: """This is not adhering to the Windows Pen Implementation state machine but we should expect the kernel to behave properly, mostly for historical reasons.""" @@ -306,7 +210,7 @@ def tolerated_transitions() -> Dict[str, Tuple[PenState, ...]]: } @staticmethod - def tolerated_transitions_with_invert() -> Dict[str, Tuple[PenState, ...]]: + def tolerated_transitions_with_invert() -> Dict[str, Tuple["PenState", ...]]: """This is the second half of the Windows Pen Implementation state machine: we now have Invert and Erase bits, so move in/out or proximity with the intend to erase. @@ -321,7 +225,7 @@ def tolerated_transitions_with_invert() -> Dict[str, Tuple[PenState, ...]]: } @staticmethod - def broken_transitions() -> Dict[str, Tuple[PenState, ...]]: + def broken_transitions() -> Dict[str, Tuple["PenState", ...]]: """Those tests are definitely not part of the Windows specification. However, a half broken device might export those transitions. For example, a pen that has the eraser button might wobble between @@ -359,6 +263,102 @@ def broken_transitions() -> Dict[str, Tuple[PenState, ...]]: } +class Pen(object): + def __init__(self, x, y): + self.x = x + self.y = y + self.tipswitch = False + self.tippressure = 15 + self.azimuth = 0 + self.inrange = False + self.width = 10 + self.height = 10 + self.barrelswitch = False + self.invert = False + self.eraser = False + self.x_tilt = 0 + self.y_tilt = 0 + self.twist = 0 + self._old_values = None + self.current_state = None + + def _restore(self): + if self._old_values is not None: + for i in [ + "x", + "y", + "tippressure", + "azimuth", + "width", + "height", + "twist", + "x_tilt", + "y_tilt", + ]: + setattr(self, i, getattr(self._old_values, i)) + + def move_to(self, state): + # fill in the previous values + if self.current_state == PenState.PEN_IS_OUT_OF_RANGE: + self._restore() + + print(f"\n *** pen is moving to {state} ***") + + if state == PenState.PEN_IS_OUT_OF_RANGE: + self._old_values = copy.copy(self) + self.x = 0 + self.y = 0 + self.tipswitch = False + self.tippressure = 0 + self.azimuth = 0 + self.inrange = False + self.width = 0 + self.height = 0 + self.invert = False + self.eraser = False + self.x_tilt = 0 + self.y_tilt = 0 + self.twist = 0 + elif state == PenState.PEN_IS_IN_RANGE: + self.tipswitch = False + self.inrange = True + self.invert = False + self.eraser = False + elif state == PenState.PEN_IS_IN_CONTACT: + self.tipswitch = True + self.inrange = True + self.invert = False + self.eraser = False + elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT: + self.tipswitch = False + self.inrange = True + self.invert = True + self.eraser = False + elif state == PenState.PEN_IS_ERASING: + self.tipswitch = False + self.inrange = True + self.invert = True + self.eraser = True + + self.current_state = state + + def __assert_axis(self, evdev, axis, value): + if ( + axis == libevdev.EV_KEY.BTN_TOOL_RUBBER + and evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] is None + ): + return + + assert ( + evdev.value[axis] == value + ), f"assert evdev.value[{axis}] ({evdev.value[axis]}) != {value}" + + def assert_expected_input_events(self, evdev): + assert evdev.value[libevdev.EV_ABS.ABS_X] == self.x + assert evdev.value[libevdev.EV_ABS.ABS_Y] == self.y + assert self.current_state == PenState.from_evdev(evdev) + + class PenDigitizer(base.UHIDTestDevice): def __init__( self, @@ -486,7 +486,7 @@ def _test_states(self, state_list, scribble): @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) @pytest.mark.parametrize( "state_list", - [pytest.param(v, id=k) for k, v in Pen.legal_transitions().items()], + [pytest.param(v, id=k) for k, v in PenState.legal_transitions().items()], ) def test_valid_pen_states(self, state_list, scribble): """This is the first half of the Windows Pen Implementation state machine: @@ -498,7 +498,10 @@ def test_valid_pen_states(self, state_list, scribble): @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) @pytest.mark.parametrize( "state_list", - [pytest.param(v, id=k) for k, v in Pen.tolerated_transitions().items()], + [ + pytest.param(v, id=k) + for k, v in PenState.tolerated_transitions().items() + ], ) def test_tolerated_pen_states(self, state_list, scribble): """This is not adhering to the Windows Pen Implementation state machine @@ -515,7 +518,7 @@ def test_tolerated_pen_states(self, state_list, scribble): "state_list", [ pytest.param(v, id=k) - for k, v in Pen.legal_transitions_with_invert().items() + for k, v in PenState.legal_transitions_with_invert().items() ], ) def test_valid_invert_pen_states(self, state_list, scribble): @@ -535,7 +538,7 @@ def test_valid_invert_pen_states(self, state_list, scribble): "state_list", [ pytest.param(v, id=k) - for k, v in Pen.tolerated_transitions_with_invert().items() + for k, v in PenState.tolerated_transitions_with_invert().items() ], ) def test_tolerated_invert_pen_states(self, state_list, scribble): @@ -553,7 +556,7 @@ def test_tolerated_invert_pen_states(self, state_list, scribble): @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) @pytest.mark.parametrize( "state_list", - [pytest.param(v, id=k) for k, v in Pen.broken_transitions().items()], + [pytest.param(v, id=k) for k, v in PenState.broken_transitions().items()], ) def test_tolerated_broken_pen_states(self, state_list, scribble): """Those tests are definitely not part of the Windows specification. From 881ccc36b42670ebbd5fb32a79b239ce1136e84d Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:57 +0100 Subject: [PATCH 22/53] selftests/hid: tablets: move move_to function to PenDigitizer We can easily subclass PenDigitizer for introducing firmware bugs when subclassing Pen is harder. Move move_to from Pen to PenDigitizer so we get that ability Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-6-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 97 ++++++++++--------- 1 file changed, 50 insertions(+), 47 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index ddf28c2450460..27260dc02cc4d 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -282,7 +282,7 @@ def __init__(self, x, y): self._old_values = None self.current_state = None - def _restore(self): + def restore(self): if self._old_values is not None: for i in [ "x", @@ -297,50 +297,8 @@ def _restore(self): ]: setattr(self, i, getattr(self._old_values, i)) - def move_to(self, state): - # fill in the previous values - if self.current_state == PenState.PEN_IS_OUT_OF_RANGE: - self._restore() - - print(f"\n *** pen is moving to {state} ***") - - if state == PenState.PEN_IS_OUT_OF_RANGE: - self._old_values = copy.copy(self) - self.x = 0 - self.y = 0 - self.tipswitch = False - self.tippressure = 0 - self.azimuth = 0 - self.inrange = False - self.width = 0 - self.height = 0 - self.invert = False - self.eraser = False - self.x_tilt = 0 - self.y_tilt = 0 - self.twist = 0 - elif state == PenState.PEN_IS_IN_RANGE: - self.tipswitch = False - self.inrange = True - self.invert = False - self.eraser = False - elif state == PenState.PEN_IS_IN_CONTACT: - self.tipswitch = True - self.inrange = True - self.invert = False - self.eraser = False - elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT: - self.tipswitch = False - self.inrange = True - self.invert = True - self.eraser = False - elif state == PenState.PEN_IS_ERASING: - self.tipswitch = False - self.inrange = True - self.invert = True - self.eraser = True - - self.current_state = state + def backup(self): + self._old_values = copy.copy(self) def __assert_axis(self, evdev, axis, value): if ( @@ -384,6 +342,51 @@ def __init__( continue self.fields = [f.usage_name for f in r] + def move_to(self, pen, state): + # fill in the previous values + if pen.current_state == PenState.PEN_IS_OUT_OF_RANGE: + pen.restore() + + print(f"\n *** pen is moving to {state} ***") + + if state == PenState.PEN_IS_OUT_OF_RANGE: + pen.backup() + pen.x = 0 + pen.y = 0 + pen.tipswitch = False + pen.tippressure = 0 + pen.azimuth = 0 + pen.inrange = False + pen.width = 0 + pen.height = 0 + pen.invert = False + pen.eraser = False + pen.x_tilt = 0 + pen.y_tilt = 0 + pen.twist = 0 + elif state == PenState.PEN_IS_IN_RANGE: + pen.tipswitch = False + pen.inrange = True + pen.invert = False + pen.eraser = False + elif state == PenState.PEN_IS_IN_CONTACT: + pen.tipswitch = True + pen.inrange = True + pen.invert = False + pen.eraser = False + elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT: + pen.tipswitch = False + pen.inrange = True + pen.invert = True + pen.eraser = False + elif state == PenState.PEN_IS_ERASING: + pen.tipswitch = False + pen.inrange = True + pen.invert = True + pen.eraser = True + + pen.current_state = state + def event(self, pen): rs = [] r = self.create_report(application=self.cur_application, data=pen) @@ -462,7 +465,7 @@ def _test_states(self, state_list, scribble): cur_state = PenState.PEN_IS_OUT_OF_RANGE p = Pen(50, 60) - p.move_to(PenState.PEN_IS_OUT_OF_RANGE) + uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE) events = self.post(uhdev, p) self.validate_transitions(cur_state, p, evdev, events) @@ -475,7 +478,7 @@ def _test_states(self, state_list, scribble): events = self.post(uhdev, p) self.validate_transitions(cur_state, p, evdev, events) assert len(events) >= 3 # X, Y, SYN - p.move_to(state) + uhdev.move_to(p, state) if scribble and state != PenState.PEN_IS_OUT_OF_RANGE: p.x += 1 p.y -= 1 From d8d7aa2266a7957e24cf05392b2b899be1031ed4 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:58 +0100 Subject: [PATCH 23/53] selftests/hid: tablets: do not set invert when the eraser is used Turns out that the chart from Microsoft is not exactly what I got here: when the rubber is used, and is touching the surface, invert can (should) be set to 0... [0] https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-7-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/test_tablet.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index 27260dc02cc4d..cb3955bf0ec58 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -382,7 +382,7 @@ def move_to(self, pen, state): elif state == PenState.PEN_IS_ERASING: pen.tipswitch = False pen.inrange = True - pen.invert = True + pen.invert = False pen.eraser = True pen.current_state = state From e08e493ff9619daab9c11e61a80c604895a4d671 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:45:59 +0100 Subject: [PATCH 24/53] selftests/hid: tablets: set initial data for tilt/twist Avoids getting a null event when these usages are set Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-8-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/test_tablet.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index cb3955bf0ec58..0ddf82695ed91 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -276,9 +276,9 @@ def __init__(self, x, y): self.barrelswitch = False self.invert = False self.eraser = False - self.x_tilt = 0 - self.y_tilt = 0 - self.twist = 0 + self.xtilt = 1 + self.ytilt = 1 + self.twist = 1 self._old_values = None self.current_state = None @@ -292,8 +292,8 @@ def restore(self): "width", "height", "twist", - "x_tilt", - "y_tilt", + "xtilt", + "ytilt", ]: setattr(self, i, getattr(self._old_values, i)) @@ -361,8 +361,8 @@ def move_to(self, pen, state): pen.height = 0 pen.invert = False pen.eraser = False - pen.x_tilt = 0 - pen.y_tilt = 0 + pen.xtilt = 0 + pen.ytilt = 0 pen.twist = 0 elif state == PenState.PEN_IS_IN_RANGE: pen.tipswitch = False From 83912f83fabcb5086613e6382756750390ed48aa Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:00 +0100 Subject: [PATCH 25/53] selftests/hid: tablets: define the elements of PenState This introduces a little bit more readability by not using the raw values but a dedicated Enum Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-9-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index 0ddf82695ed91..cec65294c9ecb 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -13,40 +13,52 @@ import libevdev import logging import pytest -from typing import Dict, Tuple +from typing import Dict, Optional, Tuple logger = logging.getLogger("hidtools.test.tablet") +class BtnTouch(Enum): + """Represents whether the BTN_TOUCH event is set to True or False""" + + DOWN = True + UP = False + + +class ToolType(Enum): + PEN = libevdev.EV_KEY.BTN_TOOL_PEN + RUBBER = libevdev.EV_KEY.BTN_TOOL_RUBBER + + class PenState(Enum): """Pen states according to Microsoft reference: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states """ - PEN_IS_OUT_OF_RANGE = (False, None) - PEN_IS_IN_RANGE = (False, libevdev.EV_KEY.BTN_TOOL_PEN) - PEN_IS_IN_CONTACT = (True, libevdev.EV_KEY.BTN_TOOL_PEN) - PEN_IS_IN_RANGE_WITH_ERASING_INTENT = (False, libevdev.EV_KEY.BTN_TOOL_RUBBER) - PEN_IS_ERASING = (True, libevdev.EV_KEY.BTN_TOOL_RUBBER) + PEN_IS_OUT_OF_RANGE = BtnTouch.UP, None + PEN_IS_IN_RANGE = BtnTouch.UP, ToolType.PEN + PEN_IS_IN_CONTACT = BtnTouch.DOWN, ToolType.PEN + PEN_IS_IN_RANGE_WITH_ERASING_INTENT = BtnTouch.UP, ToolType.RUBBER + PEN_IS_ERASING = BtnTouch.DOWN, ToolType.RUBBER - def __init__(self, touch, tool): + def __init__(self, touch: BtnTouch, tool: Optional[ToolType]): self.touch = touch self.tool = tool @classmethod def from_evdev(cls, evdev) -> "PenState": - touch = bool(evdev.value[libevdev.EV_KEY.BTN_TOUCH]) + touch = BtnTouch(evdev.value[libevdev.EV_KEY.BTN_TOUCH]) tool = None if ( evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] and not evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN] ): - tool = libevdev.EV_KEY.BTN_TOOL_RUBBER + tool = ToolType(libevdev.EV_KEY.BTN_TOOL_RUBBER) elif ( evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN] and not evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] ): - tool = libevdev.EV_KEY.BTN_TOOL_PEN + tool = ToolType(libevdev.EV_KEY.BTN_TOOL_PEN) elif ( evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN] or evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] @@ -68,7 +80,7 @@ def apply(self, events) -> "PenState": if touch_found: raise ValueError(f"duplicated BTN_TOUCH in {events}") touch_found = True - touch = bool(ev.value) + touch = BtnTouch(ev.value) elif ev in ( libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN), libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_RUBBER), @@ -77,7 +89,7 @@ def apply(self, events) -> "PenState": raise ValueError(f"duplicated BTN_TOOL_* in {events}") tool_found = True if ev.value: - tool = ev.code + tool = ToolType(ev.code) else: tool = None From 74452d6329be73dd2f5562005e5eef2c7bda7c5b Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:01 +0100 Subject: [PATCH 26/53] selftests/hid: tablets: add variants of states with buttons Turns out that there are transitions that are unlikely to happen: for example, having both the tip switch and a button being changed at the same time (in the same report) would require either a very talented and precise user or a very bad hardware with a very low sampling rate. So instead of manually building the button test by hand and forgetting about some cases, let's reuse the state machine and transitions we have. This patch only adds the states and the valid transitions. The actual tests will be replaced later. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-10-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 173 ++++++++++++++++-- 1 file changed, 160 insertions(+), 13 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index cec65294c9ecb..a77534f23c75b 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -30,25 +30,72 @@ class ToolType(Enum): RUBBER = libevdev.EV_KEY.BTN_TOOL_RUBBER +class BtnPressed(Enum): + """Represents whether a button is pressed on the stylus""" + + PRIMARY_PRESSED = libevdev.EV_KEY.BTN_STYLUS + SECONDARY_PRESSED = libevdev.EV_KEY.BTN_STYLUS2 + + class PenState(Enum): """Pen states according to Microsoft reference: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states - """ - PEN_IS_OUT_OF_RANGE = BtnTouch.UP, None - PEN_IS_IN_RANGE = BtnTouch.UP, ToolType.PEN - PEN_IS_IN_CONTACT = BtnTouch.DOWN, ToolType.PEN - PEN_IS_IN_RANGE_WITH_ERASING_INTENT = BtnTouch.UP, ToolType.RUBBER - PEN_IS_ERASING = BtnTouch.DOWN, ToolType.RUBBER + We extend it with the various buttons when we need to check them. + """ - def __init__(self, touch: BtnTouch, tool: Optional[ToolType]): + PEN_IS_OUT_OF_RANGE = BtnTouch.UP, None, None + PEN_IS_IN_RANGE = BtnTouch.UP, ToolType.PEN, None + PEN_IS_IN_RANGE_WITH_BUTTON = BtnTouch.UP, ToolType.PEN, BtnPressed.PRIMARY_PRESSED + PEN_IS_IN_RANGE_WITH_SECOND_BUTTON = ( + BtnTouch.UP, + ToolType.PEN, + BtnPressed.SECONDARY_PRESSED, + ) + PEN_IS_IN_CONTACT = BtnTouch.DOWN, ToolType.PEN, None + PEN_IS_IN_CONTACT_WITH_BUTTON = ( + BtnTouch.DOWN, + ToolType.PEN, + BtnPressed.PRIMARY_PRESSED, + ) + PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON = ( + BtnTouch.DOWN, + ToolType.PEN, + BtnPressed.SECONDARY_PRESSED, + ) + PEN_IS_IN_RANGE_WITH_ERASING_INTENT = BtnTouch.UP, ToolType.RUBBER, None + PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_BUTTON = ( + BtnTouch.UP, + ToolType.RUBBER, + BtnPressed.PRIMARY_PRESSED, + ) + PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_SECOND_BUTTON = ( + BtnTouch.UP, + ToolType.RUBBER, + BtnPressed.SECONDARY_PRESSED, + ) + PEN_IS_ERASING = BtnTouch.DOWN, ToolType.RUBBER, None + PEN_IS_ERASING_WITH_BUTTON = ( + BtnTouch.DOWN, + ToolType.RUBBER, + BtnPressed.PRIMARY_PRESSED, + ) + PEN_IS_ERASING_WITH_SECOND_BUTTON = ( + BtnTouch.DOWN, + ToolType.RUBBER, + BtnPressed.SECONDARY_PRESSED, + ) + + def __init__(self, touch: BtnTouch, tool: Optional[ToolType], button: Optional[BtnPressed]): self.touch = touch self.tool = tool + self.button = button @classmethod def from_evdev(cls, evdev) -> "PenState": touch = BtnTouch(evdev.value[libevdev.EV_KEY.BTN_TOUCH]) tool = None + button = None if ( evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER] and not evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN] @@ -65,7 +112,17 @@ def from_evdev(cls, evdev) -> "PenState": ): raise ValueError("2 tools are not allowed") - return cls((touch, tool)) + # we take only the highest button in account + for b in [libevdev.EV_KEY.BTN_STYLUS, libevdev.EV_KEY.BTN_STYLUS2]: + if bool(evdev.value[b]): + button = b + + # the kernel tends to insert an EV_SYN once removing the tool, so + # the button will be released after + if tool is None: + button = None + + return cls((touch, tool, button)) def apply(self, events) -> "PenState": if libevdev.EV_SYN.SYN_REPORT in events: @@ -74,6 +131,8 @@ def apply(self, events) -> "PenState": touch_found = False tool = self.tool tool_found = False + button = self.button + button_found = False for ev in events: if ev == libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH): @@ -88,12 +147,22 @@ def apply(self, events) -> "PenState": if tool_found: raise ValueError(f"duplicated BTN_TOOL_* in {events}") tool_found = True - if ev.value: - tool = ToolType(ev.code) - else: - tool = None + tool = ToolType(ev.code) if ev.value else None + elif ev in ( + libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS), + libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS2), + ): + if button_found: + raise ValueError(f"duplicated BTN_STYLUS* in {events}") + button_found = True + button = ev.code if ev.value else None - new_state = PenState((touch, tool)) + # the kernel tends to insert an EV_SYN once removing the tool, so + # the button will be released after + if tool is None: + button = None + + new_state = PenState((touch, tool, button)) assert ( new_state in self.valid_transitions() ), f"moving from {self} to {new_state} is forbidden" @@ -109,14 +178,20 @@ def valid_transitions(self) -> Tuple["PenState", ...]: return ( PenState.PEN_IS_OUT_OF_RANGE, PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT, PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, PenState.PEN_IS_ERASING, ) if self == PenState.PEN_IS_IN_RANGE: return ( PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, PenState.PEN_IS_OUT_OF_RANGE, PenState.PEN_IS_IN_CONTACT, ) @@ -124,6 +199,8 @@ def valid_transitions(self) -> Tuple["PenState", ...]: if self == PenState.PEN_IS_IN_CONTACT: return ( PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, PenState.PEN_IS_IN_RANGE, PenState.PEN_IS_OUT_OF_RANGE, ) @@ -142,6 +219,38 @@ def valid_transitions(self) -> Tuple["PenState", ...]: PenState.PEN_IS_OUT_OF_RANGE, ) + if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON: + return ( + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + ) + + if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON: + return ( + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_OUT_OF_RANGE, + ) + + if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON: + return ( + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + ) + + if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON: + return ( + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_OUT_OF_RANGE, + ) + return tuple() @staticmethod @@ -376,26 +485,64 @@ def move_to(self, pen, state): pen.xtilt = 0 pen.ytilt = 0 pen.twist = 0 + pen.barrelswitch = False + pen.secondarybarrelswitch = False elif state == PenState.PEN_IS_IN_RANGE: pen.tipswitch = False pen.inrange = True pen.invert = False pen.eraser = False + pen.barrelswitch = False + pen.secondarybarrelswitch = False elif state == PenState.PEN_IS_IN_CONTACT: pen.tipswitch = True pen.inrange = True pen.invert = False pen.eraser = False + pen.barrelswitch = False + pen.secondarybarrelswitch = False + elif state == PenState.PEN_IS_IN_RANGE_WITH_BUTTON: + pen.tipswitch = False + pen.inrange = True + pen.invert = False + pen.eraser = False + pen.barrelswitch = True + pen.secondarybarrelswitch = False + elif state == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON: + pen.tipswitch = True + pen.inrange = True + pen.invert = False + pen.eraser = False + pen.barrelswitch = True + pen.secondarybarrelswitch = False + elif state == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON: + pen.tipswitch = False + pen.inrange = True + pen.invert = False + pen.eraser = False + pen.barrelswitch = False + pen.secondarybarrelswitch = True + elif state == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON: + pen.tipswitch = True + pen.inrange = True + pen.invert = False + pen.eraser = False + pen.barrelswitch = False + pen.secondarybarrelswitch = True elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT: pen.tipswitch = False pen.inrange = True pen.invert = True pen.eraser = False + pen.barrelswitch = False + pen.secondarybarrelswitch = False elif state == PenState.PEN_IS_ERASING: pen.tipswitch = False pen.inrange = True pen.invert = False pen.eraser = True + pen.barrelswitch = False + pen.secondarybarrelswitch = False pen.current_state = state From 1f01537ef17efec97a8936c62e4ffa704cab7c06 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:02 +0100 Subject: [PATCH 27/53] selftests/hid: tablets: convert the primary button tests We get more descriptive in what we are doing, and also get more information of what is actually being tested. Instead of having a non exhaustive button changes that are semi-randomly done, we can describe all the states we want to test. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-11-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 160 +++++++----------- 1 file changed, 65 insertions(+), 95 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index a77534f23c75b..20a7a7fdcd9d2 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -317,6 +317,55 @@ def legal_transitions_with_invert() -> Dict[str, Tuple["PenState", ...]]: ), } + @staticmethod + def legal_transitions_with_primary_button() -> Dict[str, Tuple["PenState", ...]]: + """We revisit the Windows Pen Implementation state machine: + we now have a primary button. + """ + return { + "hover-button": (PenState.PEN_IS_IN_RANGE_WITH_BUTTON,), + "hover-button -> out-of-range": ( + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_OUT_OF_RANGE, + ), + "in-range -> button-press": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + ), + "in-range -> button-press -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE, + ), + "in-range -> touch -> button-press -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT, + ), + "in-range -> touch -> button-press -> release -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE, + ), + "in-range -> button-press -> touch -> release -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE, + ), + "in-range -> button-press -> touch -> button-release -> release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_RANGE, + ), + } + @staticmethod def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]: """This is not adhering to the Windows Pen Implementation state machine @@ -671,6 +720,22 @@ def test_tolerated_pen_states(self, state_list, scribble): reasons.""" self._test_states(state_list, scribble) + @pytest.mark.skip_if_uhdev( + lambda uhdev: "Barrel Switch" not in uhdev.fields, + "Device not compatible, missing Barrel Switch usage", + ) + @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) + @pytest.mark.parametrize( + "state_list", + [ + pytest.param(v, id=k) + for k, v in PenState.legal_transitions_with_primary_button().items() + ], + ) + def test_valid_primary_button_pen_states(self, state_list, scribble): + """Rework the transition state machine by adding the primary button.""" + self._test_states(state_list, scribble) + @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, "Device not compatible, missing Invert usage", @@ -728,101 +793,6 @@ def test_tolerated_broken_pen_states(self, state_list, scribble): state machine.""" self._test_states(state_list, scribble) - @pytest.mark.skip_if_uhdev( - lambda uhdev: "Barrel Switch" not in uhdev.fields, - "Device not compatible, missing Barrel Switch usage", - ) - def test_primary_button(self): - """Primary button (stylus) pressed, reports as pressed even while hovering. - Actual reporting from the device: hid=TIPSWITCH,BARRELSWITCH,INRANGE (code=TOUCH,STYLUS,PEN): - { 0, 0, 1 } <- hover - { 0, 1, 1 } <- primary button pressed - { 0, 1, 1 } <- liftoff - { 0, 0, 0 } <- leaves - """ - - uhdev = self.uhdev - evdev = uhdev.get_evdev() - - p = Pen(50, 60) - p.inrange = True - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 1) in events - assert evdev.value[libevdev.EV_ABS.ABS_X] == 50 - assert evdev.value[libevdev.EV_ABS.ABS_Y] == 60 - assert not evdev.value[libevdev.EV_KEY.BTN_STYLUS] - - p.barrelswitch = True - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 1) in events - - p.x += 1 - p.y -= 1 - events = self.post(uhdev, p) - assert len(events) == 3 # X, Y, SYN - assert libevdev.InputEvent(libevdev.EV_ABS.ABS_X, 51) in events - assert libevdev.InputEvent(libevdev.EV_ABS.ABS_Y, 59) in events - - p.barrelswitch = False - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 0) in events - - p.inrange = False - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 0) in events - - @pytest.mark.skip_if_uhdev( - lambda uhdev: "Barrel Switch" not in uhdev.fields, - "Device not compatible, missing Barrel Switch usage", - ) - def test_contact_primary_button(self): - """Primary button (stylus) pressed, reports as pressed even while hovering. - Actual reporting from the device: hid=TIPSWITCH,BARRELSWITCH,INRANGE (code=TOUCH,STYLUS,PEN): - { 0, 0, 1 } <- hover - { 0, 1, 1 } <- primary button pressed - { 1, 1, 1 } <- touch-down - { 1, 1, 1 } <- still touch, scribble on the screen - { 0, 1, 1 } <- liftoff - { 0, 0, 0 } <- leaves - """ - - uhdev = self.uhdev - evdev = uhdev.get_evdev() - - p = Pen(50, 60) - p.inrange = True - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 1) in events - assert evdev.value[libevdev.EV_ABS.ABS_X] == 50 - assert evdev.value[libevdev.EV_ABS.ABS_Y] == 60 - assert not evdev.value[libevdev.EV_KEY.BTN_STYLUS] - - p.barrelswitch = True - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 1) in events - - p.tipswitch = True - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH, 1) in events - assert evdev.value[libevdev.EV_KEY.BTN_STYLUS] - - p.x += 1 - p.y -= 1 - events = self.post(uhdev, p) - assert len(events) == 3 # X, Y, SYN - assert libevdev.InputEvent(libevdev.EV_ABS.ABS_X, 51) in events - assert libevdev.InputEvent(libevdev.EV_ABS.ABS_Y, 59) in events - - p.tipswitch = False - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH, 0) in events - - p.barrelswitch = False - p.inrange = False - events = self.post(uhdev, p) - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOOL_PEN, 0) in events - assert libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS, 0) in events - class GXTP_pen(PenDigitizer): def event(self, pen): From 76df1f72fb258cc7da6eff5dd8db95f4e2856517 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:03 +0100 Subject: [PATCH 28/53] selftests/hid: tablets: add a secondary barrel switch test Some tablets report 2 barrel switches. We better test those too. Use the same transistions description from the primary button tests. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-12-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index 20a7a7fdcd9d2..a82db66264c59 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -366,6 +366,56 @@ def legal_transitions_with_primary_button() -> Dict[str, Tuple["PenState", ...]] ), } + @staticmethod + def legal_transitions_with_secondary_button() -> Dict[str, Tuple["PenState", ...]]: + """We revisit the Windows Pen Implementation state machine: + we now have a secondary button. + Note: we don't looks for 2 buttons interactions. + """ + return { + "hover-button": (PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,), + "hover-button -> out-of-range": ( + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_OUT_OF_RANGE, + ), + "in-range -> button-press": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + ), + "in-range -> button-press -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE, + ), + "in-range -> touch -> button-press -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_CONTACT, + ), + "in-range -> touch -> button-press -> release -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE, + ), + "in-range -> button-press -> touch -> release -> button-release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE, + ), + "in-range -> button-press -> touch -> button-release -> release": ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_RANGE, + ), + } + @staticmethod def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]: """This is not adhering to the Windows Pen Implementation state machine @@ -444,6 +494,7 @@ def __init__(self, x, y): self.width = 10 self.height = 10 self.barrelswitch = False + self.secondarybarrelswitch = False self.invert = False self.eraser = False self.xtilt = 1 @@ -736,6 +787,22 @@ def test_valid_primary_button_pen_states(self, state_list, scribble): """Rework the transition state machine by adding the primary button.""" self._test_states(state_list, scribble) + @pytest.mark.skip_if_uhdev( + lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields, + "Device not compatible, missing Secondary Barrel Switch usage", + ) + @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) + @pytest.mark.parametrize( + "state_list", + [ + pytest.param(v, id=k) + for k, v in PenState.legal_transitions_with_secondary_button().items() + ], + ) + def test_valid_secondary_button_pen_states(self, state_list, scribble): + """Rework the transition state machine by adding the secondary button.""" + self._test_states(state_list, scribble) + @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, "Device not compatible, missing Invert usage", From ab9b82909e9baa5b559d6457487525cfd4df2738 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:04 +0100 Subject: [PATCH 29/53] selftests/hid: tablets: be stricter for some transitions To accommodate for legacy devices, we rely on the last state of a transition to be valid: for example when we test PEN_IS_OUT_OF_RANGE to PEN_IS_IN_CONTACT, any "normal" device that reports an InRange bit would insert a PEN_IS_IN_RANGE state between the 2. This is of course valid, but this solution prevents to detect false releases emitted by some firmware: when pressing an "eraser mode" button, they might send an extra PEN_IS_OUT_OF_RANGE that we may want to filter. So define 2 sets of transitions: one that is the ideal behavior, and one that is OK, it won't break user space, but we have serious doubts if we are doing the right thing. And depending on the test, either ask only for valid transitions, or tolerate weird ones. Reviewed-by: Peter Hutterer Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-13-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- .../selftests/hid/tests/test_tablet.py | 132 +++++++++++++++--- 1 file changed, 113 insertions(+), 19 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index a82db66264c59..9374bd7524efa 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -13,7 +13,7 @@ import libevdev import logging import pytest -from typing import Dict, Optional, Tuple +from typing import Dict, List, Optional, Tuple logger = logging.getLogger("hidtools.test.tablet") @@ -124,7 +124,7 @@ def from_evdev(cls, evdev) -> "PenState": return cls((touch, tool, button)) - def apply(self, events) -> "PenState": + def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState": if libevdev.EV_SYN.SYN_REPORT in events: raise ValueError("EV_SYN is in the event sequence") touch = self.touch @@ -163,13 +163,97 @@ def apply(self, events) -> "PenState": button = None new_state = PenState((touch, tool, button)) - assert ( - new_state in self.valid_transitions() - ), f"moving from {self} to {new_state} is forbidden" + if strict: + assert ( + new_state in self.valid_transitions() + ), f"moving from {self} to {new_state} is forbidden" + else: + assert ( + new_state in self.historically_tolerated_transitions() + ), f"moving from {self} to {new_state} is forbidden" return new_state def valid_transitions(self) -> Tuple["PenState", ...]: + """Following the state machine in the URL above. + + Note that those transitions are from the evdev point of view, not HID""" + if self == PenState.PEN_IS_OUT_OF_RANGE: + return ( + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_ERASING, + ) + + if self == PenState.PEN_IS_IN_RANGE: + return ( + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_IN_CONTACT, + ) + + if self == PenState.PEN_IS_IN_CONTACT: + return ( + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE, + ) + + if self == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT: + return ( + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT, + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_ERASING, + ) + + if self == PenState.PEN_IS_ERASING: + return ( + PenState.PEN_IS_ERASING, + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT, + ) + + if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON: + return ( + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + ) + + if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON: + return ( + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_RANGE_WITH_BUTTON, + ) + + if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON: + return ( + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_RANGE, + PenState.PEN_IS_OUT_OF_RANGE, + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + ) + + if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON: + return ( + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON, + PenState.PEN_IS_IN_CONTACT, + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON, + ) + + return tuple() + + def historically_tolerated_transitions(self) -> Tuple["PenState", ...]: """Following the state machine in the URL above, with a couple of addition for skipping the in-range state, due to historical reasons. @@ -693,10 +777,14 @@ def post(self, uhdev, pen): self.debug_reports(r, uhdev, events) return events - def validate_transitions(self, from_state, pen, evdev, events): + def validate_transitions( + self, from_state, pen, evdev, events, allow_intermediate_states + ): # check that the final state is correct pen.assert_expected_input_events(evdev) + state = from_state + # check that the transitions are valid sync_events = [] while libevdev.InputEvent(libevdev.EV_SYN.SYN_REPORT) in events: @@ -706,12 +794,12 @@ def validate_transitions(self, from_state, pen, evdev, events): events = events[idx + 1 :] # now check for a valid transition - from_state = from_state.apply(sync_events) + state = state.apply(sync_events, not allow_intermediate_states) if events: - from_state = from_state.apply(sync_events) + state = state.apply(sync_events, not allow_intermediate_states) - def _test_states(self, state_list, scribble): + def _test_states(self, state_list, scribble, allow_intermediate_states): """Internal method to test against a list of transition between states. state_list is a list of PenState objects @@ -726,7 +814,9 @@ def _test_states(self, state_list, scribble): p = Pen(50, 60) uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE) events = self.post(uhdev, p) - self.validate_transitions(cur_state, p, evdev, events) + self.validate_transitions( + cur_state, p, evdev, events, allow_intermediate_states + ) cur_state = p.current_state @@ -735,14 +825,18 @@ def _test_states(self, state_list, scribble): p.x += 1 p.y -= 1 events = self.post(uhdev, p) - self.validate_transitions(cur_state, p, evdev, events) + self.validate_transitions( + cur_state, p, evdev, events, allow_intermediate_states + ) assert len(events) >= 3 # X, Y, SYN uhdev.move_to(p, state) if scribble and state != PenState.PEN_IS_OUT_OF_RANGE: p.x += 1 p.y -= 1 events = self.post(uhdev, p) - self.validate_transitions(cur_state, p, evdev, events) + self.validate_transitions( + cur_state, p, evdev, events, allow_intermediate_states + ) cur_state = p.current_state @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) @@ -755,7 +849,7 @@ def test_valid_pen_states(self, state_list, scribble): we don't have Invert nor Erase bits, so just move in/out-of-range or proximity. https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states """ - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=False) @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"]) @pytest.mark.parametrize( @@ -769,7 +863,7 @@ def test_tolerated_pen_states(self, state_list, scribble): """This is not adhering to the Windows Pen Implementation state machine but we should expect the kernel to behave properly, mostly for historical reasons.""" - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=True) @pytest.mark.skip_if_uhdev( lambda uhdev: "Barrel Switch" not in uhdev.fields, @@ -785,7 +879,7 @@ def test_tolerated_pen_states(self, state_list, scribble): ) def test_valid_primary_button_pen_states(self, state_list, scribble): """Rework the transition state machine by adding the primary button.""" - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=False) @pytest.mark.skip_if_uhdev( lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields, @@ -801,7 +895,7 @@ def test_valid_primary_button_pen_states(self, state_list, scribble): ) def test_valid_secondary_button_pen_states(self, state_list, scribble): """Rework the transition state machine by adding the secondary button.""" - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=False) @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, @@ -821,7 +915,7 @@ def test_valid_invert_pen_states(self, state_list, scribble): to erase. https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states """ - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=False) @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, @@ -841,7 +935,7 @@ def test_tolerated_invert_pen_states(self, state_list, scribble): to erase. https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states """ - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=True) @pytest.mark.skip_if_uhdev( lambda uhdev: "Invert" not in uhdev.fields, @@ -858,7 +952,7 @@ def test_tolerated_broken_pen_states(self, state_list, scribble): For example, a pen that has the eraser button might wobble between touching and erasing if the tablet doesn't enforce the Windows state machine.""" - self._test_states(state_list, scribble) + self._test_states(state_list, scribble, allow_intermediate_states=True) class GXTP_pen(PenDigitizer): From ed5bc56cedca19ac429533aa527bce5287ab0a5a Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:05 +0100 Subject: [PATCH 30/53] selftests/hid: fix mypy complains No code change, only typing information added/ignored Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-14-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/base.py | 4 ++-- tools/testing/selftests/hid/tests/test_tablet.py | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/testing/selftests/hid/tests/base.py b/tools/testing/selftests/hid/tests/base.py index 5d9c26dfc4605..51433063b2270 100644 --- a/tools/testing/selftests/hid/tests/base.py +++ b/tools/testing/selftests/hid/tests/base.py @@ -14,7 +14,7 @@ from hidtools.device.base_device import BaseDevice, EvdevMatch, SysfsFile from pathlib import Path -from typing import Final +from typing import Final, List, Tuple logger = logging.getLogger("hidtools.test.base") @@ -155,7 +155,7 @@ class TestUhid(object): # if any module is not available (not compiled), the test will skip. # Each element is a tuple '(kernel driver name, kernel module)', # for example ("playstation", "hid-playstation") - kernel_modules = [] + kernel_modules: List[Tuple[str, str]] = [] def assertInputEventsIn(self, expected_events, effective_events): effective_events = effective_events.copy() diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index 9374bd7524efa..dc8b0fe9e7f36 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -87,9 +87,9 @@ class PenState(Enum): ) def __init__(self, touch: BtnTouch, tool: Optional[ToolType], button: Optional[BtnPressed]): - self.touch = touch - self.tool = tool - self.button = button + self.touch = touch # type: ignore + self.tool = tool # type: ignore + self.button = button # type: ignore @classmethod def from_evdev(cls, evdev) -> "PenState": @@ -122,7 +122,7 @@ def from_evdev(cls, evdev) -> "PenState": if tool is None: button = None - return cls((touch, tool, button)) + return cls((touch, tool, button)) # type: ignore def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState": if libevdev.EV_SYN.SYN_REPORT in events: @@ -162,7 +162,7 @@ def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState": if tool is None: button = None - new_state = PenState((touch, tool, button)) + new_state = PenState((touch, tool, button)) # type: ignore if strict: assert ( new_state in self.valid_transitions() From f556aa957df8cb3e98af0f54bf1fa65f59ae47a3 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Wed, 6 Dec 2023 11:46:06 +0100 Subject: [PATCH 31/53] selftests/hid: fix ruff linter complains rename ambiguous variables l, r, and m, and ignore the return values of uhdev.get_evdev() and uhdev.get_slot() Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231206-wip-selftests-v2-15-c0350c2f5986@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/test_mouse.py | 14 +++++++------- .../selftests/hid/tests/test_wacom_generic.py | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_mouse.py b/tools/testing/selftests/hid/tests/test_mouse.py index fd2ba62e783ab..66daf7e5975ca 100644 --- a/tools/testing/selftests/hid/tests/test_mouse.py +++ b/tools/testing/selftests/hid/tests/test_mouse.py @@ -52,13 +52,13 @@ def create_report(self, x, y, buttons=None, wheels=None, reportID=None): :param reportID: the numeric report ID for this report, if needed """ if buttons is not None: - l, r, m = buttons - if l is not None: - self.left = l - if r is not None: - self.right = r - if m is not None: - self.middle = m + left, right, middle = buttons + if left is not None: + self.left = left + if right is not None: + self.right = right + if middle is not None: + self.middle = middle left = self.left right = self.right middle = self.middle diff --git a/tools/testing/selftests/hid/tests/test_wacom_generic.py b/tools/testing/selftests/hid/tests/test_wacom_generic.py index f92fe8e02c1bf..49186a27467e6 100644 --- a/tools/testing/selftests/hid/tests/test_wacom_generic.py +++ b/tools/testing/selftests/hid/tests/test_wacom_generic.py @@ -909,7 +909,7 @@ def test_confidence_false(self): Ensure that the confidence bit being set to false should not result in a touch event. """ uhdev = self.uhdev - evdev = uhdev.get_evdev() + _evdev = uhdev.get_evdev() t0 = test_multitouch.Touch(1, 50, 100) t0.confidence = False @@ -917,6 +917,6 @@ def test_confidence_false(self): events = uhdev.next_sync_events() self.debug_reports(r, uhdev, events) - slot = self.get_slot(uhdev, t0, 0) + _slot = self.get_slot(uhdev, t0, 0) - assert not events \ No newline at end of file + assert not events From 0e63dd27f456f30c9501cf044141758db2d34fb3 Mon Sep 17 00:00:00 2001 From: Kai-Heng Feng Date: Wed, 8 Nov 2023 14:19:39 +0200 Subject: [PATCH 32/53] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup Since PCI core and ACPI core already handles PCI PME wake and GPE wake when the device has wakeup capability, use device_init_wakeup() to let them do the wakeup setting work. Also add a shutdown callback which uses pci_prepare_to_sleep() to let PCI and ACPI set OOB wakeup for S5. Cc: Jian Hui Lee Signed-off-by: Kai-Heng Feng Acked-by: Srinivas Pandruvada Signed-off-by: Jiri Kosina --- drivers/hid/intel-ish-hid/ipc/pci-ish.c | 67 ++++++------------------- 1 file changed, 15 insertions(+), 52 deletions(-) diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c index 710fda5f19e1c..65e7eeb2fa64e 100644 --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c @@ -119,50 +119,6 @@ static inline bool ish_should_leave_d0i3(struct pci_dev *pdev) return !pm_resume_via_firmware() || pdev->device == CHV_DEVICE_ID; } -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI - acpi_status acpi_sts; - struct acpi_device *adev; - struct acpi_device_wakeup *wakeup; - - adev = ACPI_COMPANION(dev); - if (!adev) { - dev_err(dev, "get acpi handle failed\n"); - return -ENODEV; - } - wakeup = &adev->wakeup; - - /* - * Call acpi_disable_gpe(), so that reference count - * gpe_event_info->runtime_count doesn't overflow. - * When gpe_event_info->runtime_count = 0, the call - * to acpi_disable_gpe() simply return. - */ - acpi_disable_gpe(wakeup->gpe_device, wakeup->gpe_number); - - acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup->gpe_number); - if (ACPI_FAILURE(acpi_sts)) { - dev_err(dev, "enable ose_gpe failed\n"); - return -EIO; - } - - return 0; -#else - return -ENODEV; -#endif -} - -static void enable_pme_wake(struct pci_dev *pdev) -{ - if ((pci_pme_capable(pdev, PCI_D0) || - pci_pme_capable(pdev, PCI_D3hot) || - pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev->dev)) { - pci_pme_active(pdev, true); - dev_dbg(&pdev->dev, "ish ipc driver pme wake enabled\n"); - } -} - /** * ish_probe() - PCI driver probe callback * @pdev: pci device @@ -233,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent) /* Enable PME for EHL */ if (pdev->device == EHL_Ax_DEVICE_ID) - enable_pme_wake(pdev); + device_init_wakeup(dev, true); ret = ish_init(ishtp); if (ret) @@ -256,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev) ish_device_disable(ishtp_dev); } + +/** + * ish_shutdown() - PCI driver shutdown callback + * @pdev: pci device + * + * This function sets up wakeup for S5 + */ +static void ish_shutdown(struct pci_dev *pdev) +{ + if (pdev->device == EHL_Ax_DEVICE_ID) + pci_prepare_to_sleep(pdev); +} + static struct device __maybe_unused *ish_resume_device; /* 50ms to get resume response */ @@ -378,13 +347,6 @@ static int __maybe_unused ish_resume(struct device *device) struct pci_dev *pdev = to_pci_dev(device); struct ishtp_device *dev = pci_get_drvdata(pdev); - /* add this to finish power flow for EHL */ - if (dev->pdev->device == EHL_Ax_DEVICE_ID) { - pci_set_power_state(pdev, PCI_D0); - enable_pme_wake(pdev); - dev_dbg(dev->devc, "set power state to D0 for ehl\n"); - } - ish_resume_device = device; dev->resume_flag = 1; @@ -400,6 +362,7 @@ static struct pci_driver ish_driver = { .id_table = ish_pci_tbl, .probe = ish_probe, .remove = ish_remove, + .shutdown = ish_shutdown, .driver.pm = &ish_pm_ops, }; From da2c1b861065b452590d75a1e2f5ee9b396fef92 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Thu, 7 Dec 2023 13:22:39 +0100 Subject: [PATCH 33/53] selftests/hid: fix failing tablet button tests An overlook from commit 74452d6329be ("selftests/hid: tablets: add variants of states with buttons"), where I don't use the Enum... Fixes: 74452d6329be ("selftests/hid: tablets: add variants of states with buttons") Acked-by: Jiri Kosina Link: https://lore.kernel.org/r/20231207-b4-wip-selftests-v1-1-c4e13fe04a70@kernel.org Signed-off-by: Benjamin Tissoires --- tools/testing/selftests/hid/tests/test_tablet.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py index dc8b0fe9e7f36..903f19f7cbe9f 100644 --- a/tools/testing/selftests/hid/tests/test_tablet.py +++ b/tools/testing/selftests/hid/tests/test_tablet.py @@ -115,7 +115,7 @@ def from_evdev(cls, evdev) -> "PenState": # we take only the highest button in account for b in [libevdev.EV_KEY.BTN_STYLUS, libevdev.EV_KEY.BTN_STYLUS2]: if bool(evdev.value[b]): - button = b + button = BtnPressed(b) # the kernel tends to insert an EV_SYN once removing the tool, so # the button will be released after @@ -155,7 +155,7 @@ def apply(self, events: List[libevdev.InputEvent], strict: bool) -> "PenState": if button_found: raise ValueError(f"duplicated BTN_STYLUS* in {events}") button_found = True - button = ev.code if ev.value else None + button = BtnPressed(ev.code) if ev.value else None # the kernel tends to insert an EV_SYN once removing the tool, so # the button will be released after From 94f18bb19945915fcdfd1903841020ef1b6af44a Mon Sep 17 00:00:00 2001 From: Ryan McClelland Date: Mon, 4 Dec 2023 00:17:21 -0800 Subject: [PATCH 34/53] HID: nintendo: add support for nso controllers This adds support for the nintendo switch online controllers which include the SNES, Genesis, and N64 Controllers. As each nso controller only implements a subset of what a pro controller can do. Each of these 'features' were broken up in to seperate functions which include right stick, left stick, imu, and dpad and depending on the controller type that it is, it will call the supported functions appropriately. Each controller now has a struct which maps the bit within the hid in report to a button. The name given to the device now comes directly from the hid device name rather than looking up a predefined string. Signed-off-by: Ryan McClelland Reviewed-by: Daniel J. Ogorchock Tested-by: Daniel J. Ogorchock Signed-off-by: Jiri Kosina --- drivers/hid/Kconfig | 13 +- drivers/hid/hid-ids.h | 5 +- drivers/hid/hid-nintendo.c | 897 ++++++++++++++++++++++++++----------- 3 files changed, 649 insertions(+), 266 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 4ce74af796579..347c284fb27ee 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -761,14 +761,15 @@ config HID_MULTITOUCH module will be called hid-multitouch. config HID_NINTENDO - tristate "Nintendo Joy-Con and Pro Controller support" + tristate "Nintendo Joy-Con, NSO, and Pro Controller support" depends on NEW_LEDS depends on LEDS_CLASS select POWER_SUPPLY help - Adds support for the Nintendo Switch Joy-Cons and Pro Controller. + Adds support for the Nintendo Switch Joy-Cons, NSO, Pro Controller. All controllers support bluetooth, and the Pro Controller also supports - its USB mode. + its USB mode. This also includes support for the Nintendo Switch Online + Controllers which include the Genesis, SNES, and N64 controllers. To compile this driver as a module, choose M here: the module will be called hid-nintendo. @@ -779,9 +780,9 @@ config NINTENDO_FF select INPUT_FF_MEMLESS help Say Y here if you have a Nintendo Switch controller and want to enable - force feedback support for it. This works for both joy-cons and the pro - controller. For the pro controller, both rumble motors can be controlled - individually. + force feedback support for it. This works for both joy-cons, the pro + controller, and the NSO N64 controller. For the pro controller, both + rumble motors can be controlled individually. config HID_NTI tristate "NTI keyboard adapters" diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index c6e4e0d1f2147..a90aa3c31dd0c 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -986,7 +986,10 @@ #define USB_DEVICE_ID_NINTENDO_JOYCONL 0x2006 #define USB_DEVICE_ID_NINTENDO_JOYCONR 0x2007 #define USB_DEVICE_ID_NINTENDO_PROCON 0x2009 -#define USB_DEVICE_ID_NINTENDO_CHRGGRIP 0x200E +#define USB_DEVICE_ID_NINTENDO_CHRGGRIP 0x200e +#define USB_DEVICE_ID_NINTENDO_SNESCON 0x2017 +#define USB_DEVICE_ID_NINTENDO_GENCON 0x201e +#define USB_DEVICE_ID_NINTENDO_N64CON 0x2019 #define USB_VENDOR_ID_NOVATEK 0x0603 #define USB_DEVICE_ID_NOVATEK_PCT 0x0600 diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 138f154fecef3..47af111ef3a20 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c @@ -3,6 +3,9 @@ * HID driver for Nintendo Switch Joy-Cons and Pro Controllers * * Copyright (c) 2019-2021 Daniel J. Ogorchock + * Portions Copyright (c) 2020 Nadia Holmquist Pedersen + * Copyright (c) 2022 Emily Strickland + * Copyright (c) 2023 Ryan McClelland * * The following resources/projects were referenced for this driver: * https://github.com/dekuNukem/Nintendo_Switch_Reverse_Engineering @@ -17,6 +20,9 @@ * This driver supports the Nintendo Switch Joy-Cons and Pro Controllers. The * Pro Controllers can either be used over USB or Bluetooth. * + * This driver also incorporates support for Nintendo Switch Online controllers + * for the NES, SNES, Sega Genesis, and N64. + * * The driver will retrieve the factory calibration info from the controllers, * so little to no user calibration should be required. * @@ -305,9 +311,14 @@ enum joycon_ctlr_state { /* Controller type received as part of device info */ enum joycon_ctlr_type { - JOYCON_CTLR_TYPE_JCL = 0x01, - JOYCON_CTLR_TYPE_JCR = 0x02, - JOYCON_CTLR_TYPE_PRO = 0x03, + JOYCON_CTLR_TYPE_JCL = 0x01, + JOYCON_CTLR_TYPE_JCR = 0x02, + JOYCON_CTLR_TYPE_PRO = 0x03, + JOYCON_CTLR_TYPE_NESL = 0x09, + JOYCON_CTLR_TYPE_NESR = 0x0A, + JOYCON_CTLR_TYPE_SNES = 0x0B, + JOYCON_CTLR_TYPE_GEN = 0x0D, + JOYCON_CTLR_TYPE_N64 = 0x0C, }; struct joycon_stick_cal { @@ -348,6 +359,137 @@ static const u32 JC_BTN_SL_L = BIT(21); static const u32 JC_BTN_L = BIT(22); static const u32 JC_BTN_ZL = BIT(23); +struct joycon_ctlr_button_mapping { + u32 code; + u32 bit; +}; + +/* + * D-pad is configured as buttons for the left Joy-Con only! + */ +static const struct joycon_ctlr_button_mapping left_joycon_button_mappings[] = { + { BTN_TL, JC_BTN_L, }, + { BTN_TL2, JC_BTN_ZL, }, + { BTN_SELECT, JC_BTN_MINUS, }, + { BTN_THUMBL, JC_BTN_LSTICK, }, + { BTN_DPAD_UP, JC_BTN_UP, }, + { BTN_DPAD_DOWN, JC_BTN_DOWN, }, + { BTN_DPAD_LEFT, JC_BTN_LEFT, }, + { BTN_DPAD_RIGHT, JC_BTN_RIGHT, }, + { BTN_Z, JC_BTN_CAP, }, + { /* sentinel */ }, +}; + +/* + * The unused *right*-side triggers become the SL/SR triggers for the *left* + * Joy-Con, if and only if we're not using a charging grip. + */ +static const struct joycon_ctlr_button_mapping left_joycon_s_button_mappings[] = { + { BTN_TR, JC_BTN_SL_L, }, + { BTN_TR2, JC_BTN_SR_L, }, + { /* sentinel */ }, +}; + +static const struct joycon_ctlr_button_mapping right_joycon_button_mappings[] = { + { BTN_EAST, JC_BTN_A, }, + { BTN_SOUTH, JC_BTN_B, }, + { BTN_NORTH, JC_BTN_X, }, + { BTN_WEST, JC_BTN_Y, }, + { BTN_TR, JC_BTN_R, }, + { BTN_TR2, JC_BTN_ZR, }, + { BTN_START, JC_BTN_PLUS, }, + { BTN_THUMBR, JC_BTN_RSTICK, }, + { BTN_MODE, JC_BTN_HOME, }, + { /* sentinel */ }, +}; + +/* + * The unused *left*-side triggers become the SL/SR triggers for the *right* + * Joy-Con, if and only if we're not using a charging grip. + */ +static const struct joycon_ctlr_button_mapping right_joycon_s_button_mappings[] = { + { BTN_TL, JC_BTN_SL_R, }, + { BTN_TL2, JC_BTN_SR_R, }, + { /* sentinel */ }, +}; + +static const struct joycon_ctlr_button_mapping procon_button_mappings[] = { + { BTN_EAST, JC_BTN_A, }, + { BTN_SOUTH, JC_BTN_B, }, + { BTN_NORTH, JC_BTN_X, }, + { BTN_WEST, JC_BTN_Y, }, + { BTN_TL, JC_BTN_L, }, + { BTN_TR, JC_BTN_R, }, + { BTN_TL2, JC_BTN_ZL, }, + { BTN_TR2, JC_BTN_ZR, }, + { BTN_SELECT, JC_BTN_MINUS, }, + { BTN_START, JC_BTN_PLUS, }, + { BTN_THUMBL, JC_BTN_LSTICK, }, + { BTN_THUMBR, JC_BTN_RSTICK, }, + { BTN_MODE, JC_BTN_HOME, }, + { BTN_Z, JC_BTN_CAP, }, + { /* sentinel */ }, +}; + +static const struct joycon_ctlr_button_mapping nescon_button_mappings[] = { + { BTN_SOUTH, JC_BTN_A, }, + { BTN_EAST, JC_BTN_B, }, + { BTN_TL, JC_BTN_L, }, + { BTN_TR, JC_BTN_R, }, + { BTN_SELECT, JC_BTN_MINUS, }, + { BTN_START, JC_BTN_PLUS, }, + { /* sentinel */ }, +}; + +static const struct joycon_ctlr_button_mapping snescon_button_mappings[] = { + { BTN_EAST, JC_BTN_A, }, + { BTN_SOUTH, JC_BTN_B, }, + { BTN_NORTH, JC_BTN_X, }, + { BTN_WEST, JC_BTN_Y, }, + { BTN_TL, JC_BTN_L, }, + { BTN_TR, JC_BTN_R, }, + { BTN_TL2, JC_BTN_ZL, }, + { BTN_TR2, JC_BTN_ZR, }, + { BTN_SELECT, JC_BTN_MINUS, }, + { BTN_START, JC_BTN_PLUS, }, + { /* sentinel */ }, +}; + +/* + * "A", "B", and "C" are mapped positionally, rather than by label (e.g., "A" + * gets assigned to BTN_EAST instead of BTN_A). + */ +static const struct joycon_ctlr_button_mapping gencon_button_mappings[] = { + { BTN_SOUTH, JC_BTN_A, }, + { BTN_EAST, JC_BTN_B, }, + { BTN_WEST, JC_BTN_R, }, + { BTN_SELECT, JC_BTN_ZR, }, + { BTN_START, JC_BTN_PLUS, }, + { BTN_MODE, JC_BTN_HOME, }, + { BTN_Z, JC_BTN_CAP, }, + { /* sentinel */ }, +}; + +/* + * N64's C buttons get assigned to d-pad directions and registered as buttons. + */ +static const struct joycon_ctlr_button_mapping n64con_button_mappings[] = { + { BTN_A, JC_BTN_A, }, + { BTN_B, JC_BTN_B, }, + { BTN_TL2, JC_BTN_ZL, }, /* Z */ + { BTN_TL, JC_BTN_L, }, + { BTN_TR, JC_BTN_R, }, + { BTN_TR2, JC_BTN_LSTICK, }, /* ZR */ + { BTN_START, JC_BTN_PLUS, }, + { BTN_FORWARD, JC_BTN_Y, }, /* C UP */ + { BTN_BACK, JC_BTN_ZR, }, /* C DOWN */ + { BTN_LEFT, JC_BTN_X, }, /* C LEFT */ + { BTN_RIGHT, JC_BTN_MINUS, }, /* C RIGHT */ + { BTN_MODE, JC_BTN_HOME, }, + { BTN_Z, JC_BTN_CAP, }, + { /* sentinel */ }, +}; + enum joycon_msg_type { JOYCON_MSG_TYPE_NONE, JOYCON_MSG_TYPE_USB, @@ -506,13 +648,182 @@ struct joycon_ctlr { /* Does this controller have inputs associated with left joycon? */ #define jc_type_has_left(ctlr) \ (ctlr->ctlr_type == JOYCON_CTLR_TYPE_JCL || \ - ctlr->ctlr_type == JOYCON_CTLR_TYPE_PRO) + ctlr->ctlr_type == JOYCON_CTLR_TYPE_PRO || \ + ctlr->ctlr_type == JOYCON_CTLR_TYPE_N64) /* Does this controller have inputs associated with right joycon? */ #define jc_type_has_right(ctlr) \ (ctlr->ctlr_type == JOYCON_CTLR_TYPE_JCR || \ ctlr->ctlr_type == JOYCON_CTLR_TYPE_PRO) + +/* + * Controller device helpers + * + * These look at the device ID known to the HID subsystem to identify a device, + * but take caution: some NSO devices lie about themselves (NES Joy-Cons and + * Sega Genesis controller). See type helpers below. + * + * These helpers are most useful early during the HID probe or in conjunction + * with the capability helpers below. + */ +static inline bool joycon_device_is_left_joycon(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_JOYCONL; +} + +static inline bool joycon_device_is_right_joycon(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_JOYCONR; +} + +static inline bool joycon_device_is_procon(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_PROCON; +} + +static inline bool joycon_device_is_chrggrip(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_CHRGGRIP; +} + +static inline bool joycon_device_is_snescon(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_SNESCON; +} + +static inline bool joycon_device_is_gencon(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_GENCON; +} + +static inline bool joycon_device_is_n64con(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->product == USB_DEVICE_ID_NINTENDO_N64CON; +} + +static inline bool joycon_device_has_usb(struct joycon_ctlr *ctlr) +{ + return joycon_device_is_procon(ctlr) || + joycon_device_is_chrggrip(ctlr) || + joycon_device_is_snescon(ctlr) || + joycon_device_is_gencon(ctlr) || + joycon_device_is_n64con(ctlr); +} + +/* + * Controller type helpers + * + * These are slightly different than the device-ID-based helpers above. They are + * generally more reliable, since they can distinguish between, e.g., Genesis + * versus SNES, or NES Joy-Cons versus regular Switch Joy-Cons. They're most + * useful for reporting available inputs. For other kinds of distinctions, see + * the capability helpers below. + * + * They have two major drawbacks: (1) they're not available until after we set + * the reporting method and then request the device info; (2) they can't + * distinguish all controllers (like the Charging Grip from the Pro controller.) + */ +static inline bool joycon_type_is_left_joycon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_JCL; +} + +static inline bool joycon_type_is_right_joycon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_JCR; +} + +static inline bool joycon_type_is_procon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_PRO; +} + +static inline bool joycon_type_is_snescon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_SNES; +} + +static inline bool joycon_type_is_gencon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_GEN; +} + +static inline bool joycon_type_is_n64con(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_N64; +} + +static inline bool joycon_type_is_left_nescon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_NESL; +} + +static inline bool joycon_type_is_right_nescon(struct joycon_ctlr *ctlr) +{ + return ctlr->ctlr_type == JOYCON_CTLR_TYPE_NESR; +} + +static inline bool joycon_type_has_left_controls(struct joycon_ctlr *ctlr) +{ + return joycon_type_is_left_joycon(ctlr) || + joycon_type_is_procon(ctlr); +} + +static inline bool joycon_type_has_right_controls(struct joycon_ctlr *ctlr) +{ + return joycon_type_is_right_joycon(ctlr) || + joycon_type_is_procon(ctlr); +} + +static inline bool joycon_type_is_any_joycon(struct joycon_ctlr *ctlr) +{ + return joycon_type_is_left_joycon(ctlr) || + joycon_type_is_right_joycon(ctlr) || + joycon_device_is_chrggrip(ctlr); +} + +static inline bool joycon_type_is_any_nescon(struct joycon_ctlr *ctlr) +{ + return joycon_type_is_left_nescon(ctlr) || + joycon_type_is_right_nescon(ctlr); +} + +/* + * Controller capability helpers + * + * These helpers combine the use of the helpers above to detect certain + * capabilities during initialization. They are always accurate but (since they + * use type helpers) cannot be used early in the HID probe. + */ +static inline bool joycon_has_imu(struct joycon_ctlr *ctlr) +{ + return joycon_device_is_chrggrip(ctlr) || + joycon_type_is_any_joycon(ctlr) || + joycon_type_is_procon(ctlr); +} + +static inline bool joycon_has_joysticks(struct joycon_ctlr *ctlr) +{ + return joycon_device_is_chrggrip(ctlr) || + joycon_type_is_any_joycon(ctlr) || + joycon_type_is_procon(ctlr) || + joycon_type_is_n64con(ctlr); +} + +static inline bool joycon_has_rumble(struct joycon_ctlr *ctlr) +{ + return joycon_device_is_chrggrip(ctlr) || + joycon_type_is_any_joycon(ctlr) || + joycon_type_is_procon(ctlr) || + joycon_type_is_n64con(ctlr); +} + +static inline bool joycon_using_usb(struct joycon_ctlr *ctlr) +{ + return ctlr->hdev->bus == BUS_USB; +} + static int __joycon_hid_send(struct hid_device *hdev, u8 *data, size_t len) { u8 *buf; @@ -1283,15 +1594,10 @@ static void joycon_parse_imu_report(struct joycon_ctlr *ctlr, } } -static void joycon_parse_report(struct joycon_ctlr *ctlr, - struct joycon_input_report *rep) +static void joycon_handle_rumble_report(struct joycon_ctlr *ctlr, struct joycon_input_report *rep) { - struct input_dev *dev = ctlr->input; unsigned long flags; - u8 tmp; - u32 btns; unsigned long msecs = jiffies_to_msecs(jiffies); - unsigned long report_delta_ms = msecs - ctlr->last_input_report_msecs; spin_lock_irqsave(&ctlr->lock, flags); if (IS_ENABLED(CONFIG_NINTENDO_FF) && rep->vibrator_report && @@ -1310,11 +1616,21 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr, queue_work(ctlr->rumble_queue, &ctlr->rumble_worker); } - /* Parse the battery status */ + spin_unlock_irqrestore(&ctlr->lock, flags); +} + +static void joycon_parse_battery_status(struct joycon_ctlr *ctlr, struct joycon_input_report *rep) +{ + u8 tmp; + unsigned long flags; + + spin_lock_irqsave(&ctlr->lock, flags); + tmp = rep->bat_con; ctlr->host_powered = tmp & BIT(0); ctlr->battery_charging = tmp & BIT(4); tmp = tmp >> 5; + switch (tmp) { case 0: /* empty */ ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL; @@ -1336,102 +1652,121 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr, hid_warn(ctlr->hdev, "Invalid battery status\n"); break; } + spin_unlock_irqrestore(&ctlr->lock, flags); +} - /* Parse the buttons and sticks */ - btns = hid_field_extract(ctlr->hdev, rep->button_status, 0, 24); - - if (jc_type_has_left(ctlr)) { - u16 raw_x; - u16 raw_y; - s32 x; - s32 y; - - /* get raw stick values */ - raw_x = hid_field_extract(ctlr->hdev, rep->left_stick, 0, 12); - raw_y = hid_field_extract(ctlr->hdev, - rep->left_stick + 1, 4, 12); - /* map the stick values */ - x = joycon_map_stick_val(&ctlr->left_stick_cal_x, raw_x); - y = -joycon_map_stick_val(&ctlr->left_stick_cal_y, raw_y); - /* report sticks */ - input_report_abs(dev, ABS_X, x); - input_report_abs(dev, ABS_Y, y); - - /* report buttons */ - input_report_key(dev, BTN_TL, btns & JC_BTN_L); - input_report_key(dev, BTN_TL2, btns & JC_BTN_ZL); - input_report_key(dev, BTN_SELECT, btns & JC_BTN_MINUS); - input_report_key(dev, BTN_THUMBL, btns & JC_BTN_LSTICK); - input_report_key(dev, BTN_Z, btns & JC_BTN_CAP); - - if (jc_type_is_joycon(ctlr)) { - /* Report the S buttons as the non-existent triggers */ - input_report_key(dev, BTN_TR, btns & JC_BTN_SL_L); - input_report_key(dev, BTN_TR2, btns & JC_BTN_SR_L); - - /* Report d-pad as digital buttons for the joy-cons */ - input_report_key(dev, BTN_DPAD_DOWN, - btns & JC_BTN_DOWN); - input_report_key(dev, BTN_DPAD_UP, btns & JC_BTN_UP); - input_report_key(dev, BTN_DPAD_RIGHT, - btns & JC_BTN_RIGHT); - input_report_key(dev, BTN_DPAD_LEFT, - btns & JC_BTN_LEFT); - } else { - int hatx = 0; - int haty = 0; - - /* d-pad x */ - if (btns & JC_BTN_LEFT) - hatx = -1; - else if (btns & JC_BTN_RIGHT) - hatx = 1; - input_report_abs(dev, ABS_HAT0X, hatx); - - /* d-pad y */ - if (btns & JC_BTN_UP) - haty = -1; - else if (btns & JC_BTN_DOWN) - haty = 1; - input_report_abs(dev, ABS_HAT0Y, haty); - } - } - if (jc_type_has_right(ctlr)) { - u16 raw_x; - u16 raw_y; - s32 x; - s32 y; - - /* get raw stick values */ - raw_x = hid_field_extract(ctlr->hdev, rep->right_stick, 0, 12); - raw_y = hid_field_extract(ctlr->hdev, - rep->right_stick + 1, 4, 12); - /* map stick values */ - x = joycon_map_stick_val(&ctlr->right_stick_cal_x, raw_x); - y = -joycon_map_stick_val(&ctlr->right_stick_cal_y, raw_y); - /* report sticks */ - input_report_abs(dev, ABS_RX, x); - input_report_abs(dev, ABS_RY, y); - - /* report buttons */ - input_report_key(dev, BTN_TR, btns & JC_BTN_R); - input_report_key(dev, BTN_TR2, btns & JC_BTN_ZR); - if (jc_type_is_joycon(ctlr)) { - /* Report the S buttons as the non-existent triggers */ - input_report_key(dev, BTN_TL, btns & JC_BTN_SL_R); - input_report_key(dev, BTN_TL2, btns & JC_BTN_SR_R); - } - input_report_key(dev, BTN_START, btns & JC_BTN_PLUS); - input_report_key(dev, BTN_THUMBR, btns & JC_BTN_RSTICK); - input_report_key(dev, BTN_MODE, btns & JC_BTN_HOME); - input_report_key(dev, BTN_WEST, btns & JC_BTN_Y); - input_report_key(dev, BTN_NORTH, btns & JC_BTN_X); - input_report_key(dev, BTN_EAST, btns & JC_BTN_A); - input_report_key(dev, BTN_SOUTH, btns & JC_BTN_B); +static void joycon_report_left_stick(struct joycon_ctlr *ctlr, + struct joycon_input_report *rep) +{ + u16 raw_x; + u16 raw_y; + s32 x; + s32 y; + + raw_x = hid_field_extract(ctlr->hdev, rep->left_stick, 0, 12); + raw_y = hid_field_extract(ctlr->hdev, rep->left_stick + 1, 4, 12); + + x = joycon_map_stick_val(&ctlr->left_stick_cal_x, raw_x); + y = -joycon_map_stick_val(&ctlr->left_stick_cal_y, raw_y); + + input_report_abs(ctlr->input, ABS_X, x); + input_report_abs(ctlr->input, ABS_Y, y); +} + +static void joycon_report_right_stick(struct joycon_ctlr *ctlr, + struct joycon_input_report *rep) +{ + u16 raw_x; + u16 raw_y; + s32 x; + s32 y; + + raw_x = hid_field_extract(ctlr->hdev, rep->right_stick, 0, 12); + raw_y = hid_field_extract(ctlr->hdev, rep->right_stick + 1, 4, 12); + + x = joycon_map_stick_val(&ctlr->right_stick_cal_x, raw_x); + y = -joycon_map_stick_val(&ctlr->right_stick_cal_y, raw_y); + + input_report_abs(ctlr->input, ABS_RX, x); + input_report_abs(ctlr->input, ABS_RY, y); +} + +static void joycon_report_dpad(struct joycon_ctlr *ctlr, + struct joycon_input_report *rep) +{ + int hatx = 0; + int haty = 0; + u32 btns = hid_field_extract(ctlr->hdev, rep->button_status, 0, 24); + + if (btns & JC_BTN_LEFT) + hatx = -1; + else if (btns & JC_BTN_RIGHT) + hatx = 1; + + if (btns & JC_BTN_UP) + haty = -1; + else if (btns & JC_BTN_DOWN) + haty = 1; + + input_report_abs(ctlr->input, ABS_HAT0X, hatx); + input_report_abs(ctlr->input, ABS_HAT0Y, haty); +} + +static void joycon_report_buttons(struct joycon_ctlr *ctlr, + struct joycon_input_report *rep, + const struct joycon_ctlr_button_mapping button_mappings[]) +{ + const struct joycon_ctlr_button_mapping *button; + u32 status = hid_field_extract(ctlr->hdev, rep->button_status, 0, 24); + + for (button = button_mappings; button->code; button++) + input_report_key(ctlr->input, button->code, status & button->bit); +} + +static void joycon_parse_report(struct joycon_ctlr *ctlr, + struct joycon_input_report *rep) +{ + unsigned long flags; + unsigned long msecs = jiffies_to_msecs(jiffies); + unsigned long report_delta_ms = msecs - ctlr->last_input_report_msecs; + + if (joycon_has_rumble(ctlr)) + joycon_handle_rumble_report(ctlr, rep); + + joycon_parse_battery_status(ctlr, rep); + + if (joycon_type_is_left_joycon(ctlr)) { + joycon_report_left_stick(ctlr, rep); + joycon_report_buttons(ctlr, rep, left_joycon_button_mappings); + if (!joycon_device_is_chrggrip(ctlr)) + joycon_report_buttons(ctlr, rep, left_joycon_s_button_mappings); + } else if (joycon_type_is_right_joycon(ctlr)) { + joycon_report_right_stick(ctlr, rep); + joycon_report_buttons(ctlr, rep, right_joycon_button_mappings); + if (!joycon_device_is_chrggrip(ctlr)) + joycon_report_buttons(ctlr, rep, right_joycon_s_button_mappings); + } else if (joycon_type_is_procon(ctlr)) { + joycon_report_left_stick(ctlr, rep); + joycon_report_right_stick(ctlr, rep); + joycon_report_dpad(ctlr, rep); + joycon_report_buttons(ctlr, rep, procon_button_mappings); + } else if (joycon_type_is_any_nescon(ctlr)) { + joycon_report_dpad(ctlr, rep); + joycon_report_buttons(ctlr, rep, nescon_button_mappings); + } else if (joycon_type_is_snescon(ctlr)) { + joycon_report_dpad(ctlr, rep); + joycon_report_buttons(ctlr, rep, snescon_button_mappings); + } else if (joycon_type_is_gencon(ctlr)) { + joycon_report_dpad(ctlr, rep); + joycon_report_buttons(ctlr, rep, gencon_button_mappings); + } else if (joycon_type_is_n64con(ctlr)) { + joycon_report_left_stick(ctlr, rep); + joycon_report_dpad(ctlr, rep); + joycon_report_buttons(ctlr, rep, n64con_button_mappings); } - input_sync(dev); + input_sync(ctlr->input); spin_lock_irqsave(&ctlr->lock, flags); ctlr->last_input_report_msecs = msecs; @@ -1471,7 +1806,7 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr, } /* parse IMU data if present */ - if (rep->id == JC_INPUT_IMU_DATA) + if ((rep->id == JC_INPUT_IMU_DATA) && joycon_has_imu(ctlr)) joycon_parse_imu_report(ctlr, rep); } @@ -1684,123 +2019,65 @@ static int joycon_play_effect(struct input_dev *dev, void *data, } #endif /* IS_ENABLED(CONFIG_NINTENDO_FF) */ -static const unsigned int joycon_button_inputs_l[] = { - BTN_SELECT, BTN_Z, BTN_THUMBL, - BTN_TL, BTN_TL2, - 0 /* 0 signals end of array */ -}; - -static const unsigned int joycon_button_inputs_r[] = { - BTN_START, BTN_MODE, BTN_THUMBR, - BTN_SOUTH, BTN_EAST, BTN_NORTH, BTN_WEST, - BTN_TR, BTN_TR2, - 0 /* 0 signals end of array */ -}; - -/* We report joy-con d-pad inputs as buttons and pro controller as a hat. */ -static const unsigned int joycon_dpad_inputs_jc[] = { - BTN_DPAD_UP, BTN_DPAD_DOWN, BTN_DPAD_LEFT, BTN_DPAD_RIGHT, - 0 /* 0 signals end of array */ -}; - -static int joycon_input_create(struct joycon_ctlr *ctlr) +static void joycon_config_left_stick(struct input_dev *idev) { - struct hid_device *hdev; - const char *name; - const char *imu_name; - int ret; - int i; - - hdev = ctlr->hdev; + input_set_abs_params(idev, + ABS_X, + -JC_MAX_STICK_MAG, + JC_MAX_STICK_MAG, + JC_STICK_FUZZ, + JC_STICK_FLAT); + input_set_abs_params(idev, + ABS_Y, + -JC_MAX_STICK_MAG, + JC_MAX_STICK_MAG, + JC_STICK_FUZZ, + JC_STICK_FLAT); +} - switch (hdev->product) { - case USB_DEVICE_ID_NINTENDO_PROCON: - name = "Nintendo Switch Pro Controller"; - imu_name = "Nintendo Switch Pro Controller IMU"; - break; - case USB_DEVICE_ID_NINTENDO_CHRGGRIP: - if (jc_type_has_left(ctlr)) { - name = "Nintendo Switch Left Joy-Con (Grip)"; - imu_name = "Nintendo Switch Left Joy-Con IMU (Grip)"; - } else { - name = "Nintendo Switch Right Joy-Con (Grip)"; - imu_name = "Nintendo Switch Right Joy-Con IMU (Grip)"; - } - break; - case USB_DEVICE_ID_NINTENDO_JOYCONL: - name = "Nintendo Switch Left Joy-Con"; - imu_name = "Nintendo Switch Left Joy-Con IMU"; - break; - case USB_DEVICE_ID_NINTENDO_JOYCONR: - name = "Nintendo Switch Right Joy-Con"; - imu_name = "Nintendo Switch Right Joy-Con IMU"; - break; - default: /* Should be impossible */ - hid_err(hdev, "Invalid hid product\n"); - return -EINVAL; - } +static void joycon_config_right_stick(struct input_dev *idev) +{ + input_set_abs_params(idev, + ABS_RX, + -JC_MAX_STICK_MAG, + JC_MAX_STICK_MAG, + JC_STICK_FUZZ, + JC_STICK_FLAT); + input_set_abs_params(idev, + ABS_RY, + -JC_MAX_STICK_MAG, + JC_MAX_STICK_MAG, + JC_STICK_FUZZ, + JC_STICK_FLAT); +} - ctlr->input = devm_input_allocate_device(&hdev->dev); - if (!ctlr->input) - return -ENOMEM; - ctlr->input->id.bustype = hdev->bus; - ctlr->input->id.vendor = hdev->vendor; - ctlr->input->id.product = hdev->product; - ctlr->input->id.version = hdev->version; - ctlr->input->uniq = ctlr->mac_addr_str; - ctlr->input->name = name; - ctlr->input->phys = hdev->phys; - input_set_drvdata(ctlr->input, ctlr); +static void joycon_config_dpad(struct input_dev *idev) +{ + input_set_abs_params(idev, + ABS_HAT0X, + -JC_MAX_DPAD_MAG, + JC_MAX_DPAD_MAG, + JC_DPAD_FUZZ, + JC_DPAD_FLAT); + input_set_abs_params(idev, + ABS_HAT0Y, + -JC_MAX_DPAD_MAG, + JC_MAX_DPAD_MAG, + JC_DPAD_FUZZ, + JC_DPAD_FLAT); +} - /* set up sticks and buttons */ - if (jc_type_has_left(ctlr)) { - input_set_abs_params(ctlr->input, ABS_X, - -JC_MAX_STICK_MAG, JC_MAX_STICK_MAG, - JC_STICK_FUZZ, JC_STICK_FLAT); - input_set_abs_params(ctlr->input, ABS_Y, - -JC_MAX_STICK_MAG, JC_MAX_STICK_MAG, - JC_STICK_FUZZ, JC_STICK_FLAT); - - for (i = 0; joycon_button_inputs_l[i] > 0; i++) - input_set_capability(ctlr->input, EV_KEY, - joycon_button_inputs_l[i]); - - /* configure d-pad differently for joy-con vs pro controller */ - if (hdev->product != USB_DEVICE_ID_NINTENDO_PROCON) { - for (i = 0; joycon_dpad_inputs_jc[i] > 0; i++) - input_set_capability(ctlr->input, EV_KEY, - joycon_dpad_inputs_jc[i]); - } else { - input_set_abs_params(ctlr->input, ABS_HAT0X, - -JC_MAX_DPAD_MAG, JC_MAX_DPAD_MAG, - JC_DPAD_FUZZ, JC_DPAD_FLAT); - input_set_abs_params(ctlr->input, ABS_HAT0Y, - -JC_MAX_DPAD_MAG, JC_MAX_DPAD_MAG, - JC_DPAD_FUZZ, JC_DPAD_FLAT); - } - } - if (jc_type_has_right(ctlr)) { - input_set_abs_params(ctlr->input, ABS_RX, - -JC_MAX_STICK_MAG, JC_MAX_STICK_MAG, - JC_STICK_FUZZ, JC_STICK_FLAT); - input_set_abs_params(ctlr->input, ABS_RY, - -JC_MAX_STICK_MAG, JC_MAX_STICK_MAG, - JC_STICK_FUZZ, JC_STICK_FLAT); - - for (i = 0; joycon_button_inputs_r[i] > 0; i++) - input_set_capability(ctlr->input, EV_KEY, - joycon_button_inputs_r[i]); - } +static void joycon_config_buttons(struct input_dev *idev, + const struct joycon_ctlr_button_mapping button_mappings[]) +{ + const struct joycon_ctlr_button_mapping *button; - /* Let's report joy-con S triggers separately */ - if (hdev->product == USB_DEVICE_ID_NINTENDO_JOYCONL) { - input_set_capability(ctlr->input, EV_KEY, BTN_TR); - input_set_capability(ctlr->input, EV_KEY, BTN_TR2); - } else if (hdev->product == USB_DEVICE_ID_NINTENDO_JOYCONR) { - input_set_capability(ctlr->input, EV_KEY, BTN_TL); - input_set_capability(ctlr->input, EV_KEY, BTN_TL2); - } + for (button = button_mappings; button->code; button++) + input_set_capability(idev, EV_KEY, button->code); +} +static void joycon_config_rumble(struct joycon_ctlr *ctlr) +{ #if IS_ENABLED(CONFIG_NINTENDO_FF) /* set up rumble */ input_set_capability(ctlr->input, EV_FF, FF_RUMBLE); @@ -1813,10 +2090,15 @@ static int joycon_input_create(struct joycon_ctlr *ctlr) joycon_set_rumble(ctlr, 0, 0, false); ctlr->rumble_msecs = jiffies_to_msecs(jiffies); #endif +} - ret = input_register_device(ctlr->input); - if (ret) - return ret; +static int joycon_imu_input_create(struct joycon_ctlr *ctlr) +{ + struct hid_device *hdev; + const char *imu_name; + int ret; + + hdev = ctlr->hdev; /* configure the imu input device */ ctlr->imu_input = devm_input_allocate_device(&hdev->dev); @@ -1828,8 +2110,14 @@ static int joycon_input_create(struct joycon_ctlr *ctlr) ctlr->imu_input->id.product = hdev->product; ctlr->imu_input->id.version = hdev->version; ctlr->imu_input->uniq = ctlr->mac_addr_str; - ctlr->imu_input->name = imu_name; ctlr->imu_input->phys = hdev->phys; + + imu_name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s (IMU)", ctlr->input->name); + if (!imu_name) + return -ENOMEM; + + ctlr->imu_input->name = imu_name; + input_set_drvdata(ctlr->imu_input, ctlr); /* configure imu axes */ @@ -1871,6 +2159,71 @@ static int joycon_input_create(struct joycon_ctlr *ctlr) return 0; } +static int joycon_input_create(struct joycon_ctlr *ctlr) +{ + struct hid_device *hdev; + int ret; + + hdev = ctlr->hdev; + + ctlr->input = devm_input_allocate_device(&hdev->dev); + if (!ctlr->input) + return -ENOMEM; + ctlr->input->id.bustype = hdev->bus; + ctlr->input->id.vendor = hdev->vendor; + ctlr->input->id.product = hdev->product; + ctlr->input->id.version = hdev->version; + ctlr->input->uniq = ctlr->mac_addr_str; + ctlr->input->name = hdev->name; + ctlr->input->phys = hdev->phys; + input_set_drvdata(ctlr->input, ctlr); + + ret = input_register_device(ctlr->input); + if (ret) + return ret; + + if (joycon_type_is_right_joycon(ctlr)) { + joycon_config_right_stick(ctlr->input); + joycon_config_buttons(ctlr->input, right_joycon_button_mappings); + if (!joycon_device_is_chrggrip(ctlr)) + joycon_config_buttons(ctlr->input, right_joycon_s_button_mappings); + } else if (joycon_type_is_left_joycon(ctlr)) { + joycon_config_left_stick(ctlr->input); + joycon_config_buttons(ctlr->input, left_joycon_button_mappings); + if (!joycon_device_is_chrggrip(ctlr)) + joycon_config_buttons(ctlr->input, left_joycon_s_button_mappings); + } else if (joycon_type_is_procon(ctlr)) { + joycon_config_left_stick(ctlr->input); + joycon_config_right_stick(ctlr->input); + joycon_config_dpad(ctlr->input); + joycon_config_buttons(ctlr->input, procon_button_mappings); + } else if (joycon_type_is_any_nescon(ctlr)) { + joycon_config_dpad(ctlr->input); + joycon_config_buttons(ctlr->input, nescon_button_mappings); + } else if (joycon_type_is_snescon(ctlr)) { + joycon_config_dpad(ctlr->input); + joycon_config_buttons(ctlr->input, snescon_button_mappings); + } else if (joycon_type_is_gencon(ctlr)) { + joycon_config_dpad(ctlr->input); + joycon_config_buttons(ctlr->input, gencon_button_mappings); + } else if (joycon_type_is_n64con(ctlr)) { + joycon_config_dpad(ctlr->input); + joycon_config_left_stick(ctlr->input); + joycon_config_buttons(ctlr->input, n64con_button_mappings); + } + + if (joycon_has_imu(ctlr)) { + ret = joycon_imu_input_create(ctlr); + if (ret) + return ret; + } + + if (joycon_has_rumble(ctlr)) + joycon_config_rumble(ctlr); + + return 0; +} + /* Because the subcommand sets all the leds at once, the brightness argument is ignored */ static int joycon_player_led_brightness_set(struct led_classdev *led, enum led_brightness brightness) @@ -2107,9 +2460,7 @@ static int joycon_read_info(struct joycon_ctlr *ctlr) struct joycon_input_report *report; req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO; - mutex_lock(&ctlr->output_mutex); ret = joycon_send_subcmd(ctlr, &req, 0, HZ); - mutex_unlock(&ctlr->output_mutex); if (ret) { hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret); return ret; @@ -2132,8 +2483,17 @@ static int joycon_read_info(struct joycon_ctlr *ctlr) return -ENOMEM; hid_info(ctlr->hdev, "controller MAC = %s\n", ctlr->mac_addr_str); - /* Retrieve the type so we can distinguish for charging grip */ + /* + * Retrieve the type so we can distinguish the controller type + * Unfortantly the hdev->product can't always be used due to a ?bug? + * with the NSO Genesis controller. Over USB, it will report the + * PID as 0x201E, but over bluetooth it will report the PID as 0x2017 + * which is the same as the NSO SNES controller. This is different from + * the rest of the controllers which will report the same PID over USB + * and bluetooth. + */ ctlr->ctlr_type = report->subcmd_reply.data[2]; + hid_dbg(ctlr->hdev, "controller type = 0x%02X\n", ctlr->ctlr_type); return 0; } @@ -2145,8 +2505,7 @@ static int joycon_init(struct hid_device *hdev) mutex_lock(&ctlr->output_mutex); /* if handshake command fails, assume ble pro controller */ - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) && - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) { + if (joycon_using_usb(ctlr) && !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) { hid_dbg(hdev, "detected USB controller\n"); /* set baudrate for improved latency */ ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ); @@ -2171,24 +2530,43 @@ static int joycon_init(struct hid_device *hdev) goto out_unlock; } - /* get controller calibration data, and parse it */ - ret = joycon_request_calibration(ctlr); + /* needed to retrieve the controller type */ + ret = joycon_read_info(ctlr); if (ret) { - /* - * We can function with default calibration, but it may be - * inaccurate. Provide a warning, and continue on. - */ - hid_warn(hdev, "Analog stick positions may be inaccurate\n"); + hid_err(hdev, "Failed to retrieve controller info; ret=%d\n", + ret); + goto out_unlock; } - /* get IMU calibration data, and parse it */ - ret = joycon_request_imu_calibration(ctlr); - if (ret) { - /* - * We can function with default calibration, but it may be - * inaccurate. Provide a warning, and continue on. - */ - hid_warn(hdev, "Unable to read IMU calibration data\n"); + if (joycon_has_joysticks(ctlr)) { + /* get controller calibration data, and parse it */ + ret = joycon_request_calibration(ctlr); + if (ret) { + /* + * We can function with default calibration, but it may be + * inaccurate. Provide a warning, and continue on. + */ + hid_warn(hdev, "Analog stick positions may be inaccurate\n"); + } + } + + if (joycon_has_imu(ctlr)) { + /* get IMU calibration data, and parse it */ + ret = joycon_request_imu_calibration(ctlr); + if (ret) { + /* + * We can function with default calibration, but it may be + * inaccurate. Provide a warning, and continue on. + */ + hid_warn(hdev, "Unable to read IMU calibration data\n"); + } + + /* Enable the IMU */ + ret = joycon_enable_imu(ctlr); + if (ret) { + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret); + goto out_unlock; + } } /* Set the reporting mode to 0x30, which is the full report mode */ @@ -2198,18 +2576,13 @@ static int joycon_init(struct hid_device *hdev) goto out_unlock; } - /* Enable rumble */ - ret = joycon_enable_rumble(ctlr); - if (ret) { - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret); - goto out_unlock; - } - - /* Enable the IMU */ - ret = joycon_enable_imu(ctlr); - if (ret) { - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret); - goto out_unlock; + if (joycon_has_rumble(ctlr)) { + /* Enable rumble */ + ret = joycon_enable_rumble(ctlr); + if (ret) { + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret); + goto out_unlock; + } } out_unlock: @@ -2354,13 +2727,6 @@ static int nintendo_hid_probe(struct hid_device *hdev, goto err_close; } - ret = joycon_read_info(ctlr); - if (ret) { - hid_err(hdev, "Failed to retrieve controller info; ret=%d\n", - ret); - goto err_close; - } - /* Initialize the leds */ ret = joycon_leds_create(ctlr); if (ret) { @@ -2432,6 +2798,12 @@ static int nintendo_hid_resume(struct hid_device *hdev) static const struct hid_device_id nintendo_hid_devices[] = { { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_PROCON) }, + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_SNESCON) }, + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_GENCON) }, + { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_N64CON) }, { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_PROCON) }, { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, @@ -2440,6 +2812,12 @@ static const struct hid_device_id nintendo_hid_devices[] = { USB_DEVICE_ID_NINTENDO_JOYCONL) }, { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_JOYCONR) }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_SNESCON) }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_GENCON) }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, + USB_DEVICE_ID_NINTENDO_N64CON) }, { } }; MODULE_DEVICE_TABLE(hid, nintendo_hid_devices); @@ -2458,6 +2836,7 @@ static struct hid_driver nintendo_hid_driver = { module_hid_driver(nintendo_hid_driver); MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Ryan McClelland "); +MODULE_AUTHOR("Emily Strickland "); MODULE_AUTHOR("Daniel J. Ogorchock "); MODULE_DESCRIPTION("Driver for Nintendo Switch Controllers"); - From 502296030ec6b0329e00f9fb15018e170cc63037 Mon Sep 17 00:00:00 2001 From: Jason Gerecke Date: Tue, 19 Dec 2023 13:33:43 -0800 Subject: [PATCH 35/53] HID: wacom: Correct behavior when processing some confidence == false touches There appear to be a few different ways that Wacom devices can deal with confidence: 1. If the device looses confidence in a touch, it will first clear the tipswitch flag in one report, and then clear the confidence flag in a second report. This behavior is used by e.g. DTH-2452. 2. If the device looses confidence in a touch, it will clear both the tipswitch and confidence flags within the same report. This behavior is used by some AES devices. 3. If the device looses confidence in a touch, it will clear *only* the confidence bit. The tipswitch bit will remain set so long as the touch is tracked. This behavior may be used in future devices. The driver does not currently handle situation 3 properly. Touches that loose confidence will remain "in prox" and essentially frozen in place until the tipswitch bit is finally cleared. Not only does this result in userspace seeing a stuck touch, but it also prevents pen arbitration from working properly (the pen won't send events until all touches are up, but we don't currently process events from non-confident touches). This commit centralizes the checking of the confidence bit in the wacom_wac_finger_slot() function and has 'prox' depend on it. In the case where situation 3 is encountered, the treat the touch as though it was removed, allowing both userspace and the pen arbitration to act normally. Signed-off-by: Tatsunosuke Tobita Signed-off-by: Ping Cheng Signed-off-by: Jason Gerecke Fixes: 7fb0413baa7f ("HID: wacom: Use "Confidence" flag to prevent reporting invalid contacts") Cc: stable@vger.kernel.org Signed-off-by: Jiri Kosina --- drivers/hid/wacom_wac.c | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-) diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index c205198ded118..da8a01fedd394 100644 --- a/drivers/hid/wacom_wac.c +++ b/drivers/hid/wacom_wac.c @@ -2659,8 +2659,8 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac, { struct hid_data *hid_data = &wacom_wac->hid_data; bool mt = wacom_wac->features.touch_max > 1; - bool prox = hid_data->tipswitch && - report_touch_events(wacom_wac); + bool touch_down = hid_data->tipswitch && hid_data->confidence; + bool prox = touch_down && report_touch_events(wacom_wac); if (touch_is_muted(wacom_wac)) { if (!wacom_wac->shared->touch_down) @@ -2710,24 +2710,6 @@ static void wacom_wac_finger_slot(struct wacom_wac *wacom_wac, } } -static bool wacom_wac_slot_is_active(struct input_dev *dev, int key) -{ - struct input_mt *mt = dev->mt; - struct input_mt_slot *s; - - if (!mt) - return false; - - for (s = mt->slots; s != mt->slots + mt->num_slots; s++) { - if (s->key == key && - input_mt_get_value(s, ABS_MT_TRACKING_ID) >= 0) { - return true; - } - } - - return false; -} - static void wacom_wac_finger_event(struct hid_device *hdev, struct hid_field *field, struct hid_usage *usage, __s32 value) { @@ -2778,14 +2760,8 @@ static void wacom_wac_finger_event(struct hid_device *hdev, } if (usage->usage_index + 1 == field->report_count) { - if (equivalent_usage == wacom_wac->hid_data.last_slot_field) { - bool touch_removed = wacom_wac_slot_is_active(wacom_wac->touch_input, - wacom_wac->hid_data.id) && !wacom_wac->hid_data.tipswitch; - - if (wacom_wac->hid_data.confidence || touch_removed) { - wacom_wac_finger_slot(wacom_wac, wacom_wac->touch_input); - } - } + if (equivalent_usage == wacom_wac->hid_data.last_slot_field) + wacom_wac_finger_slot(wacom_wac, wacom_wac->touch_input); } } From b0fb904d074e810c22c26883b8ed4489c17d1292 Mon Sep 17 00:00:00 2001 From: Jason Gerecke Date: Tue, 19 Dec 2023 13:33:44 -0800 Subject: [PATCH 36/53] HID: wacom: Add additional tests of confidence behavior Test for proper driver behavior when the touch confidence bit is set or cleared. Test the three flavors of touch confidence loss (tipswitch cleared before confidence, tipswitch and confidence cleared at the same time, and tipswitch only cleared when touch is actually removed). Also test two flavors of touch confidence gain (confidence added to a touch that was "never" confident, and confidence added to a touch that was previously confident). Signed-off-by: Jason Gerecke Signed-off-by: Jiri Kosina --- .../selftests/hid/tests/test_wacom_generic.py | 278 +++++++++++++++++- 1 file changed, 277 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/hid/tests/test_wacom_generic.py b/tools/testing/selftests/hid/tests/test_wacom_generic.py index f92fe8e02c1bf..a7ee54e5090bb 100644 --- a/tools/testing/selftests/hid/tests/test_wacom_generic.py +++ b/tools/testing/selftests/hid/tests/test_wacom_generic.py @@ -27,6 +27,7 @@ ) import attr +from collections import namedtuple from enum import Enum from hidtools.hut import HUT from hidtools.hid import HidUnit @@ -862,6 +863,8 @@ def offset_rotation(value): class TestDTH2452Tablet(test_multitouch.BaseTest.TestMultitouch, TouchTabletTest): + ContactIds = namedtuple("ContactIds", "contact_id, tracking_id, slot_num") + def create_device(self): return test_multitouch.Digitizer( "DTH 2452", @@ -869,6 +872,57 @@ def create_device(self): input_info=(0x3, 0x056A, 0x0383), ) + def make_contact(self, contact_id=0, t=0): + """ + Make a single touch contact that can move over time. + + Creates a touch object that has a well-known position in space that + does not overlap with other contacts. The value of `t` may be + incremented over time to move the point along a linear path. + """ + x = 50 + 10 * contact_id + t + y = 100 + 100 * contact_id + t + return test_multitouch.Touch(contact_id, x, y) + + def make_contacts(self, n, t=0): + """ + Make multiple touch contacts that can move over time. + + Returns a list of `n` touch objects that are positioned at well-known + locations. The value of `t` may be incremented over time to move the + points along a linear path. + """ + return [ self.make_contact(id, t) for id in range(0, n) ] + + def assert_contact(self, uhdev, evdev, contact_ids, t=0): + """ + Assert properties of a contact generated by make_contact. + """ + contact_id = contact_ids.contact_id + tracking_id = contact_ids.tracking_id + slot_num = contact_ids.slot_num + + x = 50 + 10 * contact_id + t + y = 100 + 100 * contact_id + t + + # If the data isn't supposed to be stored in any slots, there is + # nothing we can check for in the evdev stream. + if slot_num is None: + assert tracking_id == -1 + return + + assert evdev.slots[slot_num][libevdev.EV_ABS.ABS_MT_TRACKING_ID] == tracking_id + if tracking_id != -1: + assert evdev.slots[slot_num][libevdev.EV_ABS.ABS_MT_POSITION_X] == x + assert evdev.slots[slot_num][libevdev.EV_ABS.ABS_MT_POSITION_Y] == y + + def assert_contacts(self, uhdev, evdev, data, t=0): + """ + Assert properties of a list of contacts generated by make_contacts. + """ + for contact_ids in data: + self.assert_contact(uhdev, evdev, contact_ids, t) + def test_contact_id_0(self): """ Bring a finger in contact with the tablet, then hold it down and remove it. @@ -919,4 +973,226 @@ def test_confidence_false(self): slot = self.get_slot(uhdev, t0, 0) - assert not events \ No newline at end of file + assert not events + + def test_confidence_multitouch(self): + """ + Bring multiple fingers in contact with the tablet, some with the + confidence bit set, and some without. + + Ensure that all confident touches are reported and that all non- + confident touches are ignored. + """ + uhdev = self.uhdev + evdev = uhdev.get_evdev() + + touches = self.make_contacts(5) + touches[0].confidence = False + touches[2].confidence = False + touches[4].confidence = False + + r = uhdev.event(touches) + events = uhdev.next_sync_events() + self.debug_reports(r, uhdev, events) + + assert libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH, 1) in events + + self.assert_contacts(uhdev, evdev, + [ self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = None), + self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), + self.ContactIds(contact_id = 2, tracking_id = -1, slot_num = None), + self.ContactIds(contact_id = 3, tracking_id = 1, slot_num = 1), + self.ContactIds(contact_id = 4, tracking_id = -1, slot_num = None) ]) + + def confidence_change_assert_playback(self, uhdev, evdev, timeline): + """ + Assert proper behavior of contacts that move and change tipswitch / + confidence status over time. + + Given a `timeline` list of touch states to iterate over, verify + that the contacts move and are reported as up/down as expected + by the state of the tipswitch and confidence bits. + """ + t = 0 + + for state in timeline: + touches = self.make_contacts(len(state), t) + + for item in zip(touches, state): + item[0].tipswitch = item[1][1] + item[0].confidence = item[1][2] + + r = uhdev.event(touches) + events = uhdev.next_sync_events() + self.debug_reports(r, uhdev, events) + + ids = [ x[0] for x in state ] + self.assert_contacts(uhdev, evdev, ids, t) + + t += 1 + + def test_confidence_loss_a(self): + """ + Transition a confident contact to a non-confident contact by + first clearing the tipswitch. + + Ensure that the driver reports the transitioned contact as + being removed and that other contacts continue to report + normally. This mode of confidence loss is used by the + DTH-2452. + """ + uhdev = self.uhdev + evdev = uhdev.get_evdev() + + self.confidence_change_assert_playback(uhdev, evdev, [ + # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident + # Both fingers confidently in contact + [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=1: Contact 0 == !Down + confident; Contact 1 == Down + confident + # First finger looses confidence and clears only the tipswitch flag + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=2: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger has lost confidence and has both flags cleared + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=3: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger has lost confidence and has both flags cleared + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)] + ]) + + def test_confidence_loss_b(self): + """ + Transition a confident contact to a non-confident contact by + cleraing both tipswitch and confidence bits simultaneously. + + Ensure that the driver reports the transitioned contact as + being removed and that other contacts continue to report + normally. This mode of confidence loss is used by some + AES devices. + """ + uhdev = self.uhdev + evdev = uhdev.get_evdev() + + self.confidence_change_assert_playback(uhdev, evdev, [ + # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident + # Both fingers confidently in contact + [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=1: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger looses confidence and has both flags cleared simultaneously + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=2: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger has lost confidence and has both flags cleared + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=3: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger has lost confidence and has both flags cleared + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)] + ]) + + def test_confidence_loss_c(self): + """ + Transition a confident contact to a non-confident contact by + clearing only the confidence bit. + + Ensure that the driver reports the transitioned contact as + being removed and that other contacts continue to report + normally. + """ + uhdev = self.uhdev + evdev = uhdev.get_evdev() + + self.confidence_change_assert_playback(uhdev, evdev, [ + # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident + # Both fingers confidently in contact + [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=1: Contact 0 == Down + !confident; Contact 1 == Down + confident + # First finger looses confidence and clears only the confidence flag + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), True, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=2: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger has lost confidence and has both flags cleared + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=3: Contact 0 == !Down + !confident; Contact 1 == Down + confident + # First finger has lost confidence and has both flags cleared + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)] + ]) + + def test_confidence_gain_a(self): + """ + Transition a contact that was always non-confident to confident. + + Ensure that the confident contact is reported normally. + """ + uhdev = self.uhdev + evdev = uhdev.get_evdev() + + self.confidence_change_assert_playback(uhdev, evdev, [ + # t=0: Contact 0 == Down + !confident; Contact 1 == Down + confident + # Only second finger is confidently in contact + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = None), True, False), + (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)], + + # t=1: Contact 0 == Down + !confident; Contact 1 == Down + confident + # First finger gains confidence + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = None), True, False), + (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)], + + # t=2: Contact 0 == Down + confident; Contact 1 == Down + confident + # First finger remains confident + [(self.ContactIds(contact_id = 0, tracking_id = 1, slot_num = 1), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)], + + # t=3: Contact 0 == Down + confident; Contact 1 == Down + confident + # First finger remains confident + [(self.ContactIds(contact_id = 0, tracking_id = 1, slot_num = 1), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 0, slot_num = 0), True, True)] + ]) + + def test_confidence_gain_b(self): + """ + Transition a contact from non-confident to confident. + + Ensure that the confident contact is reported normally. + """ + uhdev = self.uhdev + evdev = uhdev.get_evdev() + + self.confidence_change_assert_playback(uhdev, evdev, [ + # t=0: Contact 0 == Down + confident; Contact 1 == Down + confident + # First and second finger confidently in contact + [(self.ContactIds(contact_id = 0, tracking_id = 0, slot_num = 0), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=1: Contact 0 == Down + !confident; Contact 1 == Down + confident + # Firtst finger looses confidence + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), True, False), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=2: Contact 0 == Down + confident; Contact 1 == Down + confident + # First finger gains confidence + [(self.ContactIds(contact_id = 0, tracking_id = 2, slot_num = 0), True, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)], + + # t=3: Contact 0 == !Down + confident; Contact 1 == Down + confident + # First finger goes up + [(self.ContactIds(contact_id = 0, tracking_id = -1, slot_num = 0), False, True), + (self.ContactIds(contact_id = 1, tracking_id = 1, slot_num = 1), True, True)] + ]) From 8e2f79f41a5d1b1a4a53ec524eb7609ca89f3c65 Mon Sep 17 00:00:00 2001 From: Yauhen Kharuzhy Date: Wed, 20 Dec 2023 01:15:03 +0200 Subject: [PATCH 37/53] HID: sensor-hub: Enable hid core report processing for all devices After the commit 666cf30a589a ("HID: sensor-hub: Allow multi-function sensor devices") hub devices are claimed by hidraw driver in hid_connect(). This causes stoppping of processing HID reports by hid core due to optimization. In such case, the hid-sensor-custom driver cannot match a known custom sensor in hid_sensor_custom_get_known() because it try to check custom properties which weren't filled from the report because hid core didn't parsed it. As result, custom sensors like hinge angle sensor and LISS sensors don't work. Mark the sensor hub devices claimed by some driver to avoid hidraw-related optimizations. Fixes: 666cf30a589a ("HID: sensor-hub: Allow multi-function sensor devices") Cc: stable@vger.kernel.org Signed-off-by: Yauhen Kharuzhy Tested-by: Daniel Thompson Acked-by: Srinivas Pandruvada Link: https://lore.kernel.org/r/20231219231503.1506801-1-jekhor@gmail.com Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-sensor-hub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c index 2eba152e8b905..26e93a331a510 100644 --- a/drivers/hid/hid-sensor-hub.c +++ b/drivers/hid/hid-sensor-hub.c @@ -632,7 +632,7 @@ static int sensor_hub_probe(struct hid_device *hdev, } INIT_LIST_HEAD(&hdev->inputs); - ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | HID_CONNECT_DRIVER); if (ret) { hid_err(hdev, "hw start failed\n"); return ret; From b0a1fe4610de5761a66de0e43540fc3d59638402 Mon Sep 17 00:00:00 2001 From: Jiri Kosina Date: Wed, 27 Dec 2023 12:08:45 +0100 Subject: [PATCH 38/53] HID: magicmouse: fix kerneldoc for struct magicmouse_sc Description for hdev, work and battery_timer of struct magicmouse_sc were missing. Fix that. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202312261056.AmFPDIL5-lkp@intel.com/ Signed-off-by: Jiri Kosina --- drivers/hid/hid-magicmouse.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c index c9c968d4b36a3..a46ff4e8b99f5 100644 --- a/drivers/hid/hid-magicmouse.c +++ b/drivers/hid/hid-magicmouse.c @@ -120,6 +120,9 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie * @scroll_jiffies: Time of last scroll motion. * @touches: Most recent data for a touch, indexed by tracking ID. * @tracking_ids: Mapping of current touch input data to @touches. + * @hdev: Pointer to the underlying HID device. + * @work: Workqueue to handle initialization retry for quirky devices. + * @battery_timer: Timer for obtaining battery level information. */ struct magicmouse_sc { struct input_dev *input; From 34281b4d916f167a6f77975380e1df07f06248b7 Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:31 -0800 Subject: [PATCH 39/53] HID: hid-steam: Avoid overwriting smoothing parameter The original implementation of this driver incorrectly guessed the function of this register. It's not only unnecessary to write to this register for lizard mode but actually counter-productive since it overwrites whatever previous value was intentionally set, for example by Steam. Signed-off-by: Vicki Pfau Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index b110818fc9458..7aefd52e945a1 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -340,9 +340,6 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MAPPINGS); /* enable mouse */ steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MOUSE); - steam_write_registers(steam, - STEAM_REG_RPAD_MARGIN, 0x01, /* enable margin */ - 0); cancel_delayed_work_sync(&steam->heartbeat); } else { @@ -351,7 +348,6 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) if (steam->quirks & STEAM_QUIRK_DECK) { steam_write_registers(steam, - STEAM_REG_RPAD_MARGIN, 0x00, /* disable margin */ STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */ STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ STEAM_REG_LPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */ @@ -365,7 +361,6 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) schedule_delayed_work(&steam->heartbeat, 5 * HZ); } else { steam_write_registers(steam, - STEAM_REG_RPAD_MARGIN, 0x00, /* disable margin */ STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */ STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ 0); From 917972636e8271c5691710ce5dcd66c2d3bd04f2 Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:32 -0800 Subject: [PATCH 40/53] HID: hid-steam: Disable watchdog instead of using a heartbeat The Steam Deck has a setting that controls whether or not the watchdog is enabled, so instead of using a heartbeat to keep the watchdog from triggering, this commit changes the behavior to simply disable the watchdog instead. Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index 7aefd52e945a1..efd297e0ea8c2 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -101,6 +101,7 @@ static LIST_HEAD(steam_devices); #define STEAM_REG_GYRO_MODE 0x30 #define STEAM_REG_LPAD_CLICK_PRESSURE 0x34 #define STEAM_REG_RPAD_CLICK_PRESSURE 0x35 +#define STEAM_REG_WATCHDOG_ENABLE 0x47 /* Raw event identifiers */ #define STEAM_EV_INPUT_DATA 0x01 @@ -134,7 +135,6 @@ struct steam_device { struct power_supply __rcu *battery; u8 battery_charge; u16 voltage; - struct delayed_work heartbeat; struct work_struct rumble_work; u16 rumble_left; u16 rumble_right; @@ -340,8 +340,6 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MAPPINGS); /* enable mouse */ steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MOUSE); - - cancel_delayed_work_sync(&steam->heartbeat); } else { /* disable esc, enter, cursor */ steam_send_report_byte(steam, STEAM_CMD_CLEAR_MAPPINGS); @@ -352,13 +350,8 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ STEAM_REG_LPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */ STEAM_REG_RPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */ + STEAM_REG_WATCHDOG_ENABLE, 0, /* disable watchdog that tests if Steam is active */ 0); - /* - * The Steam Deck has a watchdog that automatically enables - * lizard mode if it doesn't see any traffic for too long - */ - if (!work_busy(&steam->heartbeat.work)) - schedule_delayed_work(&steam->heartbeat, 5 * HZ); } else { steam_write_registers(steam, STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */ @@ -733,22 +726,6 @@ static bool steam_is_valve_interface(struct hid_device *hdev) return !list_empty(&rep_enum->report_list); } -static void steam_lizard_mode_heartbeat(struct work_struct *work) -{ - struct steam_device *steam = container_of(work, struct steam_device, - heartbeat.work); - - mutex_lock(&steam->mutex); - if (!steam->client_opened && steam->client_hdev) { - steam_send_report_byte(steam, STEAM_CMD_CLEAR_MAPPINGS); - steam_write_registers(steam, - STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ - 0); - schedule_delayed_work(&steam->heartbeat, 5 * HZ); - } - mutex_unlock(&steam->mutex); -} - static int steam_client_ll_parse(struct hid_device *hdev) { struct steam_device *steam = hdev->driver_data; @@ -887,7 +864,6 @@ static int steam_probe(struct hid_device *hdev, steam->quirks = id->driver_data; INIT_WORK(&steam->work_connect, steam_work_connect_cb); INIT_LIST_HEAD(&steam->list); - INIT_DEFERRABLE_WORK(&steam->heartbeat, steam_lizard_mode_heartbeat); INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb); steam->client_hdev = steam_create_client_hid(hdev); @@ -944,7 +920,6 @@ static int steam_probe(struct hid_device *hdev, hid_destroy_device(steam->client_hdev); client_hdev_fail: cancel_work_sync(&steam->work_connect); - cancel_delayed_work_sync(&steam->heartbeat); cancel_work_sync(&steam->rumble_work); steam_alloc_fail: hid_err(hdev, "%s: failed with error %d\n", @@ -965,7 +940,6 @@ static void steam_remove(struct hid_device *hdev) mutex_lock(&steam->mutex); steam->client_hdev = NULL; steam->client_opened = false; - cancel_delayed_work_sync(&steam->heartbeat); mutex_unlock(&steam->mutex); cancel_work_sync(&steam->work_connect); if (steam->quirks & STEAM_QUIRK_WIRELESS) { From 691ead124a0c35e56633dbb73e43711ff3db23ef Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:33 -0800 Subject: [PATCH 41/53] HID: hid-steam: Clean up locking This cleans up the locking logic so that the spinlock is consistently used for access to a small handful of struct variables, and the mutex is exclusively and consistently used for ensuring that mutliple threads aren't trying to send/receive reports at the same time. Previously, only some report transactions were guarded by this mutex, potentially breaking atomicity. The mutex has been renamed to reflect this usage. Signed-off-by: Vicki Pfau Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 122 +++++++++++++++++++++++----------------- 1 file changed, 69 insertions(+), 53 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index efd297e0ea8c2..57cb58941c9fc 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -124,7 +124,7 @@ struct steam_device { struct list_head list; spinlock_t lock; struct hid_device *hdev, *client_hdev; - struct mutex mutex; + struct mutex report_mutex; bool client_opened; struct input_dev __rcu *input; unsigned long quirks; @@ -267,21 +267,26 @@ static int steam_get_serial(struct steam_device *steam) * Send: 0xae 0x15 0x01 * Recv: 0xae 0x15 0x01 serialnumber (10 chars) */ - int ret; + int ret = 0; u8 cmd[] = {STEAM_CMD_GET_SERIAL, 0x15, 0x01}; u8 reply[3 + STEAM_SERIAL_LEN + 1]; + mutex_lock(&steam->report_mutex); ret = steam_send_report(steam, cmd, sizeof(cmd)); if (ret < 0) - return ret; + goto out; ret = steam_recv_report(steam, reply, sizeof(reply)); if (ret < 0) - return ret; - if (reply[0] != 0xae || reply[1] != 0x15 || reply[2] != 0x01) - return -EIO; + goto out; + if (reply[0] != 0xae || reply[1] != 0x15 || reply[2] != 0x01) { + ret = -EIO; + goto out; + } reply[3 + STEAM_SERIAL_LEN] = 0; strscpy(steam->serial_no, reply + 3, sizeof(steam->serial_no)); - return 0; +out: + mutex_unlock(&steam->report_mutex); + return ret; } /* @@ -291,13 +296,18 @@ static int steam_get_serial(struct steam_device *steam) */ static inline int steam_request_conn_status(struct steam_device *steam) { - return steam_send_report_byte(steam, STEAM_CMD_REQUEST_COMM_STATUS); + int ret; + mutex_lock(&steam->report_mutex); + ret = steam_send_report_byte(steam, STEAM_CMD_REQUEST_COMM_STATUS); + mutex_unlock(&steam->report_mutex); + return ret; } static inline int steam_haptic_rumble(struct steam_device *steam, u16 intensity, u16 left_speed, u16 right_speed, u8 left_gain, u8 right_gain) { + int ret; u8 report[11] = {STEAM_CMD_HAPTIC_RUMBLE, 9}; report[3] = intensity & 0xFF; @@ -309,7 +319,10 @@ static inline int steam_haptic_rumble(struct steam_device *steam, report[9] = left_gain; report[10] = right_gain; - return steam_send_report(steam, report, sizeof(report)); + mutex_lock(&steam->report_mutex); + ret = steam_send_report(steam, report, sizeof(report)); + mutex_unlock(&steam->report_mutex); + return ret; } static void steam_haptic_rumble_cb(struct work_struct *work) @@ -336,11 +349,14 @@ static int steam_play_effect(struct input_dev *dev, void *data, static void steam_set_lizard_mode(struct steam_device *steam, bool enable) { if (enable) { + mutex_lock(&steam->report_mutex); /* enable esc, enter, cursors */ steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MAPPINGS); /* enable mouse */ steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MOUSE); + mutex_unlock(&steam->report_mutex); } else { + mutex_lock(&steam->report_mutex); /* disable esc, enter, cursor */ steam_send_report_byte(steam, STEAM_CMD_CLEAR_MAPPINGS); @@ -352,11 +368,13 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) STEAM_REG_RPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */ STEAM_REG_WATCHDOG_ENABLE, 0, /* disable watchdog that tests if Steam is active */ 0); + mutex_unlock(&steam->report_mutex); } else { steam_write_registers(steam, STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */ STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ 0); + mutex_unlock(&steam->report_mutex); } } } @@ -364,22 +382,29 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) static int steam_input_open(struct input_dev *dev) { struct steam_device *steam = input_get_drvdata(dev); + unsigned long flags; + bool set_lizard_mode; - mutex_lock(&steam->mutex); - if (!steam->client_opened && lizard_mode) + spin_lock_irqsave(&steam->lock, flags); + set_lizard_mode = !steam->client_opened && lizard_mode; + spin_unlock_irqrestore(&steam->lock, flags); + if (set_lizard_mode) steam_set_lizard_mode(steam, false); - mutex_unlock(&steam->mutex); + return 0; } static void steam_input_close(struct input_dev *dev) { struct steam_device *steam = input_get_drvdata(dev); + unsigned long flags; + bool set_lizard_mode; - mutex_lock(&steam->mutex); - if (!steam->client_opened && lizard_mode) + spin_lock_irqsave(&steam->lock, flags); + set_lizard_mode = !steam->client_opened && lizard_mode; + spin_unlock_irqrestore(&steam->lock, flags); + if (set_lizard_mode) steam_set_lizard_mode(steam, true); - mutex_unlock(&steam->mutex); } static enum power_supply_property steam_battery_props[] = { @@ -624,6 +649,7 @@ static int steam_register(struct steam_device *steam) { int ret; bool client_opened; + unsigned long flags; /* * This function can be called several times in a row with the @@ -636,11 +662,9 @@ static int steam_register(struct steam_device *steam) * Unlikely, but getting the serial could fail, and it is not so * important, so make up a serial number and go on. */ - mutex_lock(&steam->mutex); if (steam_get_serial(steam) < 0) strscpy(steam->serial_no, "XXXXXXXXXX", sizeof(steam->serial_no)); - mutex_unlock(&steam->mutex); hid_info(steam->hdev, "Steam Controller '%s' connected", steam->serial_no); @@ -655,15 +679,13 @@ static int steam_register(struct steam_device *steam) mutex_unlock(&steam_devices_lock); } - mutex_lock(&steam->mutex); + spin_lock_irqsave(&steam->lock, flags); client_opened = steam->client_opened; - if (!client_opened) + spin_unlock_irqrestore(&steam->lock, flags); + if (!client_opened) { steam_set_lizard_mode(steam, lizard_mode); - mutex_unlock(&steam->mutex); - - if (!client_opened) ret = steam_input_register(steam); - else + } else ret = 0; return ret; @@ -746,10 +768,11 @@ static void steam_client_ll_stop(struct hid_device *hdev) static int steam_client_ll_open(struct hid_device *hdev) { struct steam_device *steam = hdev->driver_data; + unsigned long flags; - mutex_lock(&steam->mutex); + spin_lock_irqsave(&steam->lock, flags); steam->client_opened = true; - mutex_unlock(&steam->mutex); + spin_unlock_irqrestore(&steam->lock, flags); steam_input_unregister(steam); @@ -764,17 +787,14 @@ static void steam_client_ll_close(struct hid_device *hdev) bool connected; spin_lock_irqsave(&steam->lock, flags); - connected = steam->connected; + steam->client_opened = false; + connected = steam->connected && !steam->client_opened; spin_unlock_irqrestore(&steam->lock, flags); - mutex_lock(&steam->mutex); - steam->client_opened = false; - if (connected) + if (connected) { steam_set_lizard_mode(steam, lizard_mode); - mutex_unlock(&steam->mutex); - - if (connected) steam_input_register(steam); + } } static int steam_client_ll_raw_request(struct hid_device *hdev, @@ -860,19 +880,12 @@ static int steam_probe(struct hid_device *hdev, steam->hdev = hdev; hid_set_drvdata(hdev, steam); spin_lock_init(&steam->lock); - mutex_init(&steam->mutex); + mutex_init(&steam->report_mutex); steam->quirks = id->driver_data; INIT_WORK(&steam->work_connect, steam_work_connect_cb); INIT_LIST_HEAD(&steam->list); INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb); - steam->client_hdev = steam_create_client_hid(hdev); - if (IS_ERR(steam->client_hdev)) { - ret = PTR_ERR(steam->client_hdev); - goto client_hdev_fail; - } - steam->client_hdev->driver_data = steam; - /* * With the real steam controller interface, do not connect hidraw. * Instead, create the client_hid and connect that. @@ -881,10 +894,6 @@ static int steam_probe(struct hid_device *hdev, if (ret) goto hid_hw_start_fail; - ret = hid_add_device(steam->client_hdev); - if (ret) - goto client_hdev_add_fail; - ret = hid_hw_open(hdev); if (ret) { hid_err(hdev, @@ -910,15 +919,26 @@ static int steam_probe(struct hid_device *hdev, } } + steam->client_hdev = steam_create_client_hid(hdev); + if (IS_ERR(steam->client_hdev)) { + ret = PTR_ERR(steam->client_hdev); + goto client_hdev_fail; + } + steam->client_hdev->driver_data = steam; + + ret = hid_add_device(steam->client_hdev); + if (ret) + goto client_hdev_add_fail; + return 0; -input_register_fail: -hid_hw_open_fail: client_hdev_add_fail: hid_hw_stop(hdev); -hid_hw_start_fail: - hid_destroy_device(steam->client_hdev); client_hdev_fail: + hid_destroy_device(steam->client_hdev); +input_register_fail: +hid_hw_open_fail: +hid_hw_start_fail: cancel_work_sync(&steam->work_connect); cancel_work_sync(&steam->rumble_work); steam_alloc_fail: @@ -936,12 +956,10 @@ static void steam_remove(struct hid_device *hdev) return; } + cancel_work_sync(&steam->work_connect); hid_destroy_device(steam->client_hdev); - mutex_lock(&steam->mutex); steam->client_hdev = NULL; steam->client_opened = false; - mutex_unlock(&steam->mutex); - cancel_work_sync(&steam->work_connect); if (steam->quirks & STEAM_QUIRK_WIRELESS) { hid_info(hdev, "Steam wireless receiver disconnected"); } @@ -1408,10 +1426,8 @@ static int steam_param_set_lizard_mode(const char *val, mutex_lock(&steam_devices_lock); list_for_each_entry(steam, &steam_devices, list) { - mutex_lock(&steam->mutex); if (!steam->client_opened) steam_set_lizard_mode(steam, lizard_mode); - mutex_unlock(&steam->mutex); } mutex_unlock(&steam_devices_lock); return 0; From 555b818adb97eca70210a49ba3f1d27882dde092 Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:34 -0800 Subject: [PATCH 42/53] HID: hid-steam: Make client_opened a counter The client_opened variable was used to track if the hidraw was opened by any clients to silence keyboard/mouse events while opened. However, there was no counting of how many clients were opened, so opening two at the same time and then closing one would fool the driver into thinking it had no remaining opened clients. Signed-off-by: Vicki Pfau Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index 57cb58941c9fc..667b5b09f9311 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -125,7 +125,7 @@ struct steam_device { spinlock_t lock; struct hid_device *hdev, *client_hdev; struct mutex report_mutex; - bool client_opened; + unsigned long client_opened; struct input_dev __rcu *input; unsigned long quirks; struct work_struct work_connect; @@ -648,7 +648,7 @@ static void steam_battery_unregister(struct steam_device *steam) static int steam_register(struct steam_device *steam) { int ret; - bool client_opened; + unsigned long client_opened; unsigned long flags; /* @@ -771,7 +771,7 @@ static int steam_client_ll_open(struct hid_device *hdev) unsigned long flags; spin_lock_irqsave(&steam->lock, flags); - steam->client_opened = true; + steam->client_opened++; spin_unlock_irqrestore(&steam->lock, flags); steam_input_unregister(steam); @@ -787,7 +787,7 @@ static void steam_client_ll_close(struct hid_device *hdev) bool connected; spin_lock_irqsave(&steam->lock, flags); - steam->client_opened = false; + steam->client_opened--; connected = steam->connected && !steam->client_opened; spin_unlock_irqrestore(&steam->lock, flags); @@ -959,7 +959,7 @@ static void steam_remove(struct hid_device *hdev) cancel_work_sync(&steam->work_connect); hid_destroy_device(steam->client_hdev); steam->client_hdev = NULL; - steam->client_opened = false; + steam->client_opened = 0; if (steam->quirks & STEAM_QUIRK_WIRELESS) { hid_info(hdev, "Steam wireless receiver disconnected"); } From 4f9a5a9769cc77075e606537e15747e8b8e9c7c9 Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:35 -0800 Subject: [PATCH 43/53] HID: hid-steam: Update list of identifiers from SDL SDL includes a list of settings (formerly called registers in this driver), reports (formerly cmds), and various other identifiers that were provided by Valve. This commit imports a significant chunk of that list as well as replacing most of the guessed names and a handful of magic constants. It also replaces bitmask definitions that used hex with the BIT macro. Signed-off-by: Vicki Pfau Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 286 +++++++++++++++++++++++++++++++--------- 1 file changed, 221 insertions(+), 65 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index 667b5b09f9311..4f5c647f04dd0 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -71,51 +71,207 @@ static LIST_HEAD(steam_devices); /* * Commands that can be sent in a feature report. - * Thanks to Valve for some valuable hints. + * Thanks to Valve and SDL for the names. */ -#define STEAM_CMD_SET_MAPPINGS 0x80 -#define STEAM_CMD_CLEAR_MAPPINGS 0x81 -#define STEAM_CMD_GET_MAPPINGS 0x82 -#define STEAM_CMD_GET_ATTRIB 0x83 -#define STEAM_CMD_GET_ATTRIB_LABEL 0x84 -#define STEAM_CMD_DEFAULT_MAPPINGS 0x85 -#define STEAM_CMD_FACTORY_RESET 0x86 -#define STEAM_CMD_WRITE_REGISTER 0x87 -#define STEAM_CMD_CLEAR_REGISTER 0x88 -#define STEAM_CMD_READ_REGISTER 0x89 -#define STEAM_CMD_GET_REGISTER_LABEL 0x8a -#define STEAM_CMD_GET_REGISTER_MAX 0x8b -#define STEAM_CMD_GET_REGISTER_DEFAULT 0x8c -#define STEAM_CMD_SET_MODE 0x8d -#define STEAM_CMD_DEFAULT_MOUSE 0x8e -#define STEAM_CMD_FORCEFEEDBAK 0x8f -#define STEAM_CMD_REQUEST_COMM_STATUS 0xb4 -#define STEAM_CMD_GET_SERIAL 0xae -#define STEAM_CMD_HAPTIC_RUMBLE 0xeb - -/* Some useful register ids */ -#define STEAM_REG_LPAD_MODE 0x07 -#define STEAM_REG_RPAD_MODE 0x08 -#define STEAM_REG_RPAD_MARGIN 0x18 -#define STEAM_REG_LED 0x2d -#define STEAM_REG_GYRO_MODE 0x30 -#define STEAM_REG_LPAD_CLICK_PRESSURE 0x34 -#define STEAM_REG_RPAD_CLICK_PRESSURE 0x35 -#define STEAM_REG_WATCHDOG_ENABLE 0x47 - -/* Raw event identifiers */ -#define STEAM_EV_INPUT_DATA 0x01 -#define STEAM_EV_CONNECT 0x03 -#define STEAM_EV_BATTERY 0x04 -#define STEAM_EV_DECK_INPUT_DATA 0x09 +enum { + ID_SET_DIGITAL_MAPPINGS = 0x80, + ID_CLEAR_DIGITAL_MAPPINGS = 0x81, + ID_GET_DIGITAL_MAPPINGS = 0x82, + ID_GET_ATTRIBUTES_VALUES = 0x83, + ID_GET_ATTRIBUTE_LABEL = 0x84, + ID_SET_DEFAULT_DIGITAL_MAPPINGS = 0x85, + ID_FACTORY_RESET = 0x86, + ID_SET_SETTINGS_VALUES = 0x87, + ID_CLEAR_SETTINGS_VALUES = 0x88, + ID_GET_SETTINGS_VALUES = 0x89, + ID_GET_SETTING_LABEL = 0x8A, + ID_GET_SETTINGS_MAXS = 0x8B, + ID_GET_SETTINGS_DEFAULTS = 0x8C, + ID_SET_CONTROLLER_MODE = 0x8D, + ID_LOAD_DEFAULT_SETTINGS = 0x8E, + ID_TRIGGER_HAPTIC_PULSE = 0x8F, + ID_TURN_OFF_CONTROLLER = 0x9F, + + ID_GET_DEVICE_INFO = 0xA1, + + ID_CALIBRATE_TRACKPADS = 0xA7, + ID_RESERVED_0 = 0xA8, + ID_SET_SERIAL_NUMBER = 0xA9, + ID_GET_TRACKPAD_CALIBRATION = 0xAA, + ID_GET_TRACKPAD_FACTORY_CALIBRATION = 0xAB, + ID_GET_TRACKPAD_RAW_DATA = 0xAC, + ID_ENABLE_PAIRING = 0xAD, + ID_GET_STRING_ATTRIBUTE = 0xAE, + ID_RADIO_ERASE_RECORDS = 0xAF, + ID_RADIO_WRITE_RECORD = 0xB0, + ID_SET_DONGLE_SETTING = 0xB1, + ID_DONGLE_DISCONNECT_DEVICE = 0xB2, + ID_DONGLE_COMMIT_DEVICE = 0xB3, + ID_DONGLE_GET_WIRELESS_STATE = 0xB4, + ID_CALIBRATE_GYRO = 0xB5, + ID_PLAY_AUDIO = 0xB6, + ID_AUDIO_UPDATE_START = 0xB7, + ID_AUDIO_UPDATE_DATA = 0xB8, + ID_AUDIO_UPDATE_COMPLETE = 0xB9, + ID_GET_CHIPID = 0xBA, + + ID_CALIBRATE_JOYSTICK = 0xBF, + ID_CALIBRATE_ANALOG_TRIGGERS = 0xC0, + ID_SET_AUDIO_MAPPING = 0xC1, + ID_CHECK_GYRO_FW_LOAD = 0xC2, + ID_CALIBRATE_ANALOG = 0xC3, + ID_DONGLE_GET_CONNECTED_SLOTS = 0xC4, + + ID_RESET_IMU = 0xCE, + + ID_TRIGGER_HAPTIC_CMD = 0xEA, + ID_TRIGGER_RUMBLE_CMD = 0xEB, +}; + +/* Settings IDs */ +enum { + /* 0 */ + SETTING_MOUSE_SENSITIVITY, + SETTING_MOUSE_ACCELERATION, + SETTING_TRACKBALL_ROTATION_ANGLE, + SETTING_HAPTIC_INTENSITY_UNUSED, + SETTING_LEFT_GAMEPAD_STICK_ENABLED, + SETTING_RIGHT_GAMEPAD_STICK_ENABLED, + SETTING_USB_DEBUG_MODE, + SETTING_LEFT_TRACKPAD_MODE, + SETTING_RIGHT_TRACKPAD_MODE, + SETTING_MOUSE_POINTER_ENABLED, + + /* 10 */ + SETTING_DPAD_DEADZONE, + SETTING_MINIMUM_MOMENTUM_VEL, + SETTING_MOMENTUM_DECAY_AMMOUNT, + SETTING_TRACKPAD_RELATIVE_MODE_TICKS_PER_PIXEL, + SETTING_HAPTIC_INCREMENT, + SETTING_DPAD_ANGLE_SIN, + SETTING_DPAD_ANGLE_COS, + SETTING_MOMENTUM_VERTICAL_DIVISOR, + SETTING_MOMENTUM_MAXIMUM_VELOCITY, + SETTING_TRACKPAD_Z_ON, + + /* 20 */ + SETTING_TRACKPAD_Z_OFF, + SETTING_SENSITIVY_SCALE_AMMOUNT, + SETTING_LEFT_TRACKPAD_SECONDARY_MODE, + SETTING_RIGHT_TRACKPAD_SECONDARY_MODE, + SETTING_SMOOTH_ABSOLUTE_MOUSE, + SETTING_STEAMBUTTON_POWEROFF_TIME, + SETTING_UNUSED_1, + SETTING_TRACKPAD_OUTER_RADIUS, + SETTING_TRACKPAD_Z_ON_LEFT, + SETTING_TRACKPAD_Z_OFF_LEFT, + + /* 30 */ + SETTING_TRACKPAD_OUTER_SPIN_VEL, + SETTING_TRACKPAD_OUTER_SPIN_RADIUS, + SETTING_TRACKPAD_OUTER_SPIN_HORIZONTAL_ONLY, + SETTING_TRACKPAD_RELATIVE_MODE_DEADZONE, + SETTING_TRACKPAD_RELATIVE_MODE_MAX_VEL, + SETTING_TRACKPAD_RELATIVE_MODE_INVERT_Y, + SETTING_TRACKPAD_DOUBLE_TAP_BEEP_ENABLED, + SETTING_TRACKPAD_DOUBLE_TAP_BEEP_PERIOD, + SETTING_TRACKPAD_DOUBLE_TAP_BEEP_COUNT, + SETTING_TRACKPAD_OUTER_RADIUS_RELEASE_ON_TRANSITION, + + /* 40 */ + SETTING_RADIAL_MODE_ANGLE, + SETTING_HAPTIC_INTENSITY_MOUSE_MODE, + SETTING_LEFT_DPAD_REQUIRES_CLICK, + SETTING_RIGHT_DPAD_REQUIRES_CLICK, + SETTING_LED_BASELINE_BRIGHTNESS, + SETTING_LED_USER_BRIGHTNESS, + SETTING_ENABLE_RAW_JOYSTICK, + SETTING_ENABLE_FAST_SCAN, + SETTING_IMU_MODE, + SETTING_WIRELESS_PACKET_VERSION, + + /* 50 */ + SETTING_SLEEP_INACTIVITY_TIMEOUT, + SETTING_TRACKPAD_NOISE_THRESHOLD, + SETTING_LEFT_TRACKPAD_CLICK_PRESSURE, + SETTING_RIGHT_TRACKPAD_CLICK_PRESSURE, + SETTING_LEFT_BUMPER_CLICK_PRESSURE, + SETTING_RIGHT_BUMPER_CLICK_PRESSURE, + SETTING_LEFT_GRIP_CLICK_PRESSURE, + SETTING_RIGHT_GRIP_CLICK_PRESSURE, + SETTING_LEFT_GRIP2_CLICK_PRESSURE, + SETTING_RIGHT_GRIP2_CLICK_PRESSURE, + + /* 60 */ + SETTING_PRESSURE_MODE, + SETTING_CONTROLLER_TEST_MODE, + SETTING_TRIGGER_MODE, + SETTING_TRACKPAD_Z_THRESHOLD, + SETTING_FRAME_RATE, + SETTING_TRACKPAD_FILT_CTRL, + SETTING_TRACKPAD_CLIP, + SETTING_DEBUG_OUTPUT_SELECT, + SETTING_TRIGGER_THRESHOLD_PERCENT, + SETTING_TRACKPAD_FREQUENCY_HOPPING, + + /* 70 */ + SETTING_HAPTICS_ENABLED, + SETTING_STEAM_WATCHDOG_ENABLE, + SETTING_TIMP_TOUCH_THRESHOLD_ON, + SETTING_TIMP_TOUCH_THRESHOLD_OFF, + SETTING_FREQ_HOPPING, + SETTING_TEST_CONTROL, + SETTING_HAPTIC_MASTER_GAIN_DB, + SETTING_THUMB_TOUCH_THRESH, + SETTING_DEVICE_POWER_STATUS, + SETTING_HAPTIC_INTENSITY, + + /* 80 */ + SETTING_STABILIZER_ENABLED, + SETTING_TIMP_MODE_MTE, +}; + +/* Input report identifiers */ +enum +{ + ID_CONTROLLER_STATE = 1, + ID_CONTROLLER_DEBUG = 2, + ID_CONTROLLER_WIRELESS = 3, + ID_CONTROLLER_STATUS = 4, + ID_CONTROLLER_DEBUG2 = 5, + ID_CONTROLLER_SECONDARY_STATE = 6, + ID_CONTROLLER_BLE_STATE = 7, + ID_CONTROLLER_DECK_STATE = 9 +}; + +/* String attribute idenitifiers */ +enum { + ATTRIB_STR_BOARD_SERIAL, + ATTRIB_STR_UNIT_SERIAL, +}; /* Values for GYRO_MODE (bitmask) */ -#define STEAM_GYRO_MODE_OFF 0x0000 -#define STEAM_GYRO_MODE_STEERING 0x0001 -#define STEAM_GYRO_MODE_TILT 0x0002 -#define STEAM_GYRO_MODE_SEND_ORIENTATION 0x0004 -#define STEAM_GYRO_MODE_SEND_RAW_ACCEL 0x0008 -#define STEAM_GYRO_MODE_SEND_RAW_GYRO 0x0010 +enum { + SETTING_GYRO_MODE_OFF = 0, + SETTING_GYRO_MODE_STEERING = BIT(0), + SETTING_GYRO_MODE_TILT = BIT(1), + SETTING_GYRO_MODE_SEND_ORIENTATION = BIT(2), + SETTING_GYRO_MODE_SEND_RAW_ACCEL = BIT(3), + SETTING_GYRO_MODE_SEND_RAW_GYRO = BIT(4), +}; + +/* Trackpad modes */ +enum { + TRACKPAD_ABSOLUTE_MOUSE, + TRACKPAD_RELATIVE_MOUSE, + TRACKPAD_DPAD_FOUR_WAY_DISCRETE, + TRACKPAD_DPAD_FOUR_WAY_OVERLAP, + TRACKPAD_DPAD_EIGHT_WAY, + TRACKPAD_RADIAL_MODE, + TRACKPAD_ABSOLUTE_DPAD, + TRACKPAD_NONE, + TRACKPAD_GESTURE_KEYBOARD, +}; /* Other random constants */ #define STEAM_SERIAL_LEN 10 @@ -226,13 +382,13 @@ static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd) return steam_send_report(steam, &cmd, 1); } -static int steam_write_registers(struct steam_device *steam, +static int steam_write_settings(struct steam_device *steam, /* u8 reg, u16 val */...) { /* Send: 0x87 len (reg valLo valHi)* */ u8 reg; u16 val; - u8 cmd[64] = {STEAM_CMD_WRITE_REGISTER, 0x00}; + u8 cmd[64] = {ID_SET_SETTINGS_VALUES, 0x00}; int ret; va_list args; @@ -268,7 +424,7 @@ static int steam_get_serial(struct steam_device *steam) * Recv: 0xae 0x15 0x01 serialnumber (10 chars) */ int ret = 0; - u8 cmd[] = {STEAM_CMD_GET_SERIAL, 0x15, 0x01}; + u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, 0x15, ATTRIB_STR_UNIT_SERIAL}; u8 reply[3 + STEAM_SERIAL_LEN + 1]; mutex_lock(&steam->report_mutex); @@ -278,7 +434,7 @@ static int steam_get_serial(struct steam_device *steam) ret = steam_recv_report(steam, reply, sizeof(reply)); if (ret < 0) goto out; - if (reply[0] != 0xae || reply[1] != 0x15 || reply[2] != 0x01) { + if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] != 0x15 || reply[2] != ATTRIB_STR_UNIT_SERIAL) { ret = -EIO; goto out; } @@ -298,7 +454,7 @@ static inline int steam_request_conn_status(struct steam_device *steam) { int ret; mutex_lock(&steam->report_mutex); - ret = steam_send_report_byte(steam, STEAM_CMD_REQUEST_COMM_STATUS); + ret = steam_send_report_byte(steam, ID_DONGLE_GET_WIRELESS_STATE); mutex_unlock(&steam->report_mutex); return ret; } @@ -308,7 +464,7 @@ static inline int steam_haptic_rumble(struct steam_device *steam, u8 left_gain, u8 right_gain) { int ret; - u8 report[11] = {STEAM_CMD_HAPTIC_RUMBLE, 9}; + u8 report[11] = {ID_TRIGGER_RUMBLE_CMD, 9}; report[3] = intensity & 0xFF; report[4] = intensity >> 8; @@ -351,28 +507,28 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable) if (enable) { mutex_lock(&steam->report_mutex); /* enable esc, enter, cursors */ - steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MAPPINGS); - /* enable mouse */ - steam_send_report_byte(steam, STEAM_CMD_DEFAULT_MOUSE); + steam_send_report_byte(steam, ID_SET_DEFAULT_DIGITAL_MAPPINGS); + /* reset settings */ + steam_send_report_byte(steam, ID_LOAD_DEFAULT_SETTINGS); mutex_unlock(&steam->report_mutex); } else { mutex_lock(&steam->report_mutex); /* disable esc, enter, cursor */ - steam_send_report_byte(steam, STEAM_CMD_CLEAR_MAPPINGS); + steam_send_report_byte(steam, ID_CLEAR_DIGITAL_MAPPINGS); if (steam->quirks & STEAM_QUIRK_DECK) { - steam_write_registers(steam, - STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */ - STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ - STEAM_REG_LPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */ - STEAM_REG_RPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */ - STEAM_REG_WATCHDOG_ENABLE, 0, /* disable watchdog that tests if Steam is active */ + steam_write_settings(steam, + SETTING_LEFT_TRACKPAD_MODE, TRACKPAD_NONE, /* disable mouse */ + SETTING_RIGHT_TRACKPAD_MODE, TRACKPAD_NONE, /* disable mouse */ + SETTING_LEFT_TRACKPAD_CLICK_PRESSURE, 0xFFFF, /* disable haptic click */ + SETTING_RIGHT_TRACKPAD_CLICK_PRESSURE, 0xFFFF, /* disable haptic click */ + SETTING_STEAM_WATCHDOG_ENABLE, 0, /* disable watchdog that tests if Steam is active */ 0); mutex_unlock(&steam->report_mutex); } else { - steam_write_registers(steam, - STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */ - STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */ + steam_write_settings(steam, + SETTING_LEFT_TRACKPAD_MODE, TRACKPAD_NONE, /* disable mouse */ + SETTING_RIGHT_TRACKPAD_MODE, TRACKPAD_NONE, /* disable mouse */ 0); mutex_unlock(&steam->report_mutex); } @@ -1362,7 +1518,7 @@ static int steam_raw_event(struct hid_device *hdev, return 0; switch (data[2]) { - case STEAM_EV_INPUT_DATA: + case ID_CONTROLLER_STATE: if (steam->client_opened) return 0; rcu_read_lock(); @@ -1371,7 +1527,7 @@ static int steam_raw_event(struct hid_device *hdev, steam_do_input_event(steam, input, data); rcu_read_unlock(); break; - case STEAM_EV_DECK_INPUT_DATA: + case ID_CONTROLLER_DECK_STATE: if (steam->client_opened) return 0; rcu_read_lock(); @@ -1380,7 +1536,7 @@ static int steam_raw_event(struct hid_device *hdev, steam_do_deck_input_event(steam, input, data); rcu_read_unlock(); break; - case STEAM_EV_CONNECT: + case ID_CONTROLLER_WIRELESS: /* * The payload of this event is a single byte: * 0x01: disconnected. @@ -1395,7 +1551,7 @@ static int steam_raw_event(struct hid_device *hdev, break; } break; - case STEAM_EV_BATTERY: + case ID_CONTROLLER_STATUS: if (steam->quirks & STEAM_QUIRK_WIRELESS) { rcu_read_lock(); battery = rcu_dereference(steam->battery); From 43565b6788d46820da7d8f5ab1a595398419e914 Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:36 -0800 Subject: [PATCH 44/53] HID: hid-steam: Better handling of serial number length The second byte of the GET_STRING_ATTRIB report is a length, so we should set the size of the buffer to be the size we're actually requesting, and only reject the reply if the length out is nonsensical. Signed-off-by: Vicki Pfau Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index 4f5c647f04dd0..a0ed8812e7ead 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -274,7 +274,7 @@ enum { }; /* Other random constants */ -#define STEAM_SERIAL_LEN 10 +#define STEAM_SERIAL_LEN 0x15 struct steam_device { struct list_head list; @@ -421,10 +421,10 @@ static int steam_get_serial(struct steam_device *steam) { /* * Send: 0xae 0x15 0x01 - * Recv: 0xae 0x15 0x01 serialnumber (10 chars) + * Recv: 0xae 0x15 0x01 serialnumber */ int ret = 0; - u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, 0x15, ATTRIB_STR_UNIT_SERIAL}; + u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL}; u8 reply[3 + STEAM_SERIAL_LEN + 1]; mutex_lock(&steam->report_mutex); @@ -434,12 +434,13 @@ static int steam_get_serial(struct steam_device *steam) ret = steam_recv_report(steam, reply, sizeof(reply)); if (ret < 0) goto out; - if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] != 0x15 || reply[2] != ATTRIB_STR_UNIT_SERIAL) { + if (reply[0] != ID_GET_STRING_ATTRIBUTE || reply[1] < 1 || + reply[1] > sizeof(steam->serial_no) || reply[2] != ATTRIB_STR_UNIT_SERIAL) { ret = -EIO; goto out; } reply[3 + STEAM_SERIAL_LEN] = 0; - strscpy(steam->serial_no, reply + 3, sizeof(steam->serial_no)); + strscpy(steam->serial_no, reply + 3, reply[1]); out: mutex_unlock(&steam->report_mutex); return ret; From cd438e57dd05b077f4e87c1567beafb2377b6d6b Mon Sep 17 00:00:00 2001 From: Vicki Pfau Date: Tue, 19 Dec 2023 19:38:37 -0800 Subject: [PATCH 45/53] HID: hid-steam: Add gamepad-only mode switched to by holding options This commit adds a hotkey to switch between "gamepad" mode (mouse and keyboard disabled) and "desktop" mode (gamepad disabled) by holding down the options button (mapped here as the start button). This mirrors the behavior of the official Steam client. This also adds and uses a function for generating haptic pulses, as Steam also does when engaging this hotkey. Signed-off-by: Vicki Pfau Signed-off-by: Jiri Kosina --- drivers/hid/hid-steam.c | 113 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 103 insertions(+), 10 deletions(-) diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c index a0ed8812e7ead..b3c4e50e248aa 100644 --- a/drivers/hid/hid-steam.c +++ b/drivers/hid/hid-steam.c @@ -273,6 +273,11 @@ enum { TRACKPAD_GESTURE_KEYBOARD, }; +/* Pad identifiers for the deck */ +#define STEAM_PAD_LEFT 0 +#define STEAM_PAD_RIGHT 1 +#define STEAM_PAD_BOTH 2 + /* Other random constants */ #define STEAM_SERIAL_LEN 0x15 @@ -291,6 +296,9 @@ struct steam_device { struct power_supply __rcu *battery; u8 battery_charge; u16 voltage; + struct delayed_work mode_switch; + bool did_mode_switch; + bool gamepad_mode; struct work_struct rumble_work; u16 rumble_left; u16 rumble_right; @@ -460,6 +468,37 @@ static inline int steam_request_conn_status(struct steam_device *steam) return ret; } +/* + * Send a haptic pulse to the trackpads + * Duration and interval are measured in microseconds, count is the number + * of pulses to send for duration time with interval microseconds between them + * and gain is measured in decibels, ranging from -24 to +6 + */ +static inline int steam_haptic_pulse(struct steam_device *steam, u8 pad, + u16 duration, u16 interval, u16 count, u8 gain) +{ + int ret; + u8 report[10] = {ID_TRIGGER_HAPTIC_PULSE, 8}; + + /* Left and right are swapped on this report for legacy reasons */ + if (pad < STEAM_PAD_BOTH) + pad ^= 1; + + report[2] = pad; + report[3] = duration & 0xFF; + report[4] = duration >> 8; + report[5] = interval & 0xFF; + report[6] = interval >> 8; + report[7] = count & 0xFF; + report[8] = count >> 8; + report[9] = gain; + + mutex_lock(&steam->report_mutex); + ret = steam_send_report(steam, report, sizeof(report)); + mutex_unlock(&steam->report_mutex); + return ret; +} + static inline int steam_haptic_rumble(struct steam_device *steam, u16 intensity, u16 left_speed, u16 right_speed, u8 left_gain, u8 right_gain) @@ -505,6 +544,9 @@ static int steam_play_effect(struct input_dev *dev, void *data, static void steam_set_lizard_mode(struct steam_device *steam, bool enable) { + if (steam->gamepad_mode) + enable = false; + if (enable) { mutex_lock(&steam->report_mutex); /* enable esc, enter, cursors */ @@ -542,11 +584,18 @@ static int steam_input_open(struct input_dev *dev) unsigned long flags; bool set_lizard_mode; - spin_lock_irqsave(&steam->lock, flags); - set_lizard_mode = !steam->client_opened && lizard_mode; - spin_unlock_irqrestore(&steam->lock, flags); - if (set_lizard_mode) - steam_set_lizard_mode(steam, false); + /* + * Disabling lizard mode automatically is only done on the Steam + * Controller. On the Steam Deck, this is toggled manually by holding + * the options button instead, handled by steam_mode_switch_cb. + */ + if (!(steam->quirks & STEAM_QUIRK_DECK)) { + spin_lock_irqsave(&steam->lock, flags); + set_lizard_mode = !steam->client_opened && lizard_mode; + spin_unlock_irqrestore(&steam->lock, flags); + if (set_lizard_mode) + steam_set_lizard_mode(steam, false); + } return 0; } @@ -557,11 +606,13 @@ static void steam_input_close(struct input_dev *dev) unsigned long flags; bool set_lizard_mode; - spin_lock_irqsave(&steam->lock, flags); - set_lizard_mode = !steam->client_opened && lizard_mode; - spin_unlock_irqrestore(&steam->lock, flags); - if (set_lizard_mode) - steam_set_lizard_mode(steam, true); + if (!(steam->quirks & STEAM_QUIRK_DECK)) { + spin_lock_irqsave(&steam->lock, flags); + set_lizard_mode = !steam->client_opened && lizard_mode; + spin_unlock_irqrestore(&steam->lock, flags); + if (set_lizard_mode) + steam_set_lizard_mode(steam, true); + } } static enum power_supply_property steam_battery_props[] = { @@ -886,6 +937,34 @@ static void steam_work_connect_cb(struct work_struct *work) } } +static void steam_mode_switch_cb(struct work_struct *work) +{ + struct steam_device *steam = container_of(to_delayed_work(work), + struct steam_device, mode_switch); + unsigned long flags; + bool client_opened; + steam->gamepad_mode = !steam->gamepad_mode; + if (!lizard_mode) + return; + + if (steam->gamepad_mode) + steam_set_lizard_mode(steam, false); + else { + spin_lock_irqsave(&steam->lock, flags); + client_opened = steam->client_opened; + spin_unlock_irqrestore(&steam->lock, flags); + if (!client_opened) + steam_set_lizard_mode(steam, lizard_mode); + } + + steam_haptic_pulse(steam, STEAM_PAD_RIGHT, 0x190, 0, 1, 0); + if (steam->gamepad_mode) { + steam_haptic_pulse(steam, STEAM_PAD_LEFT, 0x14D, 0x14D, 0x2D, 0); + } else { + steam_haptic_pulse(steam, STEAM_PAD_LEFT, 0x1F4, 0x1F4, 0x1E, 0); + } +} + static bool steam_is_valve_interface(struct hid_device *hdev) { struct hid_report_enum *rep_enum; @@ -1040,6 +1119,7 @@ static int steam_probe(struct hid_device *hdev, mutex_init(&steam->report_mutex); steam->quirks = id->driver_data; INIT_WORK(&steam->work_connect, steam_work_connect_cb); + INIT_DELAYED_WORK(&steam->mode_switch, steam_mode_switch_cb); INIT_LIST_HEAD(&steam->list); INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb); @@ -1097,6 +1177,7 @@ static int steam_probe(struct hid_device *hdev, hid_hw_open_fail: hid_hw_start_fail: cancel_work_sync(&steam->work_connect); + cancel_delayed_work_sync(&steam->mode_switch); cancel_work_sync(&steam->rumble_work); steam_alloc_fail: hid_err(hdev, "%s: failed with error %d\n", @@ -1113,6 +1194,7 @@ static void steam_remove(struct hid_device *hdev) return; } + cancel_delayed_work_sync(&steam->mode_switch); cancel_work_sync(&steam->work_connect); hid_destroy_device(steam->client_hdev); steam->client_hdev = NULL; @@ -1398,6 +1480,17 @@ static void steam_do_deck_input_event(struct steam_device *steam, b13 = data[13]; b14 = data[14]; + if (!(b9 & BIT(6)) && steam->did_mode_switch) { + steam->did_mode_switch = false; + cancel_delayed_work_sync(&steam->mode_switch); + } else if (!steam->client_opened && (b9 & BIT(6)) && !steam->did_mode_switch) { + steam->did_mode_switch = true; + schedule_delayed_work(&steam->mode_switch, 45 * HZ / 100); + } + + if (!steam->gamepad_mode) + return; + lpad_touched = b10 & BIT(3); rpad_touched = b10 & BIT(4); From 37d158d0b05144f696323ae5bbfe1e137f7c06d3 Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Wed, 20 Dec 2023 08:38:46 +0100 Subject: [PATCH 46/53] HID: make hid_bus_type const Now that the driver core can properly handle constant struct bus_type, move the hid_bus_type variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Jiri Kosina Cc: Benjamin Tissoires Cc: linux-input@vger.kernel.org Signed-off-by: Greg Kroah-Hartman Signed-off-by: Jiri Kosina --- drivers/hid/hid-core.c | 2 +- include/linux/hid.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index e0181218ad857..de7a477d66656 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2749,7 +2749,7 @@ static int hid_uevent(const struct device *dev, struct kobj_uevent_env *env) return 0; } -struct bus_type hid_bus_type = { +const struct bus_type hid_bus_type = { .name = "hid", .dev_groups = hid_dev_groups, .drv_groups = hid_drv_groups, diff --git a/include/linux/hid.h b/include/linux/hid.h index bf43f3ff66640..7c26db874ff03 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -912,7 +912,7 @@ extern bool hid_ignore(struct hid_device *); extern int hid_add_device(struct hid_device *); extern void hid_destroy_device(struct hid_device *); -extern struct bus_type hid_bus_type; +extern const struct bus_type hid_bus_type; extern int __must_check __hid_register_driver(struct hid_driver *, struct module *, const char *mod_name); From c4a9743699f3b093bad4bcc472c4ee34c7929f33 Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Wed, 20 Dec 2023 08:38:47 +0100 Subject: [PATCH 47/53] HID: make ishtp_cl_bus_type const Now that the driver core can properly handle constant struct bus_type, move the ishtp_cl_bus_type variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Srinivas Pandruvada Cc: Jiri Kosina Cc: Benjamin Tissoires Cc: linux-input@vger.kernel.org Signed-off-by: Greg Kroah-Hartman Acked-by: Srinivas Pandruvada Signed-off-by: Jiri Kosina --- drivers/hid/intel-ish-hid/ishtp/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c index 7fc738a223755..aa6cb033bb06b 100644 --- a/drivers/hid/intel-ish-hid/ishtp/bus.c +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c @@ -378,7 +378,7 @@ static const struct dev_pm_ops ishtp_cl_bus_dev_pm_ops = { .restore = ishtp_cl_device_resume, }; -static struct bus_type ishtp_cl_bus_type = { +static const struct bus_type ishtp_cl_bus_type = { .name = "ishtp", .dev_groups = ishtp_cl_dev_groups, .probe = ishtp_cl_device_probe, From 9b0a3839e8d29663cd9ee2c43d38b06c3b91619e Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Wed, 20 Dec 2023 08:38:48 +0100 Subject: [PATCH 48/53] HID: bpf: make bus_type const in struct hid_bpf_ops The struct bus_type pointer in hid_bpf_ops just passes the pointer to the driver core, and the driver core can handle, and expects, a constant pointer, so also make the pointer constant in hid_bpf_ops. Part of the process of moving all usages of struct bus_type to be constant to move them all to read-only memory. Cc: Jiri Kosina Cc: Benjamin Tissoires Cc: linux-input@vger.kernel.org Signed-off-by: Greg Kroah-Hartman Signed-off-by: Jiri Kosina --- include/linux/hid_bpf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h index e9afb61e6ee01..840cd254172d0 100644 --- a/include/linux/hid_bpf.h +++ b/include/linux/hid_bpf.h @@ -115,7 +115,7 @@ struct hid_bpf_ops { size_t len, enum hid_report_type rtype, enum hid_class_request reqtype); struct module *owner; - struct bus_type *bus_type; + const struct bus_type *bus_type; }; extern struct hid_bpf_ops *hid_bpf_ops; From d74ac6f60a7ef677c3b7702f103b670142f84d66 Mon Sep 17 00:00:00 2001 From: Zhengqiao Xia Date: Wed, 27 Dec 2023 16:50:12 +0800 Subject: [PATCH 49/53] dt-bindings: HID: i2c-hid: elan: Introduce Ilitek ili2901 The Ilitek ili2901 touch screen chip same as Elan eKTH6915 controller has a reset gpio. The difference is that they have different post_power_delay_ms and post_gpio_reset_on_delay_ms. Ilitek ili2901 also uses 3.3V power supply. Signed-off-by: Zhengqiao Xia Acked-by: Krzysztof Kozlowski Signed-off-by: Jiri Kosina --- Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml index 3e2d216c6432b..dc4ac41f24411 100644 --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml @@ -18,8 +18,9 @@ allOf: properties: compatible: - items: - - const: elan,ekth6915 + enum: + - elan,ekth6915 + - ilitek,ili2901 reg: const: 0x10 From 03ddb7de012c68d727509c4f6e0c0fa1ebc81668 Mon Sep 17 00:00:00 2001 From: Zhengqiao Xia Date: Wed, 27 Dec 2023 16:50:13 +0800 Subject: [PATCH 50/53] HID: i2c-hid: elan: Add ili2901 timing ILI2901 requires reset to pull down time greater than 10ms, so the configuration post_power_delay_ms is 10, and the chipset initial time is required to be greater than 100ms, so the post_gpio_reset_on_delay_ms is set to 100. Reviewed-by: Linus Walleij Signed-off-by: Zhengqiao Xia Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-of-elan.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c index 31abab57ad443..5b91fb106cfc3 100644 --- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c +++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c @@ -130,9 +130,17 @@ static const struct elan_i2c_hid_chip_data ilitek_ili9882t_chip_data = { .main_supply_name = NULL, }; +static const struct elan_i2c_hid_chip_data ilitek_ili2901_chip_data = { + .post_power_delay_ms = 10, + .post_gpio_reset_on_delay_ms = 100, + .hid_descriptor_address = 0x0001, + .main_supply_name = "vcc33", +}; + static const struct of_device_id elan_i2c_hid_of_match[] = { { .compatible = "elan,ekth6915", .data = &elan_ekth6915_chip_data }, { .compatible = "ilitek,ili9882t", .data = &ilitek_ili9882t_chip_data }, + { .compatible = "ilitek,ili2901", .data = &ilitek_ili2901_chip_data }, { } }; MODULE_DEVICE_TABLE(of, elan_i2c_hid_of_match); From 4e71d262899d7bab1f0c65936a2e639afeb83e4d Mon Sep 17 00:00:00 2001 From: Basavaraj Natikar Date: Wed, 20 Dec 2023 12:30:40 +0530 Subject: [PATCH 51/53] HID: amd_sfh: rename float_to_int() to amd_sfh_float_to_int() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Current amd_sfh driver has float_to_int() to convert units from float to int. This is fine until this function gets called outside of the current scope of file. Add a prefix "amd_sfh" to float_to_int() so that function represents the driver name. This function will be called by multiple callers in the next patch. Link: https://lore.kernel.org/all/ad064333-48a4-4cfa-9428-69e8a7c44667@redhat.com/ Reviewed-by: Ilpo Järvinen Co-developed-by: Shyam Sundar S K Signed-off-by: Shyam Sundar S K Signed-off-by: Basavaraj Natikar Signed-off-by: Jiri Kosina --- drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 28 ++++++++++--------- .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 1 + 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c index 8a037de08e924..33fbdde8aff00 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c @@ -132,7 +132,7 @@ static void get_common_inputs(struct common_input_property *common, int report_i common->event_type = HID_USAGE_SENSOR_EVENT_DATA_UPDATED_ENUM; } -static int float_to_int(u32 flt32_val) +int amd_sfh_float_to_int(u32 flt32_val) { int fraction, shift, mantissa, sign, exp, zeropre; @@ -201,9 +201,9 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id, OFFSET_SENSOR_DATA_DEFAULT; memcpy_fromio(&accel_data, sensoraddr, sizeof(struct sfh_accel_data)); get_common_inputs(&acc_input.common_property, report_id); - acc_input.in_accel_x_value = float_to_int(accel_data.acceldata.x) / 100; - acc_input.in_accel_y_value = float_to_int(accel_data.acceldata.y) / 100; - acc_input.in_accel_z_value = float_to_int(accel_data.acceldata.z) / 100; + acc_input.in_accel_x_value = amd_sfh_float_to_int(accel_data.acceldata.x) / 100; + acc_input.in_accel_y_value = amd_sfh_float_to_int(accel_data.acceldata.y) / 100; + acc_input.in_accel_z_value = amd_sfh_float_to_int(accel_data.acceldata.z) / 100; memcpy(input_report, &acc_input, sizeof(acc_input)); report_size = sizeof(acc_input); break; @@ -212,9 +212,9 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id, OFFSET_SENSOR_DATA_DEFAULT; memcpy_fromio(&gyro_data, sensoraddr, sizeof(struct sfh_gyro_data)); get_common_inputs(&gyro_input.common_property, report_id); - gyro_input.in_angel_x_value = float_to_int(gyro_data.gyrodata.x) / 1000; - gyro_input.in_angel_y_value = float_to_int(gyro_data.gyrodata.y) / 1000; - gyro_input.in_angel_z_value = float_to_int(gyro_data.gyrodata.z) / 1000; + gyro_input.in_angel_x_value = amd_sfh_float_to_int(gyro_data.gyrodata.x) / 1000; + gyro_input.in_angel_y_value = amd_sfh_float_to_int(gyro_data.gyrodata.y) / 1000; + gyro_input.in_angel_z_value = amd_sfh_float_to_int(gyro_data.gyrodata.z) / 1000; memcpy(input_report, &gyro_input, sizeof(gyro_input)); report_size = sizeof(gyro_input); break; @@ -223,9 +223,9 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id, OFFSET_SENSOR_DATA_DEFAULT; memcpy_fromio(&mag_data, sensoraddr, sizeof(struct sfh_mag_data)); get_common_inputs(&magno_input.common_property, report_id); - magno_input.in_magno_x = float_to_int(mag_data.magdata.x) / 100; - magno_input.in_magno_y = float_to_int(mag_data.magdata.y) / 100; - magno_input.in_magno_z = float_to_int(mag_data.magdata.z) / 100; + magno_input.in_magno_x = amd_sfh_float_to_int(mag_data.magdata.x) / 100; + magno_input.in_magno_y = amd_sfh_float_to_int(mag_data.magdata.y) / 100; + magno_input.in_magno_z = amd_sfh_float_to_int(mag_data.magdata.z) / 100; magno_input.in_magno_accuracy = mag_data.accuracy / 100; memcpy(input_report, &magno_input, sizeof(magno_input)); report_size = sizeof(magno_input); @@ -235,13 +235,15 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id, OFFSET_SENSOR_DATA_DEFAULT; memcpy_fromio(&als_data, sensoraddr, sizeof(struct sfh_als_data)); get_common_inputs(&als_input.common_property, report_id); - als_input.illuminance_value = float_to_int(als_data.lux); + als_input.illuminance_value = amd_sfh_float_to_int(als_data.lux); memcpy_fromio(&binfo, mp2->vsbase, sizeof(struct sfh_base_info)); if (binfo.sbase.s_prop[ALS_IDX].sf.feat & 0x2) { als_input.light_color_temp = als_data.light_color_temp; - als_input.chromaticity_x_value = float_to_int(als_data.chromaticity_x); - als_input.chromaticity_y_value = float_to_int(als_data.chromaticity_y); + als_input.chromaticity_x_value = + amd_sfh_float_to_int(als_data.chromaticity_x); + als_input.chromaticity_y_value = + amd_sfh_float_to_int(als_data.chromaticity_y); } report_size = sizeof(als_input); diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h index 656c3e95ef8c3..75267b0fec707 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h @@ -166,4 +166,5 @@ struct hpd_status { void sfh_interface_init(struct amd_mp2_dev *mp2); void amd_sfh1_1_set_desc_ops(struct amd_mp2_ops *mp2_ops); +int amd_sfh_float_to_int(u32 flt32_val); #endif From b5b0774d53bb81bddbf8c609b3f183d4af6e91da Mon Sep 17 00:00:00 2001 From: Basavaraj Natikar Date: Wed, 20 Dec 2023 12:30:41 +0530 Subject: [PATCH 52/53] HID: amd_sfh: Add a new interface for exporting HPD data AMDSFH has information about the User presence information via the Human Presence Detection (HPD) sensor which is part of the AMD sensor fusion hub. Add a new interface to export this information, where other drivers like PMF can use this information to enhance user experiences. Link: https://lore.kernel.org/all/ad064333-48a4-4cfa-9428-69e8a7c44667@redhat.com/ Co-developed-by: Shyam Sundar S K Signed-off-by: Shyam Sundar S K Signed-off-by: Basavaraj Natikar Signed-off-by: Jiri Kosina --- drivers/hid/amd-sfh-hid/amd_sfh_common.h | 5 ++ drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 14 ++++++ .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 37 +++++++++++++++ .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 1 + include/linux/amd-pmf-io.h | 46 +++++++++++++++++++ 5 files changed, 103 insertions(+) create mode 100644 include/linux/amd-pmf-io.h diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h index 2643bb14fee27..cd57037bf2174 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h @@ -37,6 +37,10 @@ struct amd_mp2_sensor_info { dma_addr_t dma_address; }; +struct sfh_dev_status { + bool is_hpd_present; +}; + struct amd_mp2_dev { struct pci_dev *pdev; struct amdtp_cl_data *cl_data; @@ -47,6 +51,7 @@ struct amd_mp2_dev { struct amd_input_data in_data; /* mp2 active control status */ u32 mp2_acs; + struct sfh_dev_status dev_en; }; struct amd_mp2_ops { diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c index e9c6413af24a0..0351b0fd394a9 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c @@ -73,6 +73,12 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata) int i, status; for (i = 0; i < cl_data->num_hid_devices; i++) { + switch (cl_data->sensor_idx[i]) { + case HPD_IDX: + privdata->dev_en.is_hpd_present = false; + break; + } + if (cl_data->sensor_sts[i] == SENSOR_ENABLED) { privdata->mp2_ops->stop(privdata, cl_data->sensor_idx[i]); status = amd_sfh_wait_for_response @@ -178,6 +184,11 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata) rc = amdtp_hid_probe(i, cl_data); if (rc) goto cleanup; + switch (cl_data->sensor_idx[i]) { + case HPD_IDX: + privdata->dev_en.is_hpd_present = true; + break; + } } dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n", cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]), @@ -259,6 +270,7 @@ static void amd_mp2_pci_remove(void *privdata) { struct amd_mp2_dev *mp2 = privdata; + sfh_deinit_emp2(); amd_sfh_hid_client_deinit(privdata); mp2->mp2_ops->stop_all(mp2); pci_intx(mp2->pdev, false); @@ -311,12 +323,14 @@ int amd_sfh1_1_init(struct amd_mp2_dev *mp2) rc = amd_sfh_irq_init(mp2); if (rc) { + sfh_deinit_emp2(); dev_err(dev, "amd_sfh_irq_init failed\n"); return rc; } rc = amd_sfh1_1_hid_client_init(mp2); if (rc) { + sfh_deinit_emp2(); dev_err(dev, "amd_sfh1_1_hid_client_init failed\n"); return rc; } diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c index 4f81ef2d4f56e..197b828fc6a0b 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c @@ -7,11 +7,14 @@ * * Author: Basavaraj Natikar */ +#include #include #include #include "amd_sfh_interface.h" +static struct amd_mp2_dev *emp2; + static int amd_sfh_wait_response(struct amd_mp2_dev *mp2, u8 sid, u32 cmd_id) { struct sfh_cmd_response cmd_resp; @@ -73,7 +76,41 @@ static struct amd_mp2_ops amd_sfh_ops = { .response = amd_sfh_wait_response, }; +void sfh_deinit_emp2(void) +{ + emp2 = NULL; +} + void sfh_interface_init(struct amd_mp2_dev *mp2) { mp2->mp2_ops = &amd_sfh_ops; + emp2 = mp2; +} + +static int amd_sfh_hpd_info(u8 *user_present) +{ + struct hpd_status hpdstatus; + + if (!user_present) + return -EINVAL; + + if (!emp2 || !emp2->dev_en.is_hpd_present) + return -ENODEV; + + hpdstatus.val = readl(emp2->mmio + AMD_C2P_MSG(4)); + *user_present = hpdstatus.shpd.presence; + + return 0; +} + +int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op) +{ + if (sfh_info) { + switch (op) { + case MT_HPD: + return amd_sfh_hpd_info(&sfh_info->user_present); + } + } + return -EINVAL; } +EXPORT_SYMBOL_GPL(amd_get_sfh_info); diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h index 75267b0fec707..2c211d28764da 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h @@ -165,6 +165,7 @@ struct hpd_status { }; void sfh_interface_init(struct amd_mp2_dev *mp2); +void sfh_deinit_emp2(void); void amd_sfh1_1_set_desc_ops(struct amd_mp2_ops *mp2_ops); int amd_sfh_float_to_int(u32 flt32_val); #endif diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h new file mode 100644 index 0000000000000..5b6d29d369221 --- /dev/null +++ b/include/linux/amd-pmf-io.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * AMD Platform Management Framework Interface + * + * Copyright (c) 2023, Advanced Micro Devices, Inc. + * All Rights Reserved. + * + * Authors: Shyam Sundar S K + * Basavaraj Natikar + */ + +#ifndef AMD_PMF_IO_H +#define AMD_PMF_IO_H + +#include + +/** + * enum sfh_message_type - Query the SFH message type + * @MT_HPD: Message ID to know the Human presence info from MP2 FW + */ +enum sfh_message_type { + MT_HPD, +}; + +/** + * enum sfh_hpd_info - Query the Human presence information + * @SFH_NOT_DETECTED: Check the HPD connection information from MP2 FW + * @SFH_USER_PRESENT: Check if the user is present from HPD sensor + * @SFH_USER_AWAY: Check if the user is away from HPD sensor + */ +enum sfh_hpd_info { + SFH_NOT_DETECTED, + SFH_USER_PRESENT, + SFH_USER_AWAY, +}; + +/** + * struct amd_sfh_info - get HPD sensor info from MP2 FW + * @user_present: Populates the user presence information + */ +struct amd_sfh_info { + u8 user_present; +}; + +int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op); +#endif From 584f35a3647d42980af495fc0bc5c51eb174aa35 Mon Sep 17 00:00:00 2001 From: Basavaraj Natikar Date: Wed, 20 Dec 2023 12:30:42 +0530 Subject: [PATCH 53/53] HID: amd_sfh: Add a new interface for exporting ALS data AMDSFH has information about the Ambient light via the Ambient Light Sensor (ALS) which is part of the AMD sensor fusion hub. Add a new interface to export this information, where other drivers like PMF can use this information to enhance user experiences. Link: https://lore.kernel.org/all/ad064333-48a4-4cfa-9428-69e8a7c44667@redhat.com/ Reviewed-by: Mario Limonciello Co-developed-by: Shyam Sundar S K Signed-off-by: Shyam Sundar S K Signed-off-by: Basavaraj Natikar Signed-off-by: Jiri Kosina --- drivers/hid/amd-sfh-hid/amd_sfh_common.h | 1 + drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 6 +++++ .../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 22 +++++++++++++++++++ include/linux/amd-pmf-io.h | 4 ++++ 4 files changed, 33 insertions(+) diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h index cd57037bf2174..a1950bc6e6cef 100644 --- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h +++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h @@ -39,6 +39,7 @@ struct amd_mp2_sensor_info { struct sfh_dev_status { bool is_hpd_present; + bool is_als_present; }; struct amd_mp2_dev { diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c index 0351b0fd394a9..9dbe6f4cb2942 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c @@ -77,6 +77,9 @@ static int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata) case HPD_IDX: privdata->dev_en.is_hpd_present = false; break; + case ALS_IDX: + privdata->dev_en.is_als_present = false; + break; } if (cl_data->sensor_sts[i] == SENSOR_ENABLED) { @@ -188,6 +191,9 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata) case HPD_IDX: privdata->dev_en.is_hpd_present = true; break; + case ALS_IDX: + privdata->dev_en.is_als_present = true; + break; } } dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n", diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c index 197b828fc6a0b..ae36312bc2365 100644 --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c @@ -103,12 +103,34 @@ static int amd_sfh_hpd_info(u8 *user_present) return 0; } +static int amd_sfh_als_info(u32 *ambient_light) +{ + struct sfh_als_data als_data; + void __iomem *sensoraddr; + + if (!ambient_light) + return -EINVAL; + + if (!emp2 || !emp2->dev_en.is_als_present) + return -ENODEV; + + sensoraddr = emp2->vsbase + + (ALS_IDX * SENSOR_DATA_MEM_SIZE_DEFAULT) + + OFFSET_SENSOR_DATA_DEFAULT; + memcpy_fromio(&als_data, sensoraddr, sizeof(struct sfh_als_data)); + *ambient_light = amd_sfh_float_to_int(als_data.lux); + + return 0; +} + int amd_get_sfh_info(struct amd_sfh_info *sfh_info, enum sfh_message_type op) { if (sfh_info) { switch (op) { case MT_HPD: return amd_sfh_hpd_info(&sfh_info->user_present); + case MT_ALS: + return amd_sfh_als_info(&sfh_info->ambient_light); } } return -EINVAL; diff --git a/include/linux/amd-pmf-io.h b/include/linux/amd-pmf-io.h index 5b6d29d369221..b4f8182052169 100644 --- a/include/linux/amd-pmf-io.h +++ b/include/linux/amd-pmf-io.h @@ -17,9 +17,11 @@ /** * enum sfh_message_type - Query the SFH message type * @MT_HPD: Message ID to know the Human presence info from MP2 FW + * @MT_ALS: Message ID to know the Ambient light info from MP2 FW */ enum sfh_message_type { MT_HPD, + MT_ALS, }; /** @@ -36,9 +38,11 @@ enum sfh_hpd_info { /** * struct amd_sfh_info - get HPD sensor info from MP2 FW + * @ambient_light: Populates the ambient light information * @user_present: Populates the user presence information */ struct amd_sfh_info { + u32 ambient_light; u8 user_present; };