Hi Dmitry,
	Update the status.

-----Original Message-----
From: Dmitry Torokhov <dmitry.torokhov@gmail.com> 
Sent: Sunday, June 29, 2025 1:01 PM
To: Marge Yang <redacted>
Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Vincent Huang <redacted>; David Chiu <redacted>; Derek Cheng <redacted>; Sam Tsai <redacted>
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 <redacted>

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.

I do not see definition for this anywhere in the patch.

[Marge 07/14]
I will remove it in next patch.

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