| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<MW4PR03MB66514AEC3646E36DDA5DA732A354A@MW4PR03MB6651.namprd03.prod.outlook.com>
Date: Mon, 14 Jul 2025 03:17:34 +0000
From: Marge Yang <Marge.Yang@...synaptics.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>, Marge Yang
<Marge.Yang@...synaptics.com>
CC: "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Vincent Huang
<Vincent.huang@...synaptics.com>, David Chiu <David.Chiu@...synaptics.com>,
Derek Cheng <derek.cheng@...synaptics.com>, Sam Tsai <Sam.Tsai@...aptics.com>
Subject: RE: [PATCH V1] Input: synaptics-rmi4- Add a new feature for Forcepad.
Hi Dmitry,
Update the status.
-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
Sent: Sunday, June 29, 2025 1:01 PM
To: Marge Yang <Marge.Yang@...synaptics.com>
Cc: linux-input@...r.kernel.org; linux-kernel@...r.kernel.org; Vincent Huang <Vincent.huang@...synaptics.com>; David Chiu <David.Chiu@...synaptics.com>; Derek Cheng <derek.cheng@...synaptics.com>; Sam Tsai <Sam.Tsai@...aptics.com>
Subject: Re: [PATCH V1] Input: synaptics-rmi4- Add a new feature for Forcepad.
CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi Marge,
On Thu, Jun 19, 2025 at 11:25:00AM +0000, Marge Yang wrote:
> From: Marge Yang <Marge.Yang@...synaptics.com>
>
> Forcepad devices will use F21, for click simulation due to lack of a
> metal button, so we add F21 support to make forcepad support click
> function.
I guess with this we can remove the blacklist we have for forcepads in drivers/input/mouse/synaptics.c?
[Marge 07/14]
The current version's detection method does not support the F21 design from 12 years ago.
I will discuss with the Synaptics firmware team member how to add support, and it will be implemented in a future release.
>
> Signed-off-by: Marge Yang <Marge.Yang@...synaptics.com>
> ---
> drivers/input/rmi4/Kconfig | 8 ++
> drivers/input/rmi4/Makefile | 1 +
> drivers/input/rmi4/rmi_bus.c | 3 +
> drivers/input/rmi4/rmi_driver.h | 5 ++
> drivers/input/rmi4/rmi_f21.c | 126 ++++++++++++++++++++++++++++++++
> 5 files changed, 143 insertions(+)
> create mode 100644 drivers/input/rmi4/rmi_f21.c
>
> diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
> index c0163b983ce6..086013be6a64 100644
> --- a/drivers/input/rmi4/Kconfig
> +++ b/drivers/input/rmi4/Kconfig
> @@ -82,6 +82,14 @@ config RMI4_F12
> touchpads. For sensors that support relative pointing, F12 also
> provides mouse input.
>
> +config RMI4_F21
> + bool "RMI4 Function 21 (PRESSURE)"
> + help
> + Say Y here if you want to add support for RMI4 function 21.
> +
> + Function 21 provides buttons/pressure for RMI4 devices. This includes
> + support for buttons/pressure on PressurePad.
> +
> config RMI4_F30
> bool "RMI4 Function 30 (GPIO LED)"
> help
> diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
> index 02f14c846861..484b97eca025 100644
> --- a/drivers/input/rmi4/Makefile
> +++ b/drivers/input/rmi4/Makefile
> @@ -8,6 +8,7 @@ rmi_core-$(CONFIG_RMI4_2D_SENSOR) += rmi_2d_sensor.o
> rmi_core-$(CONFIG_RMI4_F03) += rmi_f03.o
> rmi_core-$(CONFIG_RMI4_F11) += rmi_f11.o
> rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
> +rmi_core-$(CONFIG_RMI4_F21) += rmi_f21.o
> rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
> rmi_core-$(CONFIG_RMI4_F34) += rmi_f34.o rmi_f34v7.o
> rmi_core-$(CONFIG_RMI4_F3A) += rmi_f3a.o diff --git
> a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c index
> 3aee04837205..47fe7a88c92b 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -360,6 +360,9 @@ static struct rmi_function_handler *fn_handlers[]
> = { #ifdef CONFIG_RMI4_F12
> &rmi_f12_handler,
> #endif
> +#ifdef CONFIG_RMI4_F21
> + &rmi_f21_handler,
> +#endif
> #ifdef CONFIG_RMI4_F30
> &rmi_f30_handler,
> #endif
> diff --git a/drivers/input/rmi4/rmi_driver.h
> b/drivers/input/rmi4/rmi_driver.h index 3bfe9013043e..18fdf2a166d5
> 100644
> --- a/drivers/input/rmi4/rmi_driver.h
> +++ b/drivers/input/rmi4/rmi_driver.h
> @@ -115,6 +115,10 @@ static inline int rmi_f03_overwrite_button(struct
> rmi_function *fn, static inline void rmi_f03_commit_buttons(struct
> rmi_function *fn) {} #endif
>
> +#ifdef CONFIG_RMI4_F21
> +int rmi_f21_report_pressure(struct rmi_function *fn, int i); #endif
> +
I do not see definition for this anywhere in the patch.
[Marge 07/14]
I will remove it in next patch.
> #ifdef CONFIG_RMI4_F34
> int rmi_f34_create_sysfs(struct rmi_device *rmi_dev); void
> rmi_f34_remove_sysfs(struct rmi_device *rmi_dev); @@ -133,6 +137,7 @@
> extern struct rmi_function_handler rmi_f01_handler; extern struct
> rmi_function_handler rmi_f03_handler; extern struct
> rmi_function_handler rmi_f11_handler; extern struct
> rmi_function_handler rmi_f12_handler;
> +extern struct rmi_function_handler rmi_f21_handler;
> extern struct rmi_function_handler rmi_f30_handler; extern struct
> rmi_function_handler rmi_f34_handler; extern struct
> rmi_function_handler rmi_f3a_handler; diff --git
> a/drivers/input/rmi4/rmi_f21.c b/drivers/input/rmi4/rmi_f21.c new file
> mode 100644 index 000000000000..93ef2331ed16
> --- /dev/null
> +++ b/drivers/input/rmi4/rmi_f21.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2012-2025 Synaptics Incorporated */
> +
> +#include <linux/kernel.h>
> +#include <linux/rmi.h>
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include "rmi_driver.h"
> +
> +#define RMI_f21_INPUT_REPORT_DATA_SIZE 6
> +#define RMI_F21_INPUT_REPORT_FORCE_CLICK_OFFSET 5
> +#define RMI_F21_TABLE_FORCE_CLICK_OFFSET 8
> +#define RMI_f21_FORCE_CLICK 0x01
Use BIT(0) here please.
[Marge 07/14]
I will use BIT(0) in next patch.
> +#define RMI_f21_DATA_REGS_MAX_SIZE 19
Why such a big difference in F21 data size between HID and other transports?
[Marge 07/14]
F21 Data Structure:
Data size = (number of Forcepad sensors) × 2 bytes (Currently there are 4 Forcepad sensors)
+ 1 byte for the Forcepad button state
+ (5 fingers × 2 bytes) for finger pressure values (Supports five fingers, each finger's pressure value occupies two bytes)
HID Attention: 6 bytes
The pressure values for the five fingers are each represented by one byte, so that's five bytes total, plus one byte for the Forcepad button state.
I will discuss with the Synaptics firmware team member how to add support by representing it through a lookup table.
Update it in next patch
> +#define RMI_f21_FORCEPAD_BUTTON_COUNT 1
> +
> +struct f21_data {
> + /* Query Data */
> + u8 data_regs[RMI_f21_DATA_REGS_MAX_SIZE];
> + u8 input_report_data[RMI_f21_INPUT_REPORT_DATA_SIZE];
> + struct input_dev *input;
> + u16 key_code;
> +};
> +
> +static irqreturn_t rmi_f21_attention(int irq, void *ctx) {
> + struct rmi_function *fn = ctx;
> + struct f21_data *f21 = dev_get_drvdata(&fn->dev);
> + struct rmi_driver_data *drvdata = dev_get_drvdata(&fn->rmi_dev->dev);
> + int error;
> + bool pressed;
> +
> + if (drvdata->attn_data.data) {
> + if (drvdata->attn_data.size < RMI_f21_INPUT_REPORT_DATA_SIZE) {
> + dev_warn(&fn->dev, "f21 interrupted, but data is missing\n");
> + return IRQ_HANDLED;
> + }
> + memcpy(f21->input_report_data, drvdata->attn_data.data,
> + RMI_f21_INPUT_REPORT_DATA_SIZE);
I do not think you need to do the copy, you can test the bit directly in
drvdata->attn_data.data buffer.
[Marge 07/14]
I will use drvdata->attn_data.data buffer directly in next patch.
> + drvdata->attn_data.data += RMI_f21_INPUT_REPORT_DATA_SIZE;
> + drvdata->attn_data.size -=
> + RMI_f21_INPUT_REPORT_DATA_SIZE;
> +
> + pressed = !!(f21->input_report_data[RMI_F21_INPUT_REPORT_FORCE_CLICK_OFFSET] &
> + RMI_f21_FORCE_CLICK);
No need to double negation here, converting to bool will do the right thing.
[Marge 07/14]
I will correct it in next patch.
> + } else {
> + error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr,
> + f21->data_regs, RMI_f21_DATA_REGS_MAX_SIZE);
> + if (error) {
> + dev_err(&fn->dev, "%s: Failed to read f21 data registers: %d\n",
> + __func__, error);
> + return IRQ_RETVAL(error);
> + }
> + pressed = !!(f21->data_regs[RMI_F21_TABLE_FORCE_CLICK_OFFSET] &
> + RMI_f21_FORCE_CLICK);
Same here.
[Marge 07/14]
I will correct it in next patch.
> + }
> +
> + input_report_key(f21->input, f21->key_code, pressed);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rmi_f21_config(struct rmi_function *fn) {
> + struct f21_data *f21 = dev_get_drvdata(&fn->dev);
> + struct rmi_driver *drv = fn->rmi_dev->driver;
> +
> + if (!f21)
> + return 0;
Is this actually possible for f21 to be NULL here?
[Marge 07/14]
I will remove it in next patch.
> +
> + drv->set_irq_bits(fn->rmi_dev, fn->irq_mask);
> +
> + return 0;
> +}
> +
> +static int rmi_f21_initialize(struct rmi_function *fn, struct
> +f21_data *f21) {
> + struct input_dev *input = f21->input;
> + unsigned int button = BTN_LEFT;
This variable is not needed.
[Marge 07/14]
I will correct it in next patch.
> +
> + f21->key_code = button;
f21->key_code = BTN_LEFT;
> + input_set_capability(input, EV_KEY, f21->key_code);
> + input->keycode = &(f21->key_code);
Drop extra parenthesis.
[Marge 07/14]
I will correct it in next patch.
> + input->keycodesize = sizeof(f21->key_code);
> + input->keycodemax = RMI_f21_FORCEPAD_BUTTON_COUNT;
> +
> + __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> +
> + return 0;
> +}
> +
> +static int rmi_f21_probe(struct rmi_function *fn) {
> + struct rmi_device *rmi_dev = fn->rmi_dev;
> + struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
> + struct f21_data *f21;
> + int error;
> +
> + if (!drv_data->input) {
> + dev_info(&fn->dev, "f21: no input device found, ignoring\n");
> + return -ENXIO;
> + }
> +
> + f21 = devm_kzalloc(&fn->dev, sizeof(*f21), GFP_KERNEL);
> + if (!f21)
> + return -ENOMEM;
> +
> + f21->input = drv_data->input;
> +
> + error = rmi_f21_initialize(fn, f21);
> + if (error)
> + return error;
> +
> + dev_set_drvdata(&fn->dev, f21);
> + return 0;
> +}
> +
> +struct rmi_function_handler rmi_f21_handler = {
> + .driver = {
> + .name = "rmi4_f21",
> + },
> + .func = 0x21,
> + .probe = rmi_f21_probe,
> + .config = rmi_f21_config,
> + .attention = rmi_f21_attention,
> +};
Thanks.
--
Dmitry
Powered by blists - more mailing lists