* [PATCH v4 08/24] Input: chipone_icn8318 - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/chipone_icn8318.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/chipone_icn8318.c b/drivers/input/touchscreen/chipone_icn8318.c
index 9fbeaf17f00b..d6876d10b252 100644
--- a/drivers/input/touchscreen/chipone_icn8318.c
+++ b/drivers/input/touchscreen/chipone_icn8318.c
@@ -191,12 +191,8 @@ static int icn8318_probe(struct i2c_client *client)
return -ENOMEM;
data->wake_gpio = devm_gpiod_get(dev, "wake", GPIOD_OUT_LOW);
- if (IS_ERR(data->wake_gpio)) {
- error = PTR_ERR(data->wake_gpio);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Error getting wake gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(data->wake_gpio))
+ return dev_err_probe(dev, PTR_ERR(data->wake_gpio), "Error getting wake gpio\n");
input = devm_input_allocate_device(dev);
if (!input)
--
2.34.1
^ permalink raw reply related
* [PATCH v4 07/24] Input: bu21029_ts - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:28 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/touchscreen/bu21029_ts.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/input/touchscreen/bu21029_ts.c b/drivers/input/touchscreen/bu21029_ts.c
index c8126d2efe95..3d81ebe66b66 100644
--- a/drivers/input/touchscreen/bu21029_ts.c
+++ b/drivers/input/touchscreen/bu21029_ts.c
@@ -359,23 +359,15 @@ static int bu21029_probe(struct i2c_client *client)
}
bu21029->vdd = devm_regulator_get(&client->dev, "vdd");
- if (IS_ERR(bu21029->vdd)) {
- error = PTR_ERR(bu21029->vdd);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "failed to acquire 'vdd' supply: %d\n", error);
- return error;
- }
+ if (IS_ERR(bu21029->vdd))
+ return dev_err_probe(&client->dev, PTR_ERR(bu21029->vdd),
+ "failed to acquire 'vdd' supply\n");
bu21029->reset_gpios = devm_gpiod_get_optional(&client->dev,
"reset", GPIOD_OUT_HIGH);
- if (IS_ERR(bu21029->reset_gpios)) {
- error = PTR_ERR(bu21029->reset_gpios);
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev,
- "failed to acquire 'reset' gpio: %d\n", error);
- return error;
- }
+ if (IS_ERR(bu21029->reset_gpios))
+ return dev_err_probe(&client->dev, PTR_ERR(bu21029->reset_gpios),
+ "failed to acquire 'reset' gpio\n");
in_dev = devm_input_allocate_device(&client->dev);
if (!in_dev) {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 06/24] Input: bu21013_ts - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes since v1:
1. Remove unneeded PTR_ERR_OR_ZERO, as pointed by Andy.
---
drivers/input/touchscreen/bu21013_ts.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c
index 85332cfaa29d..f811677a59f7 100644
--- a/drivers/input/touchscreen/bu21013_ts.c
+++ b/drivers/input/touchscreen/bu21013_ts.c
@@ -495,12 +495,10 @@ static int bu21013_probe(struct i2c_client *client)
/* Named "CS" on the chip, DT binding is "reset" */
ts->cs_gpiod = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
- error = PTR_ERR_OR_ZERO(ts->cs_gpiod);
- if (error) {
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev, "failed to get CS GPIO\n");
- return error;
- }
+ if (IS_ERR(ts->cs_gpiod))
+ return dev_err_probe(&client->dev, PTR_ERR(ts->cs_gpiod),
+ "failed to get CS GPIO\n");
+
gpiod_set_consumer_name(ts->cs_gpiod, "BU21013 CS");
error = devm_add_action_or_reset(&client->dev,
@@ -515,11 +513,8 @@ static int bu21013_probe(struct i2c_client *client)
ts->int_gpiod = devm_gpiod_get_optional(&client->dev,
"touch", GPIOD_IN);
error = PTR_ERR_OR_ZERO(ts->int_gpiod);
- if (error) {
- if (error != -EPROBE_DEFER)
- dev_err(&client->dev, "failed to get INT GPIO\n");
- return error;
- }
+ if (error)
+ return dev_err_probe(&client->dev, error, "failed to get INT GPIO\n");
if (ts->int_gpiod)
gpiod_set_consumer_name(ts->int_gpiod, "BU21013 INT");
--
2.34.1
^ permalink raw reply related
* [PATCH v4 05/24] Input: elan_i2c - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/mouse/elan_i2c_core.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 0cff742302a9..148a601396f9 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1221,13 +1221,8 @@ static int elan_probe(struct i2c_client *client)
mutex_init(&data->sysfs_mutex);
data->vcc = devm_regulator_get(dev, "vcc");
- if (IS_ERR(data->vcc)) {
- error = PTR_ERR(data->vcc);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get 'vcc' regulator: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(data->vcc))
+ return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
error = regulator_enable(data->vcc);
if (error) {
--
2.34.1
^ permalink raw reply related
* [PATCH v4 04/24] Input: rotary_encoder - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/misc/rotary_encoder.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 22ec62083065..fb3a34f8eccd 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -236,12 +236,8 @@ static int rotary_encoder_probe(struct platform_device *pdev)
device_property_read_bool(dev, "rotary-encoder,relative-axis");
encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
- if (IS_ERR(encoder->gpios)) {
- err = PTR_ERR(encoder->gpios);
- if (err != -EPROBE_DEFER)
- dev_err(dev, "unable to get gpios: %d\n", err);
- return err;
- }
+ if (IS_ERR(encoder->gpios))
+ return dev_err_probe(dev, PTR_ERR(encoder->gpios), "unable to get gpios\n");
if (encoder->gpios->ndescs < 2) {
dev_err(dev, "not enough gpios found\n");
return -EINVAL;
--
2.34.1
^ permalink raw reply related
* [PATCH v4 03/24] Input: pwm-vibra - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes since v3:
1. Rebase, adjust to new driver changes.
Changes since v1:
1. Remove unneeded PTR_ERR_OR_ZERO, as pointed by Andy.
---
drivers/input/misc/pwm-vibra.c | 30 +++++++++---------------------
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
index 2ba035299db8..a3cde30ee8d2 100644
--- a/drivers/input/misc/pwm-vibra.c
+++ b/drivers/input/misc/pwm-vibra.c
@@ -140,32 +140,20 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
return -ENOMEM;
vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
- err = PTR_ERR_OR_ZERO(vibrator->vcc);
- if (err) {
- if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request regulator: %d\n",
- err);
- return err;
- }
+ if (IS_ERR(vibrator->vcc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vibrator->vcc),
+ "Failed to request regulator\n");
vibrator->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
GPIOD_OUT_LOW);
- err = PTR_ERR_OR_ZERO(vibrator->enable_gpio);
- if (err) {
- if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request enable gpio: %d\n",
- err);
- return err;
- }
+ if (IS_ERR(vibrator->enable_gpio))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vibrator->enable_gpio),
+ "Failed to request enable gpio\n");
vibrator->pwm = devm_pwm_get(&pdev->dev, "enable");
- err = PTR_ERR_OR_ZERO(vibrator->pwm);
- if (err) {
- if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request main pwm: %d\n",
- err);
- return err;
- }
+ if (IS_ERR(vibrator->pwm))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vibrator->pwm),
+ "Failed to request main pwm\n");
INIT_WORK(&vibrator->play_work, pwm_vibrator_play_work);
--
2.34.1
^ permalink raw reply related
* [PATCH v4 02/24] Input: gpio-vibra - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes since v1:
1. Remove unneeded PTR_ERR_OR_ZERO, as pointed by Andy.
---
drivers/input/misc/gpio-vibra.c | 20 ++++++--------------
drivers/input/misc/pwm-beeper.c | 19 +++++--------------
2 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/drivers/input/misc/gpio-vibra.c b/drivers/input/misc/gpio-vibra.c
index 134a1309ba92..c1c3ba5960dd 100644
--- a/drivers/input/misc/gpio-vibra.c
+++ b/drivers/input/misc/gpio-vibra.c
@@ -113,22 +113,14 @@ static int gpio_vibrator_probe(struct platform_device *pdev)
return -ENOMEM;
vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
- err = PTR_ERR_OR_ZERO(vibrator->vcc);
- if (err) {
- if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request regulator: %d\n",
- err);
- return err;
- }
+ if (IS_ERR(vibrator->vcc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vibrator->vcc),
+ "Failed to request regulator\n");
vibrator->gpio = devm_gpiod_get(&pdev->dev, "enable", GPIOD_OUT_LOW);
- err = PTR_ERR_OR_ZERO(vibrator->gpio);
- if (err) {
- if (err != -EPROBE_DEFER)
- dev_err(&pdev->dev, "Failed to request main gpio: %d\n",
- err);
- return err;
- }
+ if (IS_ERR(vibrator->gpio))
+ return dev_err_probe(&pdev->dev, PTR_ERR(vibrator->gpio),
+ "Failed to request main gpio\n");
INIT_WORK(&vibrator->play_work, gpio_vibrator_play_work);
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 3cf1812384e6..1e731d8397c6 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -132,13 +132,8 @@ static int pwm_beeper_probe(struct platform_device *pdev)
return -ENOMEM;
beeper->pwm = devm_pwm_get(dev, NULL);
- if (IS_ERR(beeper->pwm)) {
- error = PTR_ERR(beeper->pwm);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to request PWM device: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(beeper->pwm))
+ return dev_err_probe(dev, PTR_ERR(beeper->pwm), "Failed to request PWM device\n");
/* Sync up PWM state and ensure it is off. */
pwm_init_state(beeper->pwm, &state);
@@ -151,13 +146,9 @@ static int pwm_beeper_probe(struct platform_device *pdev)
}
beeper->amplifier = devm_regulator_get(dev, "amp");
- if (IS_ERR(beeper->amplifier)) {
- error = PTR_ERR(beeper->amplifier);
- if (error != -EPROBE_DEFER)
- dev_err(dev, "Failed to get 'amp' regulator: %d\n",
- error);
- return error;
- }
+ if (IS_ERR(beeper->amplifier))
+ return dev_err_probe(dev, PTR_ERR(beeper->amplifier),
+ "Failed to get 'amp' regulator\n");
INIT_WORK(&beeper->work, pwm_beeper_work);
--
2.34.1
^ permalink raw reply related
* [PATCH v4 01/24] Input: gpio_keys_polled - Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski, Andy Shevchenko
In-Reply-To: <20230625162817.100397-1-krzysztof.kozlowski@linaro.org>
Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and also it prints the error value.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/input/keyboard/gpio_keys_polled.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index c3937d2fc744..ba00ecfbd343 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -299,13 +299,9 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
NULL, GPIOD_IN,
button->desc);
if (IS_ERR(bdata->gpiod)) {
- error = PTR_ERR(bdata->gpiod);
- if (error != -EPROBE_DEFER)
- dev_err(dev,
- "failed to get gpio: %d\n",
- error);
fwnode_handle_put(child);
- return error;
+ return dev_err_probe(dev, PTR_ERR(bdata->gpiod),
+ "failed to get gpio\n");
}
} else if (gpio_is_valid(button->gpio)) {
/*
--
2.34.1
^ permalink raw reply related
* [PATCH v4 00/24] Input: Simplify with dev_err_probe()
From: Krzysztof Kozlowski @ 2023-06-25 16:27 UTC (permalink / raw)
To: Dmitry Torokhov, Hans de Goede, Linus Walleij, Bastien Nocera,
Sangwon Jee, Eugen Hristev, Mika Penttilä, linux-input,
linux-kernel, platform-driver-x86
Cc: Andi Shyti, Andy Shevchenko, Krzysztof Kozlowski
Hi,
Three years ago I sent v3 of this series. There was never an anwser from Dmitry
- no comment at all. Maybe after three years this can go in? It makes
the code nicely smaller.
Changes since v3:
1. Rebase
2. Drop gpio-keys patch as it depends on GPIO helpers and I am too bored to
rebase it.
v3: https://lore.kernel.org/all/20200827185829.30096-1-krzk@kernel.org/
Changes since v2:
1. Add review tags,
2. Fixes after review (see individual patches).
3. Two new patches - 26 and 27.
Best regards,
Krzysztof
Krzysztof Kozlowski (24):
Input: gpio_keys_polled - Simplify with dev_err_probe()
Input: gpio-vibra - Simplify with dev_err_probe()
Input: pwm-vibra - Simplify with dev_err_probe()
Input: rotary_encoder - Simplify with dev_err_probe()
Input: elan_i2c - Simplify with dev_err_probe()
Input: bu21013_ts - Simplify with dev_err_probe()
Input: bu21029_ts - Simplify with dev_err_probe()
Input: chipone_icn8318 - Simplify with dev_err_probe()
Input: cy8ctma140 - Simplify with dev_err_probe()
Input: edf-ft5x06 - Simplify with dev_err_probe()
Input: ektf2127 - Simplify with dev_err_probe()
Input: elants_i2c - Simplify with dev_err_probe()
Input: goodix - Simplify with dev_err_probe()
Input: melfas_mip4 - Simplify with dev_err_probe()
Input: pixcir_i2c_ts - Simplify with dev_err_probe()
Input: raydium_i2c_ts - Simplify with dev_err_probe()
Input: resistive-adc-touch - Simplify with dev_err_probe()
Input: silead - Simplify with dev_err_probe()
Input: sis_i2c - Simplify with dev_err_probe()
Input: surface3_spi - Simplify with dev_err_probe()
Input: sx8643 - Simplify with dev_err_probe()
Input: bcm-keypad - Simplify with dev_err_probe()
Input: bu21013_ts - Use local 'client->dev' variable in probe()
Input: bu21029_ts - Use local 'client->dev' variable in probe()
drivers/input/keyboard/bcm-keypad.c | 14 ++--
drivers/input/keyboard/gpio_keys_polled.c | 8 +--
drivers/input/misc/gpio-vibra.c | 20 ++----
drivers/input/misc/pwm-beeper.c | 19 ++---
drivers/input/misc/pwm-vibra.c | 30 +++-----
drivers/input/misc/rotary_encoder.c | 8 +--
drivers/input/mouse/elan_i2c_core.c | 9 +--
drivers/input/touchscreen/bu21013_ts.c | 72 ++++++++-----------
drivers/input/touchscreen/bu21029_ts.c | 51 +++++--------
drivers/input/touchscreen/chipone_icn8318.c | 8 +--
drivers/input/touchscreen/cy8ctma140.c | 8 +--
drivers/input/touchscreen/edt-ft5x06.c | 10 +--
drivers/input/touchscreen/ektf2127.c | 8 +--
drivers/input/touchscreen/elants_i2c.c | 22 ++----
drivers/input/touchscreen/goodix.c | 40 +++--------
drivers/input/touchscreen/melfas_mip4.c | 9 +--
drivers/input/touchscreen/pixcir_i2c_ts.c | 38 ++++------
drivers/input/touchscreen/raydium_i2c_ts.c | 30 +++-----
.../input/touchscreen/resistive-adc-touch.c | 8 +--
drivers/input/touchscreen/silead.c | 8 +--
drivers/input/touchscreen/sis_i2c.c | 20 ++----
drivers/input/touchscreen/surface3_spi.c | 13 +---
drivers/input/touchscreen/sx8654.c | 10 +--
23 files changed, 146 insertions(+), 317 deletions(-)
--
2.34.1
^ permalink raw reply
* [RESEND PATCH 2/2] HID: logitech-hidpp: Add support for the Pro X Superlight
From: Mavroudis Chatzilazaridis @ 2023-06-25 16:21 UTC (permalink / raw)
To: jikos; +Cc: linux-input, benjamin.tissoires, lains, Mavroudis Chatzilazaridis
In-Reply-To: <20230625162131.17921-1-mavchatz@protonmail.com>
This patch adds support for the Pro X Superlight. Tested over USB.
Co-developed-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Mavroudis Chatzilazaridis <mavchatz@protonmail.com>
---
drivers/hid/hid-logitech-hidpp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5e1a412fd28f..a718535fb87d 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4616,6 +4616,8 @@ static const struct hid_device_id hidpp_devices[] = {
.driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS },
{ /* Logitech G Pro Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
+ { /* Logitech G Pro X Superlight Gaming Mouse over USB */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC094) },
{ /* G935 Gaming Headset */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0x0a87),
--
2.34.1
^ permalink raw reply related
* [RESEND PATCH 1/2] HID: logitech-dj: Add support for new lightspeed receiver iteration
From: Mavroudis Chatzilazaridis @ 2023-06-25 16:21 UTC (permalink / raw)
To: jikos; +Cc: linux-input, benjamin.tissoires, lains, Mavroudis Chatzilazaridis
The lightspeed receiver for the Pro X Superlight uses 13 byte mouse reports
without a report id. The workaround for such cases has been adjusted to
handle these larger packets.
libratbag recognizes the device and input events are passing through.
https://github.com/libratbag/libratbag/pull/1122
Co-developed-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Mavroudis Chatzilazaridis <mavchatz@protonmail.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-logitech-dj.c | 11 ++++++++---
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5d29abac2300..ea3a1e7be2c7 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -864,6 +864,7 @@
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2 0xc534
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f
+#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2 0xc547
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a
#define USB_DEVICE_ID_SPACETRAVELLER 0xc623
#define USB_DEVICE_ID_SPACENAVIGATOR 0xc626
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 62180414efcc..fef67da0de53 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1692,11 +1692,12 @@ static int logi_dj_raw_event(struct hid_device *hdev,
}
/*
* Mouse-only receivers send unnumbered mouse data. The 27 MHz
- * receiver uses 6 byte packets, the nano receiver 8 bytes.
+ * receiver uses 6 byte packets, the nano receiver 8 bytes,
+ * the lightspeed receiver (Pro X Superlight) 13 bytes.
*/
if (djrcv_dev->unnumbered_application == HID_GD_MOUSE &&
- size <= 8) {
- u8 mouse_report[9];
+ size <= 13){
+ u8 mouse_report[14];
/* Prepend report id */
mouse_report[0] = REPORT_TYPE_MOUSE;
@@ -1980,6 +1981,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1),
.driver_data = recvr_type_gaming_hidpp},
+ { /* Logitech lightspeed receiver (0xc547) */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+ USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_2),
+ .driver_data = recvr_type_gaming_hidpp},
{ /* Logitech 27 MHz HID++ 1.0 receiver (0xc513) */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER),
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Bastien Nocera @ 2023-06-25 8:29 UTC (permalink / raw)
To: Benjamin Tissoires, Greg KH
Cc: Filipe Laíns, Jiri Kosina, linux-input, linux-kernel, stable
In-Reply-To: <qbvmv3eexohswyagmllfh3xsxoftwa3wbmsdafmwak2bxlnlft@jz74dijlfxlz>
On Fri, 2023-06-23 at 10:37 +0200, Benjamin Tissoires wrote:
>
> On Jun 21 2023, Greg KH wrote:
> >
> > On Wed, Jun 21, 2023 at 11:42:30AM +0200, Benjamin Tissoires wrote:
> > > Make the code looks less like Pascal.
> > >
> > > Extract the internal code inside a helper function, fix the
> > > initialization of the parameters used in the helper function
> > > (`hidpp->answer_available` was not reset and `*response` wasn't
> > > too),
> > > and use a `do {...} while();` loop.
> > >
> > > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when
> > > device is busy")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > > as requested by
> > > https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> > > This is a rewrite of that particular piece of code.
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++--
> > > --------------
> > > 1 file changed, 61 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > > logitech-hidpp.c
> > > index dfe8e09a18de..3d1ffe199f08 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct
> > > hid_device *hdev,
> > > }
> > >
> > > /*
> > > - * hidpp_send_message_sync() returns 0 in case of success, and
> > > something else
> > > - * in case of a failure.
> > > - * - If ' something else' is positive, that means that an error
> > > has been raised
> > > - * by the protocol itself.
> > > - * - If ' something else' is negative, that means that we had a
> > > classic error
> > > - * (-ENOMEM, -EPIPE, etc...)
> > > + * Effectively send the message to the device, waiting for its
> > > answer.
> > > + *
> > > + * Must be called with hidpp->send_mutex locked
> > > + *
> > > + * Same return protocol than hidpp_send_message_sync():
> > > + * - success on 0
> > > + * - negative error means transport error
> > > + * - positive value means protocol error
> > > */
> > > -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > > +static int __do_hidpp_send_message_sync(struct hidpp_device
> > > *hidpp,
> > > struct hidpp_report *message,
> > > struct hidpp_report *response)
> >
> > __must_hold(&hidpp->send_mutex) ?
> >
>
> Good point. I'll add this in v2.
>
> I'm still waiting for some feedback from the people who particpated
> in
> the original BZ, but the new bug is harder to reproduce. Anyway,
> there
> is no rush IMO.
The problem is only ever going to show up in very limited circumstances
after the logic fix was applied.
You need a hardware problem (such as the controller being too busy to
answer) to trigger the problems fixed by this patch. I don't see a way
to reliably reproduce it unless you inject that hardware error.
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Bastien Nocera @ 2023-06-25 8:29 UTC (permalink / raw)
To: Benjamin Tissoires, Filipe Laíns, Jiri Kosina
Cc: linux-input, linux-kernel, stable
In-Reply-To: <20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com>
On Wed, 2023-06-21 at 11:42 +0200, Benjamin Tissoires wrote:
> Make the code looks less like Pascal.
Honestly, while this was written in jest in an email is fine, putting
this in the commit message is quite insulting.
The "retry" patch tried to fix real world problems by making minimal
code changes, eg. avoiding the review problem that the present patch
has, and even then, all of us missed the logic bug.
I also haven't written any Pascal code since 1996.
> Extract the internal code inside a helper function, fix the
> initialization of the parameters used in the helper function
> (`hidpp->answer_available` was not reset and `*response` wasn't too),
"wasn't either".
> and use a `do {...} while();` loop.
>
> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device
> is busy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> as requested by
> https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> This is a rewrite of that particular piece of code.
> ---
> drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++------
> ----------
> 1 file changed, 61 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> logitech-hidpp.c
> index dfe8e09a18de..3d1ffe199f08 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct
> hid_device *hdev,
> }
>
> /*
> - * hidpp_send_message_sync() returns 0 in case of success, and
> something else
> - * in case of a failure.
> - * - If ' something else' is positive, that means that an error has
> been raised
> - * by the protocol itself.
> - * - If ' something else' is negative, that means that we had a
> classic error
> - * (-ENOMEM, -EPIPE, etc...)
> + * Effectively send the message to the device, waiting for its
> answer.
> + *
> + * Must be called with hidpp->send_mutex locked
> + *
> + * Same return protocol than hidpp_send_message_sync():
> + * - success on 0
> + * - negative error means transport error
> + * - positive value means protocol error
> */
> -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
> struct hidpp_report *message,
> struct hidpp_report *response)
> {
> - int ret = -1;
> - int max_retries = 3;
> -
> - mutex_lock(&hidpp->send_mutex);
> + int ret;
>
> hidpp->send_receive_buf = response;
> hidpp->answer_available = false;
> @@ -300,41 +299,62 @@ static int hidpp_send_message_sync(struct
> hidpp_device *hidpp,
> */
> *response = *message;
>
> - for (; max_retries != 0 && ret; max_retries--) {
> - ret = __hidpp_send_report(hidpp->hid_dev, message);
> + ret = __hidpp_send_report(hidpp->hid_dev, message);
> + if (ret) {
> + dbg_hid("__hidpp_send_report returned err: %d\n",
> ret);
> + memset(response, 0, sizeof(struct hidpp_report));
> + return ret;
> + }
>
> - if (ret) {
> - dbg_hid("__hidpp_send_report returned err:
> %d\n", ret);
> - memset(response, 0, sizeof(struct
> hidpp_report));
> - break;
> - }
> + if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
> + 5*HZ)) {
> + dbg_hid("%s:timeout waiting for response\n",
> __func__);
> + memset(response, 0, sizeof(struct hidpp_report));
> + return -ETIMEDOUT;
> + }
>
> - if (!wait_event_timeout(hidpp->wait, hidpp-
> >answer_available,
> - 5*HZ)) {
> - dbg_hid("%s:timeout waiting for response\n",
> __func__);
> - memset(response, 0, sizeof(struct
> hidpp_report));
> - ret = -ETIMEDOUT;
> - break;
> - }
> + if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> + response->rap.sub_id == HIDPP_ERROR) {
> + ret = response->rap.params[1];
> + dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> + return ret;
> + }
>
> - if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> - response->rap.sub_id == HIDPP_ERROR) {
> - ret = response->rap.params[1];
> - dbg_hid("%s:got hidpp error %02X\n",
> __func__, ret);
> + if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> + response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
> + response->fap.feature_index == HIDPP20_ERROR) {
> + ret = response->fap.params[1];
> + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__,
> ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * hidpp_send_message_sync() returns 0 in case of success, and
> something else
> + * in case of a failure.
> + * - If ' something else' is positive, that means that an error has
> been raised
> + * by the protocol itself.
> + * - If ' something else' is negative, that means that we had a
> classic error
> + * (-ENOMEM, -EPIPE, etc...)
Do we really need to re-explain the possible return values that were
already explained above __do_hidpp_send_message_sync()?
If we do, why don't also do it for hidpp_send_fap_command_sync() and
hidpp_send_rap_command_sync(), or their callers?
If it's absolutely necessary, a "see __do_hidpp_send_message_sync()"
should be enough.
I've double-checked that none of the existing callers expected a
partially filled in "response" struct on error.
Reviewed-by: Bastien Nocera <hadess@hadess.net>
> + */
> +static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> + struct hidpp_report *message,
> + struct hidpp_report *response)
> +{
> + int ret;
> + int max_retries = 3;
> +
> + mutex_lock(&hidpp->send_mutex);
> +
> + do {
> + ret = __do_hidpp_send_message_sync(hidpp, message,
> response);
> + if (ret != HIDPP20_ERROR_BUSY)
> break;
> - }
>
> - if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> - response->report_id ==
> REPORT_ID_HIDPP_VERY_LONG) &&
> - response->fap.feature_index == HIDPP20_ERROR) {
> - ret = response->fap.params[1];
> - if (ret != HIDPP20_ERROR_BUSY) {
> - dbg_hid("%s:got hidpp 2.0 error
> %02X\n", __func__, ret);
> - break;
> - }
> - dbg_hid("%s:got busy hidpp 2.0 error %02X,
> retrying\n", __func__, ret);
> - }
> - }
> + dbg_hid("%s:got busy hidpp 2.0 error %02X,
> retrying\n", __func__, ret);
> + } while (--max_retries);
>
> mutex_unlock(&hidpp->send_mutex);
> return ret;
>
> ---
> base-commit: b98ec211af5508457e2b1c4cc99373630a83fa81
> change-id: 20230621-logitech-fixes-a4c0e66ea2ad
>
> Best regards,
^ permalink raw reply
* [syzbot] [input?] INFO: task hung in uhid_char_release
From: syzbot @ 2023-06-24 3:16 UTC (permalink / raw)
To: benjamin.tissoires, david.rheinsberg, jikos, linux-input,
linux-kernel, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: f7efed9f38f8 Add linux-next specific files for 20230616
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=11ec12f3280000
kernel config: https://syzkaller.appspot.com/x/.config?x=60b1a32485a77c16
dashboard link: https://syzkaller.appspot.com/bug?extid=8fe2d362af0e1cba8735
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=152875ef280000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1629f75b280000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/95bcbee03439/disk-f7efed9f.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/6fd295caa4de/vmlinux-f7efed9f.xz
kernel image: https://storage.googleapis.com/syzbot-assets/69c038a34b5f/bzImage-f7efed9f.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+8fe2d362af0e1cba8735@syzkaller.appspotmail.com
INFO: task syz-executor188:5032 blocked for more than 143 seconds.
Not tainted 6.4.0-rc6-next-20230616-syzkaller #0
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:syz-executor188 state:D
stack:27424 pid:5032 ppid:5028 flags:0x00004002
Call Trace:
<TASK>
context_switch kernel/sched/core.c:5380 [inline]
__schedule+0x1d15/0x5790 kernel/sched/core.c:6709
schedule+0xde/0x1a0 kernel/sched/core.c:6785
schedule_timeout+0x276/0x2b0 kernel/time/timer.c:2143
do_wait_for_common kernel/sched/completion.c:85 [inline]
__wait_for_common+0x1ce/0x5c0 kernel/sched/completion.c:106
__flush_work+0x595/0xb60 kernel/workqueue.c:3383
__cancel_work_timer+0x3f9/0x570 kernel/workqueue.c:3470
uhid_dev_destroy drivers/hid/uhid.c:585 [inline]
uhid_char_release+0xca/0x210 drivers/hid/uhid.c:663
__fput+0x289/0xac0 fs/file_table.c:378
task_work_run+0x16f/0x270 kernel/task_work.c:179
exit_task_work include/linux/task_work.h:38 [inline]
do_exit+0xadc/0x2a30 kernel/exit.c:874
do_group_exit+0xd4/0x2a0 kernel/exit.c:1024
__do_sys_exit_group kernel/exit.c:1035 [inline]
__se_sys_exit_group kernel/exit.c:1033 [inline]
__x64_sys_exit_group+0x3e/0x50 kernel/exit.c:1033
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fa196a81b19
RSP: 002b:00007ffc33991898 EFLAGS: 00000246
ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 00007fa196af5330 RCX: 00007fa196a81b19
RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffffffffffc0 R09: 0000000000000001
R10: 00007ffc33991360 R11: 0000000000000246 R12: 00007fa196af5330
R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000001
</TASK>
INFO: task syz-executor188:5038 blocked for more than 145 seconds.
Not tainted 6.4.0-rc6-next-20230616-syzkaller #0
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:syz-executor188 state:D
stack:28176 pid:5038 ppid:5036 flags:0x00004002
Call Trace:
<TASK>
context_switch kernel/sched/core.c:5380 [inline]
__schedule+0x1d15/0x5790 kernel/sched/core.c:6709
schedule+0xde/0x1a0 kernel/sched/core.c:6785
schedule_timeout+0x276/0x2b0 kernel/time/timer.c:2143
do_wait_for_common kernel/sched/completion.c:85 [inline]
__wait_for_common+0x1ce/0x5c0 kernel/sched/completion.c:106
__flush_work+0x595/0xb60 kernel/workqueue.c:3383
__cancel_work_timer+0x3f9/0x570 kernel/workqueue.c:3470
uhid_dev_destroy drivers/hid/uhid.c:585 [inline]
uhid_char_release+0xca/0x210 drivers/hid/uhid.c:663
__fput+0x289/0xac0 fs/file_table.c:378
task_work_run+0x16f/0x270 kernel/task_work.c:179
exit_task_work include/linux/task_work.h:38 [inline]
do_exit+0xadc/0x2a30 kernel/exit.c:874
do_group_exit+0xd4/0x2a0 kernel/exit.c:1024
__do_sys_exit_group kernel/exit.c:1035 [inline]
__se_sys_exit_group kernel/exit.c:1033 [inline]
__x64_sys_exit_group+0x3e/0x50 kernel/exit.c:1033
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fa196a81b19
RSP: 002b:00007ffc33991898 EFLAGS: 00000246
ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 00007fa196af5330 RCX: 00007fa196a81b19
RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffffffffffc0 R09: 0000000000000001
R10: 00007ffc33991360 R11: 0000000000000246 R12: 00007fa196af5330
R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000001
</TASK>
INFO: task syz-executor188:5042 blocked for more than 147 seconds.
Not tainted 6.4.0-rc6-next-20230616-syzkaller #0
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:syz-executor188 state:D stack:27136 pid:5042 ppid:5037 flags:0x00004002
Call Trace:
<TASK>
context_switch kernel/sched/core.c:5380 [inline]
__schedule+0x1d15/0x5790 kernel/sched/core.c:6709
schedule+0xde/0x1a0 kernel/sched/core.c:6785
schedule_timeout+0x276/0x2b0 kernel/time/timer.c:2143
do_wait_for_common kernel/sched/completion.c:85 [inline]
__wait_for_common+0x1ce/0x5c0 kernel/sched/completion.c:106
__flush_work+0x595/0xb60 kernel/workqueue.c:3383
__cancel_work_timer+0x3f9/0x570 kernel/workqueue.c:3470
uhid_dev_destroy drivers/hid/uhid.c:585 [inline]
uhid_char_release+0xca/0x210 drivers/hid/uhid.c:663
__fput+0x289/0xac0 fs/file_table.c:378
task_work_run+0x16f/0x270 kernel/task_work.c:179
exit_task_work include/linux/task_work.h:38 [inline]
do_exit+0xadc/0x2a30 kernel/exit.c:874
do_group_exit+0xd4/0x2a0 kernel/exit.c:1024
__do_sys_exit_group kernel/exit.c:1035 [inline]
__se_sys_exit_group kernel/exit.c:1033 [inline]
__x64_sys_exit_group+0x3e/0x50 kernel/exit.c:1033
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fa196a81b19
RSP: 002b:00007ffc33991898 EFLAGS: 00000246
ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 00007fa196af5330 RCX: 00007fa196a81b19
RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffffffffffc0 R09: 0000000000000001
R10: 00007ffc33991360 R11: 0000000000000246 R12: 00007fa196af5330
R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000001
</TASK>
INFO: task syz-executor188:5046 blocked for more than 150 seconds.
Not tainted 6.4.0-rc6-next-20230616-syzkaller #0
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:syz-executor188 state:D stack:28048 pid:5046 ppid:5029 flags:0x00004002
Call Trace:
<TASK>
context_switch kernel/sched/core.c:5380 [inline]
__schedule+0x1d15/0x5790 kernel/sched/core.c:6709
schedule+0xde/0x1a0 kernel/sched/core.c:6785
schedule_timeout+0x276/0x2b0 kernel/time/timer.c:2143
do_wait_for_common kernel/sched/completion.c:85 [inline]
__wait_for_common+0x1ce/0x5c0 kernel/sched/completion.c:106
__flush_work+0x595/0xb60 kernel/workqueue.c:3383
__cancel_work_timer+0x3f9/0x570 kernel/workqueue.c:3470
uhid_dev_destroy drivers/hid/uhid.c:585 [inline]
uhid_char_release+0xca/0x210 drivers/hid/uhid.c:663
__fput+0x289/0xac0 fs/file_table.c:378
task_work_run+0x16f/0x270 kernel/task_work.c:179
exit_task_work include/linux/task_work.h:38 [inline]
do_exit+0xadc/0x2a30 kernel/exit.c:874
do_group_exit+0xd4/0x2a0 kernel/exit.c:1024
__do_sys_exit_group kernel/exit.c:1035 [inline]
__se_sys_exit_group kernel/exit.c:1033 [inline]
__x64_sys_exit_group+0x3e/0x50 kernel/exit.c:1033
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7fa196a81b19
RSP: 002b:00007ffc33991898 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 00007fa196af5330 RCX: 00007fa196a81b19
RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffffffffffffffc0 R09: 0000000000000001
R10: 00007ffc33991360 R11: 0000000000000246 R12: 00007fa196af5330
R13: 0000000000000001 R14: 0000000000000000 R15: 0000000000000001
</TASK>
Showing all locks held in the system:
3 locks held by kworker/0:0/7:
1 lock held by rcu_tasks_kthre/13:
#0:
ffffffff8c9a1ab0
(
rcu_tasks.tasks_gp_mutex){+.+.}-{3:3}, at: rcu_tasks_one_gp+0x31/0xe60 kernel/rcu/tasks.h:568
1 lock held by rcu_tasks_trace/14:
#0:
ffffffff8c9a1770
(
rcu_tasks_trace.tasks_gp_mutex){+.+.}-{3:3}
, at: rcu_tasks_one_gp+0x31/0xe60 kernel/rcu/tasks.h:568
3 locks held by kworker/1:0/22:
1 lock held by khungtaskd/28:
#0: ffffffff8c9a26c0 (rcu_read_lock
){....}-{1:2}
, at: debug_show_all_locks+0x51/0x390 kernel/locking/lockdep.c:6615
3 locks held by kworker/1:2/776:
3 locks held by kworker/0:2/919:
3 locks held by kworker/0:3/3948:
2 locks held by getty/4779:
#0:
ffff88814a6b7098
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: [PATCH v2] HID: Add introduction about HID for non-kernel programmers
From: Randy Dunlap @ 2023-06-23 15:16 UTC (permalink / raw)
To: Marco Morandini, Peter Hutterer, Jiri Kosina, Benjamin Tissoires,
linux-input, Jonathan Corbet, linux-doc
In-Reply-To: <45d68dcb-7625-8592-fde6-60c9fbd5eaca@polimi.it>
Hi Marco,
On 6/23/23 03:45, Marco Morandini wrote:
>>> +The format of HID report descriptors is described by two documents,
>>> +available from the `USB Implementers Forum <https://www.usb.org/>`_
>>> +at `this <https://www.usb.org/hid>`_ addresses:
>>
>> these
>
> Not sure about this.
> "this" here was meant to be https://www.usb.org/hid , while the two documents are listed below.
>
> Would
>
> The format of HID report descriptors is described by two documents,
> available from the `USB Implementers Forum <https://www.usb.org/>`_
> `HID web page <https://www.usb.org/hid>`_:
>
> be ok?
Yes, that's good.
>>> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
>>> + it's actual Data (not constant padding), they represent
>>
>> its
>> ?
>> "its" is possessive. "it's" means "it is".
>>
>
> -> these bits are actual data
>
> ok?
Yes.
>>> + This time the data is Relative (Rel), i.e. it represent
>>
>> represents
>>
>>> +An HID devices can have Input Reports, like
>>
>> A HID device
>>
>
> -> HID devices that are not ....
>
> as suggested by Peter
> Ok?
Sure.
thanks.
--
~Randy
^ permalink raw reply
* [PATCH] hid-mcp2200: added driver for GPIOs of MCP2200
From: Johannes Roith @ 2023-06-23 11:01 UTC (permalink / raw)
To: jikos, benjamin.tissoires
Cc: linux-kernel, linux-input, christophe.jaillet, andi.shyti,
Johannes Roith
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.
Feedback from reviewers Christophe JAILLET <christophe.jaillet@wanadoo.fr>
and Andi Shyti <andi.shyti@kernel.org> was added.
Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
---
drivers/hid/Kconfig | 10 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-mcp2200.c | 416 ++++++++++++++++++++++++++++++++++++++
4 files changed, 428 insertions(+)
create mode 100644 drivers/hid/hid-mcp2200.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ce012f83253..ca7927d22c23 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1283,6 +1283,16 @@ config HID_MCP2221
To compile this driver as a module, choose M here: the module
will be called hid-mcp2221.ko.
+config HID_MCP2200
+ tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+ depends on USB_HID
+ imply 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_KUNIT_TEST
tristate "KUnit tests for HID" if !KUNIT_ALL_TESTS
depends on KUNIT=y
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 5d37cacbde33..d593fb982f7d 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_MACALLY) += hid-macally.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
+obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5d29abac2300..017e37a171a8 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -912,6 +912,7 @@
#define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
#define USB_DEVICE_ID_LUXAFOR 0xf372
#define USB_DEVICE_ID_MCP2221 0x00dd
+#define USB_DEVICE_ID_MCP2200 0x00df
#define USB_VENDOR_ID_MICROSOFT 0x045e
#define USB_DEVICE_ID_SIDEWINDER_GV 0x003b
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
new file mode 100644
index 000000000000..3a950365c0cd
--- /dev/null
+++ b/drivers/hid/hid-mcp2200.c
@@ -0,0 +1,416 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MCP2200 - Microchip USB to GPIO bridge
+ *
+ * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
+ *
+ * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
+ * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
+ */
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#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;
+};
+
+/* 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);
+ read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
+ if (!read_all)
+ return -ENOMEM;
+
+ read_all->cmd = READ_ALL;
+
+ mutex_lock(&mcp->lock);
+ len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
+ sizeof(struct mcp_read_all));
+
+ mutex_unlock(&mcp->lock);
+ kfree(read_all);
+
+ 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;
+
+ cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
+ if (!cmd)
+ return;
+
+ mutex_lock(&mcp->lock);
+
+ 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));
+
+ mutex_unlock(&mcp->lock);
+ kfree(cmd);
+
+ if (status == sizeof(struct mcp_set_clear_outputs))
+ mcp->gpio_val = value;
+}
+
+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 != 0)
+ 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) ? 1 : 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;
+
+ conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
+ if (!conf)
+ return -ENOMEM;
+ mutex_lock(&mcp->lock);
+
+ /* 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));
+
+ mutex_unlock(&mcp->lock);
+
+ if (status == sizeof(struct mcp_set_clear_outputs)) {
+ mcp->gpio_dir &= ~(1 << gpio_nr);
+ mcp->config_alt_pins = config_alt_pins;
+ } else {
+ return -EIO;
+ }
+
+ kfree(conf);
+ /* 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 == 0)
+ 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 1;
+}
+
+static void mcp2200_hid_unregister(void *ptr)
+{
+ struct hid_device *hdev = ptr;
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+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;
+ }
+
+ /*
+ * This driver uses the .raw_event callback and therefore does not need any
+ * HID_CONNECT_xxx flags.
+ */
+ 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;
+
+ ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
+ if (ret)
+ return ret;
+
+ mcp->gc = template_chip;
+ mcp->gc.parent = &hdev->dev;
+
+ ret = gpiochip_add_data(&mcp->gc, mcp);
+ if (ret < 0) {
+ dev_err(&hdev->dev, "Unable to register gpiochip\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void mcp2200_remove(struct hid_device *hdev)
+{
+ struct mcp2200 *mcp;
+
+ mcp = hid_get_drvdata(hdev);
+ gpiochip_remove(&mcp->gc);
+}
+
+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 <johannes@gnu-linux.rocks>");
+MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
+MODULE_LICENSE("GPL");
+
--
2.41.0
^ permalink raw reply related
* Re: [PATCH v2] HID: Add introduction about HID for non-kernel programmers
From: Marco Morandini @ 2023-06-23 10:45 UTC (permalink / raw)
To: Randy Dunlap, Peter Hutterer, Jiri Kosina, Benjamin Tissoires,
linux-input, Jonathan Corbet, linux-doc
In-Reply-To: <d34eae36-0d43-6cd5-f8c0-57e1bd30f338@infradead.org>
Thank you very much for your corrections.
I've applied almost everything (see below),
but I have two questions below.
Marco
>> +This chapter is meant to give a broad overview
>> +of what HID report descriptors are, and of how a casual (non kernel)
>
> (non-kernel)
Ok, thank you!
>
>> +programmer can deal with an HID device that
>
> a HID device that
-> with HID devices
as suggested by Peter
>> +may a wheel; movement sensitivity differs between different
>
> may have a wheel;
>
Ok
>> +The format of HID report descriptors is described by two documents,
>> +available from the `USB Implementers Forum <https://www.usb.org/>`_
>> +at `this <https://www.usb.org/hid>`_ addresses:
>
> these
Not sure about this.
"this" here was meant to be https://www.usb.org/hid , while the two documents are listed below.
Would
The format of HID report descriptors is described by two documents,
available from the `USB Implementers Forum <https://www.usb.org/>`_
`HID web page <https://www.usb.org/hid>`_:
be ok?
>> +In practice you should not do parse HID report descriptors by hand;
>
> s/do //
Ok
> and drop the line's trailing space. (Do that on any lines that have a
> trailing space.)
>
I forgot to run checkpatch, my bad. Will do for v3.
>> + It is being actively developed by the Linux HID subsystem mantainers.
>
> maintainers.
Ok
>> +
>> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
>> + it's actual Data (not constant padding), they represent
>
> its
> ?
> "its" is possessive. "it's" means "it is".
>
-> these bits are actual data
ok?
>> + a single variable (Var) and the value are Absolute (not relative);
>
> value is
> or values are
>
values are
thank you!
>> + This time the data is Relative (Rel), i.e. it represent
>
> represents
>
>> +An HID devices can have Input Reports, like
>
> A HID device
>
-> HID devices that are not ....
as suggested by Peter
Ok?
>> +Note that, however, that you can have different Report IDs
>
> Note, however, that
>
Thank you!
>> +
>> +There can be a number of reasons for why a device does not behave
>
> s/for //
>
Ok, thank you
>> +* the HID report descriptor may need some "quirks" (see later on);
>
> on).
Ok
>> +
>> +As a consequence, a suitable ``/dev/input/event*`` will not created
>
> not be created
>
Ok
>> +for each Application Collection, and/or the events
>> +there will match what you would expect.
>
> will not
> ?
>
will not
>> +
>> +
>> +Quirks
>> +------
>> +
>> +There are some known peculiarities of HID devices that the kernel
>> +knows how to fix - these are called the HID quirks and a list of those
>> +are available in `include/linux/hid.h`.
>
> is available
>
Ok
>> +
>> +Should this be the case,
>> +it should be enough to add the required quirk,
>
> quirk
> (no comma)
>
Ok
>> +should go into hid-quirks.c and **submitted upstream**.
>
> **be submitted upstream**.
>
Ok
>> +See, again, Documentation/process/submitting-patches.rst
>> +for guidelines on how to do submit a patch.
>
> how to submit a patch.
>
Ok
>> +for your code, see e.g. `samples/hid_mouse.bpf.c`::
>
> samples/hid/hid_mouse.bpf.c
>
Right.
>> +Check Documentation/hid/hidreport-parsing.rst
>> +if you need an help navigating the HID manuals and
>
> any
>
Ok
>> +and that particular HID device will will start
>
> will start
>
Ok
>> +
>> +.. [#hidraw] reading hidraw: see Documentation/hid/hidraw.rst and
>
> read
>
Ok
>> +We have an ``Usage Page``, thus we need to refer to HUT Sec. 3,
>
> have a
>
Ok
>> +is given in the HID spec Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
>
> have a
Ok
^ permalink raw reply
* Re: [PATCH v2 00/10] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together
From: Maxime Ripard @ 2023-06-23 9:08 UTC (permalink / raw)
To: Doug Anderson
Cc: Jiri Kosina, Benjamin Tissoires, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Neil Armstrong, Sam Ravnborg, Maarten Lankhorst,
Thomas Zimmermann, dri-devel, Dmitry Torokhov, linux-input,
Daniel Vetter, linux-kernel, hsinyi, cros-qcom-dts-watchers,
devicetree, yangcong5, linux-arm-msm, Chris Morgan
In-Reply-To: <CAD=FV=VO=GE5BEw6kKK19Qj9tcia509Pb-bvMcq0uA05sVLvHw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9423 bytes --]
On Tue, Jun 13, 2023 at 08:56:39AM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Jun 13, 2023 at 5:06 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > > > What I'm trying to say is: could we just make it work by passing a bunch
> > > > of platform_data, 2-3 callbacks and a device registration from the panel
> > > > driver directly?
> > >
> > > I think I'm still confused about what you're proposing. Sorry! :( Let
> > > me try rephrasing why I'm confused and perhaps we can get on the same
> > > page. :-)
> > >
> > > First, I guess I'm confused about how you have one of these devices
> > > "register" the other device.
> > >
> > > I can understand how one device might "register" its sub-devices in
> > > the MFD case. To make it concrete, we can look at a PMIC like
> > > max77686.c. The parent MFD device gets probed and then it's in charge
> > > of creating all of its sub-devices. These sub-devices are intimately
> > > tied to one another. They have shared data structures and can
> > > coordinate power sequencing and whatnot. All good.
> >
> > We don't necessarily need to use MFD, but yeah, we could just register a
> > device for the i2c-hid driver to probe from (using
> > i2c_new_client_device?)
>
> I think this can work for devices where the panel and touchscreen are
> truly integrated where the panel driver knows enough about the related
> touchscreen to fully describe and instantiate it. It doesn't work
> quite as well for cases where the power and reset lines are shared
> just because of what a given board designer did. To handle that, each
> panel driver would need to get enough DT properties added to it so
> that it could fully describe any arbitrary touchscreen, right?
>
> Let's think about the generic panel-edp driver. This driver runs the
> panel on many sc7180-trogdor laptops, including coachz, lazor, and
> pompom. All three of those boards have a shared power rail for the
> touchscreen and panel. If you look at "sc7180-trogdor-coachz.dtsi",
> you can see the touchscreen currently looks like this:
>
> ap_ts: touchscreen@5d {
> compatible = "goodix,gt7375p";
> reg = <0x5d>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In "sc7180-trogdor-lazor.dtsi" we have:
>
> ap_ts: touchscreen@10 {
> compatible = "hid-over-i2c";
> reg = <0x10>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_int_l>, <&ts_reset_l>;
>
> interrupt-parent = <&tlmm>;
> interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
>
> post-power-on-delay-ms = <20>;
> hid-descr-addr = <0x0001>;
>
> vdd-supply = <&pp3300_ts>;
> };
>
> In both cases "pp3300_ts" is simply another name for "pp3300_dx_edp"
>
> So I think to do what you propose, we need to add this information to
> the panel-edp DT node so that it could dynamically construct the i2c
> device for the touchscreen:
>
> a) Which touchscreen is actually connected (generic hid-over-i2c,
> goodix, ...). I guess this would be a "compatible" string?
>
> b) Which i2c bus that device is hooked up to.
>
> c) Which i2c address that device is hooked up to.
>
> d) What the touchscreen interrupt GPIO is.
>
> e) Possibly what the "hid-descr-addr" for the touchscreen is.
>
> f) Any extra timing information needed to be passed to the touchscreen
> driver, like "post-power-on-delay-ms"
>
> The "pinctrl" stuff would be easy to subsume into the panel's DT node,
> at least. ...and, in this case, we could skip the "vdd-supply" since
> the panel and eDP are sharing power rails (which is what got us into
> this situation). ...but, the above is still a lot. At this point, it
> would make sense to have a sub-node under the panel to describe it,
> which we could do but it starts to feel weird. We'd essentially be
> describing an i2c device but not under the i2c controller it belongs
> to.
>
> I guess I'd also say that the above design also need additional code
> if/when someone had a touchscreen that used a different communication
> method, like SPI.
>
> So I guess the tl;dr of all the above is that I think it could all work if:
>
> 1. We described the touchscreen in a sub-node of the panel.
>
> 2. We added a property to the panel saying what the true parent of the
> touchscreen was (an I2C controller, a SPI controller, ...) and what
> type of controller it was ("SPI" vs "I2C").
>
> 3. We added some generic helpers that panels could call that would
> understand how to instantiate the touchscreen under the appropriate
> controller.
>
> 4. From there, we added a new private / generic API between panels and
> touchscreens letting them know that the panel was turning on/off.
>
> That seems much more complex to me, though. It also seems like an
> awkward way to describe it in DT.
Yeah, I guess you're right. I wish we had something simpler, but I can't
think of any better way.
Sorry for the distraction.
> > > In any case, is there any chance that we're in violent agreement
> >
> > Is it even violent? Sorry if it came across that way, it's really isn't
> > on my end.
>
> Sorry, maybe a poor choice of words on my end. I've heard that term
> thrown about when two people spend a lot of time discussing something
> / trying to persuade the other person only to find out in the end that
> they were both on the same side of the issue. ;-)
>
> > > and that if you dig into my design more you might like it? Other than
> > > the fact that the panel doesn't "register" the touchscreen device, it
> > > kinda sounds as if what my patches are already doing is roughly what
> > > you're describing. The touchscreen and panel driver are really just
> > > coordinating with each other through a shared data structure (struct
> > > drm_panel_follower) that has a few callback functions. Just like with
> > > "hdmi-codec", the devices probe separately but find each other through
> > > a phandle. The coordination between the two happens through a few
> > > simple helper functions.
> >
> > I guess we very much agree on the end-goal, and I'd really like to get
> > this addressed somehow. There's a couple of things I'm not really
> > sold on with your proposal though:
> >
> > - It creates a ad-hoc KMS API for some problem that looks fairly
> > generic. It's also redundant with the notifier mechanism without
> > using it (probably for the best though).
> >
> > - MIPI-DSI panel probe sequence is already fairly complex and fragile
> > (See https://www.kernel.org/doc/html/latest/gpu/drm-kms-helpers.html#special-care-with-mipi-dsi-bridges).
> > I'd rather avoid creating a new dependency in that graph.
> >
> > - And yeah, to some extent it's inconsistent with how we dealt with
> > secondary devices in KMS so far.
>
> Hmmmm. To a large extent, my current implementation actually has no
> impact on the DRM probe sequence. The panel itself never looks for the
> touchscreen code and everything DRM-related can register without a
> care in the world. From reading your bullet points, I guess that's
> both a strength and a weakness of my current proposal. It's really
> outside the world of bridge chains and DRM components which makes it a
> special snowflake that people need to understand on its own. ...but,
> at the same time, the fact that it is outside all the rest of that
> stuff means it doesn't add complexity to an already complex system.
>
> I guess I'd point to the panel backlight as a preexisting design
> that's not totally unlike what I'm doing. The backlight is not part of
> the DRM bridge chain and doesn't fit in like other components. This
> actually makes sense since the backlight doesn't take in or put out
> video data and it's simply something associated with the panel. The
> backlight also has a loose connection to the panel driver and a given
> panel could be associated with any number of different backlight
> drivers depending on the board design. I guess one difference between
> the backlight and what I'm doing with "panel follower" is that we
> typically don't let the panel probe until after the backlight has
> probed. In the case of my "panel follower" proposal it's the opposite.
> As per above, from a DRM probe point of view this actually makes my
> proposal less intrusive. I guess also a difference between backlight
> and "panel follower" is that I allow an arbitrary number of followers
> but there's only one backlight.
>
> One additional note: if I actually make the panel probe function start
> registering the touchscreen, that actually _does_ add more complexity
> to the already complex DRM probe ordering. It's yet another thing that
> could fail and/or defer...
>
> Also, I'm curious: would my proposal be more or less palatable if I
> made it less generic? Instead of "panel follower", I could hardcode it
> to "touchscreen" and then remove all the list management. From a DRM
> point of view this would make it even more like the preexisting
> "backlight" except for the ordering difference.
No, that's fine. I guess I don't have any objections to your work, so
feel free to send a v2 :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: rework one more time the retries attempts
From: Benjamin Tissoires @ 2023-06-23 8:37 UTC (permalink / raw)
To: Greg KH
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina, linux-input,
linux-kernel, stable
In-Reply-To: <2023062156-trespass-pandemic-7f4f@gregkh>
On Jun 21 2023, Greg KH wrote:
>
> On Wed, Jun 21, 2023 at 11:42:30AM +0200, Benjamin Tissoires wrote:
> > Make the code looks less like Pascal.
> >
> > Extract the internal code inside a helper function, fix the
> > initialization of the parameters used in the helper function
> > (`hidpp->answer_available` was not reset and `*response` wasn't too),
> > and use a `do {...} while();` loop.
> >
> > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> > This is a rewrite of that particular piece of code.
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 102 +++++++++++++++++++++++----------------
> > 1 file changed, 61 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index dfe8e09a18de..3d1ffe199f08 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -275,21 +275,20 @@ static int __hidpp_send_report(struct hid_device *hdev,
> > }
> >
> > /*
> > - * hidpp_send_message_sync() returns 0 in case of success, and something else
> > - * in case of a failure.
> > - * - If ' something else' is positive, that means that an error has been raised
> > - * by the protocol itself.
> > - * - If ' something else' is negative, that means that we had a classic error
> > - * (-ENOMEM, -EPIPE, etc...)
> > + * Effectively send the message to the device, waiting for its answer.
> > + *
> > + * Must be called with hidpp->send_mutex locked
> > + *
> > + * Same return protocol than hidpp_send_message_sync():
> > + * - success on 0
> > + * - negative error means transport error
> > + * - positive value means protocol error
> > */
> > -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
> > struct hidpp_report *message,
> > struct hidpp_report *response)
>
> __must_hold(&hidpp->send_mutex) ?
>
Good point. I'll add this in v2.
I'm still waiting for some feedback from the people who particpated in
the original BZ, but the new bug is harder to reproduce. Anyway, there
is no rush IMO.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v2] HID: Add introduction about HID for non-kernel programmers
From: Randy Dunlap @ 2023-06-23 4:32 UTC (permalink / raw)
To: Marco Morandini, Peter Hutterer, Jiri Kosina, Benjamin Tissoires,
linux-input, Jonathan Corbet, linux-doc
In-Reply-To: <70fdef05-d3b8-e24b-77be-901bd5be369e@polimi.it>
Hi,
Please see comments below.
On 6/22/23 13:39, Marco Morandini wrote:
> Add an introduction about HID
> meant for the casual programmers that is trying
> either to fix his device or to understand what
> is going wrong.
>
> Signed-off-by: Marco Morandini <marco.morandini@polimi.it>
> ---
> v1: https://lore.kernel.org/linux-input/3mbw67akm2xzd2kgzb6sdfh4dly6im5jrz5umuvczjvrgxtf46@q5ooib3zkmfq/T/#m00b625a4d2c605dd7f62a866df7bf97ef2921d63
>
What changed v1 -> v2 goes here.
> Documentation/hid/hidintro.rst | 543 ++++++++++++++++++++++++
> Documentation/hid/hidreport-parsing.rst | 52 +++
> Documentation/hid/index.rst | 1 +
> include/linux/hid.h | 23 +
> 4 files changed, 619 insertions(+)
> create mode 100644 Documentation/hid/hidintro.rst
> create mode 100644 Documentation/hid/hidreport-parsing.rst
>
> diff --git a/Documentation/hid/hidintro.rst b/Documentation/hid/hidintro.rst
> new file mode 100644
> index 000000000000..cdaa09479147
> --- /dev/null
> +++ b/Documentation/hid/hidintro.rst
> @@ -0,0 +1,543 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +======================================
> +Introduction to HID report descriptors
> +======================================
> +
> +This chapter is meant to give a broad overview
> +of what HID report descriptors are, and of how a casual (non kernel)
(non-kernel)
> +programmer can deal with an HID device that
a HID device that
> +is not working well with Linux.
> +
> +.. contents::
> + :local:
> + :depth: 2
> +
> +.. toctree::
> + :maxdepth: 2
> +
> + hidreport-parsing
> +
> +
> +Introduction
> +============
> +
> +HID stands for Human Interface Device, and can be whatever device
> +you are using to interact with a computer, be it a mouse,
> +a touchpad, a tablet, a microphone.
> +
> +Many HID devices work out the box, even if their hardware is different.
> +For example, mice can have any number of buttons; they
> +may a wheel; movement sensitivity differs between different
may have a wheel;
> +models, and so on. Nonetheless,
> +most of the time everything just works, without the need
> +to have specialized code in the kernel for every mouse model
> +developed since 1970.
> +
> +This is because modern HID devices do advertise
> +their capabilities through the *HID report descriptor*, a
> +fixed set of bytes describing exactly what *HID reports*
> +may be sent between the device and the host and the meaning of each
> +individual bit in those reports. For example, a HID Report Descriptor
> +may specify that "in a report with ID 3 the second byte is the delta x
> +coordinate of a mouse".
> +
> +The HID report itself then merely carries the actual data values
> +without any extra meta information. Note that HID reports may be sent
> +from the device ("Input Reports", i.e. input events), to the device
> +("Output Reports" to e.g. change LEDs) or used for device
> +configuration ("Feature reports"). A device may support one or more
> +HID reports.
> +
> +The HID subsystem is in charge of parsing the HID report descriptors,
> +and converts HID events into normal input
> +device interfaces (see Documentation/hid/hid-transport.rst).
> +Devices may misbehave because the HID report descriptor
> +provided by the device is wrong,
> +or because it needs to be dealt with in a special way,
> +or because some special device or interaction mode
> +is not handled by the default code.
> +
> +The format of HID report descriptors is described by two documents,
> +available from the `USB Implementers Forum <https://www.usb.org/>`_
> +at `this <https://www.usb.org/hid>`_ addresses:
these
> +
> + * the `HID USB Device Class Definition
> + <https://www.usb.org/document-library/device-class-definition-hid-111>`_ (HID Spec from now on)
> + * the `HID Usage Tables <https://usb.org/document-library/hid-usage-tables-14>`_ (HUT from now on)
> +
> +This does not mean that the HID subsystem can deal with USB devices only;
> +rather, different transport drivers, such as I2C or Bluetooth, can be dealt
> +with, see Documentation/hid/hid-transport.rst.
> +
> +Parsing HID report descriptors
> +==============================
> +
> +The current list of HID devices can be found at ``/sys/bus/hid/devices/``.
> +For each device, say ``/sys/bus/hid/devices/0003\:093A\:2510.0002/``,
> +one can read the corresponding report descriptor::
> +
> + marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
> + 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
> + 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
> + 00000030 81 06 c0 c0 |....|
> + 00000034
> +
> +Optional stuff: the HID report descriptor can be read also by
> +directly accessing the hidraw driver [#hidraw]_.
> +
> +The basic structure of HID report descriptors is defined in the HID spec, while
> +HUT "defines constants that can be interpreted by an application to
> +identify the purpose and meaning of a data
> +field in a HID report". Each entry is defined by at least two bytes,
> +where the first one defines what type of value is following,
> +and is described in the HID spec,
> +while the second one carries the actual value,
> +and is described in the HUT.
> +
> +HID report descriptors can, in principle, be painstakingly
> +parsed by hand, byte by byte.
> +
> +A short introduction
> +on how to do this is sketched in Documentation/hid/hidreport-parsing.rst;
> +you need to understand it only if you need to patch HID report descriptors.
> +
> +In practice you should not do parse HID report descriptors by hand;
s/do //
and drop the line's trailing space. (Do that on any lines that have a
trailing space.)
> +rather, you should use an existing parser.
> +Among all the available ones
> +
> + * the online `USB Descriptor and Request Parser
> + <http://eleccelerator.com/usbdescreqparser/>`_;
> + * `hidrdd <https://github.com/abend0c1/hidrdd>`_,
> + that provides very detailed and somewhat verbose descriptions
> + (verbosity can be useful if you are not familiar with HID report descriptors);
> + * `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_,
> + a complete utility set that allows, among other things,
> + to record and replay the raw HID reports and to debug
> + and replay HID devices.
> + It is being actively developed by the Linux HID subsystem mantainers.
maintainers.
> +
> +Parsing the mouse HID report descriptor with `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_ leads to
> +(explanations interposed)::
> +
> + marco@sun:~/Programmi/linux/hid-tools (master =)> ./hid-decode /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + # device 0:0
> + # 0x05, 0x01, // Usage Page (Generic Desktop) 0
> + # 0x09, 0x02, // Usage (Mouse) 2
> + # 0xa1, 0x01, // Collection (Application) 4
> + # 0x09, 0x01, // Usage (Pointer) 6
> + # 0xa1, 0x00, // Collection (Physical) 8
> + # 0x05, 0x09, // Usage Page (Button) 10
> + what follows is a button
> +
> + # 0x19, 0x01, // Usage Minimum (1) 12
> + # 0x29, 0x03, // Usage Maximum (3) 14
> + first button is button number 1, last button is button number 3
> +
> + # 0x15, 0x00, // Logical Minimum (0) 16
> + # 0x25, 0x01, // Logical Maximum (1) 18
> + each button can send values from 0 up to including 1
> + (i.e. they are binary buttons)
> +
> + # 0x75, 0x01, // Report Size (1) 20
> + each button is sent as exactly one bit
> +
> + # 0x95, 0x03, // Report Count (3) 22
> + and there are three of those bits
> + (matching the three buttons)
> +
> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
> + it's actual Data (not constant padding), they represent
its
?
"its" is possessive. "it's" means "it is".
> + a single variable (Var) and the value are Absolute (not relative);
value is
or values are
> + See HID spec Sec. 6.2.2.5 "Input, Output, and Feature Items
> +
> + # 0x75, 0x05, // Report Size (5) 26
> + five additional padding bits, needed to reach a byte
> +
> + # 0x95, 0x01, // Report Count (1) 28
> + those five bits are repeated only once
> +
> + # 0x81, 0x01, // Input (Cnst,Arr,Abs) 30
> + and take Constant (Cnst) values
> +
> + # 0x05, 0x01, // Usage Page (Generic Desktop) 32
> + # 0x09, 0x30, // Usage (X) 34
> + # 0x09, 0x31, // Usage (Y) 36
> + # 0x09, 0x38, // Usage (Wheel) 38
> + The mouse has also two physical positions (``Usage (X)``, ``Usage (Y)``)
> + and a wheel (Usage (Wheel))
> +
> + # 0x15, 0x81, // Logical Minimum (-127) 40
> + # 0x25, 0x7f, // Logical Maximum (127) 42
> + each of them can send values ranging from -127 up to including 127
> +
> + # 0x75, 0x08, // Report Size (8) 44
> + is represented by eight bits
> +
> + # 0x95, 0x03, // Report Count (3) 46
> + and there are three of those eight bits, matching X, Y and Wheel
> +
> + # 0x81, 0x06, // Input (Data,Var,Rel) 48
> + This time the data is Relative (Rel), i.e. it represent
represents
> + the change from the previous configuration
> +
> + # 0xc0, // End Collection 50
> + # 0xc0, // End Collection 51
> + #
> + R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
> + N: device 0:0
> + I: 3 0001 0001
> +
> +
> +This Report Descriptor tells us that the mouse input will be
> +transmitted using four bytes:
> +the first one for the buttons (three bits used, five for padding),
> +the last three for the mouse X, Y and wheel changes, respectively.
> +
> +Indeed, for any event, the mouse will send a *report* of four bytes.
> +We can check the values sent by resorting e.g.
> +to the `hid-recorder` tool, from `hid-tools
> +<https://gitlab.freedesktop.org/libevdev/hid-tools>`_:
> +The sequence of bytes sent by clicking and releasing
> +button 1, then button 2, then button 3 is::
> +
> + marco@sun:~/> sudo ./hid-recorder /dev/hidraw1
> +
> + ....
> + output of hid-decode
> + ....
> +
> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000000.000000 4 01 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000000.183949 4 00 00 00 00
> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000001.959698 4 02 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000002.103899 4 00 00 00 00
> + # Button: 0 0 1 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000004.855799 4 04 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000005.103864 4 00 00 00 00
> +
> +This example shows that when button 2 is clicked,
> +the bytes ``02 00 00 00`` are sent, and the immediately subsequent
> +event (``00 00 00 00``) is the release of button 2 (no buttons are pressed,
> +remember that the data is *absolute*).
> +
> +If instead one clicks and holds button 1, then clicks and holds button 2,
> +releases button 1, and finally releases button 2, the reports are::
> +
> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000044.175830 4 01 00 00 00
> + # Button: 1 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000045.975997 4 03 00 00 00
> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000047.407930 4 02 00 00 00
> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
> + E: 000049.199919 4 00 00 00 00
> +
> +where with ``03 00 00 00`` both buttons are pressed, and with the
> +subsequent ``02 00 00 00`` button 1 is released while button 2 is still
> +active.
> +
> +Output, Input and Feature Reports
> +---------------------------------
> +
> +An HID devices can have Input Reports, like
A HID device
> +in the mouse example, Output Reports, and Features Reports.> +"Output" means that the information is fed
> +to the device. For example,
> +a joystick with force feedback will have
> +some output; the led of a keyboard would
> +need an output as well.
> +"Input" means that data
> +come from the device.
> +
> +"Feature"s are not meant to be consumed by the end user
> +and define configuration options for the device.
> +They can be queried from the host;
> +when declared as *Volatile*
> +they should be changed by the host.
> +
> +
> +Collections, Report IDs and Evdev events
> +========================================
> +
> +A single device can logically group
> +data into different, independent sets,
> +called *Collection*.
> +Collections can be nested, and there
> +are different types of collections
> +(see the HID spec 6.2.2.6
> +"Collection, End Collection Items" for details).
> +
> +Different reports are identified by means
> +of different *Report ID* fields, i.e. a number identifying
> +the structure of the immediately following report.
> +Whenever a Report ID
> +is needed it is transmitted as the first byte of any report.
> +
> +Consider the following HID report descriptor::
> +
> + 05 01 09 02 A1 01 85 01 05 09 19 01 29 05 15 00
> + 25 01 95 05 75 01 81 02 95 01 75 03 81 01 05 01
> + 09 30 09 31 16 00 F8 26 FF 07 75 0C 95 02 81 06
> + 09 38 15 80 25 7F 75 08 95 01 81 06 05 0C 0A 38
> + 02 15 80 25 7F 75 08 95 01 81 06 C0 05 01 09 02
> + A1 01 85 02 05 09 19 01 29 05 15 00 25 01 95 05
> + 75 01 81 02 95 01 75 03 81 01 05 01 09 30 09 31
> + 16 00 F8 26 FF 07 75 0C 95 02 81 06 09 38 15 80
> + 25 7F 75 08 95 01 81 06 05 0C 0A 38 02 15 80 25
> + 7F 75 08 95 01 81 06 C0 05 01 09 07 A1 01 85 05
> + 05 07 15 00 25 01 09 29 09 3E 09 4B 09 4E 09 E3
> + 09 E8 09 E8 09 E8 75 01 95 08 81 02 95 00 81 01
> + C0 05 0C 09 01 A1 01 85 06 15 00 25 01 75 01 95
> + 01 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
> + 06 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
> + 06 C0 05 0C 09 01 A1 01 85 03 09 05 15 00 26 FF
> + 00 75 08 95 02 B1 02 C0
> +
> +After parsing it (try to parse it on your own using
> +the suggested tools!)
> +one can see that the device presents two ``Mouse``
> +Application Collections
> +(with reports identified by Reports IDs 1 and 2, respectively),
> +a ``Keypad`` Application Collection (whose report is identified
> +by the Report ID 5) and two ``Consumer Controls``
> +Application Collections,
> +(with Report IDs 6 and 3, respectively).
> +Note that, however, that you can have different Report IDs
Note, however, that
> +for the same Application Collection.
> +
> +The data sent will begin with the Report ID byte, and will be followed
> +by the corresponding information. For example, the
> +data transmitted for the last consumer
> +control::
> +
> + 0x05, 0x0C, // Usage Page (Consumer)
> + 0x09, 0x01, // Usage (Consumer Control)
> + 0xA1, 0x01, // Collection (Application)
> + 0x85, 0x03, // Report ID (3)
> + 0x09, 0x05, // Usage (Headphone)
> + 0x15, 0x00, // Logical Minimum (0)
> + 0x26, 0xFF, 0x00, // Logical Maximum (255)
> + 0x75, 0x08, // Report Size (8)
> + 0x95, 0x02, // Report Count (2)
> + 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
> + 0xC0, // End Collection
> +
> +will be of three bytes: the first for the Report ID (3), the next two
> +for the headphone, with two (``Report Count (2)``) bytes
> +(``Report Size (8)``), each ranging from 0 (``Logical Minimum (0)`` to 255
> +(``Logical Maximum (255)``).
> +
> +All the Input data sent by the device should be translated into corresponding
> +Evdev events, so that the remaining part of the stack can know what is going on,
> +e.g. that a mouse button was pressed, rather
> +than a mouse has been moved in the X direction.
> +
> +Events
> +======
> +
> +In Linux, one ``/dev/input/event*`` is created for each
> +``Application Collection``.
> +Going back to the mouse example, and repeating the sequence where
> +one clicks and holds button 1, then clicks and holds button 2,
> +releases button 1, and finally releases button 2, one gets::
> +
> + marco@sun:~> sudo libinput record /dev/input/event1
> + # libinput record
> + version: 1
> + ndevices: 1
> + libinput:
> + version: "1.23.0"
> + git: "unknown"
> + system:
> + os: "opensuse-tumbleweed:20230619"
> + kernel: "6.3.7-1-default"
> + dmi: "dmi:bvnHP:bvrU77Ver.01.05.00:bd03/24/2022:br5.0:efr20.29:svnHP:pnHPEliteBook64514inchG9NotebookPC:pvr:rvnHP:rn89D2:rvrKBCVersion14.1D.00:cvnHP:ct10:cvr:sku5Y3J1EA#ABZ:"
> + devices:
> + - node: /dev/input/event1
> + evdev:
> + # Name: PixArt HP USB Optical Mouse
> + # ID: bus 0x3 vendor 0x3f0 product 0x94a version 0x111
> + # Supported Events:
> + # Event type 0 (EV_SYN)
> + # Event type 1 (EV_KEY)
> + # Event code 272 (BTN_LEFT)
> + # Event code 273 (BTN_RIGHT)
> + # Event code 274 (BTN_MIDDLE)
> + # Event type 2 (EV_REL)
> + # Event code 0 (REL_X)
> + # Event code 1 (REL_Y)
> + # Event code 8 (REL_WHEEL)
> + # Event code 11 (REL_WHEEL_HI_RES)
> + # Event type 4 (EV_MSC)
> + # Event code 4 (MSC_SCAN)
> + # Properties:
> + name: "PixArt HP USB Optical Mouse"
> + id: [3, 1008, 2378, 273]
> + codes:
> + 0: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15] # EV_SYN
> + 1: [272, 273, 274] # EV_KEY
> + 2: [0, 1, 8, 11] # EV_REL
> + 4: [4] # EV_MSC
> + properties: []
> + hid: [
> + 0x05, 0x01, 0x09, 0x02, 0xa1, 0x01, 0x09, 0x01, 0xa1, 0x00, 0x05, 0x09, 0x19, 0x01, 0x29, 0x03,
> + 0x15, 0x00, 0x25, 0x01, 0x95, 0x08, 0x75, 0x01, 0x81, 0x02, 0x05, 0x01, 0x09, 0x30, 0x09, 0x31,
> + 0x09, 0x38, 0x15, 0x81, 0x25, 0x7f, 0x75, 0x08, 0x95, 0x03, 0x81, 0x06, 0xc0, 0xc0
> + ]
> + udev:
> + properties:
> + - ID_INPUT=1
> + - ID_INPUT_MOUSE=1
> + - LIBINPUT_DEVICE_GROUP=3/3f0/94a:usb-0000:05:00.3-2
> + quirks:
> + events:
> + # Current time is 12:31:56
> + - evdev:
> + - [ 0, 0, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
> + - [ 0, 0, 1, 272, 1] # EV_KEY / BTN_LEFT 1
> + - [ 0, 0, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +0ms
> + - evdev:
> + - [ 1, 207892, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
> + - [ 1, 207892, 1, 273, 1] # EV_KEY / BTN_RIGHT 1
> + - [ 1, 207892, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +1207ms
> + - evdev:
> + - [ 2, 367823, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
> + - [ 2, 367823, 1, 272, 0] # EV_KEY / BTN_LEFT 0
> + - [ 2, 367823, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +1160ms
> + # Current time is 12:32:00
> + - evdev:
> + - [ 3, 247617, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
> + - [ 3, 247617, 1, 273, 0] # EV_KEY / BTN_RIGHT 0
> + - [ 3, 247617, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +880ms
> +
> +Note: if ``libinput`` is not available on your system try using ``evemu-record``.
> +
> +When something does not work
> +============================
> +
> +There can be a number of reasons for why a device does not behave
s/for //
> +correctly. For example
> +
> +* The HID report descriptor provided by the HID device may be wrong
> + because e.g.
> +
> + * it does not follow the standard, so that the kernel
> + will not able to make sense of the HID report descriptor;
> + * it is possible to verify, by reading the raw HID data, that
> + the HID report descriptor *does not match* what is actually
> + sent by the device;
> +
> +or
> +
> +* the HID report descriptor may need some "quirks" (see later on);
on).
> +
> +As a consequence, a suitable ``/dev/input/event*`` will not created
not be created
> +for each Application Collection, and/or the events
> +there will match what you would expect.
will not
?
> +
> +
> +Quirks
> +------
> +
> +There are some known peculiarities of HID devices that the kernel
> +knows how to fix - these are called the HID quirks and a list of those
> +are available in `include/linux/hid.h`.
is available
> +
> +Should this be the case,
> +it should be enough to add the required quirk,
quirk
(no comma)
> +in the kernel, for the HID device at hand.
> +This can be done in file `drivers/hid/hid-quirks.c`.
> +How to do it should be relatively straightforward
> +after looking into the file.
> +
> +The list of currently defined quirks, from
> +`include/linux/hid.h`, is
> +
> +.. kernel-doc:: include/linux/hid.h
> + :doc: HID quirks
> +
> +Quirks for USB devices can be specified
> +while loading the usbhid module,
> +see ``modinfo usbhid``, although the proper fix
> +should go into hid-quirks.c and **submitted upstream**.
**be submitted upstream**.
> +See, again, Documentation/process/submitting-patches.rst
> +for guidelines on how to do submit a patch.
how to submit a patch.
> +Quirks for other busses need to go into hid-quirks.c
> +
> +Fixing HID report descriptors
> +-----------------------------
> +
> +Should you need to patch HID report descriptors
> +the easiest way is to resort to eBPF, as described
> +in Documentation/hid/hid-bpf.rst.
> +
> +Basically, you can change any byte of the original HID report descriptor.
> +The examples in samples/hid should be a good starting point
> +for your code, see e.g. `samples/hid_mouse.bpf.c`::
samples/hid/hid_mouse.bpf.c
> +
> + SEC("fmod_ret/hid_bpf_rdesc_fixup")
> + int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
> + {
> + ....
> + data[39] = 0x31;
> + data[41] = 0x30;
> + return 0;
> + }
> +
> +Of course this can be also done within the kernel source
> +code, see e.g. `drivers/hid/hid-aureal.c` or
> +`drivers/hid/hid-samsung.c` for a slightly more complex file.
> +
> +Check Documentation/hid/hidreport-parsing.rst
> +if you need an help navigating the HID manuals and
any
> +understanding the exact meaning of
> +the HID report descriptor hex numbers.
> +
> +Whatever solution you come up with, please remember to **submit the
> +fix to the HID maintainers**, so that it can be directly
> +integrated in the kernel
> +and that particular HID device will will start
will start
> +working for everyone else.
> +See Documentation/process/submitting-patches.rst
> +for guidelines on how to do this.
> +
> +
> +Modifying the transmitted data on the fly
> +-----------------------------------------
> +
> +Using eBPF it is also possible to modify the data exchanged
> +with the device. See again the examples in `samples/hid`.
> +
> +Again, **please post your fix**, so that
> +it can be integrated in the kernel!
> +
> +Writing a specialized driver
> +----------------------------
> +
> +This should really be your last resort.
> +
> +
> +.. rubric:: Footnotes
> +
> +.. [#hidraw] reading hidraw: see Documentation/hid/hidraw.rst and
read
> + file `samples/hidraw/hid-example.c` for an example.
> + The output of ``hid-example`` would be, for the same mouse::
> +
> + marco@sun:~> sudo ./hid-example
> + Report Descriptor Size: 52
> + Report Descriptor:
> + 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 3 15 0 25 1 75 1 95 3 81 2 75 5 95 1 81 1 5 1 9 30 9 31 9 38 15 81 25 7f 75 8 95 3 81 6 c0 c0
> +
> + Raw Name: PixArt USB Optical Mouse
> + Raw Phys: usb-0000:05:00.4-2.3/input0
> + Raw Info:
> + bustype: 3 (USB)
> + vendor: 0x093a
> + product: 0x2510
> + ...
> diff --git a/Documentation/hid/hidreport-parsing.rst b/Documentation/hid/hidreport-parsing.rst
> new file mode 100644
> index 000000000000..a31a7aec5947
> --- /dev/null
> +++ b/Documentation/hid/hidreport-parsing.rst
> @@ -0,0 +1,52 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +========================================
> +Manual parsing of HID report descriptors
> +========================================
> +
> +Consider again the mouse HID report descriptor
> +introduced in Documentation/hid/hidintro.rst::
> +
> + marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
> + 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
> + 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
> + 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
> + 00000030 81 06 c0 c0 |....|
> + 00000034
> +
> +and try to parse it by hand.
> +
> +Start with the first number, 0x05: according to
> +the HID spec, Sec. 6.2.2.2, "Short Items"
> +
> +Start with the first number, 0x05 which carries 2 bits for the
> +length of the item, 2 bits for the type of the item and 4 bits for the
> +function::
> +
> + +----------+
> + | 00000101 |
> + +----------+
> + ^^
> + ---- Length of data (see HID spec 6.2.2.2)
> + ^^
> + ------ Type of the item (see HID spec 6.2.2.7)
> + ^^^^
> + --------- Function of the item (see HUT Sec 3)
> +
> +In our case, the length is 1 byte, the type is ``Global`` and the function
> +is ``Usage Page``, thus we need to refer to HUT Sec 3 which indicates that
> +the value 0x01 in the second byte stands for ``Generic Desktop Page``.
> +
> +The second number is the actual data, and its meaning can
> +be found in the HUT.
> +We have an ``Usage Page``, thus we need to refer to HUT Sec. 3,
have a
> +"Usage Pages"; from there, one sees that the ``0x01``
> +stands for ``Generic Desktop Page``.
> +
> +Moving now to the second two bytes, and following the same scheme, ``0x09``
> +(i.e. ``00001001``) will be followed by one byte (``01``)
> +and is a ``Local`` item.
> +Thus, the meaning of the remaining four bits (``0000``)
> +is given in the HID spec Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
have a
> +
> +The following numbers can be parsed in the same way.
> diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
> index b2028f382f11..af02cf7cfa82 100644
> --- a/Documentation/hid/index.rst
> +++ b/Documentation/hid/index.rst
> @@ -7,6 +7,7 @@ Human Interface Devices (HID)
> .. toctree::
> :maxdepth: 1
>
> + hidintro
> hiddev
> hidraw
> hid-sensor
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 7f2e8ba7d783..ad12a36d9993 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -341,6 +341,29 @@ struct hid_item {
> */
> #define MAX_USBHID_BOOT_QUIRKS 4
>
> +/**
> +* DOC: HID quirks
> +* | @HID_QUIRK_NOTOUCH:
> +* | @HID_QUIRK_IGNORE: ignore this device
> +* | @HID_QUIRK_NOGET:
> +* | @HID_QUIRK_HIDDEV_FORCE:
> +* | @HID_QUIRK_BADPAD:
> +* | @HID_QUIRK_MULTI_INPUT:
> +* | @HID_QUIRK_HIDINPUT_FORCE:
> +* | @HID_QUIRK_ALWAYS_POLL:
> +* | @HID_QUIRK_INPUT_PER_APP:
> +* | @HID_QUIRK_X_INVERT:
> +* | @HID_QUIRK_Y_INVERT:
> +* | @HID_QUIRK_SKIP_OUTPUT_REPORTS:
> +* | @HID_QUIRK_SKIP_OUTPUT_REPORT_ID:
> +* | @HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP:
> +* | @HID_QUIRK_HAVE_SPECIAL_DRIVER:
> +* | @HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE:
> +* | @HID_QUIRK_FULLSPEED_INTERVAL:
> +* | @HID_QUIRK_NO_INIT_REPORTS:
> +* | @HID_QUIRK_NO_IGNORE:
> +* | @HID_QUIRK_NO_INPUT_SYNC:
> +*/
> /* BIT(0) reserved for backward compatibility, was HID_QUIRK_INVERT */
> #define HID_QUIRK_NOTOUCH BIT(1)
> #define HID_QUIRK_IGNORE BIT(2)
Thanks.
--
~Randy
^ permalink raw reply
* [PATCH v2] HID: Add introduction about HID for non-kernel programmers
From: Marco Morandini @ 2023-06-22 20:39 UTC (permalink / raw)
To: Peter Hutterer, Jiri Kosina, Benjamin Tissoires, linux-input,
Jonathan Corbet, linux-doc
Add an introduction about HID
meant for the casual programmers that is trying
either to fix his device or to understand what
is going wrong.
Signed-off-by: Marco Morandini <marco.morandini@polimi.it>
---
v1: https://lore.kernel.org/linux-input/3mbw67akm2xzd2kgzb6sdfh4dly6im5jrz5umuvczjvrgxtf46@q5ooib3zkmfq/T/#m00b625a4d2c605dd7f62a866df7bf97ef2921d63
Documentation/hid/hidintro.rst | 543 ++++++++++++++++++++++++
Documentation/hid/hidreport-parsing.rst | 52 +++
Documentation/hid/index.rst | 1 +
include/linux/hid.h | 23 +
4 files changed, 619 insertions(+)
create mode 100644 Documentation/hid/hidintro.rst
create mode 100644 Documentation/hid/hidreport-parsing.rst
diff --git a/Documentation/hid/hidintro.rst b/Documentation/hid/hidintro.rst
new file mode 100644
index 000000000000..cdaa09479147
--- /dev/null
+++ b/Documentation/hid/hidintro.rst
@@ -0,0 +1,543 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+======================================
+Introduction to HID report descriptors
+======================================
+
+This chapter is meant to give a broad overview
+of what HID report descriptors are, and of how a casual (non kernel)
+programmer can deal with an HID device that
+is not working well with Linux.
+
+.. contents::
+ :local:
+ :depth: 2
+
+.. toctree::
+ :maxdepth: 2
+
+ hidreport-parsing
+
+
+Introduction
+============
+
+HID stands for Human Interface Device, and can be whatever device
+you are using to interact with a computer, be it a mouse,
+a touchpad, a tablet, a microphone.
+
+Many HID devices work out the box, even if their hardware is different.
+For example, mice can have any number of buttons; they
+may a wheel; movement sensitivity differs between different
+models, and so on. Nonetheless,
+most of the time everything just works, without the need
+to have specialized code in the kernel for every mouse model
+developed since 1970.
+
+This is because modern HID devices do advertise
+their capabilities through the *HID report descriptor*, a
+fixed set of bytes describing exactly what *HID reports*
+may be sent between the device and the host and the meaning of each
+individual bit in those reports. For example, a HID Report Descriptor
+may specify that "in a report with ID 3 the second byte is the delta x
+coordinate of a mouse".
+
+The HID report itself then merely carries the actual data values
+without any extra meta information. Note that HID reports may be sent
+from the device ("Input Reports", i.e. input events), to the device
+("Output Reports" to e.g. change LEDs) or used for device
+configuration ("Feature reports"). A device may support one or more
+HID reports.
+
+The HID subsystem is in charge of parsing the HID report descriptors,
+and converts HID events into normal input
+device interfaces (see Documentation/hid/hid-transport.rst).
+Devices may misbehave because the HID report descriptor
+provided by the device is wrong,
+or because it needs to be dealt with in a special way,
+or because some special device or interaction mode
+is not handled by the default code.
+
+The format of HID report descriptors is described by two documents,
+available from the `USB Implementers Forum <https://www.usb.org/>`_
+at `this <https://www.usb.org/hid>`_ addresses:
+
+ * the `HID USB Device Class Definition
+ <https://www.usb.org/document-library/device-class-definition-hid-111>`_ (HID Spec from now on)
+ * the `HID Usage Tables <https://usb.org/document-library/hid-usage-tables-14>`_ (HUT from now on)
+
+This does not mean that the HID subsystem can deal with USB devices only;
+rather, different transport drivers, such as I2C or Bluetooth, can be dealt
+with, see Documentation/hid/hid-transport.rst.
+
+Parsing HID report descriptors
+==============================
+
+The current list of HID devices can be found at ``/sys/bus/hid/devices/``.
+For each device, say ``/sys/bus/hid/devices/0003\:093A\:2510.0002/``,
+one can read the corresponding report descriptor::
+
+ marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
+ 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
+ 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
+ 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
+ 00000030 81 06 c0 c0 |....|
+ 00000034
+
+Optional stuff: the HID report descriptor can be read also by
+directly accessing the hidraw driver [#hidraw]_.
+
+The basic structure of HID report descriptors is defined in the HID spec, while
+HUT "defines constants that can be interpreted by an application to
+identify the purpose and meaning of a data
+field in a HID report". Each entry is defined by at least two bytes,
+where the first one defines what type of value is following,
+and is described in the HID spec,
+while the second one carries the actual value,
+and is described in the HUT.
+
+HID report descriptors can, in principle, be painstakingly
+parsed by hand, byte by byte.
+
+A short introduction
+on how to do this is sketched in Documentation/hid/hidreport-parsing.rst;
+you need to understand it only if you need to patch HID report descriptors.
+
+In practice you should not do parse HID report descriptors by hand;
+rather, you should use an existing parser.
+Among all the available ones
+
+ * the online `USB Descriptor and Request Parser
+ <http://eleccelerator.com/usbdescreqparser/>`_;
+ * `hidrdd <https://github.com/abend0c1/hidrdd>`_,
+ that provides very detailed and somewhat verbose descriptions
+ (verbosity can be useful if you are not familiar with HID report descriptors);
+ * `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_,
+ a complete utility set that allows, among other things,
+ to record and replay the raw HID reports and to debug
+ and replay HID devices.
+ It is being actively developed by the Linux HID subsystem mantainers.
+
+Parsing the mouse HID report descriptor with `hid-tools <https://gitlab.freedesktop.org/libevdev/hid-tools>`_ leads to
+(explanations interposed)::
+
+ marco@sun:~/Programmi/linux/hid-tools (master =)> ./hid-decode /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
+ # device 0:0
+ # 0x05, 0x01, // Usage Page (Generic Desktop) 0
+ # 0x09, 0x02, // Usage (Mouse) 2
+ # 0xa1, 0x01, // Collection (Application) 4
+ # 0x09, 0x01, // Usage (Pointer) 6
+ # 0xa1, 0x00, // Collection (Physical) 8
+ # 0x05, 0x09, // Usage Page (Button) 10
+ what follows is a button
+
+ # 0x19, 0x01, // Usage Minimum (1) 12
+ # 0x29, 0x03, // Usage Maximum (3) 14
+ first button is button number 1, last button is button number 3
+
+ # 0x15, 0x00, // Logical Minimum (0) 16
+ # 0x25, 0x01, // Logical Maximum (1) 18
+ each button can send values from 0 up to including 1
+ (i.e. they are binary buttons)
+
+ # 0x75, 0x01, // Report Size (1) 20
+ each button is sent as exactly one bit
+
+ # 0x95, 0x03, // Report Count (3) 22
+ and there are three of those bits
+ (matching the three buttons)
+
+ # 0x81, 0x02, // Input (Data,Var,Abs) 24
+ it's actual Data (not constant padding), they represent
+ a single variable (Var) and the value are Absolute (not relative);
+ See HID spec Sec. 6.2.2.5 "Input, Output, and Feature Items
+
+ # 0x75, 0x05, // Report Size (5) 26
+ five additional padding bits, needed to reach a byte
+
+ # 0x95, 0x01, // Report Count (1) 28
+ those five bits are repeated only once
+
+ # 0x81, 0x01, // Input (Cnst,Arr,Abs) 30
+ and take Constant (Cnst) values
+
+ # 0x05, 0x01, // Usage Page (Generic Desktop) 32
+ # 0x09, 0x30, // Usage (X) 34
+ # 0x09, 0x31, // Usage (Y) 36
+ # 0x09, 0x38, // Usage (Wheel) 38
+ The mouse has also two physical positions (``Usage (X)``, ``Usage (Y)``)
+ and a wheel (Usage (Wheel))
+
+ # 0x15, 0x81, // Logical Minimum (-127) 40
+ # 0x25, 0x7f, // Logical Maximum (127) 42
+ each of them can send values ranging from -127 up to including 127
+
+ # 0x75, 0x08, // Report Size (8) 44
+ is represented by eight bits
+
+ # 0x95, 0x03, // Report Count (3) 46
+ and there are three of those eight bits, matching X, Y and Wheel
+
+ # 0x81, 0x06, // Input (Data,Var,Rel) 48
+ This time the data is Relative (Rel), i.e. it represent
+ the change from the previous configuration
+
+ # 0xc0, // End Collection 50
+ # 0xc0, // End Collection 51
+ #
+ R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
+ N: device 0:0
+ I: 3 0001 0001
+
+
+This Report Descriptor tells us that the mouse input will be
+transmitted using four bytes:
+the first one for the buttons (three bits used, five for padding),
+the last three for the mouse X, Y and wheel changes, respectively.
+
+Indeed, for any event, the mouse will send a *report* of four bytes.
+We can check the values sent by resorting e.g.
+to the `hid-recorder` tool, from `hid-tools
+<https://gitlab.freedesktop.org/libevdev/hid-tools>`_:
+The sequence of bytes sent by clicking and releasing
+button 1, then button 2, then button 3 is::
+
+ marco@sun:~/> sudo ./hid-recorder /dev/hidraw1
+
+ ....
+ output of hid-decode
+ ....
+
+ # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000000.000000 4 01 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000000.183949 4 00 00 00 00
+ # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000001.959698 4 02 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000002.103899 4 00 00 00 00
+ # Button: 0 0 1 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000004.855799 4 04 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000005.103864 4 00 00 00 00
+
+This example shows that when button 2 is clicked,
+the bytes ``02 00 00 00`` are sent, and the immediately subsequent
+event (``00 00 00 00``) is the release of button 2 (no buttons are pressed,
+remember that the data is *absolute*).
+
+If instead one clicks and holds button 1, then clicks and holds button 2,
+releases button 1, and finally releases button 2, the reports are::
+
+ # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000044.175830 4 01 00 00 00
+ # Button: 1 1 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000045.975997 4 03 00 00 00
+ # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000047.407930 4 02 00 00 00
+ # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
+ E: 000049.199919 4 00 00 00 00
+
+where with ``03 00 00 00`` both buttons are pressed, and with the
+subsequent ``02 00 00 00`` button 1 is released while button 2 is still
+active.
+
+Output, Input and Feature Reports
+---------------------------------
+
+An HID devices can have Input Reports, like
+in the mouse example, Output Reports, and Features Reports.
+"Output" means that the information is fed
+to the device. For example,
+a joystick with force feedback will have
+some output; the led of a keyboard would
+need an output as well.
+"Input" means that data
+come from the device.
+
+"Feature"s are not meant to be consumed by the end user
+and define configuration options for the device.
+They can be queried from the host;
+when declared as *Volatile*
+they should be changed by the host.
+
+
+Collections, Report IDs and Evdev events
+========================================
+
+A single device can logically group
+data into different, independent sets,
+called *Collection*.
+Collections can be nested, and there
+are different types of collections
+(see the HID spec 6.2.2.6
+"Collection, End Collection Items" for details).
+
+Different reports are identified by means
+of different *Report ID* fields, i.e. a number identifying
+the structure of the immediately following report.
+Whenever a Report ID
+is needed it is transmitted as the first byte of any report.
+
+Consider the following HID report descriptor::
+
+ 05 01 09 02 A1 01 85 01 05 09 19 01 29 05 15 00
+ 25 01 95 05 75 01 81 02 95 01 75 03 81 01 05 01
+ 09 30 09 31 16 00 F8 26 FF 07 75 0C 95 02 81 06
+ 09 38 15 80 25 7F 75 08 95 01 81 06 05 0C 0A 38
+ 02 15 80 25 7F 75 08 95 01 81 06 C0 05 01 09 02
+ A1 01 85 02 05 09 19 01 29 05 15 00 25 01 95 05
+ 75 01 81 02 95 01 75 03 81 01 05 01 09 30 09 31
+ 16 00 F8 26 FF 07 75 0C 95 02 81 06 09 38 15 80
+ 25 7F 75 08 95 01 81 06 05 0C 0A 38 02 15 80 25
+ 7F 75 08 95 01 81 06 C0 05 01 09 07 A1 01 85 05
+ 05 07 15 00 25 01 09 29 09 3E 09 4B 09 4E 09 E3
+ 09 E8 09 E8 09 E8 75 01 95 08 81 02 95 00 81 01
+ C0 05 0C 09 01 A1 01 85 06 15 00 25 01 75 01 95
+ 01 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
+ 06 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
+ 06 C0 05 0C 09 01 A1 01 85 03 09 05 15 00 26 FF
+ 00 75 08 95 02 B1 02 C0
+
+After parsing it (try to parse it on your own using
+the suggested tools!)
+one can see that the device presents two ``Mouse``
+Application Collections
+(with reports identified by Reports IDs 1 and 2, respectively),
+a ``Keypad`` Application Collection (whose report is identified
+by the Report ID 5) and two ``Consumer Controls``
+Application Collections,
+(with Report IDs 6 and 3, respectively).
+Note that, however, that you can have different Report IDs
+for the same Application Collection.
+
+The data sent will begin with the Report ID byte, and will be followed
+by the corresponding information. For example, the
+data transmitted for the last consumer
+control::
+
+ 0x05, 0x0C, // Usage Page (Consumer)
+ 0x09, 0x01, // Usage (Consumer Control)
+ 0xA1, 0x01, // Collection (Application)
+ 0x85, 0x03, // Report ID (3)
+ 0x09, 0x05, // Usage (Headphone)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x26, 0xFF, 0x00, // Logical Maximum (255)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x02, // Report Count (2)
+ 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
+ 0xC0, // End Collection
+
+will be of three bytes: the first for the Report ID (3), the next two
+for the headphone, with two (``Report Count (2)``) bytes
+(``Report Size (8)``), each ranging from 0 (``Logical Minimum (0)`` to 255
+(``Logical Maximum (255)``).
+
+All the Input data sent by the device should be translated into corresponding
+Evdev events, so that the remaining part of the stack can know what is going on,
+e.g. that a mouse button was pressed, rather
+than a mouse has been moved in the X direction.
+
+Events
+======
+
+In Linux, one ``/dev/input/event*`` is created for each
+``Application Collection``.
+Going back to the mouse example, and repeating the sequence where
+one clicks and holds button 1, then clicks and holds button 2,
+releases button 1, and finally releases button 2, one gets::
+
+ marco@sun:~> sudo libinput record /dev/input/event1
+ # libinput record
+ version: 1
+ ndevices: 1
+ libinput:
+ version: "1.23.0"
+ git: "unknown"
+ system:
+ os: "opensuse-tumbleweed:20230619"
+ kernel: "6.3.7-1-default"
+ dmi: "dmi:bvnHP:bvrU77Ver.01.05.00:bd03/24/2022:br5.0:efr20.29:svnHP:pnHPEliteBook64514inchG9NotebookPC:pvr:rvnHP:rn89D2:rvrKBCVersion14.1D.00:cvnHP:ct10:cvr:sku5Y3J1EA#ABZ:"
+ devices:
+ - node: /dev/input/event1
+ evdev:
+ # Name: PixArt HP USB Optical Mouse
+ # ID: bus 0x3 vendor 0x3f0 product 0x94a version 0x111
+ # Supported Events:
+ # Event type 0 (EV_SYN)
+ # Event type 1 (EV_KEY)
+ # Event code 272 (BTN_LEFT)
+ # Event code 273 (BTN_RIGHT)
+ # Event code 274 (BTN_MIDDLE)
+ # Event type 2 (EV_REL)
+ # Event code 0 (REL_X)
+ # Event code 1 (REL_Y)
+ # Event code 8 (REL_WHEEL)
+ # Event code 11 (REL_WHEEL_HI_RES)
+ # Event type 4 (EV_MSC)
+ # Event code 4 (MSC_SCAN)
+ # Properties:
+ name: "PixArt HP USB Optical Mouse"
+ id: [3, 1008, 2378, 273]
+ codes:
+ 0: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15] # EV_SYN
+ 1: [272, 273, 274] # EV_KEY
+ 2: [0, 1, 8, 11] # EV_REL
+ 4: [4] # EV_MSC
+ properties: []
+ hid: [
+ 0x05, 0x01, 0x09, 0x02, 0xa1, 0x01, 0x09, 0x01, 0xa1, 0x00, 0x05, 0x09, 0x19, 0x01, 0x29, 0x03,
+ 0x15, 0x00, 0x25, 0x01, 0x95, 0x08, 0x75, 0x01, 0x81, 0x02, 0x05, 0x01, 0x09, 0x30, 0x09, 0x31,
+ 0x09, 0x38, 0x15, 0x81, 0x25, 0x7f, 0x75, 0x08, 0x95, 0x03, 0x81, 0x06, 0xc0, 0xc0
+ ]
+ udev:
+ properties:
+ - ID_INPUT=1
+ - ID_INPUT_MOUSE=1
+ - LIBINPUT_DEVICE_GROUP=3/3f0/94a:usb-0000:05:00.3-2
+ quirks:
+ events:
+ # Current time is 12:31:56
+ - evdev:
+ - [ 0, 0, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
+ - [ 0, 0, 1, 272, 1] # EV_KEY / BTN_LEFT 1
+ - [ 0, 0, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +0ms
+ - evdev:
+ - [ 1, 207892, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
+ - [ 1, 207892, 1, 273, 1] # EV_KEY / BTN_RIGHT 1
+ - [ 1, 207892, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +1207ms
+ - evdev:
+ - [ 2, 367823, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
+ - [ 2, 367823, 1, 272, 0] # EV_KEY / BTN_LEFT 0
+ - [ 2, 367823, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +1160ms
+ # Current time is 12:32:00
+ - evdev:
+ - [ 3, 247617, 4, 4, 30] # EV_MSC / MSC_SCAN 30 (obfuscated)
+ - [ 3, 247617, 1, 273, 0] # EV_KEY / BTN_RIGHT 0
+ - [ 3, 247617, 0, 0, 0] # ------------ SYN_REPORT (0) ---------- +880ms
+
+Note: if ``libinput`` is not available on your system try using ``evemu-record``.
+
+When something does not work
+============================
+
+There can be a number of reasons for why a device does not behave
+correctly. For example
+
+* The HID report descriptor provided by the HID device may be wrong
+ because e.g.
+
+ * it does not follow the standard, so that the kernel
+ will not able to make sense of the HID report descriptor;
+ * it is possible to verify, by reading the raw HID data, that
+ the HID report descriptor *does not match* what is actually
+ sent by the device;
+
+or
+
+* the HID report descriptor may need some "quirks" (see later on);
+
+As a consequence, a suitable ``/dev/input/event*`` will not created
+for each Application Collection, and/or the events
+there will match what you would expect.
+
+
+Quirks
+------
+
+There are some known peculiarities of HID devices that the kernel
+knows how to fix - these are called the HID quirks and a list of those
+are available in `include/linux/hid.h`.
+
+Should this be the case,
+it should be enough to add the required quirk,
+in the kernel, for the HID device at hand.
+This can be done in file `drivers/hid/hid-quirks.c`.
+How to do it should be relatively straightforward
+after looking into the file.
+
+The list of currently defined quirks, from
+`include/linux/hid.h`, is
+
+.. kernel-doc:: include/linux/hid.h
+ :doc: HID quirks
+
+Quirks for USB devices can be specified
+while loading the usbhid module,
+see ``modinfo usbhid``, although the proper fix
+should go into hid-quirks.c and **submitted upstream**.
+See, again, Documentation/process/submitting-patches.rst
+for guidelines on how to do submit a patch.
+Quirks for other busses need to go into hid-quirks.c
+
+Fixing HID report descriptors
+-----------------------------
+
+Should you need to patch HID report descriptors
+the easiest way is to resort to eBPF, as described
+in Documentation/hid/hid-bpf.rst.
+
+Basically, you can change any byte of the original HID report descriptor.
+The examples in samples/hid should be a good starting point
+for your code, see e.g. `samples/hid_mouse.bpf.c`::
+
+ SEC("fmod_ret/hid_bpf_rdesc_fixup")
+ int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
+ {
+ ....
+ data[39] = 0x31;
+ data[41] = 0x30;
+ return 0;
+ }
+
+Of course this can be also done within the kernel source
+code, see e.g. `drivers/hid/hid-aureal.c` or
+`drivers/hid/hid-samsung.c` for a slightly more complex file.
+
+Check Documentation/hid/hidreport-parsing.rst
+if you need an help navigating the HID manuals and
+understanding the exact meaning of
+the HID report descriptor hex numbers.
+
+Whatever solution you come up with, please remember to **submit the
+fix to the HID maintainers**, so that it can be directly
+integrated in the kernel
+and that particular HID device will will start
+working for everyone else.
+See Documentation/process/submitting-patches.rst
+for guidelines on how to do this.
+
+
+Modifying the transmitted data on the fly
+-----------------------------------------
+
+Using eBPF it is also possible to modify the data exchanged
+with the device. See again the examples in `samples/hid`.
+
+Again, **please post your fix**, so that
+it can be integrated in the kernel!
+
+Writing a specialized driver
+----------------------------
+
+This should really be your last resort.
+
+
+.. rubric:: Footnotes
+
+.. [#hidraw] reading hidraw: see Documentation/hid/hidraw.rst and
+ file `samples/hidraw/hid-example.c` for an example.
+ The output of ``hid-example`` would be, for the same mouse::
+
+ marco@sun:~> sudo ./hid-example
+ Report Descriptor Size: 52
+ Report Descriptor:
+ 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 3 15 0 25 1 75 1 95 3 81 2 75 5 95 1 81 1 5 1 9 30 9 31 9 38 15 81 25 7f 75 8 95 3 81 6 c0 c0
+
+ Raw Name: PixArt USB Optical Mouse
+ Raw Phys: usb-0000:05:00.4-2.3/input0
+ Raw Info:
+ bustype: 3 (USB)
+ vendor: 0x093a
+ product: 0x2510
+ ...
diff --git a/Documentation/hid/hidreport-parsing.rst b/Documentation/hid/hidreport-parsing.rst
new file mode 100644
index 000000000000..a31a7aec5947
--- /dev/null
+++ b/Documentation/hid/hidreport-parsing.rst
@@ -0,0 +1,52 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================================
+Manual parsing of HID report descriptors
+========================================
+
+Consider again the mouse HID report descriptor
+introduced in Documentation/hid/hidintro.rst::
+
+ marco@sun:~> hexdump -C /sys/bus/hid/devices/0003\:093A\:2510.0002/report_descriptor
+ 00000000 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 |..............).|
+ 00000010 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 |..%.u.....u.....|
+ 00000020 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 |...0.1.8..%.u...|
+ 00000030 81 06 c0 c0 |....|
+ 00000034
+
+and try to parse it by hand.
+
+Start with the first number, 0x05: according to
+the HID spec, Sec. 6.2.2.2, "Short Items"
+
+Start with the first number, 0x05 which carries 2 bits for the
+length of the item, 2 bits for the type of the item and 4 bits for the
+function::
+
+ +----------+
+ | 00000101 |
+ +----------+
+ ^^
+ ---- Length of data (see HID spec 6.2.2.2)
+ ^^
+ ------ Type of the item (see HID spec 6.2.2.7)
+ ^^^^
+ --------- Function of the item (see HUT Sec 3)
+
+In our case, the length is 1 byte, the type is ``Global`` and the function
+is ``Usage Page``, thus we need to refer to HUT Sec 3 which indicates that
+the value 0x01 in the second byte stands for ``Generic Desktop Page``.
+
+The second number is the actual data, and its meaning can
+be found in the HUT.
+We have an ``Usage Page``, thus we need to refer to HUT Sec. 3,
+"Usage Pages"; from there, one sees that the ``0x01``
+stands for ``Generic Desktop Page``.
+
+Moving now to the second two bytes, and following the same scheme, ``0x09``
+(i.e. ``00001001``) will be followed by one byte (``01``)
+and is a ``Local`` item.
+Thus, the meaning of the remaining four bits (``0000``)
+is given in the HID spec Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
+
+The following numbers can be parsed in the same way.
diff --git a/Documentation/hid/index.rst b/Documentation/hid/index.rst
index b2028f382f11..af02cf7cfa82 100644
--- a/Documentation/hid/index.rst
+++ b/Documentation/hid/index.rst
@@ -7,6 +7,7 @@ Human Interface Devices (HID)
.. toctree::
:maxdepth: 1
+ hidintro
hiddev
hidraw
hid-sensor
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7f2e8ba7d783..ad12a36d9993 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -341,6 +341,29 @@ struct hid_item {
*/
#define MAX_USBHID_BOOT_QUIRKS 4
+/**
+* DOC: HID quirks
+* | @HID_QUIRK_NOTOUCH:
+* | @HID_QUIRK_IGNORE: ignore this device
+* | @HID_QUIRK_NOGET:
+* | @HID_QUIRK_HIDDEV_FORCE:
+* | @HID_QUIRK_BADPAD:
+* | @HID_QUIRK_MULTI_INPUT:
+* | @HID_QUIRK_HIDINPUT_FORCE:
+* | @HID_QUIRK_ALWAYS_POLL:
+* | @HID_QUIRK_INPUT_PER_APP:
+* | @HID_QUIRK_X_INVERT:
+* | @HID_QUIRK_Y_INVERT:
+* | @HID_QUIRK_SKIP_OUTPUT_REPORTS:
+* | @HID_QUIRK_SKIP_OUTPUT_REPORT_ID:
+* | @HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP:
+* | @HID_QUIRK_HAVE_SPECIAL_DRIVER:
+* | @HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE:
+* | @HID_QUIRK_FULLSPEED_INTERVAL:
+* | @HID_QUIRK_NO_INIT_REPORTS:
+* | @HID_QUIRK_NO_IGNORE:
+* | @HID_QUIRK_NO_INPUT_SYNC:
+*/
/* BIT(0) reserved for backward compatibility, was HID_QUIRK_INVERT */
#define HID_QUIRK_NOTOUCH BIT(1)
#define HID_QUIRK_IGNORE BIT(2)
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 1/1] HID: Add introduction about HID for non-kernel programmers
From: Marco Morandini @ 2023-06-22 20:37 UTC (permalink / raw)
To: Peter Hutterer
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, Jonathan Corbet,
linux-doc
In-Reply-To: <20230622075905.GA35108@quokka>
>
> shameless plug! :) I wrote this post a few years ago, feel free to
> incorporate bits from there into here
> https://who-t.blogspot.com/2018/12/understanding-hid-report-descriptors.html
Damn! This is much better than all the tutorials
I found while trying to orient
myself, and I managed to miss it :(
Would it be ok to link to it from the doc?
>> +Many HID work out the box, even if their hardware is different.
>
> See Benjamin's comment too, I think it's better to use "HID device"
> here, primarily because we have the "HID protocol", "HID parser", "HID
> quirks", so it's more a qualifier than what the acronym actually stands
> for.
I should have fixed this, please double check.
>> +For example, mouses can have a different number of buttons; they
>
> s/a different/any/ - otherwise it's "different to what?"
Ok
>> +can have (or not) a wheel; their movement sensitivity can be
>
> I'd use "may have a wheel" but it's correct this way too
Better "may"
>
>> +significantly different, and so on. Nonetheless,
>
> maybe something like "movement sensitivity differs between different
> models"
Ok
>
>> +most of the time everything just works, without the need
>> +to have specialized code in the kernel for any mouse model
>
> s/any/every/
Ok
>
>> +developed since 1970.
>> +
>> +This is because most (if not all) of the modern HIDs do advertise
>> +the number and type of signal that can be exchanged, and
>> +describe how such signal are exchanged with the computer
>> +by means of *HID events*.
>> +This is done through the *HID report descriptor*, basically a bunch of numbers
>> +that allow the operating system to understand that the mouse at hand
>> +has (say) five rather than three buttons.
>
> I think the above lines are at risk of confusing newbies, how about
> something like this:
>
> This is because modern HID devices do advertise
> their capabilities through the *HID report descriptor*, a
> fixed set of bytes describing exactly what *HID reports*
> may be sent between the device and the host and the meaning of each
> individual bit in those reports. For example, a HID Report Descriptor
> may specify that "in a report with ID 3 the second byte is the delta x
> coordinate of a mouse".
>
> The HID report itself then merely carries the actual data values
> without any extra meta information. Note that HID reports may be sent
> from the device ("Input Reports", i.e. input events), to the device
> ("Output Reports" to e.g. change LEDs) or used for device
> configuration ("Feature reports"). A device may support one or more
> HID reports.
Much better, thank you.
>> +The HID subsystem is in charge of parsing the HID report descriptors,
>> +and of converts HID events into normal input
>
> s/of//
Ok
>
>> +device interfaces (see Documentation/hid/hiddev.rst).
>> +Whenever something does not work as it should this can be
>> +because the HID report descriptor provided by the device is wrong,
>
> "Devices may misbehave because the HID report descriptor ..."
>
>> +or because it needs to be dealt with in a special way,
>> +or because the some special device or interaction mode
>> +is not handled by the default code.
>> +
>> +The format of HID report descriptors is described by two documents,
>> +available from the `USB Implementers Forum <https://www.usb.org/>`_
>> +at `this <https://www.usb.org/hid>`_ addresses:
>> +
>> + * the `HID USB Device Class Definition <https://www.usb.org/document-library/device-class-definition-hid-111>`_ (HIDUDC from now on)
>
> Benjamin may have more opinion on that but to me this was only ever "the
> HID spec" :) The term "HIDUDC" also doesn't provide anything useful in
> the search engines so anyone reading over this bit and skipping ahead
> will be punished for it.
>
>> + * the `HID Usage Tables <https://usb.org/document-library/hid-usage-tables-14>`_ (HIDUT from now on)
>
> can we name this "HUT" please? because that's the only term I've ever
> seen referred it to other than as HID Usage Tables.
s/"HIDUDC manual"/"HID spec"/
s/"HIDUDC"/"HID spec"/
s/HIDUT/HUT/
Thnak you!
>> +This does not means that the HID subsystem can deal with USB devices only;
>
> s/means/mean/
Fixed
>
>> +rather, different transport drivers, such as I2C or Bluetooth, can be dealt
>> +with, see Documentation/hid/hid-transport.rst.
>> +
>> +Parsing an HID descriptor
>> +=========================
>
> I'm pretty sure that most people just say "hid" like a word rather than
> spelling out H-I-D, so this would mean it would be "a HID descriptor".
> In practise, this particular nasty corner of the english language can be
> avoided by always calling them the plural HID report descriptors and
> thus avoiding the windmill battles of "a" vs "an".
>
> Also, I'd rather you alway use "report descriptor" over "descriptor"
> since that is the full term and non-ambiguous.
>
Should be fixed.
>> +Alternatively, the HID report descriptor can be read by accessig the hidraw
>
> typo: accessing
Fixed
>> +driver, see Documentation/output/hid/hidraw.rst and
>> +file samples/hidraw/hid-example.c for a simple example.
>> +The output of ``hid-example`` would be, for the same device::
>> +
>> + marco@sun:~> sudo ./hid-example
>> + Report Descriptor Size: 52
>> + Report Descriptor:
>> + 5 1 9 2 a1 1 9 1 a1 0 5 9 19 1 29 3 15 0 25 1 75 1 95 3 81 2 75 5 95 1 81 1 5 1 9 30 9 31 9 38 15 81 25 7f 75 8 95 3 81 6 c0 c0
>> +
>> + Raw Name: PixArt USB Optical Mouse
>> + Raw Phys: usb-0000:05:00.4-2.3/input0
>> + Raw Info:
>> + bustype: 3 (USB)
>> + vendor: 0x093a
>> + product: 0x2510
>> + HIDIOCSFEATURE: Broken pipe
>> + HIDIOCGFEATURE: Broken pipe
>> + Error: 32
>> + write: Broken pipe
>> + read: Resource temporarily unavailable
>
> I don't think you need to include the error messages.
Ok
> Also: you now already have 2 tools to look at hid and then you use two
> more later (hidrrd and hid-tools). I'd say it's best to just stick to
> one tool to reduce the mental load of having to map different tool
> outputs to what is the same base data.
I'm not sure about this: I think that hidraw needs to be introduced somewhere,
if for nothing because it's what gets consumed by hid-record .
Furthermore, looking at samples/hidraw/hid-example.c one can learn something, if not a lot.
For the time being I'm leaving the paragraph, moving most of it into a footnote;
of course we can drop it if you have a strong opinion about this.
>
>> +
>> +The basic structure of a HID report descriptor is defined in the HIDUDC manual, while
>> +HIDUT "defines constants that can be interpreted by an application to
>> +identify the purpose and meaning of a data
>> +field in a HID report". Each entry is defined by at least two bytes,
>> +where the first one defines what type of value is following,
>> +and is described in the HIDUDC manual,
>> +and the second one carries the actual value,
>> +and is described in the HIDUT manual.
>> +
>> +Let consider the tirst number, 0x05: according to
>
> s/let/let's/
>
I changed it into "Start the first ..."
>> +HIDUDC, Sec. 6.2.2.2, "Short Items"
>> +
>> + * the first least significant two bits
>> + define the number of the following data bytes (either 0, 1, 2 or 4
>> + for the values 0, 1, 2 or 3, respectively)
>> + * the second least significant two bits identify the type of the item:
>> +
>> + * 0: ``Main``
>> + * 1: ``Global``
>> + * 2: ``Local``
>> + * 3: ``Reserved``
>> + * the remaining four bits give a numeric expression specifying
>> + the function of the item (see below);
>> +
>> +This means that ``0x05`` (i.e. ``00000101``) stands for
>> +1 byte of data to be read, and Global type.
>> +Since we are dealing with a Global item the meaning
>> +of the most significant four bits
>> +is defines in HIDUC manual, Sec. 6.2.2.7, "Global Items".
>
> typo with "HIDUC"
Ok
>
>> +The bits are ``0000``, thus we have a ``Usage Page``.
>
> I'm guessing you *don't* want to explain the full "how bit's being parsed"
> so a simpler approach would be a basic byte diagram with references
> to the official spec:
>
> Then you get something like:
>
> """
> Let's consider the first number, 0x05 which carries 2 bits for the
> length of the item, 2 bits for the type of the item and 4 bits for the
> function:
>
> +----------+
> | 00000101 |
> +----------+
> ^^
> ---- Length of data (see UDC 6.2.2.2)
> ^^
> ------ Type of the item (see UDC 6.2.2.7)
> ^^^^
> --------- Function of the item (see HUT Sec 3)
>
> In our case, the length is 1 byte, the type a "Global" and the function
> is "Usage Page", thus we need to refer to HUT Sec 3 which indicates that
> the value 0x01 in the second byte stands for ``Generic Desktop Page``.
> """
Well... my intention was to be more verbose (not every programmer out there
routinely deals with bytes and bits; for sure I'm not one of those
how are confortable with them :( )
but it's perfectly ok as you've suggested.
>> +
>> +
>> +The second number is the actual data, and its meaning can
>> +be found in the HICUT manual.
>> +We have an ``Usage Page``, thus we need to refer to HICUT Sec. 3,
>> +"Usage Pages"; from there, it is clear that the ``0x01``
>> +stands for a ``Generic Desktop Page``.
>
> note "Generic Desktop Page", you can skip the "a" here
>
Ok
>> +
>> +Moving now to the second two bytes, and following the same scheme, ``0x09``
>> +(i.e. ``00001001``) will be followed by one byte (``01``)
>> +and is a ``Local`` item.
>> +Thus, the meaning of the remaining four bits (``0000``)
>> +is given in HIDUDC Sec. 6.2.2.8 "Local Items", so that we have an ``Usage``.
>> +
>> +In this way the HID report descriptor can be painstakingly
>> +parsed, byte by byte. In practice you need not to do this,
>> +and almost no one does
>> +this: everyone resorts to specialized parsers. Among all the available ones
>
> I would be harsher here: "In practice you should not do this, use an
> existing parser" because the people who first learn about HID here
> should probably not immediately write a parser (mostly fo their own
> sanity :)
Changed into
"In practice you should not do parse HID report descriptors by hand;
rather, you should use an existing parser."
Not sure if you agree with the preceding paragraph, that I've added
after moving the hand-parsing example into a separate document, as suggested by Benjamin:
"HID report descriptors can, in principle, be painstakingly
parsed by hand, byte by byte.
A short introduction
on how to do this is sketched in Documentation/hid/hidreport-parsing.rst;
you need to understand it only if you need to patch HID report descriptors.
"
>> + # 0x75, 0x08, // Report Size (8) 44
>> + # 0x95, 0x03, // Report Count (3) 46
>> + # 0x81, 0x06, // Input (Data,Var,Rel) 48
>> + # 0xc0, // End Collection 50
>> + # 0xc0, // End Collection 51
>> + #
>> + R: 52 05 01 09 02 a1 01 09 01 a1 00 05 09 19 01 29 03 15 00 25 01 75 01 95 03 81 02 75 05 95 01 81 01 05 01 09 30 09 31 09 38 15 81 25 7f 75 08 95 03 81 06 c0 c0
>> + N: device 0:0
>> + I: 3 0001 0001
>
> Benjamin already said this but: pick one tool and use its output, there
> is no need to show different tools being different. Since Benjamin is
> both the kernel maintainer and the hid-tools maintainer, that one should
> be the favourite ;)
>
Yes, my fault: it was commented.
As already written to Bejamin, I'd like to leave the links
to hiddrd because its output is more verbose.
Deleted
>> +
>> +From it we undesratnd that
>
> typo
Fixed
>
>> +
>> + * the mouse has three (from ``Usage Minimum (1)`` to
>> + ``Usage Maximum (3)``) buttons (``Usage Page (Button)``);
>> + * buttons can take values ranging from ``0`` to ``1``;
>> + (from ``Logical Minimum (0)`` to ``Logical Maximum (1)``);
>
> so, remembering my early HID learnings - if you just throw out "Usage
> Minimum" and "Maximum" that is mostly meaningless unless one also reads
> the definition of what those mean. For me it took me a while to wrap my
> head around the term "Usage" to begin with, it not really being an
> english word that you'd encounter every day.
>
> I think it'll be more understandable annotating the button line by line
> with layperson's terms - anything specific needs to be externally looked
> up anyway. So you get something like this:
>
> """
>> + # 0x05, 0x09, // Usage Page (Button) 10
>
> what follows is a button
>
>> + # 0x19, 0x01, // Usage Minimum (1) 12
>> + # 0x29, 0x03, // Usage Maximum (3) 14
>
> first button is button number 1, last button is button number 3
>
>> + # 0x15, 0x00, // Logical Minimum (0) 16
>> + # 0x25, 0x01, // Logical Maximum (1) 18
>
> each button can send values from 0 up to including 1 (i.e. they're
> binary buttons)
>
>> + # 0x75, 0x01, // Report Size (1) 20
>
> each button is sent as exactly one bit
>
>> + # 0x95, 0x03, // Report Count (3) 22
>
> and there are three of those bits (matching our three buttons)
>
>> + # 0x81, 0x02, // Input (Data,Var,Abs) 24
>
> it's actual Data (not constant padding), they represent a single
> variable (Var) and the value are Absolute (not relative).
> See HIDUDC Sec. 6.2.2.5 "Input, Output, and Feature Items.
> """
Done, but I'm not convinced by how the result is formatted.
After some experiments I ended up putting everything
inside the same verbatim block, but if you can suggest
a better alternative that does not disrupt too much
the parsed report descriptor you are more than welcome.
>> + * information is encoded into three bits: one bit has
>> + ``Report Size (1)``,
>> + but there are three of them since ``Report Count (3)``;
>> + * the value of these bits can change
>> + (``Data`` in ``Input (Data,Var,Abs)``);
>> + * each field represents data from a physical control;
>> + * the number of bits reserved for each field is determined
>> + by the preceding ``Report Size``/``Report Count``
>> + items (``Var`` in ``Input (Data,Var,Abs)``);
>
> tbh, I think that's a mischaracterization of Var vs Arr (and tbh the
> exact detail what those mean doesn't really matter here anyway).
Ok.
>
>> + * the data is *absolute* (i.e it does not represent the
>> + change from the last report, ``Abs`` in ``Input (Data,Var,Abs)``).
>> +
>> +The meaning of the ``Input``
>> +items is explained in HIDUDC Sec. 6.2.2.5 "Input, Output, and Feature Items.
>> +
>> +There are five additional padding bits, that are needed
>> +to reach a byte: see ``Report Size (5)``, that is
>> +repeated only once (``Report Count (1)``).
>> +These bits take *constant* values (``Cnst`` in
>> +``Input (Cnst,Arr,Abs)``).
>> +
>> +The mouse has also two physical positions (``Usage (X)``, ``Usage (Y)``) and a wheel
>> +(``Usage (Wheel)``).
>> +
>> +Each of them take values ranging from ``-127`` to ``127``
>> +(from ``Logical Minimum (-127)`` to ``Logical Maximum (-127)``),
>> +it is represented by eight bits (``Report Size (8)``)
>> +and there are three of these set of bits (``Report Count (3)``).
>> +
>> +This time the data do represent the change from the previous configuration
>> +(``Rel`` in ``Input (Data,Var,Rel)``).
>> +
>> +All in all, the mouse input will be transmitted using four bytes:
>
> I would say "the Report Descriptor tells us that mouse input will be..."
> because I think that makes it a bit more obvious
Ok. Thank you!
>> +the first one for the buttons (three bits used, five for padding),
>> +the last three for the mouse X, Y and wheel changes, respectively.
>> +
>> +Indeed, for any event, the mouse will send a *report* of four bytes.
>> +We can easily check the values sent by resorting e.g.
>
> s/easily// because it may not be for some users
Ok.
>> +to the `hid-recorder` tool, from `hid-tools
>> +<https://gitlab.freedesktop.org/libevdev/hid-tools>`_:
>> +The sequence of bytes sent by clicking and releasing
>> +button 1, then button 2, then button 3 is::
>> +
>> + marco@sun:~/> sudo ./hid-recorder /dev/hidraw1
>> +
>> + ....
>> + output of hid-decode
>> + ....
>> +
>> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000000.000000 4 01 00 00 00
>> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000000.183949 4 00 00 00 00
>> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000001.959698 4 02 00 00 00
>> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000002.103899 4 00 00 00 00
>> + # Button: 0 0 1 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000004.855799 4 04 00 00 00
>> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000005.103864 4 00 00 00 00
>> +
>> +where it's clear that, for example, when button 2 is clicked
>> +the bytes ``02 00 00 00`` are sent, and the immediately subsequent
>> +event (``00 00 00 00``) is the release of button 2 (no buttons are pressed,
>> +remember that the data is *absolute*).
>
> "where it's clear" can be problematic for readers for whom it's not
> clear at all :) In this case you can write
> "this example shows that when button 2 is clicked..." to avoid
> that issue. I think there are a few uses of that phrase or similar in
> this document.
Good point, thank you.
I should have got ridden af all the "clear"
instances.
Also
"The examples in samples/hid should be relatively straightforward, see..."
->
"The examples in samples/hid should be a good starting point
for your code, see ..."
>> +If instead one clicks and holds button 1, then clicks and holds button 2,
>> +releases button 1, and finally releases button 2, the reports are::
>> +
>> + # Button: 1 0 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000044.175830 4 01 00 00 00
>> + # Button: 1 1 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000045.975997 4 03 00 00 00
>> + # Button: 0 1 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000047.407930 4 02 00 00 00
>> + # Button: 0 0 0 | # | X: 0 | Y: 0 | Wheel: 0
>> + E: 000049.199919 4 00 00 00 00
>> +
>> +where with ``03 00 00 00`` both buttons are pressed, and with the
>> +subsequent ``02 00 00 00`` button 1 is released while button 2 is still
>> +active.
>> +
>> +Outputs and Inputs
>> +------------------
>> +
>> +An HID devices can have inputs, like
>
> I would refer to those as "Input Reports" and "Output Reports", simply
> because for me it was much easier to adjust my head to accept that you
> can send something to the mouse than that the mouse "has an output".
Changed, please doulbe check for correctness.
>
>> +in the mouse example, and outputs.
>> +"Output" means that the information is fed
>> +from the device to the human; for examples,
>> +a joystick with force feedback will have
>> +some output.
>> +
>> +
>> +Report IDs and Evdev events
>> +===========================
>> +
>> +A single device can logically group
>> +data into different, independent sets.
>> +It is *as if* the HID presents
>> +itself as different devices, each exchanging
>> +its own data. The HID report descriptor is unique,
>> +but the different reports are identified by means
>> +of different ``Report ID`` fields. Whenever a ``Report ID``
>> +is needed it is transmitted as the first byte of any report.
>
> This is a bit ambiguous since it's the collections and applications that
> split the device into different sets, the reports are just different
> ways of reporting data that belongs to one (or more? not 100% sure) of
> those sets then.
>
> And the example below works because you have different collections on
> your device here but that's largely orthogonal to the use of reports.
I think this came from my misunderstanding of the role of Report IDs
and Application Collections (not that I'm sure to have undertsood everything).
The paragraph has been rewritte, please double check.
I'm glossing on the fact that the same Collection can have different Report IDs
inside (btw: is this correct? And if yes, is this something that is done, in practice?).
>> +
>> +Consider the following HID report descriptor::
>> +
>> + 05 01 09 02 A1 01 85 01 05 09 19 01 29 05 15 00
>> + 25 01 95 05 75 01 81 02 95 01 75 03 81 01 05 01
>> + 09 30 09 31 16 00 F8 26 FF 07 75 0C 95 02 81 06
>> + 09 38 15 80 25 7F 75 08 95 01 81 06 05 0C 0A 38
>> + 02 15 80 25 7F 75 08 95 01 81 06 C0 05 01 09 02
>> + A1 01 85 02 05 09 19 01 29 05 15 00 25 01 95 05
>> + 75 01 81 02 95 01 75 03 81 01 05 01 09 30 09 31
>> + 16 00 F8 26 FF 07 75 0C 95 02 81 06 09 38 15 80
>> + 25 7F 75 08 95 01 81 06 05 0C 0A 38 02 15 80 25
>> + 7F 75 08 95 01 81 06 C0 05 01 09 07 A1 01 85 05
>> + 05 07 15 00 25 01 09 29 09 3E 09 4B 09 4E 09 E3
>> + 09 E8 09 E8 09 E8 75 01 95 08 81 02 95 00 81 01
>> + C0 05 0C 09 01 A1 01 85 06 15 00 25 01 75 01 95
>> + 01 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
>> + 06 09 3F 81 06 09 3F 81 06 09 3F 81 06 09 3F 81
>> + 06 C0 05 0C 09 01 A1 01 85 03 09 05 15 00 26 FF
>> + 00 75 08 95 02 B1 02 C0
>> +
>> +After parsing it (try to parse it on your own using
>> +the suggested tools!)
>> +one can see that the device presents two mouses
>> +(Reports IDs 1 and 2, respectively),
>> +a Keypad (Report ID 5) and two consumer controls
>> +(Report IDs 6 and 3).
>> +The data sent for each of these report ids
>> +will begin with the Report ID byte, and will be followed
>> +by the corresponding information. For example, the
>> +report defined for the last consumer
>> +control::
>> +
>> + 0x05, 0x0C, // Usage Page (Consumer)
>> + 0x09, 0x01, // Usage (Consumer Control)
>> + 0xA1, 0x01, // Collection (Application)
>> + 0x85, 0x03, // Report ID (3)
>> + 0x09, 0x05, // Usage (Headphone)
>> + 0x15, 0x00, // Logical Minimum (0)
>> + 0x26, 0xFF, 0x00, // Logical Maximum (255)
>> + 0x75, 0x08, // Report Size (8)
>> + 0x95, 0x02, // Report Count (2)
>> + 0xB1, 0x02, // Feature (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
>> + 0xC0, // End Collection
>> +
>> +will be of three bytes: the first for the Report ID (5), the next two
>> +for the headphone, with two (``Report Count (2)``) bytes
>> +(``Report Size (8)``) each ranging from 0 (``Logical Minimum (0)`` to 255
>> +(``Logical Maximum (255)``).
>> +
>> +
>> +Events
>> +======
>> +
>> +One can expect that different ``/dev/input/event*`` are created for different
>> +Report IDs.
>
> as Benjamin already mentioned, this is incorrect given how the kernel
> works, so definitely needs rewording.
Done, please double check.
>
>> Going back to the mouse example, and repeating the sequence where
>> +one clicks and holds button 1, then clicks and holds button 2,
>> +releases button 1, and finally releases button 2, one gets::
>> +
>> + marco@sun:~> sudo evtest /dev/input/event4
>> + Input driver version is 1.0.1
>> + Input device ID: bus 0x3 vendor 0x3f0 product 0x94a version 0x111
>> + Input device name: "PixArt HP USB Optical Mouse"
>> + Supported events:
>> + Event type 0 (EV_SYN)
>> + Event type 1 (EV_KEY)
>> + Event code 272 (BTN_LEFT)
>> + Event code 273 (BTN_RIGHT)
>> + Event code 274 (BTN_MIDDLE)
>> + Event type 2 (EV_REL)
>> + Event code 0 (REL_X)
>> + Event code 1 (REL_Y)
>> + Event code 8 (REL_WHEEL)
>> + Event code 11 (REL_WHEEL_HI_RES)
>> + Event type 4 (EV_MSC)
>> + Event code 4 (MSC_SCAN)
>> + Properties:
>> + Testing ... (interrupt to exit)
>> + Event: time 1687254626.454252, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
>> + Event: time 1687254626.454252, type 1 (EV_KEY), code 272 (BTN_LEFT), value 1
>> + Event: time 1687254626.454252, -------------- SYN_REPORT ------------
>> + Event: time 1687254627.342093, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
>> + Event: time 1687254627.342093, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 1
>> + Event: time 1687254627.342093, -------------- SYN_REPORT ------------
>> + Event: time 1687254627.974282, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90001
>> + Event: time 1687254627.974282, type 1 (EV_KEY), code 272 (BTN_LEFT), value 0
>> + Event: time 1687254627.974282, -------------- SYN_REPORT ------------
>> + Event: time 1687254628.798240, type 4 (EV_MSC), code 4 (MSC_SCAN), value 90002
>> + Event: time 1687254628.798240, type 1 (EV_KEY), code 273 (BTN_RIGHT), value 0
>> + Event: time 1687254628.798240, -------------- SYN_REPORT ------------
>> +
>> +
>> +When everything works well
>> +==========================
>> +
>> +* The HID report descriptor makes sense;
>> +* It is possible to verify, by reading the raw hid data, that
>> + the HID report descriptor *does match* what is sent by the device;
>> +* The HID report descriptor does not need any "quirk"s (see later on)
>
> "quirks", can have that inside the quotes
Ok
>
>> +* For any Report ID a corresponding ``/dev/input/event*`` is created,
>> + and the events there match what you would expect
>> +
>> +When something does not work
>> +============================
>
> you can fold the "everything works" section into here with some prelude
> of "There can be a number of reasons for why a device does not behave
> correctly, for example..."
>
I've tried to do that, even if not having a shining "everything magically works"
section make me sad.
>> +
>> +Sometimes not everything does work as it should.
>
>
>> +
>> +Quirks
>> +------
>> +
>> +A possible reason is that the HID
>> +has some common quirk. Should this be the case,
>> +it sould be enough to add the required quirk,
>> +in the kernel, for the device at hand.
>
> this phrasing is a bit ambiguous, because in the first sentence the
> quirks is the bug in the device, but in the second sentence the quirk
> is something the kernel provides. You can rephrase this less ambiguously
> with something like:
>
> There are some known peculiarities of HID devices that the kernel
> knows how to fix - these are called the HID quirks and a list of those
> are available in `include/linux/hid.h`.
>
Thank you!
>
>> +This can be done in file drivers/hid/hid-quirks.c .
>> +How to do it should be straightforward after looking into the file.
>> +
>> +The list of currently defined quirks, from
>> +include/linux/hid.h , is
>> +
>> + * ``HID_QUIRK_NOTOUCH``, defined as ``BIT(1)``:
>> + * ``HID_QUIRK_IGNORE``, defined as ``BIT(2)``:
>> + * ``HID_QUIRK_NOGET``, defined as ``BIT(3)``:
>> + * ``HID_QUIRK_HIDDEV_FORCE``, defined as ``BIT(4)``:
>> + * ``HID_QUIRK_BADPAD``, defined as ``BIT(5)``:
>> + * ``HID_QUIRK_MULTI_INPUT``, defined as ``BIT(6)``:
>> + * ``HID_QUIRK_HIDINPUT_FORCE``, defined as ``BIT(7)``:
>> + * ``HID_QUIRK_ALWAYS_POLL``, defined as ``BIT(10)``:
>> + * ``HID_QUIRK_INPUT_PER_APP``, defined as ``BIT(11)``:
>> + * ``HID_QUIRK_X_INVERT``, defined as ``BIT(12)``:
>> + * ``HID_QUIRK_Y_INVERT``, defined as ``BIT(13)``:
>> + * ``HID_QUIRK_SKIP_OUTPUT_REPORTS``, defined as ``BIT(16)``:
>> + * ``HID_QUIRK_SKIP_OUTPUT_REPORT_ID``, defined as ``BIT(17)``:
>> + * ``HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP``, defined as ``BIT(18)``:
>> + * ``HID_QUIRK_HAVE_SPECIAL_DRIVER``, defined as ``BIT(19)``:
>> + * ``HID_QUIRK_INCREMENT_USAGE_ON_DUPLICATE``, defined as ``BIT(20)``:
>> + * ``HID_QUIRK_FULLSPEED_INTERVAL``, defined as ``BIT(28)``:
>> + * ``HID_QUIRK_NO_INIT_REPORTS``, defined as ``BIT(29)``:
>> + * ``HID_QUIRK_NO_IGNORE``, defined as ``BIT(30)``:
>> + * ``HID_QUIRK_NO_INPUT_SYNC``, defined as ``BIT(31)``:
>
> You *definitely* don't want to include this here because it will get out
> of sync (and the actual bit value just doesn't matter anyway). Use one
> or two as example just so a reader gets familiar with the
> HID_QUIRK_FOOBAR notation and can use grep to find where quirks are
> used and go from there.
Following Benjamin, I've brough in the comment I propose to
add in include/linux/hid.h
>
>> +
>> +
>> +FIXME: ADD A SHORT EXPLANATION FOR EACH QUIRK
>
> as Benjamin already mentioned, this should be documented in situ and
> linked to from here rather than split across multiple files.
>
See above
>> +
>> +Quirks for USB devices can be specified run-time,
>
> "at runtime"
I've changed this into "Quirks for USB devices can be specified
while loading the usbhid module",
>
>> +see ``modinfo usbhid``, although the proper fix
>> +should go into hid-quirks.c and be submitted upstream.
>> +Quirks for other busses need to go into hid-quirks.c
>> +
>> +Fixing the HID report descriptor
>> +--------------------------------
>> +
>> +Should you need to patch the HID report descriptor
>> +the easiest way is to resort to eBPF, as described
>> +in Documentation/output/hid/hid-bpf.rst.
>> +
>> +Basically, you can change any byte of the original report descriptor.
>> +The examples in samples/hid should be relatively straightforward,
>> +see e.g. samples/hid_mouse.bpf.c::
>> +
>> + SEC("fmod_ret/hid_bpf_rdesc_fixup")
>> + int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
>> + {
>> + ....
>> + data[39] = 0x31;
>> + data[41] = 0x30;
>> + return 0;
>> + }
>> +
>> +Of course this can be also done within the kernel source
>> +code, see e.g. drivers/hid/hid-aureal.c or
>> +drivers/hid/hid-samsung.c for a slightly more complex file.
>> +
>> +
>> +
>> +Modifying on the fly the transmitted data
>> +-----------------------------------------
>
> "Modifying the transmitted data on the fly"
>
Thank you
>> +
>> +It is also possible, always using eBPF, to modify
>
>
>> +on the fly the data exchanged with the device.
>> +See, again, the examples is samples/hid.
>
> Using eBPF it is also possible to modify the data exchanged
> with the device. See again the examples in samples/hid
+1
^ permalink raw reply
* Re: [PATCH 1/1] HID: Add introduction about HID for non-kernel programmers
From: Marco Morandini @ 2023-06-22 20:37 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, linux-input, Peter Hutterer, Jonathan Corbet,
linux-doc
In-Reply-To: <ndmq5lfkkjm4hkxv7423wz2esculnk3rfmxewnxav673tgssbr@dxsfytvyrepf>
Hope to have taken everying in consideration,
and not messed up things while trying to
account both for your and Peter's comments.
Also: I'm going to send an independent v2 with a link
to v1 after the "---".
I'm not sure whether this is the right thing to do for a single
patch like this one.
>> +Many HID work out the box, even if their hardware is different.
>
> FWIW, I always refer to a device following the HID standard to a "HID
> device". Done
>> +For example, mouses can have a different number of buttons; they
>
> s/mouses/mice/
Ok
>> +The HID subsystem is in charge of parsing the HID report descriptors,
>> +and of converts HID events into normal input
>> +device interfaces (see Documentation/hid/hiddev.rst).
>
> hiddev is deprecated I would say. There are still a few users, but I'm
> not sure they are quite actively developping products. hidraw is what
> you want when you want to talk to the HID devices at the raw level, and
> evdev (/dev/input/event*) is what libinput and Xorg consume.
>
Hm... hope to have linked the correct document this time (hid-transport.rst).
>> +The basic structure of a HID report descriptor is defined in the HIDUDC manual, while
>> +HIDUT "defines constants that can be interpreted by an application to
>> +identify the purpose and meaning of a data
>> +field in a HID report". Each entry is defined by at least two bytes,
>> +where the first one defines what type of value is following,
>> +and is described in the HIDUDC manual,
>> +and the second one carries the actual value,
>> +and is described in the HIDUT manual.
>
> I think the next part up to the various tools that allow to decode the report descriptors
> is interesting, but should probably be in a separate file, or in a footnote.
>
> IMO, what matters here is:
> - it's a "simple" language defined in those 2 documents
> - it's working as a stack: each elements adds to the previous, and
> summing everything gives you the overview (there are subtleties of
> course)
> - noone should ever try to read it by hand, there are tools :)
> - in case there is something wrong in the report descriptor, then yes
> you'll need that explanation, but it's probably too early in the doc
> to explain this IMO
>
>
I'm not sure it's better to have is somewhere else.
It's true that one should use tools. We both agree
that you really need to get the meaning of those numbers
in order to be able to fix a HID device.
The problem is that if you are not going to scratch your head over those
two manuals you are not going to understand neither the meaning of the
hex numbers, nor the meaning of the parsing tool output.
At least.... that's what happened to me.
Anyway: I've tried to emphasize the points you made above,
and to move the hand-parsing out of the way, into a different file.
>> +.. Parsing the mouse HID report descriptor with `hidrdd <https://github.com/abend0c1/hidrdd>`_ one gets::
>
> Maybe I'm too biased, but why adding the hidrdd output when you also have
> the one from hid-tools?
It was commented (".."): after writing it,
I realized that it did not add that much;
still, I did not want to throw away the text, just in case.
I did non realize that you would have reviewed the raw patch,
where the fact that it is commented is not at all obvious.
Apologize for this.
Deleted. I'm leaving, if you agree, the links to the online tool and to hiddrd.
>> +
>> +Outputs and Inputs
>> +------------------
>> +
>> +An HID devices can have inputs, like
>> +in the mouse example, and outputs.
>> +"Output" means that the information is fed
>> +from the device to the human; for examples,
>
> The other way around: outputs are from the host (computer/human) to the
> device, when input is from the device to the host.
>
I've tried to rephrase it, this time without
mentioning the human (that perhaps is confusing).
>> +a joystick with force feedback will have
>> +some output.
>
> There are also Features, which is a side channel configuration of the
> device from the host to the device.
>
> Most of the time Features have a state (are you using high reslution
> wheel events?) and can be queried from the host. Most of the time :)
>
Added Feature items (hope to have got them right).
>> +Report IDs and Evdev events
>> +===========================
>> +
>> +A single device can logically group
>> +data into different, independent sets.
>> +It is *as if* the HID presents
>> +itself as different devices, each exchanging
>> +its own data. The HID report descriptor is unique,
>> +but the different reports are identified by means
>> +of different ``Report ID`` fields. Whenever a ``Report ID``
>> +is needed it is transmitted as the first byte of any report.
>
> I wouldn't say this like that.
>
> The following is an attempt to explain to you the slight difference
> between collections and report IDs, so it should not be taken verbatim
> in the doc.
>
> You can group data by using "Collections". Each collection has a type
> and purpose. You have "application" collections, "physical" collections
> and "logical" collections. For each you can assign a purpose: for
> example a touchpanel that exports a touchscreen and a stylus would
> export 2 application collections of "Touchscreen" and "Pen".
>
> But then to be able to differentiate those collections, we have the
> "Report ID", which is handled specifically in the HID standard as the
> first byte (when defined) associated to a collection.
>
> But given that collections can be stacked, there is not a 1-to-1
> relation between Report IDs and all defined collection.
>
I've tried to rephrase the whole paragraph, also on the light of
your explanation below that Linux builds a /dev/input/event* for each
Application Collection, and not for each Report ID.
>> +one can see that the device presents two mouses
>
> s/mouses/mice/
>
Done
>> +Events
>> +======
>> +
>> +One can expect that different ``/dev/input/event*`` are created for different
>> +Report IDs. Going back to the mouse example, and repeating the sequence where
>> +one clicks and holds button 1, then clicks and holds button 2,
>> +releases button 1, and finally releases button 2, one gets::
>> +
>> + marco@sun:~> sudo evtest /dev/input/event4
>
> evtest has been deprecated for a while, and evemu too. Now, cool kids
> are using "libinput record", but it's slightly more verbose.
>
> Using evemu is probably better at least.
>
Changed to libinput. Do you think I should trim the initial output?
I'm mentioning evemu at the bottom of the paragraph.
>> +When everything works well
>> +==========================
>> +
>> +* The HID report descriptor makes sense;
>
> i.e. the current tools are capable of making some sense out of it :)
>
This is not what I meant; from the few emails I exchanged with you guys
I got the impression that it's possible, for some HID report descriptors,
to be almost completely bogus.
Should this be the case, then the kernel could still able
to make some sense out of it, but nevertheless it would not be bad to fix
the report descriptor in the first place.
Anyway, I've got rid of this Section, and merged it with the "When something does not work",
as suggested by Peter. Hope it's better now.
>> +* It is possible to verify, by reading the raw hid data, that
>> + the HID report descriptor *does match* what is sent by the device;
>
> nitpick: extra space between "does" and "match"
>
Fixed.
>> +* The HID report descriptor does not need any "quirk"s (see later on)
>> +* For any Report ID a corresponding ``/dev/input/event*`` is created,
>> + and the events there match what you would expect
>
> That last point is not stricly correct. Currently, each application
> collection gets its own input node. You can have a collection with
> several report IDs because they are all using the same device but a
> different language.
Changed. Thank you for the explanation!
>> +When something does not work
>> +============================
>> +
>> +Sometimes not everything does work as it should.
>
> *not everything works
Ok.
>> + * ``HID_QUIRK_NO_IGNORE``, defined as ``BIT(30)``:
>> + * ``HID_QUIRK_NO_INPUT_SYNC``, defined as ``BIT(31)``:
>
> We should definitely include the doc directly in hid.h and include it
> there. I already explained why in the cover letter.
>
Good point.
See whether the current solution (both in the doc and in hid.h) works for you .
I was wondering whether it could be useful to add a :block: option to the kernel-doc
directive, with the meaning that all the comments in between a :DOC and an :END_DOC
should be included in the documentation.
This could have allowed to have the documentation of the #defines
line by line. After realizing that I should have modified scripts/kernel-doc
(perl: I know nothing about it :( ) I quickly back-pedaled away from
that overengineered solution.
I have verified that It's possible to repeat the
DOC: HID quirks
comments, and they all get included,
but it would still require at least a two-lines (three?) comment in between every
#define in the source code, and I don't think you would want this.
> But moreover, quirks are supposed to be the exception. If we are adding
> too many quirks, and that the same devices work on Windows without a
> special handling, that means that the quirk should be analyzed, and we
> should probably rethink the processing of the HID devices to not have
> this quirk.
I have changed "common quirk." into "already known quirk."
Ok for you?
>> +Fixing the HID report descriptor
>> +--------------------------------
>> +
>> +Should you need to patch the HID report descriptor
>> +the easiest way is to resort to eBPF, as described
>> +in Documentation/output/hid/hid-bpf.rst.
>> +
>> +Basically, you can change any byte of the original report descriptor.
>> +The examples in samples/hid should be relatively straightforward,
>> +see e.g. samples/hid_mouse.bpf.c::
>> +
>> + SEC("fmod_ret/hid_bpf_rdesc_fixup")
>> + int BPF_PROG(hid_rdesc_fixup, struct hid_bpf_ctx *hctx)
>> + {
>> + ....
>> + data[39] = 0x31;
>> + data[41] = 0x30;
>> + return 0;
>> + }
>
> If you ever do that, please share your code on the LKML. The current
> background work is to integrate those hid-bpf programs in the kernel
> directly, so that we can share them with everyone without resorting to a
> third party userspace program.
Added three sentences about this. Too much?
Btw: would you want me to add something like "If you are completely lost
you could, as a last resort, try recording hidraw with hid-recorder (from the hid-tool utility set)
and sending it to mailto://linux-input@vger.kernel.org" ??? :-/
(if yes: the mailing list filters too big messages; for example the recording I sent you
did not reach the mailing list because the attachment was too big; how to deal with this?)
>> +Writing a specialized driver
>> +----------------------------
>> +
>> +This should really be your last resort.
>
> YES, definitely YES :)
>
:)
^ permalink raw reply
* [PATCH v3 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-22 14:29 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 13 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 172 ++++++++++++++++++++++++++
3 files changed, 186 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 5e21cca6025d..2d86615e5090 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -435,6 +435,19 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 921a2da0c2be..29524e8a83db 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..3a1bc251b32d
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "goodix_berlin.h"
+
+#define SPI_TRANS_PREFIX_LEN 1
+#define REGISTER_WIDTH 4
+#define SPI_READ_DUMMY_LEN 3
+#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN)
+#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH)
+
+#define SPI_WRITE_FLAG 0xF0
+#define SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int ret;
+
+ if (reg_size != REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
+ memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff,
+ SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ ret = spi_sync(spi, &spi_msg);
+ if (ret < 0)
+ dev_err(&spi->dev, "transfer error:%d", ret);
+ else
+ memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return ret;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int ret;
+
+ buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ ret = spi_sync(spi, &spi_msg);
+ if (ret < 0)
+ dev_err(&spi->dev, "transfer error:%d", ret);
+
+ kfree(buf);
+ return ret;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config *regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ regmap_config = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
+ sizeof(*regmap_config), GFP_KERNEL);
+ if (!regmap_config)
+ return -ENOMEM;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+ regmap_config->max_raw_read = max_size - SPI_READ_PREFIX_LEN;
+ regmap_config->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v3 3/4] input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-22 14:29 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy
Cc: linux-input, linux-arm-msm, devicetree, linux-kernel,
Neil Armstrong
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 69 +++++++++++++++++++++++++++
3 files changed, 84 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1a6f6f6da991..5e21cca6025d 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -421,6 +421,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_CORE
depends on REGMAP
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ depends on REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 29cdb042e104..921a2da0c2be 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..6407b2258eb1
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
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