* [PATCH 3/3] Input: ads7846 - don't check penirq immediately for 7845
From: Luca Ellero @ 2023-01-26 10:52 UTC (permalink / raw)
To: dmitry.torokhov, daniel, m.felsch, andriy.shevchenko,
u.kleine-koenig, mkl, miquel.raynal, imre.deak, luca.ellero
Cc: linux-input, linux-kernel, Luca Ellero
In-Reply-To: <20230126105227.47648-1-l.ellero@asem.it>
To discard false readings, one should use "ti,penirq-recheck-delay-usecs".
Checking get_pendown_state() at the beginning, most of the time fails
causing malfunctioning.
Fixes: ffa458c1bd9b ("spi: ads7846 driver")
Signed-off-by: Luca Ellero <l.ellero@asem.it>
---
drivers/input/touchscreen/ads7846.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 15da1047a577..17f11bce8113 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -843,14 +843,8 @@ static void ads7846_report_state(struct ads7846 *ts)
if (x == MAX_12BIT)
x = 0;
- if (ts->model == 7843) {
+ if (ts->model == 7843 || ts->model == 7845) {
Rt = ts->pressure_max / 2;
- } else if (ts->model == 7845) {
- if (get_pendown_state(ts))
- Rt = ts->pressure_max / 2;
- else
- Rt = 0;
- dev_vdbg(&ts->spi->dev, "x/y: %d/%d, PD %d\n", x, y, Rt);
} else if (likely(x && z1)) {
/* compute touch pressure resistance using equation #2 */
Rt = z2;
--
2.25.1
^ permalink raw reply related
* Re: [PATCH resend 2/3] Input: silead - Add a settings module-parameter
From: Hans de Goede @ 2023-01-26 9:45 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: Dmitry Torokhov, Bastien Nocera, Gregor Riepl, linux-input
In-Reply-To: <Y9IG6zQtGWWwhyLW@nixie71>
Hi,
On 1/26/23 05:51, Jeff LaBundy wrote:
> Hi Hans,
>
> On Wed, Jan 25, 2023 at 11:54:15AM +0100, Hans de Goede wrote:
>> Add a settings module-parameter which can be allowed to specify/override
>> various device-properties.
>>
>> Unlike most other touchscreen controllers Silead touchscreens don't tell us
>> the ranges of the reported x and y coordinates and Silead touchscreens also
>> often need to have their axis inverted and/or swapped to match the display
>> coordinates.
>>
>> Being able to specify the necessary properties through a module parameter,
>> allows users of new device-models to help us with bringing up support for
>> new devices without them needing to do a local kernel-build just to modify
>> these settings.
>>
>> Reviewed-by: Bastien Nocera <hadess@hadess.net>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> This LGTM, with one comment that I do not feel strongly about.
>
> Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
>> ---
>> drivers/input/touchscreen/silead.c | 26 ++++++++++++++++----------
>> 1 file changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
>> index 8a7351c4414c..22cee8d1bb22 100644
>> --- a/drivers/input/touchscreen/silead.c
>> +++ b/drivers/input/touchscreen/silead.c
>> @@ -58,6 +58,10 @@
>>
>> #define SILEAD_MAX_FINGERS 10
>>
>> +static char *settings;
>> +module_param(settings, charp, 0444);
>> +MODULE_PARM_DESC(settings, "Override touchscreen settings using a ; separated key=value list, e.g. \"touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y\"");
>> +
>> enum silead_ts_power {
>> SILEAD_POWER_ON = 1,
>> SILEAD_POWER_OFF = 0
>> @@ -133,14 +137,15 @@ static int silead_ts_request_input_dev(struct silead_ts_data *data)
>>
>> input_set_abs_params(data->input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
>> input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
>> - touchscreen_parse_properties(data->input, true, &data->prop);
>> + touchscreen_parse_properties_with_settings(data->input, true,
>> + &data->prop, settings);
>> silead_apply_efi_fw_min_max(data);
>>
>> input_mt_init_slots(data->input, data->max_fingers,
>> INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
>> INPUT_MT_TRACK);
>>
>> - if (device_property_read_bool(dev, "silead,home-button"))
>> + if (touchscreen_property_read_bool(dev, "silead,home-button", settings))
>
> While harmless, this seems like a bit of an overuse of the proposed
> functionality. Either the platform has a home button or it does not.
> Would users really need to experiment with this property enabled or
> disabled?
Thank you for the review.
Having this is actually useful, because users do need to check if
they actually have a home-button:
1. Always setting this may lead to false-positive home button presses
on some models (IIRC, this has been around for a long time)
2. The home button typical is a windows logo printed on the front of
cheap windows tablets below the screen. Recently I was adding info
for yet another such cheap tablet and I asked the user to test
the home-button since the windows logo was clearly there visually.
But on this specific model touching the windows logo does not do
anything.
Combined these 2 make it useful for users to be able to explicitly
test the home-button functionality.
Regards,
Hans
>
>> input_set_capability(data->input, EV_KEY, KEY_LEFTMETA);
>>
>> data->input->name = SILEAD_TS_NAME;
>> @@ -173,7 +178,8 @@ static int silead_ts_request_pen_input_dev(struct silead_ts_data *data)
>> input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
>> input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
>> set_bit(INPUT_PROP_DIRECT, data->pen_input->propbit);
>> - touchscreen_parse_properties(data->pen_input, false, &data->prop);
>> + touchscreen_parse_properties_with_settings(data->pen_input, false,
>> + &data->prop, settings);
>> input_abs_set_res(data->pen_input, ABS_X, data->pen_x_res);
>> input_abs_set_res(data->pen_input, ABS_Y, data->pen_y_res);
>>
>> @@ -523,8 +529,8 @@ static int silead_ts_setup(struct i2c_client *client)
>> * this.
>> */
>>
>> - if (device_property_read_bool(&client->dev,
>> - "silead,stuck-controller-bug")) {
>> + if (touchscreen_property_read_bool(&client->dev, "silead,stuck-controller-bug",
>> + settings)) {
>> pm_runtime_set_active(&client->dev);
>> pm_runtime_enable(&client->dev);
>> pm_runtime_allow(&client->dev);
>> @@ -591,8 +597,8 @@ static void silead_ts_read_props(struct i2c_client *client)
>> const char *str;
>> int error;
>>
>> - error = device_property_read_u32(dev, "silead,max-fingers",
>> - &data->max_fingers);
>> + error = touchscreen_property_read_u32(dev, "silead,max-fingers", settings,
>> + &data->max_fingers);
>> if (error) {
>> dev_dbg(dev, "Max fingers read error %d\n", error);
>> data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
>> @@ -605,9 +611,9 @@ static void silead_ts_read_props(struct i2c_client *client)
>> else
>> dev_dbg(dev, "Firmware file name read error. Using default.");
>>
>> - data->pen_supported = device_property_read_bool(dev, "silead,pen-supported");
>> - device_property_read_u32(dev, "silead,pen-resolution-x", &data->pen_x_res);
>> - device_property_read_u32(dev, "silead,pen-resolution-y", &data->pen_y_res);
>> + data->pen_supported = touchscreen_property_read_bool(dev, "silead,pen-supported", settings);
>> + touchscreen_property_read_u32(dev, "silead,pen-resolution-x", settings, &data->pen_x_res);
>> + touchscreen_property_read_u32(dev, "silead,pen-resolution-y", settings, &data->pen_y_res);
>> }
>>
>> #ifdef CONFIG_ACPI
>> --
>> 2.39.0
>>
>
> Kind regards,
> Jeff LaBundy
>
^ permalink raw reply
* [PATCH v8 3/5] input: pwm-beeper: set volume levels by devicetree
From: Manuel Traut @ 2023-01-26 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Frieder Schrempf, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Manuel Traut, linux-input, devicetree
In-Reply-To: <20230126091825.220646-1-manuel.traut@mt.com>
From: Frieder Schrempf <frieder.schrempf@kontron.de>
Add devicetree bindings to define supported volume levels and the
default volume level.
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Signed-off-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: Manuel Traut <manuel.traut@mt.com>
---
drivers/input/misc/pwm-beeper.c | 58 +++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 17 deletions(-)
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 865e1ec5c39d..82b05f7f4c70 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -181,8 +181,9 @@ static int pwm_beeper_probe(struct platform_device *pdev)
struct pwm_beeper *beeper;
struct pwm_state state;
u32 bell_frequency;
- int error;
+ int error, length;
size_t size;
+ u32 value;
beeper = devm_kzalloc(dev, sizeof(*beeper), GFP_KERNEL);
if (!beeper)
@@ -228,23 +229,46 @@ static int pwm_beeper_probe(struct platform_device *pdev)
beeper->bell_frequency = bell_frequency;
- beeper->max_volume = 4;
-
- size = sizeof(*beeper->volume_levels) *
- (beeper->max_volume + 1);
-
- beeper->volume_levels = devm_kzalloc(&(pdev->dev), size,
- GFP_KERNEL);
- if (!beeper->volume_levels)
- return -ENOMEM;
-
- beeper->volume_levels[0] = 0;
- beeper->volume_levels[1] = 8;
- beeper->volume_levels[2] = 20;
- beeper->volume_levels[3] = 40;
- beeper->volume_levels[4] = 500;
+ /* determine the number of volume levels */
+ length = device_property_read_u32_array(&pdev->dev, "volume-levels", NULL, 0);
+ if (length <= 0) {
+ dev_dbg(&pdev->dev, "no volume levels specified, using max volume\n");
+ beeper->max_volume = 1;
+ } else
+ beeper->max_volume = length;
+
+ /* read volume levels from DT property */
+ if (beeper->max_volume > 0) {
+ size = sizeof(*beeper->volume_levels) * beeper->max_volume;
+
+ beeper->volume_levels = devm_kzalloc(&(pdev->dev), size,
+ GFP_KERNEL);
+ if (!beeper->volume_levels)
+ return -ENOMEM;
+
+ if (length > 0) {
+ error = device_property_read_u32_array(&pdev->dev, "volume-levels",
+ beeper->volume_levels,
+ beeper->max_volume);
+
+ if (error < 0)
+ return error;
+
+ error = device_property_read_u32(&pdev->dev, "default-volume-level",
+ &value);
+
+ if (error < 0) {
+ dev_dbg(&pdev->dev, "no default volume specified, using max volume\n");
+ value = beeper->max_volume - 1;
+ }
+ } else {
+ beeper->volume_levels[0] = 500;
+ value = 0;
+ }
- beeper->volume = beeper->max_volume;
+ beeper->volume = value;
+ beeper->max_volume--;
+ }
beeper->input = devm_input_allocate_device(dev);
if (!beeper->input) {
--
2.39.0
^ permalink raw reply related
* [PATCH v8 4/5] dt-bindings: input: pwm-beeper: add volume
From: Manuel Traut @ 2023-01-26 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Manuel Traut, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Frieder Schrempf, linux-input, devicetree
In-Reply-To: <20230126091825.220646-1-manuel.traut@mt.com>
Adds an array of supported volume levels and a default volume level.
Signed-off-by: Manuel Traut <manuel.traut@mt.com>
---
.../devicetree/bindings/input/pwm-beeper.yaml | 20 +++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.yaml b/Documentation/devicetree/bindings/input/pwm-beeper.yaml
index 351df83d5cbe..f1f9283ca855 100644
--- a/Documentation/devicetree/bindings/input/pwm-beeper.yaml
+++ b/Documentation/devicetree/bindings/input/pwm-beeper.yaml
@@ -26,6 +26,24 @@ properties:
beeper-hz:
description: bell frequency in Hz
+ volume-levels:
+ description: >
+ Array of PWM duty cycle values that correspond to
+ linear volume levels. These need to be in the range of
+ 0 to 500, while 0 means 0% duty cycle (mute) and 500
+ means 50% duty cycle (max volume).
+ Please note that the actual volume of most beepers is
+ highly non-linear, which means that low volume levels
+ are probably somewhere in the range of 1 to 30 (0.1-3%
+ duty cycle).
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+
+ default-volume-level:
+ description: >
+ The default volume level (index into the array defined
+ by the "volume-levels" property).
+ $ref: /schemas/types.yaml#/definitions/uint32
+
required:
- compatible
- pwms
@@ -45,4 +63,6 @@ examples:
compatible = "pwm-beeper";
pwms = <&pwm0>;
amp-supply = <&beeper_amp>;
+ volume-levels = <0 8 20 40 500>;
+ default-volume-level = <4>;
};
--
2.39.0
^ permalink raw reply related
* [PATCH v8 5/5] input: pwm-beeper: handle module unloading properly
From: Manuel Traut @ 2023-01-26 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Manuel Traut, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Frieder Schrempf, linux-input, devicetree
In-Reply-To: <20230126091825.220646-1-manuel.traut@mt.com>
'input: pwm-beeper: add feature to set volume via sysfs' adds device
attributes without removing them on error or if the module is unloaded.
If the module will be unloaded and loaded again it fails:
[ 1007.918180] sysfs: cannot create duplicate filename '/devices/platform/buzzer/volume'
Therefore remove device attributes on module unloading and in case
registration at the input subsystem fails.
Signed-off-by: Manuel Traut <manuel.traut@mt.com>
---
drivers/input/misc/pwm-beeper.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 82b05f7f4c70..736b89bd1b42 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -299,6 +299,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
error = input_register_device(beeper->input);
if (error) {
+ sysfs_remove_group(&pdev->dev.kobj, &pwm_beeper_attribute_group);
dev_err(dev, "Failed to register input device: %d\n", error);
return error;
}
@@ -308,6 +309,17 @@ static int pwm_beeper_probe(struct platform_device *pdev)
return 0;
}
+static int pwm_beeper_remove(struct platform_device *pdev)
+{
+ struct pwm_beeper *beeper;
+
+ beeper = platform_get_drvdata(pdev);
+ input_unregister_device(beeper->input);
+ sysfs_remove_group(&pdev->dev.kobj, &pwm_beeper_attribute_group);
+
+ return 0;
+}
+
static int __maybe_unused pwm_beeper_suspend(struct device *dev)
{
struct pwm_beeper *beeper = dev_get_drvdata(dev);
@@ -353,6 +365,7 @@ MODULE_DEVICE_TABLE(of, pwm_beeper_match);
static struct platform_driver pwm_beeper_driver = {
.probe = pwm_beeper_probe,
+ .remove = pwm_beeper_remove,
.driver = {
.name = "pwm-beeper",
.pm = &pwm_beeper_pm_ops,
--
2.39.0
^ permalink raw reply related
* [PATCH v8 2/5] input: pwm-beeper: add feature to set volume via sysfs
From: Manuel Traut @ 2023-01-26 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Frieder Schrempf, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Manuel Traut, linux-input, devicetree
In-Reply-To: <20230126091825.220646-1-manuel.traut@mt.com>
From: Frieder Schrempf <frieder.schrempf@kontron.de>
Make the driver accept switching volume levels via sysfs.
This can be helpful if the beep/bell sound intensity needs
to be adapted to the environment of the device.
The volume adjustment is done by changing the duty cycle of
the pwm signal.
Add a sysfs interface with 5 default volume levels:
0 - mute
..
4 - max. volume
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Signed-off-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: Manuel Traut <manuel.traut@mt.com>
---
.../ABI/testing/sysfs-devices-pwm-beeper | 17 ++++
drivers/input/misc/pwm-beeper.c | 83 ++++++++++++++++++-
2 files changed, 99 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/testing/sysfs-devices-pwm-beeper
diff --git a/Documentation/ABI/testing/sysfs-devices-pwm-beeper b/Documentation/ABI/testing/sysfs-devices-pwm-beeper
new file mode 100644
index 000000000000..d2a22516f31d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-pwm-beeper
@@ -0,0 +1,17 @@
+What: /sys/devices/.../pwm-beeper/volume
+Date: January 2023
+KernelVersion:
+Contact: Frieder Schrempf <frieder.schrempf@kontron.de>
+Description:
+ Control the volume of this pwm-beeper. Values
+ are between 0 and max_volume. This file will also
+ show the current volume level stored in the driver.
+
+What: /sys/devices/.../pwm-beeper/max_volume
+Date: February 2023
+KernelVersion:
+Contact: Frieder Schrempf <frieder.schrempf@kontron.de>
+Description:
+ This file shows the maximum volume level that can be
+ assigned to volume.
+
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index d6b12477748a..865e1ec5c39d 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -1,9 +1,14 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
+ *
+ * Copyright (C) 2016, Frieder Schrempf <frieder.schrempf@kontron.de>
+ * (volume support)
+ *
* PWM beeper driver
*/
+#include <linux/device.h>
#include <linux/input.h>
#include <linux/regulator/consumer.h>
#include <linux/module.h>
@@ -24,10 +29,61 @@ struct pwm_beeper {
unsigned int bell_frequency;
bool suspended;
bool amplifier_on;
+ unsigned int volume;
+ unsigned int *volume_levels;
+ unsigned int max_volume;
};
#define HZ_TO_NANOSECONDS(x) (1000000000UL/(x))
+static ssize_t volume_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pwm_beeper *beeper = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", beeper->volume);
+}
+
+static ssize_t max_volume_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct pwm_beeper *beeper = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", beeper->max_volume);
+}
+
+static ssize_t volume_store(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ int rc;
+ struct pwm_beeper *beeper = dev_get_drvdata(dev);
+ unsigned int volume;
+
+ rc = kstrtouint(buf, 0, &volume);
+ if (rc)
+ return rc;
+
+ if (volume > beeper->max_volume)
+ return -EINVAL;
+ pr_debug("set volume to %u\n", volume);
+ beeper->volume = volume;
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(volume);
+static DEVICE_ATTR_RO(max_volume);
+
+static struct attribute *pwm_beeper_attributes[] = {
+ &dev_attr_volume.attr,
+ &dev_attr_max_volume.attr,
+ NULL,
+};
+
+static struct attribute_group pwm_beeper_attribute_group = {
+ .attrs = pwm_beeper_attributes,
+};
+
static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
{
struct pwm_state state;
@@ -37,7 +93,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
state.enabled = true;
state.period = period;
- pwm_set_relative_duty_cycle(&state, 50, 100);
+ pwm_set_relative_duty_cycle(&state, beeper->volume_levels[beeper->volume], 1000);
error = pwm_apply_state(beeper->pwm, &state);
if (error)
@@ -126,6 +182,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
struct pwm_state state;
u32 bell_frequency;
int error;
+ size_t size;
beeper = devm_kzalloc(dev, sizeof(*beeper), GFP_KERNEL);
if (!beeper)
@@ -171,6 +228,24 @@ static int pwm_beeper_probe(struct platform_device *pdev)
beeper->bell_frequency = bell_frequency;
+ beeper->max_volume = 4;
+
+ size = sizeof(*beeper->volume_levels) *
+ (beeper->max_volume + 1);
+
+ beeper->volume_levels = devm_kzalloc(&(pdev->dev), size,
+ GFP_KERNEL);
+ if (!beeper->volume_levels)
+ return -ENOMEM;
+
+ beeper->volume_levels[0] = 0;
+ beeper->volume_levels[1] = 8;
+ beeper->volume_levels[2] = 20;
+ beeper->volume_levels[3] = 40;
+ beeper->volume_levels[4] = 500;
+
+ beeper->volume = beeper->max_volume;
+
beeper->input = devm_input_allocate_device(dev);
if (!beeper->input) {
dev_err(dev, "Failed to allocate input device\n");
@@ -192,6 +267,12 @@ static int pwm_beeper_probe(struct platform_device *pdev)
input_set_drvdata(beeper->input, beeper);
+ error = sysfs_create_group(&pdev->dev.kobj, &pwm_beeper_attribute_group);
+ if (error) {
+ dev_err(&pdev->dev, "Failed to create sysfs group: %d\n", error);
+ return error;
+ }
+
error = input_register_device(beeper->input);
if (error) {
dev_err(dev, "Failed to register input device: %d\n", error);
--
2.39.0
^ permalink raw reply related
* [PATCH v8 0/5] input: pwm-beeper: add feature to set volume level
From: Manuel Traut @ 2023-01-26 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Manuel Traut, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Frieder Schrempf, linux-input, devicetree
This implements volume control for the pwm-beeper via sysfs.
The first patch changes the devicetree documentation from txt to yaml.
The original author of this is Frieder Schrempf.
I picked them from this [0] LKML thread from 2017. Since it looks like
his email address changed in the meantime I changed it in the Author
and Signed-off-by, as well as in the copyright statements.
I did some minor changes on the patches that they apply and work with
the current kernel.
The last patch was added to fix loading/unloading of the driver.
checkpatch still reports warnings regarding the changes:
* from txt to yaml of the devicetree documentation:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
WARNING: DT binding docs and includes should be a separate patch.
* and the introduction of Documentation/ABI/testing/sysfs-devices-pwm-beeper:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
I am not sure how to fix these warnings. So any suggestion would be helpful.
Changes since v7 [1]:
* yaml devicetree doc:
* Use shorter subject
* Fix indent
* Use units
* 'make dt_binding_check' succeeds
* 'make dtbs_check' does not report new errors
* Reworded commit messages avoiding 'this patch' phrase
* Fix wrong indent in [PATCH 5/5 v7] input: pwm-beeper: handle module unloading properly
* Use current date in Documentation/ABI/testing/sysfs-devices-pwm-beeper
* Hopefully fixed my setup that
* mails are CC'ed correctly
* patches are sent as replies to the cover letter
Changes since v6 [2]:
* Convert devicetree binding documentation from txt to yaml
* Use DEVICE_ATTR_[RO|RW] properly
* Change Frieders Mail address
* Added Signed-off and Tested-by statements
* Fix module unloading
Frieder Schrempf (2):
input: pwm-beeper: add feature to set volume via sysfs
input: pwm-beeper: set volume levels by devicetree
Manuel Traut (3):
dt-bindings: input: pwm-beeper: Convert txt bindings to yaml
dt-bindings: input: pwm-beeper: add volume
input: pwm-beeper: handle module unloading properly
.../ABI/testing/sysfs-devices-pwm-beeper | 17 +++
.../devicetree/bindings/input/pwm-beeper.txt | 24 ----
.../devicetree/bindings/input/pwm-beeper.yaml | 68 ++++++++++
drivers/input/misc/pwm-beeper.c | 122 +++++++++++++++++-
4 files changed, 205 insertions(+), 26 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-devices-pwm-beeper
delete mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt
create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.yaml
Regards
Manuel
[0] https://lore.kernel.org/all/cover.1487323753.git.frieder.schrempf@exceet.de/
[1] https://lore.kernel.org/all/Y9AIq3cSNzI9T%2FdU@mt.com/
[2] https://lkml.org/lkml/2023/1/24/379
--
2.39.0
^ permalink raw reply
* [PATCH v8 1/5] dt-bindings: input: pwm-beeper: Convert txt bindings to yaml
From: Manuel Traut @ 2023-01-26 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Manuel Traut, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Frieder Schrempf, linux-input, devicetree
In-Reply-To: <20230126091825.220646-1-manuel.traut@mt.com>
Converts txt binding to new YAML format.
Signed-off-by: Manuel Traut <manuel.traut@mt.com>
---
.../devicetree/bindings/input/pwm-beeper.txt | 24 ----------
.../devicetree/bindings/input/pwm-beeper.yaml | 48 +++++++++++++++++++
2 files changed, 48 insertions(+), 24 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt
create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.yaml
diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.txt b/Documentation/devicetree/bindings/input/pwm-beeper.txt
deleted file mode 100644
index 8fc0e48c20db..000000000000
--- a/Documentation/devicetree/bindings/input/pwm-beeper.txt
+++ /dev/null
@@ -1,24 +0,0 @@
-* PWM beeper device tree bindings
-
-Registers a PWM device as beeper.
-
-Required properties:
-- compatible: should be "pwm-beeper"
-- pwms: phandle to the physical PWM device
-
-Optional properties:
-- amp-supply: phandle to a regulator that acts as an amplifier for the beeper
-- beeper-hz: bell frequency in Hz
-
-Example:
-
-beeper_amp: amplifier {
- compatible = "fixed-regulator";
- gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
-};
-
-beeper {
- compatible = "pwm-beeper";
- pwms = <&pwm0>;
- amp-supply = <&beeper_amp>;
-};
diff --git a/Documentation/devicetree/bindings/input/pwm-beeper.yaml b/Documentation/devicetree/bindings/input/pwm-beeper.yaml
new file mode 100644
index 000000000000..351df83d5cbe
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/pwm-beeper.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/input/pwm-beeper.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: PWM beeper
+
+maintainers:
+ - Dmitry Torokhov <dmitry.torokhov@gmail.com>
+
+description: Registers a PWM device as beeper.
+
+properties:
+ compatible:
+ const: pwm-beeper
+
+ pwms:
+ maxItems: 1
+
+ amp-supply:
+ description: >
+ phandle to a regulator that acts as an amplifier for
+ the beeper
+
+ beeper-hz:
+ description: bell frequency in Hz
+
+required:
+ - compatible
+ - pwms
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ beeper_amp: amplifier {
+ compatible = "fixed-regulator";
+ gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ beeper {
+ compatible = "pwm-beeper";
+ pwms = <&pwm0>;
+ amp-supply = <&beeper_amp>;
+ };
--
2.39.0
^ permalink raw reply related
* Re: [PATCH 5/5] HID: sony_remove: manually unregister leds
From: Sven Eckelmann @ 2023-01-26 8:32 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Pietro Borrello
Cc: linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander,
Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v1-5-9a5192dcef16@diag.uniroma1.it>
[-- Attachment #1: Type: text/plain, Size: 535 bytes --]
On Thursday, 26 January 2023 01:24:57 CET Pietro Borrello wrote:
> Unregister the LED controller before device removal, as
> sony_led_set_brightness() may schedule sc->state_worker
> after the structure has been freed, causing a use-after-free.
>
> Fixes: 0a286ef27852 ("HID: sony: Add LED support for Sixaxis/Dualshock3 USB")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
> drivers/hid/hid-sony.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Reviewed-by: Sven Eckelmann <sven@narfation.org>
Thanks,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH resend 3/3] Input: goodix - Add a settings module-parameter
From: Jeff LaBundy @ 2023-01-26 4:53 UTC (permalink / raw)
To: Hans de Goede; +Cc: Dmitry Torokhov, Bastien Nocera, Gregor Riepl, linux-input
In-Reply-To: <20230125105416.17406-4-hdegoede@redhat.com>
On Wed, Jan 25, 2023 at 11:54:16AM +0100, Hans de Goede wrote:
> Add a settings module-parameter which can be allowed to specify/override
> various device-properties.
>
> Usuually Goodix touchscreens on x86 devices (where there is no devicetree
> to specify the properties) just work. But in some cases the touchscreen is
> mounted 90 or 180 degrees rotated vs the display, requiring setting the
> touchscreen-x/y-inverted or touchscreen-swapped-x-y properties.
>
> Being able to specify the necessary properties through a module parameter,
> allows users of new device-models to help us with bringing up support for
> new devices without them needing to do a local kernel-build just to modify
> these settings.
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/touchscreen/goodix.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index 620f5ec0962f..a105dee04156 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -53,6 +53,10 @@
> #define ACPI_GPIO_SUPPORT
> #endif
>
> +static char *settings;
> +module_param(settings, charp, 0444);
> +MODULE_PARM_DESC(settings, "Override touchscreen settings using a ; separated key=value list, e.g. \"touchscreen-fuzz-x=5;touchscreen-fuzz-y=5;touchscreen-swapped-x-y\"");
> +
> struct goodix_chip_id {
> const char *id;
> const struct goodix_chip_data *data;
> @@ -1197,7 +1201,8 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
> goodix_read_config(ts);
>
> /* Try overriding touchscreen parameters via device properties */
> - touchscreen_parse_properties(ts->input_dev, true, &ts->prop);
> + touchscreen_parse_properties_with_settings(ts->input_dev, true,
> + &ts->prop, settings);
>
> if (!ts->prop.max_x || !ts->prop.max_y || !ts->max_touch_num) {
> if (!ts->reset_controller_at_probe &&
> --
> 2.39.0
>
^ permalink raw reply
* Re: [PATCH resend 2/3] Input: silead - Add a settings module-parameter
From: Jeff LaBundy @ 2023-01-26 4:51 UTC (permalink / raw)
To: Hans de Goede; +Cc: Dmitry Torokhov, Bastien Nocera, Gregor Riepl, linux-input
In-Reply-To: <20230125105416.17406-3-hdegoede@redhat.com>
Hi Hans,
On Wed, Jan 25, 2023 at 11:54:15AM +0100, Hans de Goede wrote:
> Add a settings module-parameter which can be allowed to specify/override
> various device-properties.
>
> Unlike most other touchscreen controllers Silead touchscreens don't tell us
> the ranges of the reported x and y coordinates and Silead touchscreens also
> often need to have their axis inverted and/or swapped to match the display
> coordinates.
>
> Being able to specify the necessary properties through a module parameter,
> allows users of new device-models to help us with bringing up support for
> new devices without them needing to do a local kernel-build just to modify
> these settings.
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This LGTM, with one comment that I do not feel strongly about.
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/touchscreen/silead.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c
> index 8a7351c4414c..22cee8d1bb22 100644
> --- a/drivers/input/touchscreen/silead.c
> +++ b/drivers/input/touchscreen/silead.c
> @@ -58,6 +58,10 @@
>
> #define SILEAD_MAX_FINGERS 10
>
> +static char *settings;
> +module_param(settings, charp, 0444);
> +MODULE_PARM_DESC(settings, "Override touchscreen settings using a ; separated key=value list, e.g. \"touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y\"");
> +
> enum silead_ts_power {
> SILEAD_POWER_ON = 1,
> SILEAD_POWER_OFF = 0
> @@ -133,14 +137,15 @@ static int silead_ts_request_input_dev(struct silead_ts_data *data)
>
> input_set_abs_params(data->input, ABS_MT_POSITION_X, 0, 4095, 0, 0);
> input_set_abs_params(data->input, ABS_MT_POSITION_Y, 0, 4095, 0, 0);
> - touchscreen_parse_properties(data->input, true, &data->prop);
> + touchscreen_parse_properties_with_settings(data->input, true,
> + &data->prop, settings);
> silead_apply_efi_fw_min_max(data);
>
> input_mt_init_slots(data->input, data->max_fingers,
> INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED |
> INPUT_MT_TRACK);
>
> - if (device_property_read_bool(dev, "silead,home-button"))
> + if (touchscreen_property_read_bool(dev, "silead,home-button", settings))
While harmless, this seems like a bit of an overuse of the proposed
functionality. Either the platform has a home button or it does not.
Would users really need to experiment with this property enabled or
disabled?
> input_set_capability(data->input, EV_KEY, KEY_LEFTMETA);
>
> data->input->name = SILEAD_TS_NAME;
> @@ -173,7 +178,8 @@ static int silead_ts_request_pen_input_dev(struct silead_ts_data *data)
> input_set_capability(data->pen_input, EV_KEY, BTN_TOUCH);
> input_set_capability(data->pen_input, EV_KEY, BTN_TOOL_PEN);
> set_bit(INPUT_PROP_DIRECT, data->pen_input->propbit);
> - touchscreen_parse_properties(data->pen_input, false, &data->prop);
> + touchscreen_parse_properties_with_settings(data->pen_input, false,
> + &data->prop, settings);
> input_abs_set_res(data->pen_input, ABS_X, data->pen_x_res);
> input_abs_set_res(data->pen_input, ABS_Y, data->pen_y_res);
>
> @@ -523,8 +529,8 @@ static int silead_ts_setup(struct i2c_client *client)
> * this.
> */
>
> - if (device_property_read_bool(&client->dev,
> - "silead,stuck-controller-bug")) {
> + if (touchscreen_property_read_bool(&client->dev, "silead,stuck-controller-bug",
> + settings)) {
> pm_runtime_set_active(&client->dev);
> pm_runtime_enable(&client->dev);
> pm_runtime_allow(&client->dev);
> @@ -591,8 +597,8 @@ static void silead_ts_read_props(struct i2c_client *client)
> const char *str;
> int error;
>
> - error = device_property_read_u32(dev, "silead,max-fingers",
> - &data->max_fingers);
> + error = touchscreen_property_read_u32(dev, "silead,max-fingers", settings,
> + &data->max_fingers);
> if (error) {
> dev_dbg(dev, "Max fingers read error %d\n", error);
> data->max_fingers = 5; /* Most devices handle up-to 5 fingers */
> @@ -605,9 +611,9 @@ static void silead_ts_read_props(struct i2c_client *client)
> else
> dev_dbg(dev, "Firmware file name read error. Using default.");
>
> - data->pen_supported = device_property_read_bool(dev, "silead,pen-supported");
> - device_property_read_u32(dev, "silead,pen-resolution-x", &data->pen_x_res);
> - device_property_read_u32(dev, "silead,pen-resolution-y", &data->pen_y_res);
> + data->pen_supported = touchscreen_property_read_bool(dev, "silead,pen-supported", settings);
> + touchscreen_property_read_u32(dev, "silead,pen-resolution-x", settings, &data->pen_x_res);
> + touchscreen_property_read_u32(dev, "silead,pen-resolution-y", settings, &data->pen_y_res);
> }
>
> #ifdef CONFIG_ACPI
> --
> 2.39.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH resend 1/3] Input: touchscreen - Extend touchscreen_parse_properties() to allow overriding settings with a module option
From: Jeff LaBundy @ 2023-01-26 4:41 UTC (permalink / raw)
To: Hans de Goede; +Cc: Dmitry Torokhov, Bastien Nocera, Gregor Riepl, linux-input
In-Reply-To: <20230125105416.17406-2-hdegoede@redhat.com>
Hi Hans,
On Wed, Jan 25, 2023 at 11:54:14AM +0100, Hans de Goede wrote:
> On x86/ACPI platforms touchscreens mostly just work without needing any
> device/model specific configuration. But in some cases (mostly with Silead
> and Goodix touchscreens) it is still necessary to manually specify various
> touchscreen-properties on a per model basis.
>
> This is handled by drivers/platform/x86/touchscreen_dmi.c which contains
> a large list of per-model touchscreen properties which it attaches to the
> (i2c)device before the touchscreen driver's probe() method gets called.
> This means that ATM changing these settings requires recompiling the
> kernel. This makes figuring out what settings/properties a specific
> touchscreen needs very hard for normal users to do.
>
> Add a new, optional, settings_override string argument to
> touchscreen_parse_properties(), which takes a list of ; separated
> property-name=value pairs, e.g. :
> "touchscreen-size-x=1665;touchscreen-size-y=1140;touchscreen-swapped-x-y".
>
> This new argument can be used by drivers to implement a module option which
> allows users to easily specify alternative settings for testing.
>
> The 2 new touchscreen_property_read_u32() and
> touchscreen_property_read_bool() helpers are also exported so that
> drivers can use these to add settings-override support to the code
> for driver-specific properties.
>
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
Thank you for resurrecting this series. Perhaps I can give my own $.02 as
a fellow customer of input.
I can appreciate the hesitancy that was expressed in the past, as this is
not a generic solution and is specific to touch devices. However, I also
agree with your point that extending dts overrides to all properties opens
a can of worms which should not necessarily gate this benign series (i.e.,
"good is not the enemy of great").
Personally I am highly in favor of this series because I myself have had
to support this very situation where a panel arrives 180 degrees from the
expected orientation, and fellow teammates who are not in a position to
quickly spin a build need a means to quickly unblock themselves through a
serial console or other means.
The code itself also LGTM and I verified there were no regressions on one
of my own drivers that expects these properties to come from dts, and so:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> Changes in v2:
> - Instead of patching all drivers rename touchscreen_parse_properties()
> to touchscreen_parse_properties_with_override() and add
> a static inline wrapper which passes NULL.
> ---
> drivers/input/touchscreen.c | 103 ++++++++++++++++++++++++++----
> include/linux/input/touchscreen.h | 19 +++++-
> 2 files changed, 109 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/touchscreen.c b/drivers/input/touchscreen.c
> index 4620e20d0190..3b9505d5468d 100644
> --- a/drivers/input/touchscreen.c
> +++ b/drivers/input/touchscreen.c
> @@ -12,15 +12,80 @@
> #include <linux/input/touchscreen.h>
> #include <linux/module.h>
>
> +static int touchscreen_get_prop_from_settings_string(const char *settings,
> + const char *propname,
> + bool is_boolean,
> + u32 *val_ret)
> +{
> + char *begin, *end;
> + u32 val;
> +
> + if (!settings)
> + return -ENOENT;
> +
> + begin = strstr(settings, propname);
> + if (!begin)
> + return -ENOENT;
> +
> + /* begin must be either the begin of settings, or be preceded by a ';' */
> + if (begin != settings && begin[-1] != ';')
> + return -EINVAL;
> +
> + end = begin + strlen(propname);
> + if (*end != '=') {
> + if (is_boolean && (*end == '\0' || *end == ';')) {
> + *val_ret = true;
> + return 0;
> + }
> + return -EINVAL;
> + }
> +
> + val = simple_strtoul(end + 1, &end, 0);
> + if (*end != '\0' && *end != ';')
> + return -EINVAL;
> +
> + *val_ret = val;
> + return 0;
> +}
> +
> +int touchscreen_property_read_u32(struct device *dev, const char *propname,
> + const char *settings, u32 *val)
> +{
> + int error;
> +
> + error = device_property_read_u32(dev, propname, val);
> +
> + if (touchscreen_get_prop_from_settings_string(settings, propname,
> + false, val) == 0)
> + error = 0;
> +
> + return error;
> +}
> +EXPORT_SYMBOL(touchscreen_property_read_u32);
> +
> +bool touchscreen_property_read_bool(struct device *dev, const char *propname,
> + const char *settings)
> +{
> + u32 val;
> +
> + val = device_property_read_bool(dev, propname);
> +
> + touchscreen_get_prop_from_settings_string(settings, propname, true, &val);
> +
> + return val;
> +}
> +EXPORT_SYMBOL(touchscreen_property_read_bool);
> +
> static bool touchscreen_get_prop_u32(struct device *dev,
> const char *property,
> + const char *settings,
> unsigned int default_value,
> unsigned int *value)
> {
> u32 val;
> int error;
>
> - error = device_property_read_u32(dev, property, &val);
> + error = touchscreen_property_read_u32(dev, property, settings, &val);
> if (error) {
> *value = default_value;
> return false;
> @@ -50,20 +115,28 @@ static void touchscreen_set_params(struct input_dev *dev,
> }
>
> /**
> - * touchscreen_parse_properties - parse common touchscreen properties
> + * touchscreen_parse_properties_with_settings - parse common touchscreen properties
> * @input: input device that should be parsed
> * @multitouch: specifies whether parsed properties should be applied to
> * single-touch or multi-touch axes
> * @prop: pointer to a struct touchscreen_properties into which to store
> * axis swap and invert info for use with touchscreen_report_x_y();
> * or %NULL
> + * @settings: string with ; separated name=value pairs overriding
> + * the device-properties or %NULL.
> *
> * This function parses common properties for touchscreens and sets up the
> * input device accordingly. The function keeps previously set up default
> * values if no value is specified.
> + *
> + * Callers can optional specify a settings string overriding the
> + * device-properties, this can be used to implement a module option which
> + * allows users to easily specify alternative settings for testing.
> */
> -void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
> - struct touchscreen_properties *prop)
> +void touchscreen_parse_properties_with_settings(struct input_dev *input,
> + bool multitouch,
> + struct touchscreen_properties *prop,
> + const char *settings)
> {
> struct device *dev = input->dev.parent;
> struct input_absinfo *absinfo;
> @@ -79,26 +152,32 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
> axis_y = multitouch ? ABS_MT_POSITION_Y : ABS_Y;
>
> data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
> + settings,
> input_abs_get_min(input, axis_x),
> &minimum);
> data_present |= touchscreen_get_prop_u32(dev, "touchscreen-size-x",
> + settings,
> input_abs_get_max(input,
> axis_x) + 1,
> &maximum);
> data_present |= touchscreen_get_prop_u32(dev, "touchscreen-fuzz-x",
> + settings,
> input_abs_get_fuzz(input, axis_x),
> &fuzz);
> if (data_present)
> touchscreen_set_params(input, axis_x, minimum, maximum - 1, fuzz);
>
> data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
> + settings,
> input_abs_get_min(input, axis_y),
> &minimum);
> data_present |= touchscreen_get_prop_u32(dev, "touchscreen-size-y",
> + settings,
> input_abs_get_max(input,
> axis_y) + 1,
> &maximum);
> data_present |= touchscreen_get_prop_u32(dev, "touchscreen-fuzz-y",
> + settings,
> input_abs_get_fuzz(input, axis_y),
> &fuzz);
> if (data_present)
> @@ -107,10 +186,12 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
> axis = multitouch ? ABS_MT_PRESSURE : ABS_PRESSURE;
> data_present = touchscreen_get_prop_u32(dev,
> "touchscreen-max-pressure",
> + settings,
> input_abs_get_max(input, axis),
> &maximum);
> data_present |= touchscreen_get_prop_u32(dev,
> "touchscreen-fuzz-pressure",
> + settings,
> input_abs_get_fuzz(input, axis),
> &fuzz);
> if (data_present)
> @@ -122,28 +203,28 @@ void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
> prop->max_x = input_abs_get_max(input, axis_x);
> prop->max_y = input_abs_get_max(input, axis_y);
>
> - prop->invert_x =
> - device_property_read_bool(dev, "touchscreen-inverted-x");
> + prop->invert_x = touchscreen_property_read_bool(dev, "touchscreen-inverted-x",
> + settings);
> if (prop->invert_x) {
> absinfo = &input->absinfo[axis_x];
> absinfo->maximum -= absinfo->minimum;
> absinfo->minimum = 0;
> }
>
> - prop->invert_y =
> - device_property_read_bool(dev, "touchscreen-inverted-y");
> + prop->invert_y = touchscreen_property_read_bool(dev, "touchscreen-inverted-y",
> + settings);
> if (prop->invert_y) {
> absinfo = &input->absinfo[axis_y];
> absinfo->maximum -= absinfo->minimum;
> absinfo->minimum = 0;
> }
>
> - prop->swap_x_y =
> - device_property_read_bool(dev, "touchscreen-swapped-x-y");
> + prop->swap_x_y = touchscreen_property_read_bool(dev, "touchscreen-swapped-x-y",
> + settings);
> if (prop->swap_x_y)
> swap(input->absinfo[axis_x], input->absinfo[axis_y]);
> }
> -EXPORT_SYMBOL(touchscreen_parse_properties);
> +EXPORT_SYMBOL(touchscreen_parse_properties_with_settings);
>
> static void
> touchscreen_apply_prop_to_x_y(const struct touchscreen_properties *prop,
> diff --git a/include/linux/input/touchscreen.h b/include/linux/input/touchscreen.h
> index fe66e2b58f62..0023c6e368ba 100644
> --- a/include/linux/input/touchscreen.h
> +++ b/include/linux/input/touchscreen.h
> @@ -17,8 +17,23 @@ struct touchscreen_properties {
> bool swap_x_y;
> };
>
> -void touchscreen_parse_properties(struct input_dev *input, bool multitouch,
> - struct touchscreen_properties *prop);
> +void touchscreen_parse_properties_with_settings(struct input_dev *input,
> + bool multitouch,
> + struct touchscreen_properties *prop,
> + const char *settings);
> +
> +static inline void touchscreen_parse_properties(struct input_dev *input,
> + bool multitouch,
> + struct touchscreen_properties *prop)
> +{
> + touchscreen_parse_properties_with_settings(input, multitouch, prop, NULL);
> +}
> +
> +int touchscreen_property_read_u32(struct device *dev, const char *propname,
> + const char *settings, u32 *val);
> +
> +bool touchscreen_property_read_bool(struct device *dev, const char *propname,
> + const char *settings);
>
> void touchscreen_set_mt_pos(struct input_mt_pos *pos,
> const struct touchscreen_properties *prop,
> --
> 2.39.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: azoteq: Fix differing types
From: Rob Herring @ 2023-01-26 3:10 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Krzysztof Kozlowski, linux-input, devicetree,
linux-kernel
In-Reply-To: <Y9Hce2OqJOB/uiyM@nixie71>
On Wed, Jan 25, 2023 at 7:51 PM Jeff LaBundy <jeff@labundy.com> wrote:
>
> Hi Rob,
>
> On Wed, Jan 25, 2023 at 04:14:16PM -0600, Rob Herring wrote:
> > 'azoteq,ati-base' and 'azoteq,thresh' properties are defined in multiple
> > bindings, but have differing types defined. Both 'uint32' and
> > 'uint32-array' are used. Unify these to use 'uint32-array' everywhere.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Thank you for the patch. While this is a step forward in moving toward
> a common binding for this vendor like we have discussed in the past, I
> do not agree with this approach and will instead propose an alternative
> that accomplishes the same goal.
>
> For all of these devices, a single sensing channel takes a base and a
> threshold property. IQS626A is unique in that a fixed number of channels
> form a trackpad, and I decided at the time to simply define the base and
> target properties for all channels as a uint32-array.
>
> For all other existing drivers, as well as others coming down the pipe,
> base and threshold are uint32s. I find it confusing to redefine all of
> those as single-element arrays, especially on account of one device.
>
> In hindsight, a better design would have been to define a child node
> for each channel under the trackpad node, with each child node accepting
> a uint32 base and threshold. That would follow other devices, be more
> representative of the actual hardware, and keep the definitions the same
> across each binding.
>
> So, that's what I propose to do here instead. I happen to have a fix in
> review [1] that addresses a bug related to this parsing code, and would
> be happy to build this solution on top assuming it can wait until the
> next cycle. Does this compromise sound OK?
Shrug
How exactly are you going to change an existing property and not break
existing users? Or are there not any users?
We have the same properties with 2 definitions. At the end of the day,
I just want to fix that...
Rob
^ permalink raw reply
* [PATCH v2 1/2] HID: hid-steam: Add Steam Deck support
From: Vicki Pfau @ 2023-01-26 3:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Lyude Paul, linux-input; +Cc: Vicki Pfau
In-Reply-To: <20230111012336.2915827-2-vi@endrift.com>
Add preliminary support for the Steam Deck's controller interface. Currently,
this only supports the controller inputs and toggling lizard mode (Valve's name
for a hardware kb/m emulation mode). It does not support any of the advanced
features, such as the motion sensors or force-feedback.
The Steam Deck also includes a heartbeat for lizard mode that switches it back
on if no reports have been received within a few milliseconds. The official
Steam client handles this by sending a handful of configuration reports every
few ms, so we copy this behavior by sending configuration reports to disable
the mouse and reset the digital mappings every 5ms. As this isn't needed for
the older Steam Controller, this is only done on the Steam Deck.
Signed-off-by: Vicki Pfau <vi@endrift.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
drivers/hid/Kconfig | 4 +-
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-steam.c | 329 ++++++++++++++++++++++++++++++++++++----
3 files changed, 306 insertions(+), 28 deletions(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index e2a5d30c8895..20402d0a7eab 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1018,10 +1018,10 @@ config HID_SPEEDLINK
Support for Speedlink Vicious and Divine Cezanne mouse.
config HID_STEAM
- tristate "Steam Controller support"
+ tristate "Steam Controller/Deck support"
select POWER_SUPPLY
help
- Say Y here if you have a Steam Controller if you want to use it
+ Say Y here if you have a Steam Controller or Deck if you want to use it
without running the Steam Client. It supports both the wired and
the wireless adaptor.
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 82713ef3aaa6..33269113acc7 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1183,6 +1183,7 @@
#define USB_VENDOR_ID_VALVE 0x28de
#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
+#define USB_DEVICE_ID_STEAM_DECK 0x1205
#define USB_VENDOR_ID_STEELSERIES 0x1038
#define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 8ee43cb225fc..09330d9bfee3 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -3,6 +3,7 @@
* HID driver for Valve Steam Controller
*
* Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
+ * Copyright (c) 2022 Valve Software
*
* Supports both the wired and wireless interfaces.
*
@@ -53,6 +54,7 @@ static DEFINE_MUTEX(steam_devices_lock);
static LIST_HEAD(steam_devices);
#define STEAM_QUIRK_WIRELESS BIT(0)
+#define STEAM_QUIRK_DECK BIT(1)
/* Touch pads are 40 mm in diameter and 65535 units */
#define STEAM_PAD_RESOLUTION 1638
@@ -60,6 +62,10 @@ static LIST_HEAD(steam_devices);
#define STEAM_TRIGGER_RESOLUTION 51
/* Joystick runs are about 5 mm and 256 units */
#define STEAM_JOYSTICK_RESOLUTION 51
+/* Trigger runs are about 6 mm and 32768 units */
+#define STEAM_DECK_TRIGGER_RESOLUTION 5461
+/* Joystick runs are about 5 mm and 32768 units */
+#define STEAM_DECK_JOYSTICK_RESOLUTION 6553
#define STEAM_PAD_FUZZ 256
@@ -92,11 +98,14 @@ static LIST_HEAD(steam_devices);
#define STEAM_REG_RPAD_MARGIN 0x18
#define STEAM_REG_LED 0x2d
#define STEAM_REG_GYRO_MODE 0x30
+#define STEAM_REG_LPAD_CLICK_PRESSURE 0x34
+#define STEAM_REG_RPAD_CLICK_PRESSURE 0x35
/* Raw event identifiers */
#define STEAM_EV_INPUT_DATA 0x01
#define STEAM_EV_CONNECT 0x03
#define STEAM_EV_BATTERY 0x04
+#define STEAM_EV_DECK_INPUT_DATA 0x09
/* Values for GYRO_MODE (bitmask) */
#define STEAM_GYRO_MODE_OFF 0x0000
@@ -124,6 +133,7 @@ struct steam_device {
struct power_supply __rcu *battery;
u8 battery_charge;
u16 voltage;
+ struct delayed_work heartbeat;
};
static int steam_recv_report(struct steam_device *steam,
@@ -193,7 +203,7 @@ static int steam_send_report(struct steam_device *steam,
*/
do {
ret = hid_hw_raw_request(steam->hdev, 0,
- buf, size + 1,
+ buf, max(size, 64) + 1,
HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
if (ret != -EPIPE)
break;
@@ -219,6 +229,7 @@ static int steam_write_registers(struct steam_device *steam,
u8 reg;
u16 val;
u8 cmd[64] = {STEAM_CMD_WRITE_REGISTER, 0x00};
+ int ret;
va_list args;
va_start(args, steam);
@@ -234,7 +245,16 @@ static int steam_write_registers(struct steam_device *steam,
}
va_end(args);
- return steam_send_report(steam, cmd, 2 + cmd[1]);
+ ret = steam_send_report(steam, cmd, 2 + cmd[1]);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Sometimes a lingering report for this command can
+ * get read back instead of the last set report if
+ * this isn't explicitly queried
+ */
+ return steam_recv_report(steam, cmd, 2 + cmd[1]);
}
static int steam_get_serial(struct steam_device *steam)
@@ -280,13 +300,33 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
steam_write_registers(steam,
STEAM_REG_RPAD_MARGIN, 0x01, /* enable margin */
0);
+
+ cancel_delayed_work_sync(&steam->heartbeat);
} else {
/* disable esc, enter, cursor */
steam_send_report_byte(steam, STEAM_CMD_CLEAR_MAPPINGS);
- steam_write_registers(steam,
- STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */
- STEAM_REG_RPAD_MARGIN, 0x00, /* disable margin */
- 0);
+
+ if (steam->quirks & STEAM_QUIRK_DECK) {
+ steam_write_registers(steam,
+ STEAM_REG_RPAD_MARGIN, 0x00, /* disable margin */
+ STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */
+ STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */
+ STEAM_REG_LPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */
+ STEAM_REG_RPAD_CLICK_PRESSURE, 0xFFFF, /* disable clicky pad */
+ 0);
+ /*
+ * The Steam Deck has a watchdog that automatically enables
+ * lizard mode if it doesn't see any traffic for too long
+ */
+ if (!work_busy(&steam->heartbeat.work))
+ schedule_delayed_work(&steam->heartbeat, 5 * HZ);
+ } else {
+ steam_write_registers(steam,
+ STEAM_REG_RPAD_MARGIN, 0x00, /* disable margin */
+ STEAM_REG_LPAD_MODE, 0x07, /* disable mouse */
+ STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */
+ 0);
+ }
}
}
@@ -413,8 +453,8 @@ static int steam_input_register(struct steam_device *steam)
input->open = steam_input_open;
input->close = steam_input_close;
- input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
- "Wireless Steam Controller" :
+ input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ? "Wireless Steam Controller" :
+ (steam->quirks & STEAM_QUIRK_DECK) ? "Steam Deck" :
"Steam Controller";
input->phys = hdev->phys;
input->uniq = steam->serial_no;
@@ -438,33 +478,67 @@ static int steam_input_register(struct steam_device *steam)
input_set_capability(input, EV_KEY, BTN_SELECT);
input_set_capability(input, EV_KEY, BTN_MODE);
input_set_capability(input, EV_KEY, BTN_START);
- input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
- input_set_capability(input, EV_KEY, BTN_GEAR_UP);
input_set_capability(input, EV_KEY, BTN_THUMBR);
input_set_capability(input, EV_KEY, BTN_THUMBL);
input_set_capability(input, EV_KEY, BTN_THUMB);
input_set_capability(input, EV_KEY, BTN_THUMB2);
+ if (steam->quirks & STEAM_QUIRK_DECK) {
+ input_set_capability(input, EV_KEY, BTN_BASE);
+ input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY1);
+ input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY2);
+ input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY3);
+ input_set_capability(input, EV_KEY, BTN_TRIGGER_HAPPY4);
+ } else {
+ input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
+ input_set_capability(input, EV_KEY, BTN_GEAR_UP);
+ }
- input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
- input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
- input_set_abs_params(input, ABS_RX, -32767, 32767,
- STEAM_PAD_FUZZ, 0);
- input_set_abs_params(input, ABS_RY, -32767, 32767,
- STEAM_PAD_FUZZ, 0);
+
input_set_abs_params(input, ABS_HAT0X, -32767, 32767,
STEAM_PAD_FUZZ, 0);
input_set_abs_params(input, ABS_HAT0Y, -32767, 32767,
STEAM_PAD_FUZZ, 0);
- input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
- input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
- input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
- input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
+
+ if (steam->quirks & STEAM_QUIRK_DECK) {
+ input_set_abs_params(input, ABS_HAT2Y, 0, 32767, 0, 0);
+ input_set_abs_params(input, ABS_HAT2X, 0, 32767, 0, 0);
+
+ input_set_abs_params(input, ABS_RX, -32767, 32767, 0, 0);
+ input_set_abs_params(input, ABS_RY, -32767, 32767, 0, 0);
+
+ input_set_abs_params(input, ABS_HAT1X, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_HAT1Y, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+
+ input_abs_set_res(input, ABS_X, STEAM_DECK_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_Y, STEAM_DECK_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_RX, STEAM_DECK_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_RY, STEAM_DECK_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT1X, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT1Y, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT2Y, STEAM_DECK_TRIGGER_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT2X, STEAM_DECK_TRIGGER_RESOLUTION);
+ } else {
+ input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
+
+ input_set_abs_params(input, ABS_RX, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_RY, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+
+ input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
+ }
input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
- input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
- input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
ret = input_register_device(input);
if (ret)
@@ -612,6 +686,22 @@ static bool steam_is_valve_interface(struct hid_device *hdev)
return !list_empty(&rep_enum->report_list);
}
+static void steam_lizard_mode_heartbeat(struct work_struct *work)
+{
+ struct steam_device *steam = container_of(work, struct steam_device,
+ heartbeat.work);
+
+ mutex_lock(&steam->mutex);
+ if (!steam->client_opened && steam->client_hdev) {
+ steam_send_report_byte(steam, STEAM_CMD_CLEAR_MAPPINGS);
+ steam_write_registers(steam,
+ STEAM_REG_RPAD_MODE, 0x07, /* disable mouse */
+ 0);
+ schedule_delayed_work(&steam->heartbeat, 5 * HZ);
+ }
+ mutex_unlock(&steam->mutex);
+}
+
static int steam_client_ll_parse(struct hid_device *hdev)
{
struct steam_device *steam = hdev->driver_data;
@@ -750,6 +840,7 @@ static int steam_probe(struct hid_device *hdev,
steam->quirks = id->driver_data;
INIT_WORK(&steam->work_connect, steam_work_connect_cb);
INIT_LIST_HEAD(&steam->list);
+ INIT_DEFERRABLE_WORK(&steam->heartbeat, steam_lizard_mode_heartbeat);
steam->client_hdev = steam_create_client_hid(hdev);
if (IS_ERR(steam->client_hdev)) {
@@ -805,6 +896,7 @@ static int steam_probe(struct hid_device *hdev,
hid_destroy_device(steam->client_hdev);
client_hdev_fail:
cancel_work_sync(&steam->work_connect);
+ cancel_delayed_work_sync(&steam->heartbeat);
steam_alloc_fail:
hid_err(hdev, "%s: failed with error %d\n",
__func__, ret);
@@ -821,7 +913,11 @@ static void steam_remove(struct hid_device *hdev)
}
hid_destroy_device(steam->client_hdev);
+ mutex_lock(&steam->mutex);
+ steam->client_hdev = NULL;
steam->client_opened = false;
+ cancel_delayed_work_sync(&steam->heartbeat);
+ mutex_unlock(&steam->mutex);
cancel_work_sync(&steam->work_connect);
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
hid_info(hdev, "Steam wireless receiver disconnected");
@@ -906,10 +1002,10 @@ static inline s16 steam_le16(u8 *data)
* 8.5 | BTN_B | button B
* 8.6 | BTN_X | button X
* 8.7 | BTN_A | button A
- * 9.0 | BTN_DPAD_UP | lef-pad up
- * 9.1 | BTN_DPAD_RIGHT | lef-pad right
- * 9.2 | BTN_DPAD_LEFT | lef-pad left
- * 9.3 | BTN_DPAD_DOWN | lef-pad down
+ * 9.0 | BTN_DPAD_UP | left-pad up
+ * 9.1 | BTN_DPAD_RIGHT | left-pad right
+ * 9.2 | BTN_DPAD_LEFT | left-pad left
+ * 9.3 | BTN_DPAD_DOWN | left-pad down
* 9.4 | BTN_SELECT | menu left
* 9.5 | BTN_MODE | steam logo
* 9.6 | BTN_START | menu right
@@ -993,6 +1089,172 @@ static void steam_do_input_event(struct steam_device *steam,
input_sync(input);
}
+/*
+ * The size for this message payload is 56.
+ * The known values are:
+ * Offset| Type | Mapped to |Meaning
+ * -------+-------+-----------+--------------------------
+ * 4-7 | u32 | -- | sequence number
+ * 8-15 | u64 | see below | buttons
+ * 16-17 | s16 | ABS_HAT0X | left-pad X value
+ * 18-19 | s16 | ABS_HAT0Y | left-pad Y value
+ * 20-21 | s16 | ABS_HAT1X | right-pad X value
+ * 22-23 | s16 | ABS_HAT1Y | right-pad Y value
+ * 24-25 | s16 | -- | accelerometer X value
+ * 26-27 | s16 | -- | accelerometer Y value
+ * 28-29 | s16 | -- | accelerometer Z value
+ * 30-31 | s16 | -- | gyro X value
+ * 32-33 | s16 | -- | gyro Y value
+ * 34-35 | s16 | -- | gyro Z value
+ * 36-37 | s16 | -- | quaternion W value
+ * 38-39 | s16 | -- | quaternion X value
+ * 40-41 | s16 | -- | quaternion Y value
+ * 42-43 | s16 | -- | quaternion Z value
+ * 44-45 | u16 | ABS_HAT2Y | left trigger (uncalibrated)
+ * 46-47 | u16 | ABS_HAT2X | right trigger (uncalibrated)
+ * 48-49 | s16 | ABS_X | left joystick X
+ * 50-51 | s16 | ABS_Y | left joystick Y
+ * 52-53 | s16 | ABS_RX | right joystick X
+ * 54-55 | s16 | ABS_RY | right joystick Y
+ * 56-57 | u16 | -- | left pad pressure
+ * 58-59 | u16 | -- | right pad pressure
+ *
+ * The buttons are:
+ * Bit | Mapped to | Description
+ * ------+------------+--------------------------------
+ * 8.0 | BTN_TR2 | right trigger fully pressed
+ * 8.1 | BTN_TL2 | left trigger fully pressed
+ * 8.2 | BTN_TR | right shoulder
+ * 8.3 | BTN_TL | left shoulder
+ * 8.4 | BTN_Y | button Y
+ * 8.5 | BTN_B | button B
+ * 8.6 | BTN_X | button X
+ * 8.7 | BTN_A | button A
+ * 9.0 | BTN_DPAD_UP | left-pad up
+ * 9.1 | BTN_DPAD_RIGHT | left-pad right
+ * 9.2 | BTN_DPAD_LEFT | left-pad left
+ * 9.3 | BTN_DPAD_DOWN | left-pad down
+ * 9.4 | BTN_SELECT | menu left
+ * 9.5 | BTN_MODE | steam logo
+ * 9.6 | BTN_START | menu right
+ * 9.7 | BTN_TRIGGER_HAPPY3 | left bottom grip button
+ * 10.0 | BTN_TRIGGER_HAPPY4 | right bottom grip button
+ * 10.1 | BTN_THUMB | left pad pressed
+ * 10.2 | BTN_THUMB2 | right pad pressed
+ * 10.3 | -- | left pad touched
+ * 10.4 | -- | right pad touched
+ * 10.5 | -- | unknown
+ * 10.6 | BTN_THUMBL | left joystick clicked
+ * 10.7 | -- | unknown
+ * 11.0 | -- | unknown
+ * 11.1 | -- | unknown
+ * 11.2 | BTN_THUMBR | right joystick clicked
+ * 11.3 | -- | unknown
+ * 11.4 | -- | unknown
+ * 11.5 | -- | unknown
+ * 11.6 | -- | unknown
+ * 11.7 | -- | unknown
+ * 12.0 | -- | unknown
+ * 12.1 | -- | unknown
+ * 12.2 | -- | unknown
+ * 12.3 | -- | unknown
+ * 12.4 | -- | unknown
+ * 12.5 | -- | unknown
+ * 12.6 | -- | unknown
+ * 12.7 | -- | unknown
+ * 13.0 | -- | unknown
+ * 13.1 | BTN_TRIGGER_HAPPY1 | left top grip button
+ * 13.2 | BTN_TRIGGER_HAPPY2 | right top grip button
+ * 13.3 | -- | unknown
+ * 13.4 | -- | unknown
+ * 13.5 | -- | unknown
+ * 13.6 | -- | left joystick touched
+ * 13.7 | -- | right joystick touched
+ * 14.0 | -- | unknown
+ * 14.1 | -- | unknown
+ * 14.2 | BTN_BASE | quick access button
+ * 14.3 | -- | unknown
+ * 14.4 | -- | unknown
+ * 14.5 | -- | unknown
+ * 14.6 | -- | unknown
+ * 14.7 | -- | unknown
+ * 15.0 | -- | unknown
+ * 15.1 | -- | unknown
+ * 15.2 | -- | unknown
+ * 15.3 | -- | unknown
+ * 15.4 | -- | unknown
+ * 15.5 | -- | unknown
+ * 15.6 | -- | unknown
+ * 15.7 | -- | unknown
+ */
+static void steam_do_deck_input_event(struct steam_device *steam,
+ struct input_dev *input, u8 *data)
+{
+ u8 b8, b9, b10, b11, b13, b14;
+ bool lpad_touched, rpad_touched;
+
+ b8 = data[8];
+ b9 = data[9];
+ b10 = data[10];
+ b11 = data[11];
+ b13 = data[13];
+ b14 = data[14];
+
+ lpad_touched = b10 & BIT(3);
+ rpad_touched = b10 & BIT(4);
+
+ if (lpad_touched) {
+ input_report_abs(input, ABS_HAT0X, steam_le16(data + 16));
+ input_report_abs(input, ABS_HAT0Y, steam_le16(data + 18));
+ } else {
+ input_report_abs(input, ABS_HAT0X, 0);
+ input_report_abs(input, ABS_HAT0Y, 0);
+ }
+
+ if (rpad_touched) {
+ input_report_abs(input, ABS_HAT1X, steam_le16(data + 20));
+ input_report_abs(input, ABS_HAT1Y, steam_le16(data + 22));
+ } else {
+ input_report_abs(input, ABS_HAT1X, 0);
+ input_report_abs(input, ABS_HAT1Y, 0);
+ }
+
+ input_report_abs(input, ABS_X, steam_le16(data + 48));
+ input_report_abs(input, ABS_Y, -steam_le16(data + 50));
+ input_report_abs(input, ABS_RX, steam_le16(data + 52));
+ input_report_abs(input, ABS_RY, -steam_le16(data + 54));
+
+ input_report_abs(input, ABS_HAT2Y, steam_le16(data + 44));
+ input_report_abs(input, ABS_HAT2X, steam_le16(data + 46));
+
+ input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
+ input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
+ input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
+ input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
+ input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
+ input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
+ input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
+ input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
+ input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
+ input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
+ input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
+ input_event(input, EV_KEY, BTN_TRIGGER_HAPPY3, !!(b9 & BIT(7)));
+ input_event(input, EV_KEY, BTN_TRIGGER_HAPPY4, !!(b10 & BIT(0)));
+ input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
+ input_event(input, EV_KEY, BTN_THUMBR, !!(b11 & BIT(2)));
+ input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
+ input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
+ input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
+ input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
+ input_event(input, EV_KEY, BTN_THUMB, !!(b10 & BIT(1)));
+ input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(2)));
+ input_event(input, EV_KEY, BTN_TRIGGER_HAPPY1, !!(b13 & BIT(1)));
+ input_event(input, EV_KEY, BTN_TRIGGER_HAPPY2, !!(b13 & BIT(2)));
+ input_event(input, EV_KEY, BTN_BASE, !!(b14 & BIT(2)));
+
+ input_sync(input);
+}
+
/*
* The size for this message payload is 11.
* The known values are:
@@ -1052,6 +1314,7 @@ static int steam_raw_event(struct hid_device *hdev,
* 0x01: input data (60 bytes)
* 0x03: wireless connect/disconnect (1 byte)
* 0x04: battery status (11 bytes)
+ * 0x09: Steam Deck input data (56 bytes)
*/
if (size != 64 || data[0] != 1 || data[1] != 0)
@@ -1067,6 +1330,15 @@ static int steam_raw_event(struct hid_device *hdev,
steam_do_input_event(steam, input, data);
rcu_read_unlock();
break;
+ case STEAM_EV_DECK_INPUT_DATA:
+ if (steam->client_opened)
+ return 0;
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ if (likely(input))
+ steam_do_deck_input_event(steam, input, data);
+ rcu_read_unlock();
+ break;
case STEAM_EV_CONNECT:
/*
* The payload of this event is a single byte:
@@ -1141,6 +1413,11 @@ static const struct hid_device_id steam_controllers[] = {
USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
.driver_data = STEAM_QUIRK_WIRELESS
},
+ { /* Steam Deck */
+ HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+ USB_DEVICE_ID_STEAM_DECK),
+ .driver_data = STEAM_QUIRK_DECK
+ },
{}
};
--
2.39.1
^ permalink raw reply related
* [PATCH v2 2/2] HID: hid-steam: Add rumble on Deck
From: Vicki Pfau @ 2023-01-26 3:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Lyude Paul, linux-input; +Cc: Vicki Pfau
In-Reply-To: <20230111012336.2915827-2-vi@endrift.com>
The Steam Deck includes a new report that allows for emulating XInput-style
rumble motors with the Deck's actuators. This adds support for passing these
values directly to the Deck.
Signed-off-by: Vicki Pfau <vi@endrift.com>
Reviewed-by: Lyude Paul <lyude@redhat.com>
---
drivers/hid/Kconfig | 8 ++++++
drivers/hid/hid-steam.c | 54 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 20402d0a7eab..ce488cf8765f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1025,6 +1025,14 @@ config HID_STEAM
without running the Steam Client. It supports both the wired and
the wireless adaptor.
+config STEAM_FF
+ bool "Steam Deck force feedback support"
+ depends on HID_STEAM
+ select INPUT_FF_MEMLESS
+ help
+ Say Y here if you want to enable force feedback support for the Steam
+ Deck.
+
config HID_STEELSERIES
tristate "Steelseries SRW-S1 steering wheel support"
help
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 09330d9bfee3..aaca390ca299 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -91,6 +91,7 @@ static LIST_HEAD(steam_devices);
#define STEAM_CMD_FORCEFEEDBAK 0x8f
#define STEAM_CMD_REQUEST_COMM_STATUS 0xb4
#define STEAM_CMD_GET_SERIAL 0xae
+#define STEAM_CMD_HAPTIC_RUMBLE 0xeb
/* Some useful register ids */
#define STEAM_REG_LPAD_MODE 0x07
@@ -134,6 +135,9 @@ struct steam_device {
u8 battery_charge;
u16 voltage;
struct delayed_work heartbeat;
+ struct work_struct rumble_work;
+ u16 rumble_left;
+ u16 rumble_right;
};
static int steam_recv_report(struct steam_device *steam,
@@ -290,6 +294,45 @@ static inline int steam_request_conn_status(struct steam_device *steam)
return steam_send_report_byte(steam, STEAM_CMD_REQUEST_COMM_STATUS);
}
+static inline int steam_haptic_rumble(struct steam_device *steam,
+ u16 intensity, u16 left_speed, u16 right_speed,
+ u8 left_gain, u8 right_gain)
+{
+ u8 report[11] = {STEAM_CMD_HAPTIC_RUMBLE, 9};
+
+ report[3] = intensity & 0xFF;
+ report[4] = intensity >> 8;
+ report[5] = left_speed & 0xFF;
+ report[6] = left_speed >> 8;
+ report[7] = right_speed & 0xFF;
+ report[8] = right_speed >> 8;
+ report[9] = left_gain;
+ report[10] = right_gain;
+
+ return steam_send_report(steam, report, sizeof(report));
+}
+
+static void steam_haptic_rumble_cb(struct work_struct *work)
+{
+ struct steam_device *steam = container_of(work, struct steam_device,
+ rumble_work);
+ steam_haptic_rumble(steam, 0, steam->rumble_left,
+ steam->rumble_right, 2, 0);
+}
+
+#ifdef CONFIG_STEAM_FF
+static int steam_play_effect(struct input_dev *dev, void *data,
+ struct ff_effect *effect)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+
+ steam->rumble_left = effect->u.rumble.strong_magnitude;
+ steam->rumble_right = effect->u.rumble.weak_magnitude;
+
+ return schedule_work(&steam->rumble_work);
+}
+#endif
+
static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
{
if (enable) {
@@ -540,6 +583,15 @@ static int steam_input_register(struct steam_device *steam)
input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
+#ifdef CONFIG_STEAM_FF
+ if (steam->quirks & STEAM_QUIRK_DECK) {
+ input_set_capability(input, EV_FF, FF_RUMBLE);
+ ret = input_ff_create_memless(input, NULL, steam_play_effect);
+ if (ret)
+ goto input_register_fail;
+ }
+#endif
+
ret = input_register_device(input);
if (ret)
goto input_register_fail;
@@ -841,6 +893,7 @@ static int steam_probe(struct hid_device *hdev,
INIT_WORK(&steam->work_connect, steam_work_connect_cb);
INIT_LIST_HEAD(&steam->list);
INIT_DEFERRABLE_WORK(&steam->heartbeat, steam_lizard_mode_heartbeat);
+ INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
steam->client_hdev = steam_create_client_hid(hdev);
if (IS_ERR(steam->client_hdev)) {
@@ -897,6 +950,7 @@ static int steam_probe(struct hid_device *hdev,
client_hdev_fail:
cancel_work_sync(&steam->work_connect);
cancel_delayed_work_sync(&steam->heartbeat);
+ cancel_work_sync(&steam->rumble_work);
steam_alloc_fail:
hid_err(hdev, "%s: failed with error %d\n",
__func__, ret);
--
2.39.1
^ permalink raw reply related
* [PATCH v2 0/2] HID: hid-steam: Add Steam Deck support
From: Vicki Pfau @ 2023-01-26 3:01 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Lyude Paul, linux-input; +Cc: Vicki Pfau
In-Reply-To: <20230111012336.2915827-2-vi@endrift.com>
Revised version of this patchset based on received feedback, as well as an
additional bugfix for the edge case where the driver attempts to schedule the
alread-scheduled lizard mode heartbeat, leading to a deadlock.
Vicki Pfau (2):
HID: hid-steam: Add Steam Deck support
HID: hid-steam: Add rumble on Deck
drivers/hid/Kconfig | 12 +-
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-steam.c | 383 +++++++++++++++++++++++++++++++++++++---
3 files changed, 368 insertions(+), 28 deletions(-)
--
2.39.1
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: azoteq: Fix differing types
From: Jeff LaBundy @ 2023-01-26 1:50 UTC (permalink / raw)
To: Rob Herring
Cc: Dmitry Torokhov, Krzysztof Kozlowski, linux-input, devicetree,
linux-kernel
In-Reply-To: <20230125221416.3058051-1-robh@kernel.org>
Hi Rob,
On Wed, Jan 25, 2023 at 04:14:16PM -0600, Rob Herring wrote:
> 'azoteq,ati-base' and 'azoteq,thresh' properties are defined in multiple
> bindings, but have differing types defined. Both 'uint32' and
> 'uint32-array' are used. Unify these to use 'uint32-array' everywhere.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Thank you for the patch. While this is a step forward in moving toward
a common binding for this vendor like we have discussed in the past, I
do not agree with this approach and will instead propose an alternative
that accomplishes the same goal.
For all of these devices, a single sensing channel takes a base and a
threshold property. IQS626A is unique in that a fixed number of channels
form a trackpad, and I decided at the time to simply define the base and
target properties for all channels as a uint32-array.
For all other existing drivers, as well as others coming down the pipe,
base and threshold are uint32s. I find it confusing to redefine all of
those as single-element arrays, especially on account of one device.
In hindsight, a better design would have been to define a child node
for each channel under the trackpad node, with each child node accepting
a uint32 base and threshold. That would follow other devices, be more
representative of the actual hardware, and keep the definitions the same
across each binding.
So, that's what I propose to do here instead. I happen to have a fix in
review [1] that addresses a bug related to this parsing code, and would
be happy to build this solution on top assuming it can wait until the
next cycle. Does this compromise sound OK?
[1] https://patchwork.kernel.org/patch/https://patchwork.kernel.org/patch/13087768/
> ---
> .../bindings/input/azoteq,iqs7222.yaml | 12 ++++---
> .../devicetree/bindings/input/iqs269a.yaml | 34 +++++++++++--------
> .../devicetree/bindings/input/iqs626a.yaml | 12 ++++---
> 3 files changed, 33 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
> index 9ddba7f2e7aa..f2382a56884d 100644
> --- a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
> +++ b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
> @@ -354,10 +354,11 @@ patternProperties:
> description: Specifies the channel's ATI target.
>
> azoteq,ati-base:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - multipleOf: 16
> - minimum: 0
> - maximum: 496
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - multipleOf: 16
> + minimum: 0
> + maximum: 496
> description: Specifies the channel's ATI base.
>
> azoteq,ati-mode:
> @@ -440,7 +441,8 @@ patternProperties:
> slider gesture).
>
> azoteq,thresh:
> - $ref: /schemas/types.yaml#/definitions/uint32
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + maxItems: 1
> description:
> Specifies the threshold for the event. Valid entries range from
> 0-127 and 0-255 for proximity and touch events, respectively.
> diff --git a/Documentation/devicetree/bindings/input/iqs269a.yaml b/Documentation/devicetree/bindings/input/iqs269a.yaml
> index 3c430d38594f..4fa20f0f6847 100644
> --- a/Documentation/devicetree/bindings/input/iqs269a.yaml
> +++ b/Documentation/devicetree/bindings/input/iqs269a.yaml
> @@ -334,9 +334,10 @@ patternProperties:
> 3: Full
>
> azoteq,ati-base:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - enum: [75, 100, 150, 200]
> - default: 100
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - enum: [75, 100, 150, 200]
> + default: 100
> description: Specifies the channel's ATI base.
>
> azoteq,ati-target:
> @@ -391,10 +392,11 @@ patternProperties:
>
> properties:
> azoteq,thresh:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - minimum: 0
> - maximum: 255
> - default: 10
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - minimum: 0
> + maximum: 255
> + default: 10
> description: Specifies the threshold for the event.
>
> linux,code: true
> @@ -408,10 +410,11 @@ patternProperties:
>
> properties:
> azoteq,thresh:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - minimum: 0
> - maximum: 255
> - default: 8
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - minimum: 0
> + maximum: 255
> + default: 8
> description: Specifies the threshold for the event.
>
> azoteq,hyst:
> @@ -432,10 +435,11 @@ patternProperties:
>
> properties:
> azoteq,thresh:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - minimum: 0
> - maximum: 255
> - default: 26
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - minimum: 0
> + maximum: 255
> + default: 26
> description: Specifies the threshold for the event.
>
> azoteq,hyst:
> diff --git a/Documentation/devicetree/bindings/input/iqs626a.yaml b/Documentation/devicetree/bindings/input/iqs626a.yaml
> index 7a27502095f3..dbd63d48605c 100644
> --- a/Documentation/devicetree/bindings/input/iqs626a.yaml
> +++ b/Documentation/devicetree/bindings/input/iqs626a.yaml
> @@ -234,8 +234,9 @@ patternProperties:
> about the available RUI options.
>
> azoteq,ati-base:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - enum: [75, 100, 150, 200]
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - enum: [75, 100, 150, 200]
> description:
> Specifies the channel's ATI base. The default value is a function
> of the channel and the device's RUI.
> @@ -475,9 +476,10 @@ patternProperties:
>
> properties:
> azoteq,thresh:
> - $ref: /schemas/types.yaml#/definitions/uint32
> - minimum: 0
> - maximum: 255
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + items:
> + - minimum: 0
> + maximum: 255
> description: Specifies the threshold for the event.
>
> azoteq,hyst:
> --
> 2.39.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
From: Pietro Borrello @ 2023-01-26 1:01 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann,
linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander
In-Reply-To: <CAEc3jaCEKfqEJSV4=6GRj1Ry97xH+HwVSeEOZReNwkt=rLNvNQ@mail.gmail.com>
On Thu, 26 Jan 2023 at 01:44, Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> Hi Pietro,
>
> Thanks for your patch. For sure for ds4/dualsense there have been edge
> cases around rumble removal and others. Those were prevented by this
> 'output_worker_initialized' variable, which is checked during the
> centralized work scheduling function (dualshock4_schedule_work /
> dualsense_schedule_work). That said I don't mind the change as it
> prevents the work scheduling functions to get called unnecessarily.
Hi Roderick,
Thank you for your fast response. You are right, the combination
of the `output_worker_initialized` variable and the spinlock prevents
the work to be scheduled during device removal for ds4/dualsense.
Thank you for the clarification!
Thanks,
Pietro
^ permalink raw reply
* Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
From: Roderick Colenbrander @ 2023-01-26 0:47 UTC (permalink / raw)
To: Pietro Borrello
Cc: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Pavel Machek,
Lee Jones, Roderick Colenbrander, Sven Eckelmann, linux-leds,
Cristiano Giuffrida, Bos, H.J., Jakob Koschel, linux-input,
linux-kernel, Jiri Kosina, Roderick Colenbrander
In-Reply-To: <CAEc3jaCEKfqEJSV4=6GRj1Ry97xH+HwVSeEOZReNwkt=rLNvNQ@mail.gmail.com>
One other little note. For completeness I guess we need a similar
patch for the multicolor led class.
On Wed, Jan 25, 2023 at 4:43 PM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> Hi Pietro,
>
> Thanks for your patch. For sure for ds4/dualsense there have been edge
> cases around rumble removal and others. Those were prevented by this
> 'output_worker_initialized' variable, which is checked during the
> centralized work scheduling function (dualshock4_schedule_work /
> dualsense_schedule_work). That said I don't mind the change as it
> prevents the work scheduling functions to get called unnecessarily.
>
> Thanks,
> Roderick Colenbrander
>
> On Wed, Jan 25, 2023 at 4:26 PM Pietro Borrello
> <borrello@diag.uniroma1.it> wrote:
> >
> > Unregister the LED controller before device removal, as
> > dualsense_player_led_set_brightness() may schedule output_worker
> > after the structure has been freed, causing a use-after-free.
> >
> > Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
> > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> > ---
> > drivers/hid/hid-playstation.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index 27c40894acab..9e23860b7e95 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > @@ -1503,11 +1503,15 @@ static void dualsense_remove(struct ps_device *ps_dev)
> > {
> > struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> > unsigned long flags;
> > + int i;
> >
> > spin_lock_irqsave(&ds->base.lock, flags);
> > ds->output_worker_initialized = false;
> > spin_unlock_irqrestore(&ds->base.lock, flags);
> >
> > + for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> > + devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
> > +
> > cancel_work_sync(&ds->output_worker);
> > }
> >
> >
> > --
> > 2.25.1
^ permalink raw reply
* Re: [PATCH 3/5] HID: dualsense_remove: manually unregister leds
From: Roderick Colenbrander @ 2023-01-26 0:43 UTC (permalink / raw)
To: Pietro Borrello
Cc: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann,
linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander
In-Reply-To: <20230125-hid-unregister-leds-v1-3-9a5192dcef16@diag.uniroma1.it>
Hi Pietro,
Thanks for your patch. For sure for ds4/dualsense there have been edge
cases around rumble removal and others. Those were prevented by this
'output_worker_initialized' variable, which is checked during the
centralized work scheduling function (dualshock4_schedule_work /
dualsense_schedule_work). That said I don't mind the change as it
prevents the work scheduling functions to get called unnecessarily.
Thanks,
Roderick Colenbrander
On Wed, Jan 25, 2023 at 4:26 PM Pietro Borrello
<borrello@diag.uniroma1.it> wrote:
>
> Unregister the LED controller before device removal, as
> dualsense_player_led_set_brightness() may schedule output_worker
> after the structure has been freed, causing a use-after-free.
>
> Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
> drivers/hid/hid-playstation.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 27c40894acab..9e23860b7e95 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -1503,11 +1503,15 @@ static void dualsense_remove(struct ps_device *ps_dev)
> {
> struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> unsigned long flags;
> + int i;
>
> spin_lock_irqsave(&ds->base.lock, flags);
> ds->output_worker_initialized = false;
> spin_unlock_irqrestore(&ds->base.lock, flags);
>
> + for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> + devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
> +
> cancel_work_sync(&ds->output_worker);
> }
>
>
> --
> 2.25.1
^ permalink raw reply
* [PATCH 4/5] HID: dualshock4_remove: manually unregister leds
From: Pietro Borrello @ 2023-01-26 0:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
Cc: linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander,
Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v1-0-9a5192dcef16@diag.uniroma1.it>
Unregister the LED controller before device removal, as
dualshock4_led_set_brightness() may schedule output_worker
after the structure has been freed, causing a use-after-free.
Fixes: 4521109a8f40 ("HID: playstation: support DualShock4 lightbar.")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-playstation.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 9e23860b7e95..6fae5a24eaad 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -2432,11 +2432,15 @@ static void dualshock4_remove(struct ps_device *ps_dev)
{
struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
unsigned long flags;
+ int i;
spin_lock_irqsave(&ds4->base.lock, flags);
ds4->output_worker_initialized = false;
spin_unlock_irqrestore(&ds4->base.lock, flags);
+ for (i = 0; i < ARRAY_SIZE(ds4->lightbar_leds); i++)
+ devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds4->lightbar_leds[i]);
+
cancel_work_sync(&ds4->output_worker);
if (ps_dev->hdev->product == USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE)
--
2.25.1
^ permalink raw reply related
* [PATCH 5/5] HID: sony_remove: manually unregister leds
From: Pietro Borrello @ 2023-01-26 0:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
Cc: linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander,
Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v1-0-9a5192dcef16@diag.uniroma1.it>
Unregister the LED controller before device removal, as
sony_led_set_brightness() may schedule sc->state_worker
after the structure has been freed, causing a use-after-free.
Fixes: 0a286ef27852 ("HID: sony: Add LED support for Sixaxis/Dualshock3 USB")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-sony.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 13125997ab5e..146677c8319c 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -3083,6 +3083,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
static void sony_remove(struct hid_device *hdev)
{
struct sony_sc *sc = hid_get_drvdata(hdev);
+ int n;
if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) {
del_timer_sync(&sc->ghl_poke_timer);
@@ -3100,6 +3101,13 @@ static void sony_remove(struct hid_device *hdev)
if (sc->hw_version_created)
device_remove_file(&sc->hdev->dev, &dev_attr_hardware_version);
+ if (sc->quirks & SONY_LED_SUPPORT) {
+ for (n = 0; n < sc->led_count; n++) {
+ if (sc->leds[n])
+ devm_led_classdev_unregister(&hdev->dev, sc->leds[n]);
+ }
+ }
+
sony_cancel_work_sync(sc);
sony_remove_dev_list(sc);
--
2.25.1
^ permalink raw reply related
* [PATCH 3/5] HID: dualsense_remove: manually unregister leds
From: Pietro Borrello @ 2023-01-26 0:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
Cc: linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander,
Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v1-0-9a5192dcef16@diag.uniroma1.it>
Unregister the LED controller before device removal, as
dualsense_player_led_set_brightness() may schedule output_worker
after the structure has been freed, causing a use-after-free.
Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-playstation.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 27c40894acab..9e23860b7e95 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -1503,11 +1503,15 @@ static void dualsense_remove(struct ps_device *ps_dev)
{
struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
unsigned long flags;
+ int i;
spin_lock_irqsave(&ds->base.lock, flags);
ds->output_worker_initialized = false;
spin_unlock_irqrestore(&ds->base.lock, flags);
+ for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
+ devm_led_classdev_unregister(&ps_dev->hdev->dev, &ds->player_leds[i]);
+
cancel_work_sync(&ds->output_worker);
}
--
2.25.1
^ permalink raw reply related
* [PATCH 2/5] HID: asus_remove: manually unregister led
From: Pietro Borrello @ 2023-01-26 0:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
Cc: linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander,
Pietro Borrello
In-Reply-To: <20230125-hid-unregister-leds-v1-0-9a5192dcef16@diag.uniroma1.it>
Unregister the LED controller before device removal, as
asus_kbd_backlight_set() may schedule led->work after the structure
has been freed, causing a use-after-free.
Fixes: af22a610bc38 ("HID: asus: support backlight on USB keyboards")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
drivers/hid/hid-asus.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index f99752b998f3..0f274c8d1bef 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -1122,6 +1122,7 @@ static void asus_remove(struct hid_device *hdev)
if (drvdata->kbd_backlight) {
drvdata->kbd_backlight->removed = true;
+ devm_led_classdev_unregister(&hdev->dev, &drvdata->kbd_backlight->cdev);
cancel_work_sync(&drvdata->kbd_backlight->work);
}
--
2.25.1
^ permalink raw reply related
* [PATCH 0/5] HID: manually unregister leds on device removal to prevent UAFs
From: Pietro Borrello @ 2023-01-26 0:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Hanno Zulla, Carlo Caione,
Pavel Machek, Lee Jones, Roderick Colenbrander, Sven Eckelmann
Cc: linux-leds, Cristiano Giuffrida, Bos, H.J., Jakob Koschel,
linux-input, linux-kernel, Jiri Kosina, Roderick Colenbrander,
Pietro Borrello
I noticed a recurring pattern is present in multiple hid devices in the
Linux tree, where the LED controller of a device schedules a work_struct
to interact with the hardware.
The work_struct is embedded in the device structure and thus, is freed
at device removal.
The issue is that a LED worker may be scheduled by a timer concurrently
with device removal, causing the work_struct to be accessed after having
been freed.
I was able to trigger the issue in hid-bigbenff.c and hid-asus.c
where the work_structs may be scheduled by the LED controller
while the device is disconnecting, triggering use-after-frees.
I can attach the reproducer, but it's very simple USB configuration,
using the /dev/raw-gadget interface with some more USB interactions
to manage LEDs configuration and pass checks in asus_kbd_init()
and asus_kbd_get_functions() in case of hid-asus.c.
I triggered the issue by connecting a device and immediately
disconnecting it, so that the remove function runs before the LED one
which remains pending.
More drivers have the same pattern (hid-lg-g15.c, hid-playstation.c,
hid-sony.c) but I wasn't able to properly pass the right descriptors
to trigger the led configurations needed to trigger the workers.
Some other drivers manually unregister at removal (hid-corsair.c,
hid-gt683r.c, hid-lenovo.c) since they do not use the managed
interface, which is safe, to my understanding.
Also, a similar pattern is present with callbacks which schedule
a worker originating from input_ff_create_memless() (e.g.,
in hid-bigbenff.c) but in these cases, I wasn't able to trigger
the race condition with the event handling to schedule the worker
during device removal. However, I have no experience with the USB
protocol and I'm not able to say that they cannot be triggered.
I am currently wondering if this is due to some emulation of the
/dev/raw-gadget interface or if it's effectively an issue with how each
device manages resource removal.
But I wonder why syzkaller didn't find any crash while fuzzing the
interface with upstream-usb.config, as they seem pretty
straightforward to trigger.
Configuring the kernel with CONFIG_DEBUG_OBJECTS, it emits
a warning in debug_check_no_obj_freed, which makes it clear that
device removal is freeing resources in use.
KASAN detects them as use-after-free.
I am attaching multiple patches for all the drivers I suspect the bug
is present.
The proposed patches unregister the LED controllers before removing the
device itself.
I attach the (partial for brevity) ODEBUG dumps:
```hid-bigbenff.c
[ 37.803135][ T1170] usb 1-1: USB disconnect, device number 2
[ 37.827979][ T1170] ODEBUG: free active (active state 0) object
type: work_struct hint: bigben_worker+0x0/0x860
[ 37.829634][ T1170] WARNING: CPU: 0 PID: 1170 at
lib/debugobjects.c:505 debug_check_no_obj_freed+0x43a/0x630
[ 37.830904][ T1170] Modules linked in:
[ 37.831413][ T1170] CPU: 0 PID: 1170 Comm: kworker/0:3 Not tainted
6.1.0-rc4-dirty #43
[ 37.832465][ T1170] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 37.833751][ T1170] Workqueue: usb_hub_wq hub_event
[ 37.834409][ T1170] RIP: 0010:debug_check_no_obj_freed+0x43a/0x630
[ 37.835218][ T1170] Code: 48 89 ef e8 28 82 58 ff 49 8b 14 24 4c 8b
45 00 48 c7 c7 40 5f 09 87 48 c7 c6 60 5b 09 87 89 d9 4d 89 f9 31 c0
e8 46 25 ef fe <0f> 0b 4c 8b 64 24 20 48 ba 00 00 00 00 00 fc ff df ff
05 4f 7c 17
[ 37.837667][ T1170] RSP: 0018:ffffc900006fee60 EFLAGS: 00010246
[ 37.838503][ T1170] RAX: 0d2d19ffcded3d00 RBX: 0000000000000000
RCX: ffff888117fc9b00
[ 37.839519][ T1170] RDX: 0000000000000000 RSI: 0000000000000000
RDI: 0000000000000000
[ 37.840570][ T1170] RBP: ffffffff86e88380 R08: ffffffff8130793b
R09: fffff520000dfd85
[ 37.841618][ T1170] R10: fffff520000dfd85 R11: 0000000000000000
R12: ffffffff87095fb8
[ 37.842649][ T1170] R13: ffff888117770ad8 R14: ffff888117770acc
R15: ffffffff852b7420
[ 37.843728][ T1170] FS: 0000000000000000(0000)
GS:ffff8881f6600000(0000) knlGS:0000000000000000
[ 37.844877][ T1170] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 37.845749][ T1170] CR2: 00007f992eaab380 CR3: 000000011834b000
CR4: 00000000001006f0
[ 37.846794][ T1170] Call Trace:
[ 37.847245][ T1170] <TASK>
[ 37.847643][ T1170] slab_free_freelist_hook+0x89/0x160
[ 37.848409][ T1170] ? devres_release_all+0x262/0x350
[ 37.849156][ T1170] __kmem_cache_free+0x71/0x110
[ 37.849829][ T1170] devres_release_all+0x262/0x350
[ 37.850478][ T1170] ? devres_release+0x90/0x90
[ 37.851118][ T1170] device_release_driver_internal+0x5e5/0x8a0
[ 37.851944][ T1170] bus_remove_device+0x2ea/0x400
[ 37.852611][ T1170] device_del+0x64f/0xb40
[ 37.853212][ T1170] ? kill_device+0x150/0x150
[ 37.853831][ T1170] ? print_irqtrace_events+0x1f0/0x1f0
[ 37.854564][ T1170] hid_destroy_device+0x66/0x100
[ 37.855226][ T1170] usbhid_disconnect+0x9a/0xc0
[ 37.855887][ T1170] usb_unbind_interface+0x1e1/0x890
```
``` hid-asus.c
[ 77.409878][ T1169] usb 1-1: USB disconnect, device number 2
[ 77.423606][ T1169] ODEBUG: free active (active state 0) object
type: work_struct hint: asus_kbd_backlight_work+0x0/0x2c0
[ 77.425222][ T1169] WARNING: CPU: 0 PID: 1169 at
lib/debugobjects.c:505 debug_check_no_obj_freed+0x43a/0x630
[ 77.426599][ T1169] Modules linked in:
[ 77.427322][ T1169] CPU: 0 PID: 1169 Comm: kworker/0:3 Not tainted
6.1.0-rc4-dirty #43
[ 77.428404][ T1169] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[ 77.429644][ T1169] Workqueue: usb_hub_wq hub_event
[ 77.430296][ T1169] RIP: 0010:debug_check_no_obj_freed+0x43a/0x630
[ 77.431142][ T1169] Code: 48 89 ef e8 28 82 58 ff 49 8b 14 24 4c 8b
45 00 48 c7 c7 40 5f 09 87 48 c7 c6 60 5b 09 87 89 d9 4d 89 f9 31 c0
e8 46 25 ef fe <0f> 0b 4c 8b 64 24 20 48 ba 00 00 00 00 00 fc ff df ff
05 4f 7c 17
[ 77.433691][ T1169] RSP: 0018:ffffc9000069ee60 EFLAGS: 00010246
[ 77.434470][ T1169] RAX: b85d2b40c12d7600 RBX: 0000000000000000
RCX: ffff888117a78000
[ 77.435507][ T1169] RDX: 0000000000000000 RSI: 0000000080000000
RDI: 0000000000000000
[ 77.436521][ T1169] RBP: ffffffff86e88380 R08: ffffffff8130793b
R09: ffffed103ecc4ed6
[ 77.437582][ T1169] R10: ffffed103ecc4ed6 R11: 0000000000000000
R12: ffffffff87095fb8
[ 77.438593][ T1169] R13: ffff88810e348fe0 R14: ffff88810e348fd4
R15: ffffffff852b5780
[ 77.439667][ T1169] FS: 0000000000000000(0000)
GS:ffff8881f6600000(0000) knlGS:0000000000000000
[ 77.440842][ T1169] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 77.441688][ T1169] CR2: 00007ffc05495ff0 CR3: 000000010cdf0000
CR4: 00000000001006f0
[ 77.442720][ T1169] Call Trace:
[ 77.443167][ T1169] <TASK>
[ 77.443555][ T1169] slab_free_freelist_hook+0x89/0x160
[ 77.444302][ T1169] ? devres_release_all+0x262/0x350
[ 77.444990][ T1169] __kmem_cache_free+0x71/0x110
[ 77.445638][ T1169] devres_release_all+0x262/0x350
[ 77.446309][ T1169] ? devres_release+0x90/0x90
[ 77.446978][ T1169] device_release_driver_internal+0x5e5/0x8a0
[ 77.447748][ T1169] bus_remove_device+0x2ea/0x400
[ 77.448421][ T1169] device_del+0x64f/0xb40
[ 77.448976][ T1169] ? kill_device+0x150/0x150
[ 77.449577][ T1169] ? print_irqtrace_events+0x1f0/0x1f0
[ 77.450307][ T1169] hid_destroy_device+0x66/0x100
[ 77.450938][ T1169] usbhid_disconnect+0x9a/0xc0
```
To: Jiri Kosina <jikos@kernel.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Hanno Zulla <kontakt@hanno.de>
To: Carlo Caione <carlo@endlessm.com>
To: Pavel Machek <pavel@ucw.cz>
To: Lee Jones <lee@kernel.org>
To: Roderick Colenbrander <roderick.colenbrander@sony.com>
To: Sven Eckelmann <sven@narfation.org>
Cc: linux-leds@vger.kernel.org
Cc: Cristiano Giuffrida <c.giuffrida@vu.nl>
Cc: "Bos, H.J." <h.j.bos@vu.nl>
Cc: Jakob Koschel <jkl820.git@gmail.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Roderick Colenbrander <roderick@gaikai.com>
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
Pietro Borrello (5):
HID: bigben_remove: manually unregister leds
HID: asus_remove: manually unregister led
HID: dualsense_remove: manually unregister leds
HID: dualshock4_remove: manually unregister leds
HID: sony_remove: manually unregister leds
drivers/hid/hid-asus.c | 1 +
drivers/hid/hid-bigbenff.c | 5 +++++
drivers/hid/hid-playstation.c | 8 ++++++++
drivers/hid/hid-sony.c | 8 ++++++++
4 files changed, 22 insertions(+)
---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230125-hid-unregister-leds-4cbf67099e1d
Best regards,
--
Pietro Borrello <borrello@diag.uniroma1.it>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox