* [PATCH] input: goodix: Add option to power off the controller during suspend
From: Jan Jasper de Kroon @ 2023-03-10 17:00 UTC (permalink / raw)
To: linux-input; +Cc: Ondřej Jirman, Jan Jasper de Kroon
From: Ondřej Jirman <megi@xff.cz>
For whatever reason the controller is not suspended on Pinephone
by the default procedure. It consumes quite a bit of power (~40mW)
during system sleep, and more when the screen is touched.
Let's power off the controller during system sleep instead.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
input: goodix: Don't disable regulators during suspend
It does no harm to disable them, but on Pinephone we have other
peripherals attached to the same power supplies, whose drivers
will not reference the regulator, so powering down the regulators
from Goodix driver will break those other devices.
Until those drivers gain the regulator support, don't disable
the regulators in Goodix driver.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
drivers/input/touchscreen/goodix.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b348172f19c3..e6d054ede960 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
static int goodix_ts_probe(struct i2c_client *client)
{
+ struct device_node *np = client->dev.of_node;
struct goodix_ts_data *ts;
const char *cfg_name;
int error;
@@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
init_completion(&ts->firmware_loading_complete);
ts->contact_size = GOODIX_CONTACT_SIZE;
+ ts->poweroff_in_suspend = of_property_read_bool(np, "poweroff-in-suspend");
error = goodix_get_gpio_config(ts);
if (error)
@@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
+ if (ts->poweroff_in_suspend) {
+ goodix_free_irq(ts);
+ goodix_irq_direction_output(ts, 0);
+ gpiod_direction_output(ts->gpiod_rst, 0);
+ return 0;
+ }
+
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
disable_irq(client->irq);
@@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
u8 config_ver;
int error;
+ if (ts->poweroff_in_suspend) {
+ error = goodix_reset(ts);
+ if (error) {
+ dev_err(dev, "Controller reset failed.\n");
+ return error;
+ }
+
+ error = goodix_request_irq(ts);
+ if (error)
+ return error;
+
+ return 0;
+ }
+
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
enable_irq(client->irq);
return 0;
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..dc358bcda698 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -104,6 +104,7 @@ struct goodix_ts_data {
u8 main_clk[GOODIX_MAIN_CLK_LEN];
int bak_ref_len;
u8 *bak_ref;
+ bool poweroff_in_suspend;
};
int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
--
2.34.3
^ permalink raw reply related
* Re: [PATCH] HID: topre: Add support for 87 keys Realforce R2
From: Jiri Kosina @ 2023-03-10 18:00 UTC (permalink / raw)
To: Alessandro Manca; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20230310164933.4798-1-crizan.git@gmail.com>
On Fri, 10 Mar 2023, Alessandro Manca wrote:
> The tenkeyless version of the Realforce R2 has the same issue of the
> full size one, the report fixup is needed to make n-key rollover
> work instead of 6 key rollover
Applied, thanks Alessandro.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] HID: topre: Add support for 87 keys Realforce R2
From: Alessandro Manca @ 2023-03-10 16:49 UTC (permalink / raw)
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Alessandro Manca
The tenkeyless version of the Realforce R2 has the same issue of the
full size one, the report fixup is needed to make n-key rollover
work instead of 6 key rollover
Signed-off-by: Alessandro Manca <crizan.git@gmail.com>
---
drivers/hid/Kconfig | 2 +-
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-topre.c | 2 ++
3 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 82f64fb31fda..4ce012f83253 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1122,7 +1122,7 @@ config HID_TOPRE
tristate "Topre REALFORCE keyboards"
depends on HID
help
- Say Y for N-key rollover support on Topre REALFORCE R2 108 key keyboards.
+ Say Y for N-key rollover support on Topre REALFORCE R2 108/87 key keyboards.
config HID_THINGM
tristate "ThingM blink(1) USB RGB LED"
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 63545cd307e5..50728e0a5067 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1249,6 +1249,7 @@
#define USB_VENDOR_ID_TOPRE 0x0853
#define USB_DEVICE_ID_TOPRE_REALFORCE_R2_108 0x0148
+#define USB_DEVICE_ID_TOPRE_REALFORCE_R2_87 0x0146
#define USB_VENDOR_ID_TOPSEED 0x0766
#define USB_DEVICE_ID_TOPSEED_CYBERLINK 0x0204
diff --git a/drivers/hid/hid-topre.c b/drivers/hid/hid-topre.c
index 88a91cdad5f8..d1d5ca310ead 100644
--- a/drivers/hid/hid-topre.c
+++ b/drivers/hid/hid-topre.c
@@ -36,6 +36,8 @@ static __u8 *topre_report_fixup(struct hid_device *hdev, __u8 *rdesc,
static const struct hid_device_id topre_id_table[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_TOPRE,
USB_DEVICE_ID_TOPRE_REALFORCE_R2_108) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_TOPRE,
+ USB_DEVICE_ID_TOPRE_REALFORCE_R2_87) },
{ }
};
MODULE_DEVICE_TABLE(hid, topre_id_table);
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v5 0/2] Apple Magic Keyboard Backlight
From: Aditya Garg @ 2023-03-10 16:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Orlando Chamberlain, linux-doc@vger.kernel.org,
linux-input@vger.kernel.org, Jonathan Corbet, Benjamin Tissoires,
linux-kernel@vger.kernel.org, Pavel Machek, Aun-Ali Zaidi,
Kerem Karabay, Andy Shevchenko, Thomas Weißschuh
In-Reply-To: <nycvar.YFH.7.76.2303101535440.1142@cbobk.fhfr.pm>
> Hi,
>
> thanks for creating the support for backlight.
>
> Is there any reason why not to fold all this into existing hid-apple? I
> don't think we need separate driver for the backlist, separated from the
> rest of hid-apple support.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
As far as I know, hid-apple manages the keyboards and trackpads on Macs.
The magic backlight is managed by the touchbar on T2 Macs, so if you wanna integrate the driver in some other one, then it should be the to-be-upstreamed touchbar driver.
But when we did that, the MacBook Air 2020, the model which has magic backlight, but no touchbar faced issues. lsusb interestingly shows presence of touch bar backlight even on this model, but backlight is registered at the 0th interface on Air, and 1st interface on the Pros. So, the co-author, Kerem Karabay suggested using a separate driver.
Although, the authors may give more detailed reason for the same.
^ permalink raw reply
* Re: [PATCH v2] HID: apple: Set the tilde quirk flag on the Geyser 4 and later
From: Jiri Kosina @ 2023-03-10 14:50 UTC (permalink / raw)
To: Alex Henrie; +Cc: linux-input
In-Reply-To: <20230227030614.827480-1-alexhenrie24@gmail.com>
On Sun, 26 Feb 2023, Alex Henrie wrote:
> I recently tested several old MacBooks and as far as I can tell, all
> MacBooks that have an ISO keyboard have the tilde key quirk:
>
> Product Model Year System CPU Shape Labels Country Quirky
> ============================================================================
> 05ac:021b A1181 2006 MacBook2,1 T5600 ISO British 13 Yes
> 05ac:021b A1181 2007 MacBook2,1 T7200 ISO Québécois 13 Yes
> 05ac:0229 A1181 2007 MacBook4,1 T8300 ANSI Usonian 33 No
> 05ac:022a A1181 2007 MacBook4,1 T8100 ISO English 13 Yes
> 05ac:022a A1181 2007 MacBook5,2 P7350 ISO Québécois 13 Yes
> 05ac:0237 A1278 2008 MacBook5,1 P7350 ISO Dutch 13 Yes
> 05ac:0237 A1278 2009 MacBook5,5 P7550 ISO British 13 Yes
>
> The model number and year are from the laptop case. Since Apple printed
> the same model and year on many different laptops, the system name (as
> reported in the SMBIOS tables) and CPU form a more precise identifier.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> v2: Added system names from SMBIOS tables to commit message, no code
> changes
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] Input: zinitix: Use of_property_present() for testing DT property presence
From: Rob Herring @ 2023-03-10 14:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: devicetree, linux-input, linux-kernel
It is preferred to use typed property access functions (i.e.
of_property_read_<type> functions) rather than low-level
of_get_property/of_find_property functions for reading properties. As
part of this, convert of_get_property/of_find_property calls to the
recently added of_property_present() helper when we just want to test
for presence of a property and nothing more.
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/input/touchscreen/zinitix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index cdf9bcd744db..b6ece47151b8 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -260,7 +260,7 @@ static int zinitix_init_regulators(struct bt541_ts_data *bt541)
* so check if "vddo" is present and in that case use these names.
* Else use the proper supply names on the component.
*/
- if (of_find_property(dev->of_node, "vddo-supply", NULL)) {
+ if (of_property_present(dev->of_node, "vddo-supply")) {
bt541->supplies[0].supply = "vdd";
bt541->supplies[1].supply = "vddo";
} else {
--
2.39.2
^ permalink raw reply related
* [PATCH] Input: Use of_property_read_bool() for boolean properties
From: Rob Herring @ 2023-03-10 14:47 UTC (permalink / raw)
To: Dmitry Torokhov, Laxman Dewangan, Thierry Reding, Jonathan Hunter
Cc: devicetree, linux-input, linux-kernel, linux-tegra
It is preferred to use typed property access functions (i.e.
of_property_read_<type> functions) rather than low-level
of_get_property/of_find_property functions for reading properties.
Convert reading boolean properties to to of_property_read_bool().
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/input/keyboard/matrix_keypad.c | 6 ++----
drivers/input/keyboard/omap4-keypad.c | 3 +--
drivers/input/keyboard/samsung-keypad.c | 3 +--
drivers/input/keyboard/tegra-kbc.c | 3 +--
4 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 203310727d88..a1b037891af2 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -425,14 +425,12 @@ matrix_keypad_parse_dt(struct device *dev)
return ERR_PTR(-EINVAL);
}
- if (of_get_property(np, "linux,no-autorepeat", NULL))
- pdata->no_autorepeat = true;
+ pdata->no_autorepeat = of_property_read_bool(np, "linux,no-autorepeat");
pdata->wakeup = of_property_read_bool(np, "wakeup-source") ||
of_property_read_bool(np, "linux,wakeup"); /* legacy */
- if (of_get_property(np, "gpio-activelow", NULL))
- pdata->active_low = true;
+ pdata->active_low = of_property_read_bool(np, "gpio-activelow");
pdata->drive_inactive_cols =
of_property_read_bool(np, "drive-inactive-cols");
diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index 4426120398b0..9f085d5679db 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -274,8 +274,7 @@ static int omap4_keypad_parse_dt(struct device *dev,
if (err)
return err;
- if (of_get_property(np, "linux,input-no-autorepeat", NULL))
- keypad_data->no_autorepeat = true;
+ keypad_data->no_autorepeat = of_property_read_bool(np, "linux,input-no-autorepeat");
return 0;
}
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index 09e883ea1352..d85dd2489293 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -291,8 +291,7 @@ samsung_keypad_parse_dt(struct device *dev)
*keymap++ = KEY(row, col, key_code);
}
- if (of_get_property(np, "linux,input-no-autorepeat", NULL))
- pdata->no_autorepeat = true;
+ pdata->no_autorepeat = of_property_read_bool(np, "linux,input-no-autorepeat");
pdata->wakeup = of_property_read_bool(np, "wakeup-source") ||
/* legacy name */
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index da4019cf0c83..d5a6c7d8eb25 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -504,8 +504,7 @@ static int tegra_kbc_parse_dt(struct tegra_kbc *kbc)
if (!of_property_read_u32(np, "nvidia,repeat-delay-ms", &prop))
kbc->repeat_cnt = prop;
- if (of_find_property(np, "nvidia,needs-ghost-filter", NULL))
- kbc->use_ghost_filter = true;
+ kbc->use_ghost_filter = of_property_present(np, "nvidia,needs-ghost-filter");
if (of_property_read_bool(np, "wakeup-source") ||
of_property_read_bool(np, "nvidia,wakeup-source")) /* legacy */
--
2.39.2
^ permalink raw reply related
* Re: [PATCH] HID: add HP 13t-aw100 & 14t-ea100 digitizer battery quirks
From: Jiri Kosina @ 2023-03-10 14:42 UTC (permalink / raw)
To: Philippe Troin; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20230306185026.8B85A1A0008D@ceramic.home.fifi.org>
On Mon, 6 Mar 2023, Philippe Troin wrote:
> Similar to many other devices using the Synopsys Designware Elantech
> hardware, HP Spectre x360 13t-aw100 and 14t-ea100 report an empty
> battery devices, supposedly for the active stylus.
>
> Apply the HID_BATTERY_QUIRK_IGNORE quirk to ignore the battery reports
> from these devices. Note that there are multiple versions of the panel
> installed in the 14t-ea100.
>
> Signed-off-by: Philippe Troin <phil@fifi.org>
> ---
> drivers/hid/hid-ids.h | 3 +++
> drivers/hid/hid-input.c | 6 ++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 63545cd307e5..22e716b66fb8 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -420,6 +420,9 @@
> #define I2C_DEVICE_ID_SURFACE_GO_TOUCHSCREEN 0x261A
> #define I2C_DEVICE_ID_SURFACE_GO2_TOUCHSCREEN 0x2A1C
> #define I2C_DEVICE_ID_LENOVO_YOGA_C630_TOUCHSCREEN 0x279F
> +#define I2C_DEVICE_ID_HP_SPECTRE_X360_13T_AW100 0x29F5
> +#define I2C_DEVICE_ID_HP_SPECTRE_X360_14T_EA100_V1 0x2BED
> +#define I2C_DEVICE_ID_HP_SPECTRE_X360_14T_EA100_V2 0x2BEE
>
> #define USB_VENDOR_ID_ELECOM 0x056e
> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 7fc967964dd8..5c65a584b3fa 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -398,6 +398,12 @@ static const struct hid_device_id hid_battery_quirks[] = {
> HID_BATTERY_QUIRK_IGNORE },
> { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_LENOVO_YOGA_C630_TOUCHSCREEN),
> HID_BATTERY_QUIRK_IGNORE },
> + { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_HP_SPECTRE_X360_13T_AW100),
> + HID_BATTERY_QUIRK_IGNORE },
> + { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_HP_SPECTRE_X360_14T_EA100_V1),
> + HID_BATTERY_QUIRK_IGNORE },
> + { HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_HP_SPECTRE_X360_14T_EA100_V2),
> + HID_BATTERY_QUIRK_IGNORE },
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v5 0/2] Apple Magic Keyboard Backlight
From: Jiri Kosina @ 2023-03-10 14:36 UTC (permalink / raw)
To: Orlando Chamberlain
Cc: linux-doc, linux-input, Jonathan Corbet, Benjamin Tissoires,
linux-kernel, Pavel Machek, Aditya Garg, Aun-Ali Zaidi,
Kerem Karabay, Andy Shevchenko, Thomas Weißschuh
In-Reply-To: <20230220115203.76154-1-orlandoch.dev@gmail.com>
On Mon, 20 Feb 2023, Orlando Chamberlain wrote:
> This patchseries adds support for the internal keyboard backlight of
> Macs with Apple's "Magic" keyboard (MacBookPro16,* and MacBookAir9,1),
> and also documents what names should be used for keyboard backlight
> leds in Documentation/leds/well-known-leds.txt.
>
> v4->v5:
> - use <tab><space><space> for help in Kconfig
> - prepend "hid-" to filename in MAINTAINERS
>
> v3->v4:
> - collect reviews from Andy and Thomas
> - remove now unused hdev member of apple_magic_backlight
>
> v2->v3:
> - remove unneeded header inclusion
> - use s32 for report value type
> - remove unneeded null check
> - don't set drvdata as its never used
> - prepend "hid-" to module name
>
> v1->v2:
> - drop unneeded remove function
> - combine set functions
> - add missing header inclusions
> - avoid char as argument in favour of u8
> - handful of style/formatting fixes
> - use standard led name ":white:kbd_backlight"
> - rename USAGE_MAGIC_BL to HID_USAGE_MAGIC_BL
> - New patch documenting preferred keyboard backlight names
>
> v1: https://lore.kernel.org/linux-input/7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com/
> v2: https://lore.kernel.org/linux-input/20230216041224.4731-1-orlandoch.dev@gmail.com/
> v3: https://lore.kernel.org/linux-input/20230217102319.3419-1-orlandoch.dev@gmail.com/
> v4: https://lore.kernel.org/linux-input/20230218090709.7467-1-orlandoch.dev@gmail.com/
>
> Orlando Chamberlain (2):
> Documentation: leds: standardise keyboard backlight led names
> HID: hid-apple-magic-backlight: Add driver for keyboard backlight on
> internal Magic Keyboards
>
> Documentation/leds/well-known-leds.txt | 8 ++
> MAINTAINERS | 6 ++
> drivers/hid/Kconfig | 13 +++
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-apple-magic-backlight.c | 120 ++++++++++++++++++++++++
> 5 files changed, 148 insertions(+)
> create mode 100644 drivers/hid/hid-apple-magic-backlight.c
Hi,
thanks for creating the support for backlight.
Is there any reason why not to fold all this into existing hid-apple? I
don't think we need separate driver for the backlist, separated from the
rest of hid-apple support.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: BUG: hid-sensor-ids code includes binary data in device name
From: Philipp Jungkamp @ 2023-03-10 14:35 UTC (permalink / raw)
To: srinivas pandruvada, todd.e.brandt, linux-input, linux-kernel,
Even Xu
Cc: Jonathan.Cameron, jkosina, todd.e.brandt
In-Reply-To: <317ce138f63b9317ac7be1949a68db5117c19b92.camel@linux.intel.com>
Hello,
on v3 of the patchset I had this comment on the 'real_usage'
initialization:
> > - char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> > + char real_usage[HID_SENSOR_USAGE_LENGTH];
> > struct platform_device *custom_pdev;
> > const char *dev_name;
> > char *c;
> >
> > - /* copy real usage id */
> > - memcpy(real_usage, known_sensor_luid[index], 4);
> > + memcpy(real_usage, match->luid, 4);
> > + real_usage[4] = '\0';
>
> Why the change in approach for setting the NULL character?
> Doesn't seem relevant to main purpose of this patch.
Based on the comment, I changed that in the final v4 revision to:
> - char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> + char real_usage[HID_SENSOR_USAGE_LENGTH];
> struct platform_device *custom_pdev;
> const char *dev_name;
> char *c;
>
> - /* copy real usage id */
> - memcpy(real_usage, known_sensor_luid[index], 4);
> + memcpy(real_usage, match->luid, 4);
I ommitted the line adding the null terminator to the string but kept
that I didn't initialize the 'real_usage' as { 0 } anymore. The string
now misses the null terminator which leads to the broken utf-8.
The simple fix is to reintroduce the 0 initialization in
hid_sensor_register_platform_device. E.g.
- char real_usage[HID_SENSOR_USAGE_LENGTH];
+ char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
Where do I need to submit a patch for this? And on which tree should I
base the patch?
I'm sorry for the problems my patch caused.
Regards,
Philipp Jungkamp
On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> +Even
>
> On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > Hi all, I've run into an issue in 6.3.0-rc1 that causes problems
> > with
> > ftrace and I've bisected it to this commit:
> >
> > commit 98c062e8245199fa9121141a0bf1035dc45ae90e (HEAD,
> > refs/bisect/bad)
> > Author: Philipp Jungkamp p.jungkamp@gmx.net
> > Date: Fri Nov 25 00:38:38 2022 +0100
> >
> > HID: hid-sensor-custom: Allow more custom iio sensors
> >
> > The known LUID table for established/known custom HID sensors
> > was
> > limited to sensors with "INTEL" as manufacturer. But some
> > vendors
> > such
> > as Lenovo also include fairly standard iio sensors (e.g.
> > ambient
> > light)
> > in their custom sensors.
> >
> > Expand the known custom sensors table by a tag used for the
> > platform
> > device name and match sensors based on the LUID as well as
> > optionally
> > on model and manufacturer properties.
> >
> > Signed-off-by: Philipp Jungkamp p.jungkamp@gmx.net
> > Reviewed-by: Jonathan Cameron Jonathan.Cameron@huawei.com
> > Acked-by: Srinivas Pandruvada
> > srinivas.pandruvada@linux.intel.com
> > Signed-off-by: Jiri Kosina jkosina@suse.cz
> >
> > You're using raw data as part of the devname in the "real_usage"
> > string, but it includes chars other than ASCII, and those chars end
> > up being printed out in the ftrace log which is meant to be ASCII
> > only.
> >
> > - /* HID-SENSOR-INT-REAL_USAGE_ID */
> > - dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s",
> > real_usage);
> > + /* HID-SENSOR-TAG-REAL_USAGE_ID */
> > + dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> > + match->tag, real_usage);
> >
> > My sleepgraph tool started to crash because it read these lines
> > from
> > ftrace:
> >
> > device_pm_callback_start: platform HID-SENSOR-INT-020b?.39.auto,
> > parent: 001F:8087:0AC2.0003, [suspend]
> > device_pm_callback_end: platform HID-SENSOR-INT-020b?.39.auto,
> > err=0
> >
>
> Here tag is:
> .tag = "INT",
> .luid = "020B000000000000",
>
>
> The LUID is still a string. Probably too long for a dev_name.
>
> Even,
>
> Please check.
>
> Thanks.
> Srinivas
>
>
> > The "HID-SENSOR-INT-020b?.39.auto" string includes a binary char
> > that
> > kills
> > python3 code that loops through an ascii file as such:
> >
> > File "/usr/bin/sleepgraph", line 5579, in executeSuspend
> > for line in fp:
> > File "/usr/lib/python3.10/codecs.py", line 322, in decode
> > (result, consumed) = self._buffer_decode(data, self.errors,
> > final)
> > UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in
> > position
> > 1568: invalid start byte
> >
> > I've updated sleepgraph to handle random non-ascii chars, but other
> > tools
> > may suffer the same fate. Can you rewrite this to ensure that no
> > binary
> > chars make it into the devname?
> >
>
^ permalink raw reply
* Re: [PATCH v4] HID: add KEY_CAMERA_FOCUS event in HID
From: Jiri Kosina @ 2023-03-10 14:21 UTC (permalink / raw)
To: Qi Feng; +Cc: benjamin.tissoires, linux-input, linux-kernel, fengqi
In-Reply-To: <3f8627d20de711d08b8cafe0a11481a2b9ca941e.1676537236.git.fengqi@xiaomi.com>
On Thu, 16 Feb 2023, Qi Feng wrote:
> From: fengqi <fengqi@xiaomi.com>
>
> Our HID device need KEY_CAMERA_FOCUS event to control camera,
> but this event is non-existent in current HID driver.
> So we add this event in hid-input.c.
>
> Signed-off-by: fengqi <fengqi@xiaomi.com>
Applied, thank you.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 0/4] HID: kye: Add support for all kye tablets
From: Jiri Kosina @ 2023-03-10 14:06 UTC (permalink / raw)
To: David Yang; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <20230207043318.23842-1-mmyangfl@gmail.com>
On Tue, 7 Feb 2023, David Yang wrote:
> This series refactor kye tablet descriptor fixup routine, by using a
> template and filling parameters on the fly, and add support for all
> possible kye tablets.
> ---
> v2: fix missing rsize assignment
> v3: fix geometry
> v4: split patches
>
> David Yang (4):
> HID: kye: Rewrite tablet descriptor fixup routine
> HID: kye: Generate tablet fixup descriptors on the fly
> HID: kye: Sort kye devices
> HID: kye: Add support for all kye tablets
>
> drivers/hid/hid-ids.h | 9 +-
> drivers/hid/hid-kye.c | 917 +++++++++++++++++----------------------
> drivers/hid/hid-quirks.c | 14 +-
> 3 files changed, 414 insertions(+), 526 deletions(-)
Now queued in hid.git#for-6.4/kye, thanks David.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v0 0/2] HID: nintendo: avoid BT rumble disconnections
From: Jiri Kosina @ 2023-03-10 14:02 UTC (permalink / raw)
To: Daniel J. Ogorchock
Cc: linux-input, benjamin.tissoires, Roderick.Colenbrander
In-Reply-To: <20230203215119.435091-1-djogorchock@gmail.com>
On Fri, 3 Feb 2023, Daniel J. Ogorchock wrote:
> The hid-nintendo driver has been plagued by an issue where rumble
> command traffic to bluetooth-connected controllers can cause frequent
> power downs of the controllers.
>
> Digging into other pro controller driver implementations, I've not found
> anything that hid-nintendo is doing incorrectly. Some implementations
> seem to be working around the same problem (e.g. libsdl's hidapi
> implementation rate limits rumble commands to avoid the problem).
>
> hid-nintendo already rate limits rumble control packets, but that does
> not solve the problem.
>
> Using btmon output, I've fuond the the disconnections reliably occur
> shortly after the controller's reporting rate become erratic. The
> controller is meant to report input packets roughly every 15ms. Sending
> rumble commands can sometimes result in the input packets arriving in
> bursts/batches. Once the controller and/or BT stack enters this state,
> even halting rumble commands will never allow the reporting rate to
> recover to nominal. The controller will eventually disconnect.
>
> This patch set strives to avoid the problematic scenario. It detects if
> input reports arrive at clearly incorrect deltas. During these times,
> the driver will hold off on transmitting any rumble commands to the
> controller. This approach has allowed the reporting rate to reliably
> recover in my testing. I've not been able to generate a controller
> disconnection during hours of testing games with frequent rumble.
>
> The behavior of this mechanism is tunable using #defines. We may need to
> tweak/tune as the mitigation is used on different hardware setups.
>
> My suspicion is that the core issue is somewhere in the bluez stack. My
> next step is to investigate that lead in more detail. This patchset at
> least allows for use of the controllers via bluetooth with rumble
> enabled without frequently disconnecting.
>
> Daniel J. Ogorchock (2):
> HID: nintendo: prevent rumble queue overruns
> HID: nintendo: fix rumble rate limiter
>
> drivers/hid/hid-nintendo.c | 95 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 86 insertions(+), 9 deletions(-)
Now queued in hid.git#for-6.4/nintendo. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: wacom: insert timestamp to packed Bluetooth (BT) events
From: Jiri Kosina @ 2023-03-10 10:13 UTC (permalink / raw)
To: Ping Cheng; +Cc: linux-input, Ping Cheng, Jason Gerecke
In-Reply-To: <20230224162643.3799-1-ping.cheng@wacom.com>
On Fri, 24 Feb 2023, Ping Cheng wrote:
> To fully utilize the BT polling/refresh rate, a few input events
> are sent together to reduce event delay. This causes issue to the
> timestamp generated by input_sync since all the events in the same
> packet would pretty much have the same timestamp. This patch inserts
> time interval to the events by averaging the total time used for
> sending the packet.
>
> This decision was mainly based on observing the actual time interval
> between each BT polling. The interval doesn't seem to be constant,
> due to the network and system environment. So, using solutions other
> than averaging doesn't end up with valid timestamps.
>
> Signed-off-by: Ping Cheng <ping.cheng@wacom.com>
> Reviewed-by: Jason Gerecke <jason.gerecke@wacom.com>
Applied now, sorry for the delay.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: BUG: hid-sensor-ids code includes binary data in device name
From: srinivas pandruvada @ 2023-03-10 9:51 UTC (permalink / raw)
To: todd.e.brandt, linux-input, linux-kernel, Even Xu
Cc: p.jungkamp, Jonathan.Cameron, jkosina, todd.e.brandt
In-Reply-To: <592bcdcbb3603cf5dfefd09abdd6916db4efc691.camel@linux.intel.com>
+Even
On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> Hi all, I've run into an issue in 6.3.0-rc1 that causes problems with
> ftrace and I've bisected it to this commit:
>
> commit 98c062e8245199fa9121141a0bf1035dc45ae90e (HEAD,
> refs/bisect/bad)
> Author: Philipp Jungkamp p.jungkamp@gmx.net
> Date: Fri Nov 25 00:38:38 2022 +0100
>
> HID: hid-sensor-custom: Allow more custom iio sensors
>
> The known LUID table for established/known custom HID sensors was
> limited to sensors with "INTEL" as manufacturer. But some vendors
> such
> as Lenovo also include fairly standard iio sensors (e.g. ambient
> light)
> in their custom sensors.
>
> Expand the known custom sensors table by a tag used for the
> platform
> device name and match sensors based on the LUID as well as
> optionally
> on model and manufacturer properties.
>
> Signed-off-by: Philipp Jungkamp p.jungkamp@gmx.net
> Reviewed-by: Jonathan Cameron Jonathan.Cameron@huawei.com
> Acked-by: Srinivas Pandruvada srinivas.pandruvada@linux.intel.com
> Signed-off-by: Jiri Kosina jkosina@suse.cz
>
> You're using raw data as part of the devname in the "real_usage"
> string, but it includes chars other than ASCII, and those chars end
> up being printed out in the ftrace log which is meant to be ASCII
> only.
>
> - /* HID-SENSOR-INT-REAL_USAGE_ID */
> - dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s",
> real_usage);
> + /* HID-SENSOR-TAG-REAL_USAGE_ID */
> + dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> + match->tag, real_usage);
>
> My sleepgraph tool started to crash because it read these lines from
> ftrace:
>
> device_pm_callback_start: platform HID-SENSOR-INT-020b?.39.auto,
> parent: 001F:8087:0AC2.0003, [suspend]
> device_pm_callback_end: platform HID-SENSOR-INT-020b?.39.auto, err=0
>
Here tag is:
.tag = "INT",
.luid = "020B000000000000",
The LUID is still a string. Probably too long for a dev_name.
Even,
Please check.
Thanks.
Srinivas
> The "HID-SENSOR-INT-020b?.39.auto" string includes a binary char that
> kills
> python3 code that loops through an ascii file as such:
>
> File "/usr/bin/sleepgraph", line 5579, in executeSuspend
> for line in fp:
> File "/usr/lib/python3.10/codecs.py", line 322, in decode
> (result, consumed) = self._buffer_decode(data, self.errors,
> final)
> UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position
> 1568: invalid start byte
>
> I've updated sleepgraph to handle random non-ascii chars, but other
> tools
> may suffer the same fate. Can you rewrite this to ensure that no
> binary
> chars make it into the devname?
>
^ permalink raw reply
* Re: [PATCH] Input: touchscreen - Add new Novatek NVT-ts driver
From: Jeff LaBundy @ 2023-03-10 2:16 UTC (permalink / raw)
To: Hans de Goede; +Cc: Dmitry Torokhov, linux-input
In-Reply-To: <b5142af9-fa24-ccce-5c06-c21dadd1f394@redhat.com>
Hi Hans,
[...]
> >> + if (width > NVT_TS_MAX_SIZE || height >= NVT_TS_MAX_SIZE ||
> >> + data->max_touches > NVT_TS_MAX_TOUCHES ||
> >> + irq_type >= ARRAY_SIZE(nvt_ts_irq_type) ||
> >> + data->buf[NVT_TS_PARAMS_WAKE_TYPE] != NVT_TS_SUPPORTED_WAKE_TYPE ||
> >> + data->buf[NVT_TS_PARAMS_CHIP_ID] != NVT_TS_SUPPORTED_CHIP_ID) {
> >> + dev_err(dev, "Unsupported touchscreen parameters: %*ph\n",
> >> + NVT_TS_PARAMS_SIZE, data->buf);
> >> + return -EIO;
> >
> > Nit: because there was no I/O error here necessarily, but rather invalid or
> > unacceptable values, I think -EINVAL is better here.
>
> AFAIK -EINVAL is reserved for invalid function parameters, typically
> on a syscall / ioctl. Where as here we are receiving invalid data,
> which is more like a a checksum/crc error which is an IO error.
>
> With that said I have no strong preference either way, so let me know
> if you want me to switch to EINVAL for v2 or not.
Based on this additional information, I agree that -EIO is a better choice. In
theory, only an I/O error could make the device return nonsensical values. The
controller FW is unlikely to store values it cannot support.
[...]
> >> + ret = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
> >> + IRQF_ONESHOT | IRQF_NO_AUTOEN | nvt_ts_irq_type[irq_type],
> >> + client->name, data);
> >
> > Interesting, it seems interrupt polarity is configurable?
>
> On the controller-side, yes. The goodix touchscreens have much the same
> thing.
>
> > For my own
> > understanding, what if there is an inverter on the board?
>
> Then things break I guess since we program the GPIO input IRQs polarity
> to match the controller polarity when then will be wrong.
>
> Luckily this has never happened so far AFAIK (mostly talking goodix
> here, since I know only 1 device with this new touchscreen).
ACK.
> >> + if (ret)
> >> + return dev_err_probe(dev, ret, "requesting irq\n");
> >
> > dev_err_probe() tends not to be accepted in input, the argument being
> > that the callers who can return EPROBE_DEFER be responsible for setting
> > the reason as opposed to every driver calling a separate function that
> > does so.
>
> To me dev_err_probe() is not so much about setting the probe-defer
> reason, it is is very useful because:
>
> 1) It deals with not logging afalse-postivive error msg on -EPROBE_DEFER and
> you can return its return value, leading to much more compact code and
> thus IMOH more readable code
>
> 2) It leads to a consistent format for the printed errors
>
> To illustrate 1. without dev_err_probe() the reset_gpio request error
> handling turns from this:
>
> if (IS_ERR(data->reset_gpio))
> return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n");
>
> into:
>
> if (IS_ERR(data->reset_gpio)) {
> error = PTR_ERR(data->reset_gpio);
> if (error == -EPROBE_DEFER)
> return error;
> dev_err(dev, "Error %d requesting reset GPIO\n", error);
> return error;
> }
>
> Which is 7 lines vs 2 lines when using dev_err_probe() and more
> importantly IMHO the error handling code using using dev_err_probe()
> is just much more readable and thus more maintainable IMHO.
>
> Which is why IMHO using dev_err_probe() for errors getting resources
> is just much better.
>
> So unless you feel really strongly about not using this I would
> prefer to keep using dev_err_probe().
I do not personally feel strongly about this and I think your reasoning is
sound. A quick grep through drivers/ shows it is immensely popular. However
the same grep through drivers/input shows it has yet to be accepted there.
That is the only reason I mention it; I however have no issue with it being
left as-is for v2.
> Once more thank you for the review. If you can clarify what
> you want for the EINVAL vs EIO and for the dev_err_probe()
> review remarks then I'll prepare a v2.
Thank you for the productive discussion as always :)
>
> Regards,
>
> Hans
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* BUG: hid-sensor-ids code includes binary data in device name
From: Todd Brandt @ 2023-03-09 23:33 UTC (permalink / raw)
To: linux-input, linux-kernel
Cc: p.jungkamp, Jonathan.Cameron, srinivas.pandruvada, jkosina,
todd.e.brandt
Hi all, I've run into an issue in 6.3.0-rc1 that causes problems with
ftrace and I've bisected it to this commit:
commit 98c062e8245199fa9121141a0bf1035dc45ae90e (HEAD, refs/bisect/bad)
Author: Philipp Jungkamp p.jungkamp@gmx.net
Date: Fri Nov 25 00:38:38 2022 +0100
HID: hid-sensor-custom: Allow more custom iio sensors
The known LUID table for established/known custom HID sensors was
limited to sensors with "INTEL" as manufacturer. But some vendors
such
as Lenovo also include fairly standard iio sensors (e.g. ambient
light)
in their custom sensors.
Expand the known custom sensors table by a tag used for the
platform
device name and match sensors based on the LUID as well as
optionally
on model and manufacturer properties.
Signed-off-by: Philipp Jungkamp p.jungkamp@gmx.net
Reviewed-by: Jonathan Cameron Jonathan.Cameron@huawei.com
Acked-by: Srinivas Pandruvada srinivas.pandruvada@linux.intel.com
Signed-off-by: Jiri Kosina jkosina@suse.cz
You're using raw data as part of the devname in the "real_usage"
string, but it includes chars other than ASCII, and those chars end
up being printed out in the ftrace log which is meant to be ASCII only.
- /* HID-SENSOR-INT-REAL_USAGE_ID */
- dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s",
real_usage);
+ /* HID-SENSOR-TAG-REAL_USAGE_ID */
+ dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
+ match->tag, real_usage);
My sleepgraph tool started to crash because it read these lines from
ftrace:
device_pm_callback_start: platform HID-SENSOR-INT-020b?.39.auto,
parent: 001F:8087:0AC2.0003, [suspend]
device_pm_callback_end: platform HID-SENSOR-INT-020b?.39.auto, err=0
The "HID-SENSOR-INT-020b?.39.auto" string includes a binary char that
kills
python3 code that loops through an ascii file as such:
File "/usr/bin/sleepgraph", line 5579, in executeSuspend
for line in fp:
File "/usr/lib/python3.10/codecs.py", line 322, in decode
(result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position
1568: invalid start byte
I've updated sleepgraph to handle random non-ascii chars, but other
tools
may suffer the same fate. Can you rewrite this to ensure that no binary
chars make it into the devname?
^ permalink raw reply
* Re: [PATCH v2] Input: xen-kbdfront - drop keys to shrink modalias
From: Mattijs Korpershoek @ 2023-03-09 16:09 UTC (permalink / raw)
To: Jason Andryuk
Cc: linux-kernel, xen-devel, Phillip Susi, stable, Dmitry Torokhov,
linux-input
In-Reply-To: <CAKf6xpsGy7rjK3Rkosevr3dD+64-WqCEzAecBbHEHpBMeAHh7A@mail.gmail.com>
Hi Jason,
On mer., mars 08, 2023 at 11:26, Jason Andryuk <jandryuk@gmail.com> wrote:
> On Thu, Dec 15, 2022 at 8:54 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> On Fri, Dec 09, 2022 at 09:26, Jason Andryuk <jandryuk@gmail.com> wrote:
>>
>> > xen kbdfront registers itself as being able to deliver *any* key since
>> > it doesn't know what keys the backend may produce.
>> >
>> > Unfortunately, the generated modalias gets too large and uevent creation
>> > fails with -ENOMEM.
>> >
>> > This can lead to gdm not using the keyboard since there is no seat
>> > associated [1] and the debian installer crashing [2].
>> >
>> > Trim the ranges of key capabilities by removing some BTN_* ranges.
>> > While doing this, some neighboring undefined ranges are removed to trim
>> > it further.
>> >
>> > An upper limit of KEY_KBD_LCD_MENU5 is still too large. Use an upper
>> > limit of KEY_BRIGHTNESS_MENU.
>> >
>> > This removes:
>> > BTN_DPAD_UP(0x220)..BTN_DPAD_RIGHT(0x223)
>> > Empty space 0x224..0x229
>> >
>> > Empty space 0x28a..0x28f
>> > KEY_MACRO1(0x290)..KEY_MACRO30(0x2ad)
>> > KEY_MACRO_RECORD_START 0x2b0
>> > KEY_MACRO_RECORD_STOP 0x2b1
>> > KEY_MACRO_PRESET_CYCLE 0x2b2
>> > KEY_MACRO_PRESET1(0x2b3)..KEY_MACRO_PRESET3(0xb5)
>> > Empty space 0x2b6..0x2b7
>> > KEY_KBD_LCD_MENU1(0x2b8)..KEY_KBD_LCD_MENU5(0x2bc)
>> > Empty space 0x2bd..0x2bf
>> > BTN_TRIGGER_HAPPY(0x2c0)..BTN_TRIGGER_HAPPY40(0x2e7)
>> > Empty space 0x2e8..0x2ff
>> >
>> > The modalias shrinks from 2082 to 1550 bytes.
>> >
>> > A chunk of keys need to be removed to allow the keyboard to be used.
>> > This may break some functionality, but the hope is these macro keys are
>> > uncommon and don't affect any users.
>> >
>> > [1] https://github.com/systemd/systemd/issues/22944
>> > [2] https://lore.kernel.org/xen-devel/87o8dw52jc.fsf@vps.thesusis.net/T/
>> >
>> > Cc: Phillip Susi <phill@thesusis.net>
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
> Thank you, Mattjis.
>
> Any other thoughts? Can this patch be applied?
That's not up to to decide, Dmitry might pick this up or give you a
review whenever he has time.
>
> Thanks,
> Jason
^ permalink raw reply
* Re: [PATCH v2] Add rumble support to latest xbox controllers
From: Benjamin Tissoires @ 2023-03-09 14:56 UTC (permalink / raw)
To: Bastien Nocera; +Cc: Siarhei Vishniakou, Jiri Kosina, linux-input, linux-kernel
In-Reply-To: <785eae0426a82ce5bc6891482b54d2629f5558aa.camel@hadess.net>
On Wed, Mar 8, 2023 at 7:54 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Wed, 2023-03-08 at 09:55 -0800, Siarhei Vishniakou wrote:
> > Thanks Bastien!
> >
> > I can definitely add a link to the wikipedia page.
> >
> > Are you talking about adding the link to the commit message, or to
> > hid-ids.h ?
>
> I think the commit message would be enough so we know which device
> we're talking about, but now that you mention it, maybe the source code
> would be a good idea too.
Agree, adding the link in the source would be fine.
>
> As far as I've understood, and Benjamin can correct me, we don't need
> to have IDs be in hid-ids.h because we don't need to declare them both
> as a blocklist in the hid core, and then again in the driver itself.
We don't need to have the blocklist anymore, but using hid-ids.h is
still used, and sometimes has the benefit of raising eyebrows when you
add support for a new device and realize that it was already defined.
So keeping the list around is not so much of a bad thing.
>
> My take is that the patches could move the IDs from hid-ids.h to hid-
> microsoft.c as they're changed. You then wouldn't need the macros, just
> add a comment for each of variants, and that Wikipedia article can be
> linked above the whole XBox controller section.
We are definitely in the bikeshedding phase, but I would leave the
code as it is in this patch :)
One more comment below:
>
> What do you think?
>
> >
> >
> > On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > On Tue, 2023-03-07 at 13:35 -0800, Siarhei Vishniakou wrote:
> > > > Currently, rumble is only supported via bluetooth on a single
> > > > xbox
> > > > controller, called 'model 1708'. On the back of the device, it's
> > > > named
> > > > 'wireless controller for xbox one'. However, in 2021, Microsoft
> > > > released
> > > > a firmware update for this controller. As part of this update,
> > > > the
> > > > HID
> > > > descriptor of the device changed. The product ID was also changed
> > > > from
> > > > 0x02fd to 0x0b20. On this controller, rumble was supported via
> > > > hid-microsoft, which matched against the old product id (0x02fd).
> > > > As
> > > > a
> > > > result, the firmware update broke rumble support on this
> > > > controller.
> > > >
> > > > The hid-microsoft driver actually supports rumble on the new
> > > > firmware,
> > > > as well. So simply adding new product id is sufficient to bring
> > > > back
> > > > this support.
> > > >
> > > > After discussing further with the xbox team, it was pointed out
> > > > that
> > > > another xbox controller, xbox elite series 2, can be supported in
> > > > a
> > > > similar way.
> > > >
> > > > Add rumble support for all of these devices in this patch. Two of
> > > > the
> > > > devices have received firmware updates that caused their product
> > > > id's
> > > > to
> > > > change. Both old and new firmware versions of these devices were
> > > > tested.
> > > >
> > > > The tested controllers are:
> > > >
> > > > 1. 'wireless controller for xbox one', model 1708
> > > > 2. 'xbox wireless controller', model 1914. This is also sometimes
> > > > referred to as 'xbox series S|X'.
> > > > 3. 'elite series 2', model 1797.
> > > >
> > > > The tested configurations are:
> > > > 1. model 1708, pid 0x02fd (old firmware)
> > > > 2. model 1708, pid 0x0b20 (new firmware)
> > > > 3. model 1914, pid 0x0b13
> > > > 4. model 1797, pid 0x0b05 (old firmware)
> > > > 5. model 1797, pid 0x0b22 (new firmware)
> > > >
> > > > I verified rumble support on both bluetooth and usb.
> > >
> > > Looks good although I would personally have preferred separate
> > > patches
> > > for each controller.
> > >
> > > Reviewed-by: Bastien Nocera <hadess@hadess.net>
> > >
> > > Would a link to:
> > > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary
> > > also be useful to make which model is which clearer in the minds of
> > > future readers?
> > >
> > > Cheers
> > >
> > > >
> > > > Signed-off-by: Siarhei Vishniakou <svv@google.com>
> > > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff
That change-id should be dropped, it has no meaning to us.
Cheers,
Benjamin
> > > > ---
> > > > drivers/hid/hid-ids.h | 6 +++++-
> > > > drivers/hid/hid-microsoft.c | 11 ++++++++++-
> > > > 2 files changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > index 053853a891c5..c9b75f8ba49a 100644
> > > > --- a/drivers/hid/hid-ids.h
> > > > +++ b/drivers/hid/hid-ids.h
> > > > @@ -903,7 +903,11 @@
> > > > #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
> > > > #define USB_DEVICE_ID_MS_POWER_COVER 0x07da
> > > > #define USB_DEVICE_ID_MS_SURFACE3_COVER 0x07de
> > > > -#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708 0x02fd
> > > > +#define
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE 0x0b20
> > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914 0x0b13
> > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797 0x0b05
> > > > +#define
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE 0x0b22
> > > > #define USB_DEVICE_ID_MS_PIXART_MOUSE 0x00cb
> > > > #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS 0x02e0
> > > >
> > > > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-
> > > > microsoft.c
> > > > index 071fd093a5f4..9345e2bfd56e 100644
> > > > --- a/drivers/hid/hid-microsoft.c
> > > > +++ b/drivers/hid/hid-microsoft.c
> > > > @@ -446,7 +446,16 @@ static const struct hid_device_id
> > > > ms_devices[] =
> > > > {
> > > > .driver_data = MS_PRESENTER },
> > > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, 0x091B),
> > > > .driver_data = MS_SURFACE_DIAL },
> > > > - { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER),
> > > > +
> > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
> > > > + .driver_data = MS_QUIRK_FF },
> > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
> > > > + .driver_data = MS_QUIRK_FF },
> > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
> > > > + .driver_data = MS_QUIRK_FF },
> > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
> > > > + .driver_data = MS_QUIRK_FF },
> > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
> > > > .driver_data = MS_QUIRK_FF },
> > > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
> > > > .driver_data = MS_QUIRK_FF },
> > >
>
^ permalink raw reply
* Re: [PATCH] HID: hidraw: Keep the report ID on buffer in get_report
From: Benjamin Tissoires @ 2023-03-09 14:44 UTC (permalink / raw)
To: Antoine C; +Cc: Jiri Kosina, linux-input@vger.kernel.org
In-Reply-To: <38F34842-3087-43CB-B814-CDBC52FD2084@getmailspring.com>
On Thu, Mar 9, 2023 at 12:36 PM Antoine C <contact@antoinecolombier.fr> wrote:
>
> The ioctl syscall with arg HIDIOCGINPUT must not override
> the report ID contained in the first byte of the buffer
> and should offset the report data next to it.
>
> Signed-off-by: Antoine Colombier <kernel@acolombier.dev>
> ---
> Hello,
>
> Apologies for the resend, I forgot to disable the HTML format on the
> previous email. Please ignore the previous one.
>
> This addresses the bug report in the hidapi: https://github.com/libusb/hidapi/issues/514
> The patch was tested using the test snippets attached in the issue above
> on 6.2.0-76060200-generic (PopOS 22.04)
The problem is that hidapi is not the sole user of hidraw, and this is
a breaking change for everyone.
If we were to accept this, when hidraw has always been that way on
linux since 2011 when it was introduced, you can be sure that there
are going to be very angry users who suddenly have a failure when
retrieving the input/feature report.
So if hidapi expects the first byte to stay the same, just fix that
case when calling that hidraw ioctl in hidapi.
A possible solution would be to add a new ioctl with a "better"
behavior, but a new ioctl will actually be worse because you have to
update both the kernel *and* hidapi to make use of the new ioctl, at
which point, just fixing userspace is actually simpler and better.
Cheers,
Benjamin
>
> drivers/hid/hidraw.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 197b1e7bf029..2c12f25817e6 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -231,9 +231,10 @@ static ssize_t hidraw_get_report(struct file *file,
> char __user *buffer, size_t
> if (ret < 0)
> goto out_free;
>
> + count--;
> len = (ret < count) ? ret : count;
>
> - if (copy_to_user(buffer, buf, len)) {
> + if (copy_to_user(buffer + 1, buf, len)) {
> ret = -EFAULT;
> goto out_free;
> }
>
^ permalink raw reply
* Re: [PATCH v3 1/6] HID: logitech-hidpp: Simplify array length check
From: Greg Kroah-Hartman @ 2023-03-09 14:18 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230302105555.51417-1-hadess@hadess.net>
On Thu, Mar 02, 2023 at 11:55:50AM +0100, Bastien Nocera wrote:
> Use the compiler to force a 100-length array, rather than check the
> length after the fact.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> New in v2, following a review comment in the 1f20 enablement patch.
> No changes in v3.
>
> drivers/hid/hid-logitech-hidpp.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
These all look good to me, and probably should go through the HID tree,
so I've acked the USB-specific ones to allow that.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 4/6] USB: core: Add wireless_status sysfs attribute
From: Greg Kroah-Hartman @ 2023-03-09 14:17 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230302105555.51417-4-hadess@hadess.net>
On Thu, Mar 02, 2023 at 11:55:53AM +0100, Bastien Nocera wrote:
> Add a wireless_status sysfs attribute to USB devices to keep track of
> whether a USB device that's comprised of a receiver dongle and an emitter
> device over a, most of the time proprietary, wireless link has its emitter
> connected or disconnected.
>
> This will be used by user-space OS components to determine whether the
> battery-powered part of the device is wirelessly connected or not,
> allowing, for example:
> - upower to hide the battery for devices where the device is turned off
> but the receiver plugged in, rather than showing 0%, or other values
> that could be confusing to users
> - Pipewire to hide a headset from the list of possible inputs or outputs
> or route audio appropriately if the headset is suddenly turned off, or
> turned on
> - libinput to determine whether a keyboard or mouse is present when its
> receiver is plugged in.
>
> This is done at the USB interface level as:
> - the interface on which the wireless status is detected is sometimes
> not the same as where it could be consumed (eg. the audio interface
> on a headset dongle will still appear even if the headset is turned
> off), and we cannot have synchronisation of status across subsystems.
> - this behaviour is not specific to HID devices, even if the protocols
> used to determine whether or not the remote device is connected can
> be HID.
>
> This is not an attribute that is meant to replace protocol specific
> APIs, such as the ones available for WWAN, WLAN/Wi-Fi, or Bluetooth
> or any other sort of networking, but solely for wireless devices with
> an ad-hoc “lose it and your device is e-waste” receiver dongle.
>
> The USB interface will only be exporting the wireless_status sysfs
> attribute if it gets set through the API exported in the next commit.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> Updated commit message and documentation in v2 so that the commit
> doesn't need to reference older discussions.
>
> Trimmed the width of the sysfs docs in v3.
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: [PATCH v3 5/6] USB: core: Add API to change the wireless_status
From: Greg Kroah-Hartman @ 2023-03-09 14:17 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-usb, linux-input, Alan Stern, Benjamin Tissoires,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230302105555.51417-5-hadess@hadess.net>
On Thu, Mar 02, 2023 at 11:55:54AM +0100, Bastien Nocera wrote:
> This adds the API that allows device specific drivers to tell user-space
> about whether the wireless device is connected to its receiver dongle.
>
> See "USB: core: Add wireless_status sysfs attribute" for a detailed
> explanation of what this attribute should be used for.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> Fixed locking/use-after-free in v2, thanks to Alan Stern
>
> Fixed ordering of locking/put in v3, thanks to Alan Stern again
>
> drivers/usb/core/message.c | 40 ++++++++++++++++++++++++++++++++++++++
> include/linux/usb.h | 5 +++++
> 2 files changed, 45 insertions(+)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support
From: Andy Shevchenko @ 2023-03-09 13:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Daniel Kaehn, Hans de Goede, robh+dt, krzysztof.kozlowski+dt,
jikos, bartosz.golaszewski, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <20230309093811.se7exh6eocdgx5jv@mail.corp.redhat.com>
On Thu, Mar 09, 2023 at 10:38:11AM +0100, Benjamin Tissoires wrote:
> On Mar 08 2023, Andy Shevchenko wrote:
...
> Looks like gpiolib-acpi.c doesn't care about fwnode at all.
>
> if I do the following:
>
> ---
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index d8a421ce26a8..5aebc266426b 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
>
> static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> {
> - return gc->parent && device_match_acpi_handle(gc->parent, data);
> + return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
> }
>
> /**
> ---
This seems a legit fix.
> I can now directly reference the GPIO ACPI node in my GpioInt()
> declaration. And AFAICT this should be safe to do because gpiolib ensure
> that gc->fwnode is set, using the one from the parent if it is not set
> previously.
>
> I need to check if this works with my icelake laptop, and if so I'll
> send it to the list.
>
> The reason the intel gpios are working (the only one I checked) is
> because the \\SB.GPI0 node refers to the pinctrl controller (driver
> pinctrl-icelake.c in my case, which then creates a subdevice for
> handling the gpio).
Good catch!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] Input: touchscreen - Add new Novatek NVT-ts driver
From: Hans de Goede @ 2023-03-09 13:04 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: Dmitry Torokhov, linux-input
In-Reply-To: <Y/Q84gI1AnE6oqRU@nixie71>
Hi Jeff,
On 2/21/23 04:39, Jeff LaBundy wrote:
> Hi Hans,
>
> On Fri, Feb 17, 2023 at 04:07:49PM +0100, Hans de Goede wrote:
>> Add a new driver for the Novatek i2c touchscreen controller as found
>> on the Acer Iconia One 7 B1-750 tablet. Unfortunately the touchscreen
>> controller model-number is unknown. Even with the tablet opened up it
>> is impossible to read the model-number.
>>
>> Android calls this a "NVT-ts" touchscreen, but that may apply to other
>> Novatek controller models too.
>>
>> This appears to be the same controller as the one supported by
>> https://github.com/advx9600/android/blob/master/touchscreen/NVTtouch_Android4.0/NVTtouch.c
>> but unfortunately that does not give us a model-number either.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> This is a great driver; I have only a few comments below.
Thanks and thank you for the review!
>> ---
>> MAINTAINERS | 6 +
>> drivers/input/touchscreen/Kconfig | 10 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/novatek-nvt-ts.c | 288 +++++++++++++++++++++
>> 4 files changed, 305 insertions(+)
>> create mode 100644 drivers/input/touchscreen/novatek-nvt-ts.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 60c0ded06e3f..0c051a973e6b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14835,6 +14835,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git
>> F: tools/include/nolibc/
>> F: tools/testing/selftests/nolibc/
>>
>> +NOVATEK NVT-TS I2C TOUCHSCREEN DRIVER
>> +M: Hans de Goede <hdegoede@redhat.com>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: drivers/input/touchscreen/novatek-nvt-ts.c
>> +
>> NSDEPS
>> M: Matthias Maennich <maennich@google.com>
>> S: Maintained
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 68d99a112e14..59ca8bfe9a95 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -666,6 +666,16 @@ config TOUCHSCREEN_MTOUCH
>> To compile this driver as a module, choose M here: the
>> module will be called mtouch.
>>
>> +config TOUCHSCREEN_NOVATEK_NVT_TS
>> + tristate "Novatek NVT-ts touchscreen support"
>> + depends on I2C
>> + help
>> + Say Y here if you have a Novatek NVT-ts touchscreen.
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called novatek-nvt-ts.
>> +
>> config TOUCHSCREEN_IMAGIS
>> tristate "Imagis touchscreen support"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 4968c370479a..41654239f89c 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -67,6 +67,7 @@ obj-$(CONFIG_TOUCHSCREEN_MMS114) += mms114.o
>> obj-$(CONFIG_TOUCHSCREEN_MSG2638) += msg2638.o
>> obj-$(CONFIG_TOUCHSCREEN_MTOUCH) += mtouch.o
>> obj-$(CONFIG_TOUCHSCREEN_MK712) += mk712.o
>> +obj-$(CONFIG_TOUCHSCREEN_NOVATEK_NVT_TS) += novatek-nvt-ts.o
>> obj-$(CONFIG_TOUCHSCREEN_HP600) += hp680_ts_input.o
>> obj-$(CONFIG_TOUCHSCREEN_HP7XX) += jornada720_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_IPAQ_MICRO) += ipaq-micro-ts.o
>> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
>> new file mode 100644
>> index 000000000000..2464c758ca14
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Driver for Novatek i2c touchscreen controller as found on
>> + * the Acer Iconia One 7 B1-750 tablet. The Touchscreen controller
>> + * model-number is unknown. Android calls this a "NVT-ts" touchscreen,
>> + * but that may apply to other Novatek controller models too.
>> + *
>> + * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/i2c.h>
>> +#include <linux/input.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/module.h>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#define NVT_TS_TOUCH_START 0x00
>> +#define NVT_TS_TOUCH_SIZE 6
>> +
>> +#define NVT_TS_PARAMETERS_START 0x78
>> +/* These are offsets from NVT_TS_PARAMETERS_START */
>> +#define NVT_TS_PARAMS_WIDTH 0x04
>> +#define NVT_TS_PARAMS_HEIGHT 0x06
>> +#define NVT_TS_PARAMS_MAX_TOUCH 0x09
>> +#define NVT_TS_PARAMS_MAX_BUTTONS 0x0a
>> +#define NVT_TS_PARAMS_IRQ_TYPE 0x0b
>> +#define NVT_TS_PARAMS_WAKE_TYPE 0x0c
>> +#define NVT_TS_PARAMS_CHIP_ID 0x0e
>> +#define NVT_TS_PARAMS_SIZE 0x0f
>> +
>> +#define NVT_TS_SUPPORTED_WAKE_TYPE 0x05
>> +#define NVT_TS_SUPPORTED_CHIP_ID 0x05
>> +
>> +#define NVT_TS_MAX_TOUCHES 10
>> +#define NVT_TS_MAX_SIZE 4096
>> +
>> +#define NVT_TS_TOUCH_NEW 1
>> +#define NVT_TS_TOUCH_UPDATE 2
>> +#define NVT_TS_TOUCH_RELEASE 3
>> +
>> +static const int nvt_ts_irq_type[4] = {
>> + IRQF_TRIGGER_RISING,
>> + IRQF_TRIGGER_FALLING,
>> + IRQF_TRIGGER_LOW,
>> + IRQF_TRIGGER_HIGH
>> +};
>> +
>> +struct nvt_ts_data {
>> + struct i2c_client *client;
>> + struct input_dev *input;
>> + struct gpio_desc *reset_gpio;
>> + struct touchscreen_properties prop;
>> + int max_touches;
>> + u8 buf[NVT_TS_TOUCH_SIZE * NVT_TS_MAX_TOUCHES];
>> +};
>> +
>> +static int nvt_ts_read_data(struct i2c_client *client, u8 reg, u8 *data, int count)
>> +{
>> + struct i2c_msg msg[2] = {
>> + {
>> + .addr = client->addr,
>> + .len = 1,
>> + .buf = ®
>
> Nit: there is no trailing comma here, yet one trails 'buf' below.
Ack, I'll add the trailing comma here too.
>> + },
>> + {
>> + .addr = client->addr,
>> + .flags = I2C_M_RD,
>> + .len = count,
>> + .buf = data,
>> + }
>> + };
>> + int ret;
>> +
>> + ret = i2c_transfer(client->adapter, msg, 2);
>> + if (ret != 2) {
>> + dev_err(&client->dev, "Error reading from 0x%02x: %d\n", reg, ret);
>> + return (ret < 0) ? ret : -EIO;
>> + }
>
> This is idiomatic, but I feel it is clearer to write ARRAY_SIZE(msg) instead
> of 2 throughout; this way the length is hard-coded only once.
Sure, will update for v2.
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t nvt_ts_irq(int irq, void *dev_id)
>> +{
>> + struct nvt_ts_data *data = dev_id;
>> + struct device *dev = &data->client->dev;
>> + int i, ret, slot, x, y;
>
> In input, return values for functions that only return zero on success tend to
> be named 'error'.
Ack, will update for v2.
>> + bool active;
>> + u8 *touch;
>> +
>> + ret = nvt_ts_read_data(data->client, NVT_TS_TOUCH_START, data->buf,
>> + data->max_touches * NVT_TS_TOUCH_SIZE);
>> + if (ret)
>> + return IRQ_HANDLED;
>> +
>> + for (i = 0; i < data->max_touches; i++) {
>> + touch = &data->buf[i * NVT_TS_TOUCH_SIZE];
>> +
>> + /* 0xff means no touch */
>> + if (touch[0] == 0xff)
>> + continue;
>> +
>> + slot = touch[0] >> 3;
>> + if (slot < 1 || slot > data->max_touches) {
>> + dev_warn(dev, "slot %d out of range, ignoring\n", slot);
>> + continue;
>> + }
>> +
>> + switch (touch[0] & 7) {
>
> With all other fields and values defined so nicely, it seems most clear to
> also define the bit field name in this case.
Ack, I'll add a define for this.
>
>> + case NVT_TS_TOUCH_NEW:
>> + case NVT_TS_TOUCH_UPDATE:
>> + active = true;
>> + break;
>> + case NVT_TS_TOUCH_RELEASE:
>> + active = false;
>> + break;
>> + default:
>> + dev_warn(dev, "slot %d unknown state %d\n", slot, touch[0] & 7);
>> + continue;
>> + }
>> +
>> + slot--;
>> + x = (touch[1] << 4) | (touch[3] >> 4);
>> + y = (touch[2] << 4) | (touch[3] & 0x0f);
>> +
>> + input_mt_slot(data->input, slot);
>> + input_mt_report_slot_state(data->input, MT_TOOL_FINGER, active);
>> + touchscreen_report_pos(data->input, &data->prop, x, y, true);
>> + }
>> +
>> + input_mt_sync_frame(data->input);
>> + input_sync(data->input);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int nvt_ts_start(struct input_dev *dev)
>> +{
>> + struct nvt_ts_data *data = input_get_drvdata(dev);
>> +
>> + enable_irq(data->client->irq);
>> + gpiod_set_value_cansleep(data->reset_gpio, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static void nvt_ts_stop(struct input_dev *dev)
>> +{
>> + struct nvt_ts_data *data = input_get_drvdata(dev);
>> +
>> + disable_irq(data->client->irq);
>> + gpiod_set_value_cansleep(data->reset_gpio, 1);
>> +}
>> +
>> +static int nvt_ts_suspend(struct device *dev)
>> +{
>> + struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> + mutex_lock(&data->input->mutex);
>> + if (input_device_enabled(data->input))
>> + nvt_ts_stop(data->input);
>> + mutex_unlock(&data->input->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int nvt_ts_resume(struct device *dev)
>> +{
>> + struct nvt_ts_data *data = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> + mutex_lock(&data->input->mutex);
>> + if (input_device_enabled(data->input))
>> + nvt_ts_start(data->input);
>> + mutex_unlock(&data->input->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(nvt_ts_pm_ops, nvt_ts_suspend, nvt_ts_resume);
>> +
>> +static int nvt_ts_probe(struct i2c_client *client)
>> +{
>> + struct device *dev = &client->dev;
>> + int ret, width, height, irq_type;
>> + struct nvt_ts_data *data;
>> + struct input_dev *input;
>> +
>> + if (!client->irq) {
>> + dev_err(dev, "Error no irq specified\n");
>> + return -EINVAL;
>> + }
>> +
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->client = client;
>> + i2c_set_clientdata(client, data);
>> +
>> + data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(data->reset_gpio))
>> + return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n");
>> +
>> + /* Wait for controller to come out of reset before params read */
>> + msleep(100);
>> + ret = nvt_ts_read_data(data->client, NVT_TS_PARAMETERS_START, data->buf,
>> + NVT_TS_PARAMS_SIZE);
>> + gpiod_set_value_cansleep(data->reset_gpio, 1); /* Put back in reset */
>> + if (ret)
>> + return ret;
>> +
>> + width = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_WIDTH]);
>> + height = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_HEIGHT]);
>> + data->max_touches = data->buf[NVT_TS_PARAMS_MAX_TOUCH];
>> + irq_type = data->buf[NVT_TS_PARAMS_IRQ_TYPE];
>> +
>> + if (width > NVT_TS_MAX_SIZE || height >= NVT_TS_MAX_SIZE ||
>> + data->max_touches > NVT_TS_MAX_TOUCHES ||
>> + irq_type >= ARRAY_SIZE(nvt_ts_irq_type) ||
>> + data->buf[NVT_TS_PARAMS_WAKE_TYPE] != NVT_TS_SUPPORTED_WAKE_TYPE ||
>> + data->buf[NVT_TS_PARAMS_CHIP_ID] != NVT_TS_SUPPORTED_CHIP_ID) {
>> + dev_err(dev, "Unsupported touchscreen parameters: %*ph\n",
>> + NVT_TS_PARAMS_SIZE, data->buf);
>> + return -EIO;
>
> Nit: because there was no I/O error here necessarily, but rather invalid or
> unacceptable values, I think -EINVAL is better here.
AFAIK -EINVAL is reserved for invalid function parameters, typically
on a syscall / ioctl. Where as here we are receiving invalid data,
which is more like a a checksum/crc error which is an IO error.
With that said I have no strong preference either way, so let me know
if you want me to switch to EINVAL for v2 or not.
>
>> + }
>> +
>> + dev_info(dev, "Detected %dx%d touchscreen with %d max touches\n",
>> + width, height, data->max_touches);
>
> This is also idiomatic, but this seems better as dev_dbg.
Ack, this was mostly useful during development, I'll change this
to dev_dbg() for v2.
>
>> +
>> + if (data->buf[NVT_TS_PARAMS_MAX_BUTTONS])
>> + dev_warn(dev, "Touchscreen buttons are not supported\n");
>> +
>> + input = devm_input_allocate_device(dev);
>> + if (!input)
>> + return -ENOMEM;
>> +
>> + input->name = client->name;
>> + input->id.bustype = BUS_I2C;
>> + input->open = nvt_ts_start;
>> + input->close = nvt_ts_stop;
>> + input->dev.parent = dev;
>
> devm_input_allocate_device() already sets the parent for us.
Ah, nice. I'll drop this line then.
>
>> +
>> + input_set_abs_params(input, ABS_MT_POSITION_X, 0, width - 1, 0, 0);
>> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0, height - 1, 0, 0);
>> + touchscreen_parse_properties(input, true, &data->prop);
>> +
>> + ret = input_mt_init_slots(input, data->max_touches,
>> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
>> + if (ret)
>> + return ret;
>> +
>> + data->input = input;
>> + input_set_drvdata(input, data);
>> +
>> + ret = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
>> + IRQF_ONESHOT | IRQF_NO_AUTOEN | nvt_ts_irq_type[irq_type],
>> + client->name, data);
>
> Interesting, it seems interrupt polarity is configurable?
On the controller-side, yes. The goodix touchscreens have much the same
thing.
> For my own
> understanding, what if there is an inverter on the board?
Then things break I guess since we program the GPIO input IRQs polarity
to match the controller polarity when then will be wrong.
Luckily this has never happened so far AFAIK (mostly talking goodix
here, since I know only 1 device with this new touchscreen).
> Is the
> expectation that the customer reprograms the controller's firmware?
>
>> + if (ret)
>> + return dev_err_probe(dev, ret, "requesting irq\n");
>
> dev_err_probe() tends not to be accepted in input, the argument being
> that the callers who can return EPROBE_DEFER be responsible for setting
> the reason as opposed to every driver calling a separate function that
> does so.
To me dev_err_probe() is not so much about setting the probe-defer
reason, it is is very useful because:
1) It deals with not logging afalse-postivive error msg on -EPROBE_DEFER and
you can return its return value, leading to much more compact code and
thus IMOH more readable code
2) It leads to a consistent format for the printed errors
To illustrate 1. without dev_err_probe() the reset_gpio request error
handling turns from this:
if (IS_ERR(data->reset_gpio))
return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n");
into:
if (IS_ERR(data->reset_gpio)) {
error = PTR_ERR(data->reset_gpio);
if (error == -EPROBE_DEFER)
return error;
dev_err(dev, "Error %d requesting reset GPIO\n", error);
return error;
}
Which is 7 lines vs 2 lines when using dev_err_probe() and more
importantly IMHO the error handling code using using dev_err_probe()
is just much more readable and thus more maintainable IMHO.
Which is why IMHO using dev_err_probe() for errors getting resources
is just much better.
So unless you feel really strongly about not using this I would
prefer to keep using dev_err_probe().
>> +
>> + return input_register_device(input);
>> +}
>> +
>> +static const struct i2c_device_id nvt_ts_i2c_id[] = {
>> + { "NVT-ts" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, nvt_ts_i2c_id);
>> +
>> +static struct i2c_driver nvt_ts_driver = {
>> + .driver = {
>> + .name = "novatek-nvt-ts",
>> + .pm = &nvt_ts_pm_ops,
>
> I believe we need pm_sleep_ptr() here now.
Ack, will fix for v2.
>> + },
>> + .probe_new = nvt_ts_probe,
>> + .id_table = nvt_ts_i2c_id,
>> +};
>> +
>> +module_i2c_driver(nvt_ts_driver);
>> +
>> +MODULE_DESCRIPTION("Novatek NVT-ts touchscreen driver");
>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.39.1
>>
Once more thank you for the review. If you can clarify what
you want for the EINVAL vs EIO and for the dev_err_probe()
review remarks then I'll prepare a v2.
Regards,
Hans
^ 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