* Re: [PATCH] HID:i2c-hid:goodix:Modify post_power_delay_ms to avoid touchscreen not working
From: Doug Anderson @ 2023-11-06 16:40 UTC (permalink / raw)
To: xiazhengqiao
Cc: jikos, benjamin.tissoires, mka, fshao, linux-input, linux-kernel,
xuxinxiong
In-Reply-To: <20231105063602.10737-1-xiazhengqiao@huaqin.corp-partner.google.com>
Hi,
On Sat, Nov 4, 2023 at 11:36 PM xiazhengqiao
<xiazhengqiao@huaqin.corp-partner.google.com> wrote:
>
> For "goodix,gt7375p" touchscreen, When the device restarts,
> the reset pin of the touchscreen will be pulled down by 10ms,
> but this time will make the touchscreen have a probability of
> not working properly. Increase post_power_delay_ms to 20ms,
> so that the reset pin is pulled down 20ms, and toucsreen works fine.
s/toucsreen/touchscreen
> Signed-off-by: xiazhengqiao <xiazhengqiao@huaqin.corp-partner.google.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index f1597ad67e7c..caabf7a62cde 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -111,7 +111,7 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
> }
>
> static const struct goodix_i2c_hid_timing_data goodix_gt7375p_timing_data = {
> - .post_power_delay_ms = 10,
> + .post_power_delay_ms = 20,
Do you actually have a Goodix "GT7375P" touchscreen, or do you have
some other touchscreen that is using the Goodix GT7375P compatible
string? As far as I know the 10 ms here came from the Goodix GT7375P
datasheet and has been working fine with devices that have Goodix
GT7373P. Has this always been wrong for the Goodix GT7373P, or (as I
suspect) do you actually have a different touchscreen and for that
different touchscreen you need a longer delay?
-Doug
^ permalink raw reply
* [PATCH v7 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-11-06 16:41 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel, Conor Dooley,
Krzysztof Kozlowski
Adds bindings for the Adafruit Seesaw Gamepad.
The gamepad functions as an i2c device with the default address of 0x50
and has an IRQ pin that can be enabled in the driver to allow for a rising
edge trigger on each button press or joystick movement.
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v7:
- no updates
Changes for v6:
- no updates
Changes for v5:
- Added link to the datasheet
Changes for v4:
- Fixed the URI for the id field
- Added `interrupts` property
Changes for v3:
- Updated id field to reflect updated file name from previous version
- Added `reg` property
Changes for v2:
- Renamed file to `adafruit,seesaw-gamepad.yaml`
- Removed quotes for `$id` and `$schema`
- Removed "Bindings for" from the description
- Changed node name to the generic name "joystick"
- Changed compatible to 'adafruit,seesaw-gamepad' instead of
'adafruit,seesaw_gamepad'
---
.../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
new file mode 100644
index 000000000000..3f0d1c5a3b9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Adafruit Mini I2C Gamepad with seesaw
+
+maintainers:
+ - Anshul Dalal <anshulusr@gmail.com>
+
+description: |
+ Adafruit Mini I2C Gamepad
+
+ +-----------------------------+
+ | ___ |
+ | / \ (X) |
+ | | S | __ __ (Y) (A) |
+ | \___/ |ST| |SE| (B) |
+ | |
+ +-----------------------------+
+
+ S -> 10-bit percision bidirectional analog joystick
+ ST -> Start
+ SE -> Select
+ X, A, B, Y -> Digital action buttons
+
+ Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
+ Product page: https://www.adafruit.com/product/5743
+ Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
+
+properties:
+ compatible:
+ const: adafruit,seesaw-gamepad
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description:
+ The gamepad's IRQ pin triggers a rising edge if interrupts are enabled.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ joystick@50 {
+ compatible = "adafruit,seesaw-gamepad";
+ reg = <0x50>;
+ };
+ };
--
2.42.0
^ permalink raw reply related
* [PATCH v7 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-11-06 16:41 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Weißschuh, Jeff LaBundy, Shuah Khan,
linux-kernel-mentees, linux-kernel
In-Reply-To: <20231106164134.114668-1-anshulusr@gmail.com>
Adds a driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Driver tested on RPi Zero 2W
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v7:
adafruit-seesaw.c
- Fixed formatting for macro definitions
- Made SEESAW_BUTTON_MASK static const
- Removed __be16 type for x and y fields of seesaw_data
- Used sparse_keymap implementation instead of custom keymap
- Used i2c_msg instead of i2c_master_send and recv in
seesaw_register_read
- Use temporary variable `adc_data` to store data received from ADC
- Changed read_buf from u8[4] to __be32
- Use usleep_range instead of msleep
- Removed 'Reviewed-by: Thomas Weißschuh' due to large number of changes
since last review
Kconfig:
- Added `select INPUT_SPARSEKMAP`
Changes for v6:
- Added TODO
- Removed `clang-format` directives
- Namespaced device buttons
- Removed `char physical_path[32]` field from `struct seesaw_gamepad`
- Added `packed` attribute to `struct seesaw_data`
- Moved from having booleans per button to single `u32 button_state`
- Added `seesaw_button_description` array to directly associate device
buttons with respective keycodes
- Added wrapper functions `seesaw_register_` around `i2c_master_` API
- Ratelimited input error reporting with `dev_err_ratelimited`
- Removed `of_device_id`
Changes for v5:
- Added link to the datasheet
- Added debug log message when `seesaw_read_data` fails
Changes for v4:
- Changed `1UL << BUTTON_` to BIT(BUTTON_)
- Removed `hardware_id` field from `struct seesaw_gamepad`
- Removed redundant checks for the number of bytes written and received by
`i2c_master_send` and `i2c_master_recv`
- Used `get_unaligned_be32` to instantiate `u32 result` from `read_buf`
- Changed `result & (1UL << BUTTON_)` to
`test_bit(BUTTON_, (long *)&result)`
- Changed `KBUILD_MODNAME` in id-tables to `SEESAW_DEVICE_NAME`
- Fixed formatting issues
- Changed button reporting:
Since the gamepad had the action buttons in a non-standard layout:
(X)
(Y) (A)
(B)
Therefore moved to using generic directional action button event codes
instead of BTN_[ABXY].
Changes for v3:
- no updates
Changes for v2:
adafruit-seesaw.c:
- Renamed file from 'adafruit_seesaw.c'
- Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
- Changed count parameter for receiving joystick x on line 118:
`2` to `sizeof(write_buf)`
- Fixed invalid buffer size on line 123 and 126:
`data->y` to `sizeof(data->y)`
- Added comment for the `mdelay(10)` on line 169
- Changed inconsistent indentation on line 271
Kconfig:
- Fixed indentation for the help text
- Updated module name
Makefile:
- Updated module object file name
MAINTAINERS:
- Updated file name for the driver and bindings
---
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 10 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit-seesaw.c | 315 +++++++++++++++++++++++
4 files changed, 333 insertions(+)
create mode 100644 drivers/input/joystick/adafruit-seesaw.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..cd4f9deb77e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/touchscreen/ad7879.c
+ADAFRUIT MINI I2C GAMEPAD
+M: Anshul Dalal <anshulusr@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/adafruit_seesaw.yaml
+F: drivers/input/joystick/adafruit_seesaw.c
+
ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
M: Jiri Kosina <jikos@kernel.org>
S: Maintained
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index ac6925ce8366..7914bef999b7 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,14 @@ config JOYSTICK_SENSEHAT
To compile this driver as a module, choose M here: the
module will be called sensehat_joystick.
+config JOYSTICK_SEESAW
+ tristate "Adafruit Mini I2C Gamepad with Seesaw"
+ depends on I2C
+ select INPUT_SPARSEKMAP
+ help
+ Say Y here if you want to use the Adafruit Mini I2C Gamepad.
+
+ To compile this driver as a module, choose M here: the module will be
+ called adafruit_seesaw.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 3937535f0098..fdc653209542 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit_seesaw.o
obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
new file mode 100644
index 000000000000..8e8ef26a585f
--- /dev/null
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Adafruit Mini I2C Gamepad
+ *
+ * Based on the work of:
+ * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
+ *
+ * Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf
+ * Product page: https://www.adafruit.com/product/5743
+ * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
+ *
+ * TODO:
+ * - Add interrupt support
+ */
+
+#include <asm-generic/unaligned.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define SEESAW_DEVICE_NAME "seesaw-gamepad"
+
+#define SEESAW_STATUS_BASE 0x00
+#define SEESAW_GPIO_BASE 0x01
+#define SEESAW_ADC_BASE 0x09
+
+#define SEESAW_GPIO_DIRCLR_BULK 0x03
+#define SEESAW_GPIO_BULK 0x04
+#define SEESAW_GPIO_BULK_SET 0x05
+#define SEESAW_GPIO_PULLENSET 0x0b
+
+#define SEESAW_STATUS_HW_ID 0x01
+#define SEESAW_STATUS_SWRST 0x7f
+
+#define SEESAW_ADC_OFFSET 0x07
+
+#define SEESAW_BUTTON_A 0x05
+#define SEESAW_BUTTON_B 0x01
+#define SEESAW_BUTTON_X 0x06
+#define SEESAW_BUTTON_Y 0x02
+#define SEESAW_BUTTON_START 0x10
+#define SEESAW_BUTTON_SELECT 0x00
+
+#define SEESAW_ANALOG_X 0x0e
+#define SEESAW_ANALOG_Y 0x0f
+
+#define SEESAW_JOYSTICK_MAX_AXIS 1023
+#define SEESAW_JOYSTICK_FUZZ 2
+#define SEESAW_JOYSTICK_FLAT 4
+
+#define SEESAW_GAMEPAD_POLL_INTERVAL 16
+#define SEESAW_GAMEPAD_POLL_MIN 8
+#define SEESAW_GAMEPAD_POLL_MAX 32
+
+static const u32 SEESAW_BUTTON_MASK =
+ BIT(SEESAW_BUTTON_A) | BIT(SEESAW_BUTTON_B) | BIT(SEESAW_BUTTON_X) |
+ BIT(SEESAW_BUTTON_Y) | BIT(SEESAW_BUTTON_START) |
+ BIT(SEESAW_BUTTON_SELECT);
+
+struct seesaw_gamepad {
+ struct input_dev *input_dev;
+ struct i2c_client *i2c_client;
+};
+
+struct seesaw_data {
+ u16 x;
+ u16 y;
+ u32 button_state;
+};
+
+struct seesaw_button_description {
+ unsigned int code;
+ unsigned int bit;
+};
+
+static const struct key_entry seesaw_buttons_new[] = {
+ { KE_KEY, SEESAW_BUTTON_A, .keycode = BTN_SOUTH },
+ { KE_KEY, SEESAW_BUTTON_B, .keycode = BTN_EAST },
+ { KE_KEY, SEESAW_BUTTON_X, .keycode = BTN_NORTH },
+ { KE_KEY, SEESAW_BUTTON_Y, .keycode = BTN_WEST },
+ { KE_KEY, SEESAW_BUTTON_START, .keycode = BTN_START },
+ { KE_KEY, SEESAW_BUTTON_SELECT, .keycode = BTN_SELECT },
+ { KE_END, 0 },
+};
+
+static int seesaw_register_read(struct i2c_client *client, u8 register_high,
+ u8 register_low, char *buf, int count)
+{
+ int ret;
+ u8 register_buf[2] = { register_high, register_low };
+
+ struct i2c_msg message_buf[2] = {
+ {
+ .addr = client->addr,
+ .flags = client->flags,
+ .len = sizeof(register_buf),
+ .buf = register_buf
+ },
+ {
+ .addr = client->addr,
+ .flags = client->flags | I2C_M_RD,
+ .len = count,
+ .buf = buf
+ },
+ };
+ ret = i2c_transfer(client->adapter, message_buf,
+ ARRAY_SIZE(message_buf));
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_register_write_u8(struct i2c_client *client, u8 register_high,
+ u8 register_low, u8 value)
+{
+ int ret;
+ u8 write_buf[3] = { register_high, register_low, value };
+
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_register_write_u32(struct i2c_client *client,
+ u8 register_high, u8 register_low,
+ u32 value)
+{
+ int ret;
+ u8 write_buf[6] = { register_high, register_low };
+
+ put_unaligned_be32(value, write_buf + 2);
+ ret = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
+{
+ int ret;
+ __be16 adc_data;
+ __be32 read_buf;
+
+ ret = seesaw_register_read(client, SEESAW_GPIO_BASE, SEESAW_GPIO_BULK,
+ (char *)&read_buf, sizeof(read_buf));
+ if (ret)
+ return ret;
+
+ data->button_state = ~be32_to_cpu(read_buf);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_X,
+ (char *)&adc_data, sizeof(adc_data));
+ if (ret)
+ return ret;
+ /*
+ * ADC reads left as max and right as 0, must be reversed since kernel
+ * expects reports in opposite order.
+ */
+ data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(adc_data);
+
+ ret = seesaw_register_read(client, SEESAW_ADC_BASE,
+ SEESAW_ADC_OFFSET + SEESAW_ANALOG_Y,
+ (char *)&adc_data, sizeof(adc_data));
+ if (ret)
+ return ret;
+ data->y = be16_to_cpu(adc_data);
+
+ return 0;
+}
+
+static void seesaw_poll(struct input_dev *input)
+{
+ int err, i;
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+ struct seesaw_data data;
+
+ err = seesaw_read_data(private->i2c_client, &data);
+ if (err) {
+ dev_err_ratelimited(&input->dev,
+ "failed to read joystick state: %d\n", err);
+ return;
+ }
+
+ input_report_abs(input, ABS_X, data.x);
+ input_report_abs(input, ABS_Y, data.y);
+
+ for_each_set_bit(i, (long *)&SEESAW_BUTTON_MASK,
+ BITS_PER_TYPE(SEESAW_BUTTON_MASK)) {
+ if (!sparse_keymap_report_event(
+ input, i, data.button_state & BIT(i), false)) {
+ dev_err_ratelimited(&input->dev,
+ "failed to report keymap event");
+ return;
+ };
+ }
+
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int ret;
+ u8 hardware_id;
+ struct seesaw_gamepad *seesaw;
+
+ ret = seesaw_register_write_u8(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF);
+ if (ret)
+ return ret;
+
+ /* Wait for the registers to reset before proceeding */
+ usleep_range(10000, 15000);
+
+ seesaw = devm_kzalloc(&client->dev, sizeof(*seesaw), GFP_KERNEL);
+ if (!seesaw)
+ return -ENOMEM;
+
+ ret = seesaw_register_read(client, SEESAW_STATUS_BASE,
+ SEESAW_STATUS_HW_ID, &hardware_id,
+ sizeof(hardware_id));
+ if (ret)
+ return ret;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ hardware_id);
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_DIRCLR_BULK,
+ SEESAW_BUTTON_MASK);
+ if (ret)
+ return ret;
+ ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_PULLENSET,
+ SEESAW_BUTTON_MASK);
+ if (ret)
+ return ret;
+ ret = seesaw_register_write_u32(client, SEESAW_GPIO_BASE,
+ SEESAW_GPIO_BULK_SET,
+ SEESAW_BUTTON_MASK);
+ if (ret)
+ return ret;
+
+ seesaw->i2c_client = client;
+ seesaw->input_dev = devm_input_allocate_device(&client->dev);
+ if (!seesaw->input_dev)
+ return -ENOMEM;
+
+ seesaw->input_dev->id.bustype = BUS_I2C;
+ seesaw->input_dev->name = "Adafruit Seesaw Gamepad";
+ seesaw->input_dev->phys = "i2c/" SEESAW_DEVICE_NAME;
+ input_set_drvdata(seesaw->input_dev, seesaw);
+ input_set_abs_params(seesaw->input_dev, ABS_X, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_abs_params(seesaw->input_dev, ABS_Y, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+
+ ret = sparse_keymap_setup(seesaw->input_dev, seesaw_buttons_new, NULL);
+ if (ret) {
+ dev_err(&client->dev,
+ "failed to set up input device keymap: %d\n", ret);
+ return ret;
+ }
+
+ ret = input_setup_polling(seesaw->input_dev, seesaw_poll);
+ if (ret) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", ret);
+ return ret;
+ }
+
+ input_set_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_INTERVAL);
+ input_set_max_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MAX);
+ input_set_min_poll_interval(seesaw->input_dev, SEESAW_GAMEPAD_POLL_MIN);
+
+ ret = input_register_device(seesaw->input_dev);
+ if (ret) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id seesaw_id_table[] = {
+ { SEESAW_DEVICE_NAME, 0 },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ },
+ .id_table = seesaw_id_table,
+ .probe = seesaw_probe,
+};
+module_i2c_driver(seesaw_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-06 16:59 UTC (permalink / raw)
To: David Revoy
Cc: Illia Ostapyshyn, jkosina, jason.gerecke, jose.exposito89,
linux-input, linux-kernel, nils, peter.hutterer, ping.cheng,
bagasdotme
In-Reply-To: <bokQB3BK040-4fGy8tNfZrdM2mNmWxZud9O-KMmYqOkfa1JTC1ocUjoAzCEpPsbsAvY5qb5TcSP6XsQLaja2XO0gapOcsZyeVdCvq6T31qA=@protonmail.com>
Hi David,
On Mon, Nov 6, 2023 at 2:18 PM David Revoy <davidrevoy@protonmail.com> wrote:
>
> Hi Illia, Jiri, Bagas,
>
> Thanks to Jiri for forwarding my request for help to this mailing list.
>
> I apologise in advance to Bagas and you all for not being able to properly configure my email client to follow the correct etiquette (Protonmail, unsuitable for kernel development, but we've made some progress with them on Mastodon on the encryption issue [1]).
Hopefully you'll be able to figure out a way to have your emails
posted to the lkml soon. It's very valuable to have them in
lore.kernel.org to get the thread context.
>
> Thanks to Illia for the detailed explanation. Thanks also for fixing it, and for explaining how the fix broke my workflow, and for your kind words about the situation. I appreciate it.
>
> However, after reading your reply, I still have my doubts about the preference to put an eraser on the top button of the pen by default. But after a few days and re-reading your first sentence "The XP-Pen hardware reports the Eraser usage for the upper stylus button.", I *think* I understood it: this is an internal signal that is sent by the device itself, and is therefore a specification that has to be followed, right? In this case, it makes sense for a generic driver to follow such a signal if it is sent by the hardware.
Yes, you are correct. The device talks a given protocol (HID) and is
supposed to use the specification from Microsoft[0] on how to behave
properly. As such, it has to convey the "eraser" state by adding
dedicated information in the event stream.
In your case, I think we (the kernel and your stylus) simply talk past
each other and we lose information.
>
> Having said that, behaving like this still makes no sense in one case: I'm thinking of my other display tablet, the XPPEN 16 Artist Pro (Gen2). In this case, the stylus has the top button as an eraser, and an eraser tip as well, as you can see in this photo [2]. Was it also a decision by XPPEN to include this signal in the hardware, knowing that they already had an eraser tip? In this case, please let me know, as it sounds like a hardware problem at the design stage and I have probably a way of passing on the information to their technical team.
If the pen has 2 buttons, and an eraser side, it would be a serious
design flow for XPPEN to report both as eraser.
Could you please use sudo hid-recorder from hid-tools[1] on any kernel
version and send us the logs here?
I'll be able to replay the events locally, and understand why the
kernel doesn't work properly.
And if there is a design flaw that can be fixed, we might even be able
to use hid-bpf to change it :)
>
> > You can still remap the eraser button to a right click using xsetwacom:
> > xsetwacom set "UGTABLET 24 inch PenDisplay eraser" "Button" "1" "3"
>
> Unfortunately, it doesn't work.
>
> Firstly, such tablets are often connected to a computer that already has a display. So each device (pen/eraser) needs to be mapped to the correct display and set to the correct 'Area' for calibration. A better syntax in my case to test your workaround is written like this:
>
> tableteraser="UGTABLET 24 inch PenDisplay eraser"
> xsetwacom set "$tableteraser" MapToOutput "DisplayPort-2"
> xsetwacom set "$tableteraser" Area 100 120 32794 32797
> xsetwacom set "$tableteraser" "Button" "1" "3"
>
> Secondly, forwarding the eraser button 1 to button 3 (a right click) in xsetwacom doesn't work.
>
> Pressing the button switches the device to an eraser and no right click happens. You'll need to hold down the button and click with the tip of the pen to create a right-click event.
>
> In a digital painting session in Krita, the software will switch your device to an eraser tool preset while you hold down the button, and the feedback on the canvas will be the eraser cursor. You'll then have to click "with the eraser" to get the right-click that triggers the pop-up palette [3].
>
> I'd also like to mention that if you haven't correctly mapped and calibrated your eraser device with xsetwacom, as I did in the code snippet above, the cursor will by default jump to a different location when you hold down the eraser button. It can be on a different display.
>
> Finally, in the case of the XPPEN 16 Artist Pro (Gen2) with a real eraser device (Photo, [2]), the setting 'xsetwacom set "$tableteraser" "Button" "1" "3"' will affect both erasers. Flipping the stylus on the eraser side and entering in 'hover' mode will return a correct eraser, but as soon as you start erasing with the tip, a right click will also be entered.
>
> > You can also do this with "xinput set-button-map", which works for libinput as well.
>
> $ xinput list
> ⎡ Virtual core pointer
> ⎜ ↳ UGTABLET 24 inch PenDisplay Mouse id=8 [slave pointer (2)]
> ⎜ ↳ UGTABLET 24 inch PenDisplay stylus id=10 [slave pointer (2)]
> ⎜ ↳ UGTABLET 24 inch PenDisplay eraser id=17 [slave pointer (2)]
> [...]
> $ xinput get-button-map 17
> 1 2 3 4 5 6 7 8
> $ xinput set-button-map 17 3 3 3 3 3 3 3 3
> $ xinput get-button-map 17
> 3 3 3 3 3 3 3 3
>
> Even after that, it doesn't work. My original article [4] mentions in the last paragraph that I have tested almost all methods, and this was one of them. Even a udev rule doesn't fix it. For reference, xinput produces the same behaviour as xsetwacom: you have to hold the button (it triggers an eraser device switch) then click with the tip to get a right-click...
Generally speaking, relying on X to fix your hardware is going to be a
dead end. When you switch to wayland, you'll lose all of your fixes,
which isn't great.
>
> > We have tested this with several XP-Pen devices, including Artist 24.
>
> Sorry if my tests show something different. Maybe there is something wrong with my GNU/Linux operating system (Fedora 38 KDE spin). Let me know if you have any idea why I get these results and guide me with what distro I should switch to get a similar observation than you do (and also to report the issue to the Fedora team).
>
> ---
>
> All in all (and in the case that my observations and tests are correct), I still stand by the points that I made in my blog post:
>
> - I don't have any way to customise the hardcoded eraser buttons in userspace and **it is a problem**: for devices without a touchscreen or mouse, not having access to a right-click by default on the device is a handicap. Many actions on the D.E. and applications use the right click. The preference to replace it with an 'eraser switch' action by default is IMHO highly debatable here.
AFAIU, the kernel now "merges" both buttons, which is a problem. It
seems to be a serious regression. This case is also worrying because I
added regression tests on hid, but I don't have access to all of the
various tablets, so I implemented them from the Microsoft
specification[0]. We need a special case for you here.
> - In the case of a pen that already has an eraser tip on the other side of the device [2], it makes no sense to exchange the right-click top button for another way to erase. Also in userspace, there seems to be no way to distinguish between the two buttons (the eraser tip and the eraser button). All actions from xsetwacom, xinput/libinput target the two eraser devices, and they target them unsuccessfully too.
Again, we can't do more than what the device reports. Well, we can
always quirk it in some cases, but ideally we don't have to do
anything. MS spec [0], only shows a single button pen with an eraser
tail or a pen with 2 buttons, one of them being the eraser one. I'm
not sure if they decided to prevent dual button pens with eraser tail,
but Wacom definitely has some, and they work.
Without the actual data from your pen, I can not tell much, because I
don't follow which path we are taking on the kernel side. Please
report with your hid-recorder logs, so I can understand why this is
happening and how we can fix it.
>
> [1]: https://mastodon.social/@protonmail/111346195283677411
> [2]: https://www.davidrevoy.com/data/images/blog/2023/2023-11-01_linux-kernel-broke-my-stylus_xp-pen-artist-pro-16-gen2_net.jpg
> [3]: https://docs.krita.org/en/reference_manual/popup-palette.html
> [4]: https://www.davidrevoy.com/article995/how-a-kernel-update-broke-my-stylus-need-help
>
>
> ------- Original Message -------
> On Friday, November 3rd, 2023 at 21:05, Illia Ostapyshyn <ostapyshyn@sra.uni-hannover.de> wrote:
>
>
> > Hello David, Hello Jiri,
> >
> > The XP-Pen hardware reports the Eraser usage for the upper stylus button.
> > Generally, styli report Invert usages when erasing, as described in [1].
> > XP-Pen digitizers, however, tend to omit them.
> >
> > The generic driver maps the Eraser usage to BTN_TOUCH and the Invert
> > usage to BTN_TOOL_RUBBER. Pens conforming to [1] send the Invert usage
> > first (switching the tool to BTN_TOOL_RUBBER) followed by Eraser, which
> > appears in userspace as a BTN_TOUCH event with the rubber tool set.
> >
> > Due to an oversight, devices not reporting Invert had the BTN_TOOL_RUBBER
> > event masked. This has caused the kernel to send only BTN_TOUCH events
> > without the tool switch when erasing.
> >
> > The situation got worse with refactoring done in 87562fcd1342. An eraser
> > without Invert caused the hidinput_hid_event state machine to get stuck
> > with BTN_TOOL_RUBBER internally (due to it being masked). For the
> > userspace, this looked as if the pen was never hovering again, rendering
> > it unusable until the next reset. 276e14e6c3 fixes this by adding
> > support for digitizers that do not report Invert usages when erasing.
AFAICT 276e14e6c3 already had the hid tests integrated at
tools/testing/selftests/hid/tests/test_tablet.py
It would have been great to add the cases you are mentioning here,
because there is a strong chance I'll break things once again when we
try to fix that regression.
> >
> > ---
> >
> > David, we are sorry that our patch broke your workflow. However,
> > forwarding hardware events as-is to the userspace has always been the
> > intended behavior, with a kernel bug preventing it so far. You can still
> > remap the eraser button to a right click using xsetwacom:
> >
> > xsetwacom set "UGTABLET 24 inch PenDisplay eraser" "Button" "1" "3"
> >
> > Replace the device name with the corresponding eraser device from
> > "xsetwacom list devices". You can also do this with "xinput set-button-map",
> > which works for libinput as well. We have tested this with several
> > XP-Pen devices, including Artist 24.
> >
> > [1] https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
>
Cheers,
Benjamin
[0] https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
[1] https://gitlab.freedesktop.org/libevdev/hid-tools
^ permalink raw reply
* Re: [PATCH 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()
From: Doug Anderson @ 2023-11-06 18:50 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-2-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -482,21 +442,49 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>
> ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
> if (ret)
> - goto out_unlock;
> + goto err_unlock;
>
> - ret = i2c_hid_execute_reset(ihid);
> + /* Prepare reset command. Command register goes first. */
> + *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> + length += sizeof(__le16);
> + /* Next is RESET command itself */
> + length += i2c_hid_encode_command(ihid->cmdbuf + length,
> + I2C_HID_OPCODE_RESET, 0, 0);
> +
> + set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +
> + ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> 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;
> + goto err_clear_reset;
> }
>
> + if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
> + msleep(100);
> + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> + }
> +
> + 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))) {
> + ret = -ENODATA;
> + goto err_clear_reset;
> + }
> + i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
super nitty, but I wonder if your i2c_hid_dbg() message saying
"waiting" should move above the check for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET. Then you'll have a message printed
before your msleep. I guess technically you could then add an "else
if" for the second "if" statement which would make it more obvious to
the reader that the "wait_event_timeout" won't happen when the quirk
is present.
Above is just a nit, so:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions
From: Doug Anderson @ 2023-11-06 18:53 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-3-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -460,6 +460,20 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
> goto err_clear_reset;
> }
>
> + return 0;
The mutex "contract" between i2c_hid_start_hwreset() and
i2c_hid_finish_hwreset() is non-obvious and, IMO, deserves a comment.
Specifically i2c_hid_start_hwreset() will grab and leave the mutex
locked if it returns 0. Then i2c_hid_finish_hwreset() expects the
mutex pre-locked and will always release it.
While reviewing later patches, I actually realized that _I think_
things would be cleaner by moving the mutex lock/unlock to the
callers. Maybe take a look at how the code looks with that?
> @@ -732,7 +745,9 @@ static int i2c_hid_parse(struct hid_device *hid)
> }
>
> do {
> - ret = i2c_hid_hwreset(ihid);
> + ret = i2c_hid_start_hwreset(ihid);
> + if (ret == 0)
> + ret = i2c_hid_finish_hwreset(ihid);
> if (ret)
> msleep(1000);
nit: it's slightly weird that two "if" tests next to each other use
different style. One compares against 0 and the other just implicitly
treats an int as a bool. I'm fine with either way, but it's nice to
keep the style the same within any given function (even better if it
can be the same throughout the driver).
> @@ -975,10 +990,13 @@ 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) {
> + ret = i2c_hid_start_hwreset(ihid);
> + if (ret == 0)
> + ret = i2c_hid_finish_hwreset(ihid);
nit: Above is also a "if (ret == 0)" vs below "if (ret)"...
> + } else {
> ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
> + }
>
> if (ret)
> return ret;
The above is mostly nits. I'd be OK with:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 3/7] HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling
From: Doug Anderson @ 2023-11-06 18:53 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-4-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor
From: Doug Anderson @ 2023-11-06 18:53 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-5-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:17 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -746,8 +752,6 @@ static int i2c_hid_parse(struct hid_device *hid)
>
> do {
> ret = i2c_hid_start_hwreset(ihid);
> - if (ret == 0)
> - ret = i2c_hid_finish_hwreset(ihid);
The mutex contract (talked about in a previous patch) is a little more
confusing now. ;-) Feels like it needs a comment somewhere in here so
the reader of the code understands that the reset_mutex might or might
not be locked here.
...or would it make more sense for the caller to just handle the mutex
to make it more obvious. The "I2C_HID_QUIRK_RESET_ON_RESUME" code
would need to grab the mutex too, but that really doesn't seem
terrible. In fact, I suspect it cleans up your error handling and
makes everything cleaner?
> if (ret)
> msleep(1000);
> } while (tries-- > 0 && ret);
> @@ -763,9 +767,8 @@ static int i2c_hid_parse(struct hid_device *hid)
> i2c_hid_dbg(ihid, "Using a HID report descriptor override\n");
> } else {
> rdesc = kzalloc(rsize, GFP_KERNEL);
> -
> if (!rdesc) {
> - dbg_hid("couldn't allocate rdesc memory\n");
> + i2c_hid_abort_hwreset(ihid);
> return -ENOMEM;
> }
>
> @@ -776,10 +779,21 @@ static int i2c_hid_parse(struct hid_device *hid)
> rdesc, rsize);
> if (ret) {
> hid_err(hid, "reading report descriptor failed\n");
> + i2c_hid_abort_hwreset(ihid);
> goto out;
> }
> }
>
> + /*
> + * 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);
> + if (ret)
> + goto out;
Given your new understanding, I wonder if you should start reading the
report descriptor even if "use_override" is set? You'd throw away what
you read but maybe it would be important to make the touchscreen
properly de-assert reset? I guess this is the subject of the next
patch...
Also: I guess there's the assumption that the touchscreens needing the
other caller of the reset functions (I2C_HID_QUIRK_RESET_ON_RESUME)
never need to read the report like this?
-Doug
^ permalink raw reply
* Re: [PATCH 5/7] HID: i2c-hid: Remove I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks
From: Doug Anderson @ 2023-11-06 18:54 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-6-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
Seems OK to me assuming it's not somehow better to read/throw away the
report on devices needing the override, as discussed in a previous
patch.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 6/7] HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
From: Doug Anderson @ 2023-11-06 18:55 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-7-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 7/7] HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines
From: Doug Anderson @ 2023-11-06 18:55 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231104111743.14668-8-hdegoede@redhat.com>
Hi,
On Sat, Nov 4, 2023 at 4:18 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Illia Ostapyshyn @ 2023-11-06 20:06 UTC (permalink / raw)
To: Benjamin Tissoires, David Revoy
Cc: jkosina, jason.gerecke, jose.exposito89, linux-input,
linux-kernel, nils, peter.hutterer, ping.cheng, bagasdotme
In-Reply-To: <CAO-hwJLpKTb9yxvxaPDLZkF9kDF8u2VRJUf9yiQd+neOyxPeug@mail.gmail.com>
On 11/6/23 17:59, Benjamin Tissoires wrote:
> If the pen has 2 buttons, and an eraser side, it would be a serious
> design flow for XPPEN to report both as eraser.
>
> Could you please use sudo hid-recorder from hid-tools[1] on any kernel
> version and send us the logs here?
> I'll be able to replay the events locally, and understand why the
> kernel doesn't work properly.
>
> And if there is a design flaw that can be fixed, we might even be able
> to use hid-bpf to change it :)
My wild guess is that XP-Pen 16 Artist Pro reports an Eraser usage
without Invert for the upper button and Eraser with Invert for the
eraser tip. A device-specific driver could work with that, but there
seems to be no way to incorporate two different erasers (thus, allowing
userspace to map them to different actions arbitrarily) in the generic
driver currently.
> Generally speaking, relying on X to fix your hardware is going to be a
> dead end. When you switch to wayland, you'll lose all of your fixes,
> which isn't great.
> AFAIU, the kernel now "merges" both buttons, which is a problem. It
> seems to be a serious regression. This case is also worrying because I
> added regression tests on hid, but I don't have access to all of the
> various tablets, so I implemented them from the Microsoft
> specification[0]. We need a special case for you here.
The issue preventing David from mapping HID_DG_ERASER to BTN_STYLUS2 is
that the hidinput_hid_event is not compatible with hidinput_setkeycode.
If usage->code is no longer BTN_TOUCH after remapping, it won't be
released when Eraser reports 0. A simple fix is:
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1589,7 +1589,7 @@ void hidinput_hid_event(struct hid_device *hid,
struct hid_field *field, struct
/* value is off, tool is not rubber, ignore */
return;
else if (*quirks & HID_QUIRK_NOINVERT &&
- !test_bit(BTN_TOUCH, input->key)) {
+ !test_bit(usage->code, input->key)) {
/*
* There is no invert to release the tool, let hid_input
* send BTN_TOUCH with scancode and release the tool after.
This change alone fixes David's problem and the right-click mapping in
hwdb works again. However, the tool switches to rubber for the remapped
eraser (here BTN_STYLUS2) events, both for devices with and without
Invert. This does no harm but is not useful either. A cleaner solution
for devices without Invert would be to omit the whole tool switching
logic in this case:
@@ -1577,6 +1577,9 @@ void hidinput_hid_event(struct hid_device *hid,
struct hid_field *field, struct
switch (usage->hid) {
case HID_DG_ERASER:
+ if (*quirks & HID_QUIRK_NOINVERT && usage->code != BTN_TOUCH)
+ break;
+
report->tool_active |= !!value;
Remapping Invert does not work anyway as the Invert tool is hardcoded in
hidinput_hid_event. Even worse, I guess (not tested) trying to do so
would mask BTN_TOOL_RUBBER from dev->keybit and could cause weird
behavior similar to one between 87562fcd1342 and 276e14e6c3. This
raises the question: should users be able to remap Invert after all?
^ permalink raw reply
* [PATCH v2 0/2] HID: glorious: fix Glorious Model I HID report and rename vendor ID
From: Brett Raye @ 2023-11-06 23:55 UTC (permalink / raw)
To: jikos; +Cc: sergeantsagara, benjamin.tissoires, linux-input, Brett Raye
The Glorious Model I mouse presents a faulty HID report descriptor which
prevents some of its reprogrammable buttons from working. This patch
series performs a fixup on the faulty descriptor, and renames a vendor ID
for consistency and clarity.
Changes in v2:
- Split vendor ID rename into its own patch.
- Link to v1: https://lore.kernel.org/linux-input/20231103011038.27462-1-braye@fastmail.com/
Brett Raye (2):
HID: glorious: fix Glorious Model I HID report
HID: glorious: rename USB_VENDOR_ID_GLORIOUS
drivers/hid/hid-glorious.c | 16 ++++++++++++++--
drivers/hid/hid-ids.h | 11 +++++++----
2 files changed, 21 insertions(+), 6 deletions(-)
base-commit: bab19d1b21547046b0a38dde948086f6cbcaefaa
--
2.40.1
^ permalink raw reply
* [PATCH v2 1/2] HID: glorious: fix Glorious Model I HID report
From: Brett Raye @ 2023-11-06 23:55 UTC (permalink / raw)
To: jikos; +Cc: sergeantsagara, benjamin.tissoires, linux-input, Brett Raye
In-Reply-To: <20231106235557.8741-1-braye@fastmail.com>
The Glorious Model I mouse has a buggy HID report descriptor for its
keyboard endpoint (used for programmable buttons). For report ID 2, there
is a mismatch between Logical Minimum and Usage Minimum in the array that
reports keycodes.
The offending portion of the descriptor: (from hid-decode)
0x95, 0x05, // Report Count (5) 30
0x75, 0x08, // Report Size (8) 32
0x15, 0x00, // Logical Minimum (0) 34
0x25, 0x65, // Logical Maximum (101) 36
0x05, 0x07, // Usage Page (Keyboard) 38
0x19, 0x01, // Usage Minimum (1) 40
0x29, 0x65, // Usage Maximum (101) 42
0x81, 0x00, // Input (Data,Arr,Abs) 44
This bug shifts all programmed keycodes up by 1. Importantly, this causes
"empty" array indexes of 0x00 to be interpreted as 0x01, ErrorRollOver.
The presence of ErrorRollOver causes the system to ignore all keypresses
from the endpoint and breaks the ability to use the programmable buttons.
Setting byte 41 to 0x00 fixes this, and causes keycodes to be interpreted
correctly.
Signed-off-by: Brett Raye <braye@fastmail.com>
---
drivers/hid/hid-glorious.c | 12 ++++++++++++
drivers/hid/hid-ids.h | 3 +++
2 files changed, 15 insertions(+)
diff --git a/drivers/hid/hid-glorious.c b/drivers/hid/hid-glorious.c
index 558eb08c19ef..e44a39a17151 100644
--- a/drivers/hid/hid-glorious.c
+++ b/drivers/hid/hid-glorious.c
@@ -21,6 +21,10 @@ MODULE_DESCRIPTION("HID driver for Glorious PC Gaming Race mice");
* Glorious Model O and O- specify the const flag in the consumer input
* report descriptor, which leads to inputs being ignored. Fix this
* by patching the descriptor.
+ *
+ * Glorious Model I incorrectly specifes the Usage Minimum for its
+ * keyboard HID report, causing keycodes to be misinterpreted.
+ * Fix this by setting Usage Minimum to 0 in that report.
*/
static __u8 *glorious_report_fixup(struct hid_device *hdev, __u8 *rdesc,
unsigned int *rsize)
@@ -32,6 +36,10 @@ static __u8 *glorious_report_fixup(struct hid_device *hdev, __u8 *rdesc,
rdesc[85] = rdesc[113] = rdesc[141] = \
HID_MAIN_ITEM_VARIABLE | HID_MAIN_ITEM_RELATIVE;
}
+ if (*rsize == 156 && rdesc[41] == 1) {
+ hid_info(hdev, "patching Glorious Model I keyboard report descriptor\n");
+ rdesc[41] = 0;
+ }
return rdesc;
}
@@ -44,6 +52,8 @@ static void glorious_update_name(struct hid_device *hdev)
model = "Model O"; break;
case USB_DEVICE_ID_GLORIOUS_MODEL_D:
model = "Model D"; break;
+ case USB_DEVICE_ID_GLORIOUS_MODEL_I:
+ model = "Model I"; break;
}
snprintf(hdev->name, sizeof(hdev->name), "%s %s", "Glorious", model);
@@ -70,6 +80,8 @@ static const struct hid_device_id glorious_devices[] = {
USB_DEVICE_ID_GLORIOUS_MODEL_O) },
{ HID_USB_DEVICE(USB_VENDOR_ID_GLORIOUS,
USB_DEVICE_ID_GLORIOUS_MODEL_D) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_LAVIEW,
+ USB_DEVICE_ID_GLORIOUS_MODEL_I) },
{ }
};
MODULE_DEVICE_TABLE(hid, glorious_devices);
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index e4d2dfd5d253..c30bca343982 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -744,6 +744,9 @@
#define USB_VENDOR_ID_LABTEC 0x1020
#define USB_DEVICE_ID_LABTEC_WIRELESS_KEYBOARD 0x0006
+#define USB_VENDOR_ID_LAVIEW 0x22D4
+#define USB_DEVICE_ID_GLORIOUS_MODEL_I 0x1503
+
#define USB_VENDOR_ID_LCPOWER 0x1241
#define USB_DEVICE_ID_LCPOWER_LC1000 0xf767
--
2.40.1
^ permalink raw reply related
* [PATCH v2 2/2] HID: glorious: rename USB_VENDOR_ID_GLORIOUS
From: Brett Raye @ 2023-11-06 23:55 UTC (permalink / raw)
To: jikos; +Cc: sergeantsagara, benjamin.tissoires, linux-input, Brett Raye
In-Reply-To: <20231106235557.8741-1-braye@fastmail.com>
USB_VENDOR_ID_GLORIOUS is changed to USB_VENDOR_ID_SINOWEALTH. Glorious
seems to be white-labeling controller boards or mice from many vendors.
There isn't a single canonical vendor ID for Glorious products.
Signed-off-by: Brett Raye <braye@fastmail.com>
---
drivers/hid/hid-glorious.c | 4 ++--
drivers/hid/hid-ids.h | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-glorious.c b/drivers/hid/hid-glorious.c
index e44a39a17151..281b3a7187ce 100644
--- a/drivers/hid/hid-glorious.c
+++ b/drivers/hid/hid-glorious.c
@@ -76,9 +76,9 @@ static int glorious_probe(struct hid_device *hdev,
}
static const struct hid_device_id glorious_devices[] = {
- { HID_USB_DEVICE(USB_VENDOR_ID_GLORIOUS,
+ { HID_USB_DEVICE(USB_VENDOR_ID_SINOWEALTH,
USB_DEVICE_ID_GLORIOUS_MODEL_O) },
- { HID_USB_DEVICE(USB_VENDOR_ID_GLORIOUS,
+ { HID_USB_DEVICE(USB_VENDOR_ID_SINOWEALTH,
USB_DEVICE_ID_GLORIOUS_MODEL_D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_LAVIEW,
USB_DEVICE_ID_GLORIOUS_MODEL_I) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index c30bca343982..9ed9ec03ad1b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -510,10 +510,6 @@
#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
#define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
-#define USB_VENDOR_ID_GLORIOUS 0x258a
-#define USB_DEVICE_ID_GLORIOUS_MODEL_D 0x0033
-#define USB_DEVICE_ID_GLORIOUS_MODEL_O 0x0036
-
#define I2C_VENDOR_ID_GOODIX 0x27c6
#define I2C_DEVICE_ID_GOODIX_01F0 0x01f0
@@ -1162,6 +1158,10 @@
#define USB_VENDOR_ID_SIGMATEL 0x066F
#define USB_DEVICE_ID_SIGMATEL_STMP3780 0x3780
+#define USB_VENDOR_ID_SINOWEALTH 0x258a
+#define USB_DEVICE_ID_GLORIOUS_MODEL_D 0x0033
+#define USB_DEVICE_ID_GLORIOUS_MODEL_O 0x0036
+
#define USB_VENDOR_ID_SIS_TOUCH 0x0457
#define USB_DEVICE_ID_SIS9200_TOUCH 0x9200
#define USB_DEVICE_ID_SIS817_TOUCH 0x0817
--
2.40.1
^ permalink raw reply related
* Re: [PATCH 3/7] ARM: dts: qcom: Add support for Samsung Galaxy Tab 4 8.0 Wi-Fi
From: kernel test robot @ 2023-11-07 1:28 UTC (permalink / raw)
To: Bryant Mairs, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Linus Walleij, linux-arm-msm, devicetree, linux-kernel,
linux-input
Cc: oe-kbuild-all
In-Reply-To: <20231105204759.37107-4-bryant@mai.rs>
Hi Bryant,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on dtor-input/next linus/master v6.6 next-20231106]
[cannot apply to dtor-input/for-linus]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Bryant-Mairs/dt-bindings-input-melfas-mms114-add-MMS252-compatible/20231106-045021
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20231105204759.37107-4-bryant%40mai.rs
patch subject: [PATCH 3/7] ARM: dts: qcom: Add support for Samsung Galaxy Tab 4 8.0 Wi-Fi
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20231107/202311070844.zoUJwRlS-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231107/202311070844.zoUJwRlS-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311070844.zoUJwRlS-lkp@intel.com/
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/qcom/qcom-apq8026-samsung-milletwifi.dts:14.15-26 Label or path mba_region not found
>> Error: arch/arm/boot/dts/qcom/qcom-apq8026-samsung-milletwifi.dts:15.15-27 Label or path mpss_region not found
>> Error: arch/arm/boot/dts/qcom/qcom-apq8026-samsung-milletwifi.dts:17.15-28 Label or path wcnss_region not found
FATAL ERROR: Syntax error parsing input tree
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 0/2] HID: glorious: fix Glorious Model I HID report and rename vendor ID
From: Rahul Rameshbabu @ 2023-11-07 1:39 UTC (permalink / raw)
To: Brett Raye; +Cc: jikos, benjamin.tissoires, linux-input
In-Reply-To: <20231106235557.8741-1-braye@fastmail.com>
On Mon, 06 Nov, 2023 15:55:55 -0800 "Brett Raye" <braye@fastmail.com> wrote:
> The Glorious Model I mouse presents a faulty HID report descriptor which
> prevents some of its reprogrammable buttons from working. This patch
> series performs a fixup on the faulty descriptor, and renames a vendor ID
> for consistency and clarity.
Thanks for the patches.
Reviewed-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
^ permalink raw reply
* [PATCH] HID: multitouch: Add quirk for HONOR GLO-GXXX touchpad
From: Aoba K @ 2023-11-07 3:16 UTC (permalink / raw)
To: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires; +Cc: linux-input
Honor MagicBook 13 2023 has a touchpad which do not switch to the
multitouch mode until the input mode feature is written by the host.
The touchpad do report the input mode at touchpad(3), while itself
working under mouse mode. As a workaround, it is possible to call
MT_QUIRE_FORCE_GET_FEATURE to force set feature in mt_set_input_mode
for such device.
The touchpad reports as BLTP7853, which cannot retrive any useful
manufacture information on the internel by this string at present.
As the serial number of the laptop is GLO-G52, while DMI info reports
the laptop serial number as GLO-GXXX, this workaround should applied
to all models which has the GLO-GXXX.
Signed-off-by: Aoba K <nexp_0x17@outlook.com>
---
drivers/hid/hid-multitouch.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index e31be0cb8b..f655a76ff2 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2055,6 +2055,11 @@ static const struct hid_device_id mt_devices[] = {
MT_USB_DEVICE(USB_VENDOR_ID_HANVON_ALT,
USB_DEVICE_ID_HANVON_ALT_MULTITOUCH) },
+ /* HONOR GLO-GXXX panel */
+ { .driver_data = MT_CLS_VTL,
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
+ 0x347d, 0x7853) },
+
/* Ilitek dual touch panel */
{ .driver_data = MT_CLS_NSMU,
MT_USB_DEVICE(USB_VENDOR_ID_ILITEK,
base-commit: 28d3fe32354701decc3e76d89712569c269b5e4f
---
2.41.0
^ permalink raw reply related
* Re: [PATCH 3/7] ARM: dts: qcom: Add support for Samsung Galaxy Tab 4 8.0 Wi-Fi
From: Krzysztof Kozlowski @ 2023-11-07 7:29 UTC (permalink / raw)
To: Bryant Mairs, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Linus Walleij, linux-arm-msm, devicetree, linux-kernel,
linux-input
In-Reply-To: <20231105204759.37107-4-bryant@mai.rs>
On 05/11/2023 21:46, Bryant Mairs wrote:
> Add support for this tablet based on the MSM8226 SoC, codenamed
> "milletwifi".
>
> Signed-off-by: Bryant Mairs <bryant@mai.rs>
> ---
> arch/arm/boot/dts/qcom/Makefile | 1 +
> .../qcom/qcom-apq8026-samsung-milletwifi.dts | 543 ++++++++++++++++++
> 2 files changed, 544 insertions(+)
> create mode 100644 arch/arm/boot/dts/qcom/qcom-apq8026-samsung-milletwifi.dts
>
LKP reports that patches were most likely not even built :(
Best regards,
Krzysztof
^ permalink raw reply
* Re:Re: [PATCH v2] HID: fix a crash in hid_debug_events_release
From: be286 @ 2023-11-07 7:59 UTC (permalink / raw)
To: Rahul Rameshbabu; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <87ttpzzdpm.fsf@protonmail.com>
Hi Rahul,
Thank you for your reply. It has been very helpful to me and I accept it.
With regards,
Charles Yi
At 2023-11-06 12:51:38, "Rahul Rameshbabu" <sergeantsagara@protonmail.com> wrote:
>Lets clean up the subject/commit message heading.
>
> HID: fix HID device resource race between HID core and debugging support
>
>In the commit message body, we can expand on the details a bit more.
>
>On Tue, 31 Oct, 2023 12:32:39 +0800 "Charles Yi" <be286@163.com> wrote:
>> hid_debug_events_release() access released memory by
>> hid_device_release(). This is fixed by the patch.
>>
>> When hid_debug_events_release() was being called, in most case,
>> hid_device_release() finish already, the memory of list->hdev
>> freed by hid_device_release(), if list->hdev memory
>> reallocate by others, and it's modified, zeroed, then
>> list->hdev->debug_list_lock occasioned crash come out.
>
>Lets clean up these paragraphs a bit.
>
> hid_debug_events_release releases resources bound to the HID device
> instance. hid_device_release releases the underlying HID device
> instance potentially before hid_debug_events_release has completed
> releasing debug resources bound to the same HID device instance.
>
> Reference count to prevent the HID device instance from being torn
> down preemptively when HID debugging support is used. When count
> reaches zero, release core resources of HID device instance using
> hiddev_free.
>
>Feel free to use the above if you think its nice or feel free to polish
>up the commit message body you originally had a bit more.
>
>>
>> The crash:
>>
>> [ 120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
>> [ 120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>> [ 120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
>> [ 120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
>> [ 120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
>> [ 120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
>> [ 120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
>> [ 120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
>> [ 120.779120][ T4396] sp : ffffffc01e62bb60
>> [ 120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
>> [ 120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
>> [ 120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
>> [ 120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
>> [ 120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
>> [ 120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
>> [ 120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
>> [ 120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
>> [ 120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
>> [ 120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
>> [ 120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
>> [ 120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
>> [ 120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
>> [ 120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
>> [ 120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
>> [ 120.873122][ T4396] Call trace:
>> [ 120.876259][ T4396] __list_del_entry_valid+0x98/0xac
>> [ 120.881304][ T4396] hid_debug_events_release+0x48/0x12c
>> [ 120.886617][ T4396] full_proxy_release+0x50/0xbc
>> [ 120.891323][ T4396] __fput+0xdc/0x238
>> [ 120.895075][ T4396] ____fput+0x14/0x24
>> [ 120.898911][ T4396] task_work_run+0x90/0x148
>> [ 120.903268][ T4396] do_exit+0x1bc/0x8a4
>> [ 120.907193][ T4396] do_group_exit+0x8c/0xa4
>> [ 120.911458][ T4396] get_signal+0x468/0x744
>> [ 120.915643][ T4396] do_signal+0x84/0x280
>> [ 120.919650][ T4396] do_notify_resume+0xd0/0x218
>> [ 120.924262][ T4396] work_pending+0xc/0x3f0
>>
>> Fixes: <cd667ce24796> (HID: use debugfs for events/reports dumping)
>
>The formatting of the Fixes: tag would look like the following.
>
> Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
>
>You can also eliminate the whitespace between your git trailers, so the
>end result looks like the following (minus the indentation).
>
> Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
> Signed-off-by: Charles Yi <be286@163.com>
>
>>
>> Signed-off-by: Charles Yi <be286@163.com>
>>
>> ---
>> Changes in V2:
>> - Add "Fixes:" tag and call trace to commit message.
>> ---
>> drivers/hid/hid-core.c | 12 ++++++++++--
>> drivers/hid/hid-debug.c | 3 +++
>> include/linux/hid.h | 3 +++
>> 3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8992e3c1e769..e0181218ad85 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
>> * Free a device structure, all reports, and all fields.
>> */
>>
>> -static void hid_device_release(struct device *dev)
>> +void hiddev_free(struct kref *ref)
>
>Lets call this hid_hiddev_free. Took a look through hid-core.c, and I
>think this would be better than calling it just hiddev_free.
>
>> {
>> - struct hid_device *hid = to_hid_device(dev);
>> + struct hid_device *hid = container_of(ref, struct hid_device, ref);
>>
>> hid_close_report(hid);
>> kfree(hid->dev_rdesc);
>> kfree(hid);
>> }
>>
>> +static void hid_device_release(struct device *dev)
>> +{
>> + struct hid_device *hid = to_hid_device(dev);
>> +
>> + kref_put(&hid->ref, hiddev_free);
>> +}
>> +
>> /*
>> * Fetch a report description item from the data stream. We support long
>> * items, though they are not used yet.
>> @@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
>> spin_lock_init(&hdev->debug_list_lock);
>> sema_init(&hdev->driver_input_lock, 1);
>> mutex_init(&hdev->ll_open_lock);
>> + kref_init(&hdev->ref);
>>
>> hid_bpf_device_init(hdev);
>>
>> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
>> index e7ef1ea107c9..7dd83ec74f8a 100644
>> --- a/drivers/hid/hid-debug.c
>> +++ b/drivers/hid/hid-debug.c
>> @@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
>> goto out;
>> }
>> list->hdev = (struct hid_device *) inode->i_private;
>> + kref_get(&list->hdev->ref);
>> file->private_data = list;
>> mutex_init(&list->read_mutex);
>>
>> @@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
>> list_del(&list->node);
>> spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
>> kfifo_free(&list->hid_debug_fifo);
>> +
>> + kref_put(&list->hdev->ref, hiddev_free);
>> kfree(list);
>>
>> return 0;
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 964ca1f15e3f..3b08a2957229 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
>> struct list_head debug_list;
>> spinlock_t debug_list_lock;
>> wait_queue_head_t debug_wait;
>> + struct kref ref;
>>
>> unsigned int id; /* system unique id */
>>
>> @@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
>> #endif /* CONFIG_BPF */
>> };
>>
>> +void hiddev_free(struct kref *ref);
>> +
>> #define to_hid_device(pdev) \
>> container_of(pdev, struct hid_device, dev)
>
>--
>Thanks for the patch,
>
>Rahul Rameshbabu
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-07 7:59 UTC (permalink / raw)
To: Illia Ostapyshyn
Cc: David Revoy, jkosina, jason.gerecke, jose.exposito89, linux-input,
linux-kernel, nils, peter.hutterer, ping.cheng, bagasdotme
In-Reply-To: <eb8e22f3-77dc-4923-a7ba-e237ee226edb@sra.uni-hannover.de>
On Mon, Nov 6, 2023 at 9:06 PM Illia Ostapyshyn
<ostapyshyn@sra.uni-hannover.de> wrote:
>
> On 11/6/23 17:59, Benjamin Tissoires wrote:
>
> > If the pen has 2 buttons, and an eraser side, it would be a serious
> > design flow for XPPEN to report both as eraser.
> >
> > Could you please use sudo hid-recorder from hid-tools[1] on any kernel
> > version and send us the logs here?
> > I'll be able to replay the events locally, and understand why the
> > kernel doesn't work properly.
> >
> > And if there is a design flaw that can be fixed, we might even be able
> > to use hid-bpf to change it :)
>
> My wild guess is that XP-Pen 16 Artist Pro reports an Eraser usage
> without Invert for the upper button and Eraser with Invert for the
> eraser tip. A device-specific driver could work with that, but there
> seems to be no way to incorporate two different erasers (thus, allowing
> userspace to map them to different actions arbitrarily) in the generic
> driver currently.
That's exactly why I want to see the exact event flow. We can not do
"wild guesses" unfortunately (not meaning any offenses).
And I am very suspicious about the fact that the stylus reports 2
identical erasers. Because in the past David seemed to be able to have
2 distincts behaviors for the 2 "buttons" (physical button and eraser
tail).
>
>
> > Generally speaking, relying on X to fix your hardware is going to be a
> > dead end. When you switch to wayland, you'll lose all of your fixes,
> > which isn't great.
>
> > AFAIU, the kernel now "merges" both buttons, which is a problem. It
> > seems to be a serious regression. This case is also worrying because I
> > added regression tests on hid, but I don't have access to all of the
> > various tablets, so I implemented them from the Microsoft
> > specification[0]. We need a special case for you here.
>
> The issue preventing David from mapping HID_DG_ERASER to BTN_STYLUS2 is
> that the hidinput_hid_event is not compatible with hidinput_setkeycode.
> If usage->code is no longer BTN_TOUCH after remapping, it won't be
> released when Eraser reports 0. A simple fix is:
I must confess, being the one who refactored everything, I still don't
believe this is as simple as it may seem. I paged out all of the
special cases, and now, without seeing the event flow I just can not
understand why this would fix the situation.
And BTW, if you have a tool affected by 276e14e6c3, I'd be curious to
get a hid-recorder sample for it so I can get regression tests for it.
>
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1589,7 +1589,7 @@ void hidinput_hid_event(struct hid_device *hid,
> struct hid_field *field, struct
> /* value is off, tool is not rubber, ignore */
> return;
> else if (*quirks & HID_QUIRK_NOINVERT &&
> - !test_bit(BTN_TOUCH, input->key)) {
> + !test_bit(usage->code, input->key)) {
I don't want to be rude, but this feels very much like black magic,
especially because there is a comment just below and it is not
updated. So either the explanation was wrong, or it's not explaining
the situation (I also understand that this is not a formal submission,
so maybe that's the reason why the comment is not updated).
> /*
> * There is no invert to release the tool, let hid_input
> * send BTN_TOUCH with scancode and release the tool after.
>
> This change alone fixes David's problem and the right-click mapping in
> hwdb works again. However, the tool switches to rubber for the remapped
> eraser (here BTN_STYLUS2) events, both for devices with and without
> Invert. This does no harm but is not useful either. A cleaner solution
> for devices without Invert would be to omit the whole tool switching
> logic in this case:
>
> @@ -1577,6 +1577,9 @@ void hidinput_hid_event(struct hid_device *hid,
> struct hid_field *field, struct
>
> switch (usage->hid) {
> case HID_DG_ERASER:
> + if (*quirks & HID_QUIRK_NOINVERT && usage->code != BTN_TOUCH)
> + break;
> +
> report->tool_active |= !!value;
>
> Remapping Invert does not work anyway as the Invert tool is hardcoded in
> hidinput_hid_event. Even worse, I guess (not tested) trying to do so
> would mask BTN_TOOL_RUBBER from dev->keybit and could cause weird
> behavior similar to one between 87562fcd1342 and 276e14e6c3. This
> raises the question: should users be able to remap Invert after all?
>
The kernel is supposed to transfer what the device is. So if it says
this is an eraser, we should not try to change it. Users can then
tweak their own device if they wish through hid-bpf or through
libinput quirks, but when you install a fresh kernel without tweaks,
we should be as accurate as possible.
My main concern is that now we have a device which exports 2 different
interactions as being the same. So either the firmware is wrong, and
we need to quirk it, or the kernel is wrong and merges both, and this
needs fixes as well.
Once every interaction on the device gets its own behavior, userspace
can do whatever they want. It's not the kernel's concern anymore.
BTW, David, were you able to do a revert of 276e14e6c3?
Cheers,
Benjamin
^ permalink raw reply
* [PATCH] HID: fix HID device resource race between HID core and debugging support
From: Charles Yi @ 2023-11-07 8:04 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Charles Yi
hid_debug_events_release releases resources bound to the HID device
instance. hid_device_release releases the underlying HID device
instance potentially before hid_debug_events_release has completed
releasing debug resources bound to the same HID device instance.
Reference count to prevent the HID device instance from being torn
down preemptively when HID debugging support is used. When count
reaches zero, release core resources of HID device instance using
hid_hiddev_free.
The crash:
[ 120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
[ 120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[ 120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
[ 120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
[ 120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
[ 120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[ 120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
[ 120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
[ 120.779120][ T4396] sp : ffffffc01e62bb60
[ 120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
[ 120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
[ 120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
[ 120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
[ 120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
[ 120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
[ 120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
[ 120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
[ 120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
[ 120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
[ 120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
[ 120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
[ 120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
[ 120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
[ 120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
[ 120.873122][ T4396] Call trace:
[ 120.876259][ T4396] __list_del_entry_valid+0x98/0xac
[ 120.881304][ T4396] hid_debug_events_release+0x48/0x12c
[ 120.886617][ T4396] full_proxy_release+0x50/0xbc
[ 120.891323][ T4396] __fput+0xdc/0x238
[ 120.895075][ T4396] ____fput+0x14/0x24
[ 120.898911][ T4396] task_work_run+0x90/0x148
[ 120.903268][ T4396] do_exit+0x1bc/0x8a4
[ 120.907193][ T4396] do_group_exit+0x8c/0xa4
[ 120.911458][ T4396] get_signal+0x468/0x744
[ 120.915643][ T4396] do_signal+0x84/0x280
[ 120.919650][ T4396] do_notify_resume+0xd0/0x218
[ 120.924262][ T4396] work_pending+0xc/0x3f0
Fixes: <cd667ce24796> (HID: use debugfs for events/reports dumping)
Signed-off-by: Charles Yi <be286@163.com>
---
drivers/hid/hid-core.c | 12 ++++++++++--
drivers/hid/hid-debug.c | 3 +++
include/linux/hid.h | 3 +++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8992e3c1e769..bd2f58e0e87a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
* Free a device structure, all reports, and all fields.
*/
-static void hid_device_release(struct device *dev)
+void hid_hiddev_free(struct kref *ref)
{
- struct hid_device *hid = to_hid_device(dev);
+ struct hid_device *hid = container_of(ref, struct hid_device, ref);
hid_close_report(hid);
kfree(hid->dev_rdesc);
kfree(hid);
}
+static void hid_device_release(struct device *dev)
+{
+ struct hid_device *hid = to_hid_device(dev);
+
+ kref_put(&hid->ref, hid_hiddev_free);
+}
+
/*
* Fetch a report description item from the data stream. We support long
* items, though they are not used yet.
@@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
spin_lock_init(&hdev->debug_list_lock);
sema_init(&hdev->driver_input_lock, 1);
mutex_init(&hdev->ll_open_lock);
+ kref_init(&hdev->ref);
hid_bpf_device_init(hdev);
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index e7ef1ea107c9..36c47012d79f 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
goto out;
}
list->hdev = (struct hid_device *) inode->i_private;
+ kref_get(&list->hdev->ref);
file->private_data = list;
mutex_init(&list->read_mutex);
@@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
list_del(&list->node);
spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
kfifo_free(&list->hid_debug_fifo);
+
+ kref_put(&list->hdev->ref, hid_hiddev_free);
kfree(list);
return 0;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 964ca1f15e3f..7867f571376c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
struct list_head debug_list;
spinlock_t debug_list_lock;
wait_queue_head_t debug_wait;
+ struct kref ref;
unsigned int id; /* system unique id */
@@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
#endif /* CONFIG_BPF */
};
+void hid_hiddev_free(struct kref *ref);
+
#define to_hid_device(pdev) \
container_of(pdev, struct hid_device, dev)
--
2.34.1
^ permalink raw reply related
* [PATCH v2] HID: fix HID device resource race between HID core and debugging support
From: Charles Yi @ 2023-11-07 8:16 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Charles Yi
hid_debug_events_release releases resources bound to the HID device
instance. hid_device_release releases the underlying HID device
instance potentially before hid_debug_events_release has completed
releasing debug resources bound to the same HID device instance.
Reference count to prevent the HID device instance from being torn
down preemptively when HID debugging support is used. When count
reaches zero, release core resources of HID device instance using
hid_hiddev_free.
The crash:
[ 120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
[ 120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[ 120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
[ 120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
[ 120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
[ 120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[ 120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
[ 120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
[ 120.779120][ T4396] sp : ffffffc01e62bb60
[ 120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
[ 120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
[ 120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
[ 120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
[ 120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
[ 120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
[ 120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
[ 120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
[ 120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
[ 120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
[ 120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
[ 120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
[ 120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
[ 120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
[ 120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
[ 120.873122][ T4396] Call trace:
[ 120.876259][ T4396] __list_del_entry_valid+0x98/0xac
[ 120.881304][ T4396] hid_debug_events_release+0x48/0x12c
[ 120.886617][ T4396] full_proxy_release+0x50/0xbc
[ 120.891323][ T4396] __fput+0xdc/0x238
[ 120.895075][ T4396] ____fput+0x14/0x24
[ 120.898911][ T4396] task_work_run+0x90/0x148
[ 120.903268][ T4396] do_exit+0x1bc/0x8a4
[ 120.907193][ T4396] do_group_exit+0x8c/0xa4
[ 120.911458][ T4396] get_signal+0x468/0x744
[ 120.915643][ T4396] do_signal+0x84/0x280
[ 120.919650][ T4396] do_notify_resume+0xd0/0x218
[ 120.924262][ T4396] work_pending+0xc/0x3f0
Fixes: cd667ce24796 ("HID: use debugfs for events/reports dumping")
Signed-off-by: Charles Yi <be286@163.com>
---
Changes in V2:
-The formatting of the "Fixes" tag
---
drivers/hid/hid-core.c | 12 ++++++++++--
drivers/hid/hid-debug.c | 3 +++
include/linux/hid.h | 3 +++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8992e3c1e769..bd2f58e0e87a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
* Free a device structure, all reports, and all fields.
*/
-static void hid_device_release(struct device *dev)
+void hid_hiddev_free(struct kref *ref)
{
- struct hid_device *hid = to_hid_device(dev);
+ struct hid_device *hid = container_of(ref, struct hid_device, ref);
hid_close_report(hid);
kfree(hid->dev_rdesc);
kfree(hid);
}
+static void hid_device_release(struct device *dev)
+{
+ struct hid_device *hid = to_hid_device(dev);
+
+ kref_put(&hid->ref, hid_hiddev_free);
+}
+
/*
* Fetch a report description item from the data stream. We support long
* items, though they are not used yet.
@@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
spin_lock_init(&hdev->debug_list_lock);
sema_init(&hdev->driver_input_lock, 1);
mutex_init(&hdev->ll_open_lock);
+ kref_init(&hdev->ref);
hid_bpf_device_init(hdev);
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index e7ef1ea107c9..36c47012d79f 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
goto out;
}
list->hdev = (struct hid_device *) inode->i_private;
+ kref_get(&list->hdev->ref);
file->private_data = list;
mutex_init(&list->read_mutex);
@@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
list_del(&list->node);
spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
kfifo_free(&list->hid_debug_fifo);
+
+ kref_put(&list->hdev->ref, hid_hiddev_free);
kfree(list);
return 0;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 964ca1f15e3f..7867f571376c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
struct list_head debug_list;
spinlock_t debug_list_lock;
wait_queue_head_t debug_wait;
+ struct kref ref;
unsigned int id; /* system unique id */
@@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
#endif /* CONFIG_BPF */
};
+void hid_hiddev_free(struct kref *ref);
+
#define to_hid_device(pdev) \
container_of(pdev, struct hid_device, dev)
--
2.34.1
^ permalink raw reply related
* Re[2]: [PATCH] input: gpio-keys - optimize wakeup sequence.
From: Abhishek Kumar Singh @ 2023-11-07 8:29 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com
Cc: robh@kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, SRI-N IT Security
In-Reply-To: <ZT2_XI-6D24gjbrO@google.com>
Dear Dmitry,
Greetings!!!
Thank you so much for your response.
I checked in detailed again and observed the below points, please help to review
and approve it.
There is ISR "gpio_keys_gpio_isr" which is called when the key state is 1 i.e.
key pressed.
Therefore modified code will not impact on the existing driver code.
//For key pressed event:
<3>[ 549.180072] I[0: swapper/0: 0] gpio_keys_gpio_isr
<3>[ 549.196126] [1: kworker/1:1: 78] gpio_keys_gpio_work_func
<3>[ 549.196198] [1: kworker/1:1: 78] gpio-keys soc:gpio_keys: gpio_keys_gpio_report_event key = 115, value = 1
Performance:
I have calculated the differece between entry & exit time
with modified and without modified code and observed that
0.3ms extra computation time in current scenario in each entry/exit time.
Because below APIs will not be called in every resume functions:
1. static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
2. static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
3. gpiod_get_value_cansleep(bdata->gpiod);
4. input_event(input, type, *bdata->code, state);
5. input_sync(input)
So we can save 0.3ms computation time, resume operations will faster and save battery as well.
With changes:
Line 311960: 07-18 16:50:09.359 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:20:37.573207725 UTC
Line 312626: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:20.503579404 UTC
Line 312627: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:20.503656644 UTC
Line 313301: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:33.865626325 UTC
Line 313302: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:33.865724502 UTC
Line 313572: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:37.678468979 UTC
Line 313573: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:37.678566167 UTC
Line 314209: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:23:05.925340634 UTC
Line 314210: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:23:05.925439384 UTC
Without changes:
Line 372095: 07-18 16:10:24.250 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:43:38.592548979 UTC
Line 372344: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:11.589164226 UTC
Line 372346: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:11.589514955 UTC
Line 372573: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:22.606227138 UTC
Line 372575: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:22.606490107 UTC
Line 372944: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:29.024121927 UTC
Line 372946: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:29.024528958 UTC
Line 373181: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:30.904866770 UTC
Line 373183: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:30.905126353 UTC
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India
--------- Original Message ---------
Sender : dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
Date : 2023-10-29 07:42 (GMT+5:30)
Title : Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
To : Abhishek Kumar Singh<abhi1.singh@samsung.com>
CC : robh@kernel.org<robh@kernel.org>, linux-input@vger.kernel.org<linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org<linux-kernel@vger.kernel.org>, SRI-N IT Security<sri-n.itsec@samsung.com>
Hi Abhishek,
On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
>
>
> The patch removes unused many APIs call chain for every suspend/resume of the device
> if no key press event triggered.
>
>
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
>
>
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
>
>
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time
Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.
I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.
Thanks.
--
Dmitry
Thanks and Regards,
^ permalink raw reply
* Re: [PATCH 3/7] ARM: dts: qcom: Add support for Samsung Galaxy Tab 4 8.0 Wi-Fi
From: Bryant Mairs @ 2023-11-07 11:26 UTC (permalink / raw)
To: Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Linus Walleij, linux-arm-msm, devicetree, linux-kernel,
linux-input
In-Reply-To: <466ffc6c-19c9-44e0-b97b-8fa4358f34ab@linaro.org>
Indeed, very sorry about that! Forgot where I left off when doing this and missed an (the?) important step.
I'll take the feedback I've already gotten, clean this up, make sure things build ;), and resubmit soon!
- B
On November 7, 2023 8:29:10 AM GMT+01:00, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>On 05/11/2023 21:46, Bryant Mairs wrote:
>> Add support for this tablet based on the MSM8226 SoC, codenamed
>> "milletwifi".
>>
>> Signed-off-by: Bryant Mairs <bryant@mai.rs>
>> ---
>> arch/arm/boot/dts/qcom/Makefile | 1 +
>> .../qcom/qcom-apq8026-samsung-milletwifi.dts | 543 ++++++++++++++++++
>> 2 files changed, 544 insertions(+)
>> create mode 100644 arch/arm/boot/dts/qcom/qcom-apq8026-samsung-milletwifi.dts
>>
>
>LKP reports that patches were most likely not even built :(
>
>Best regards,
>Krzysztof
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox