Re: [PATCH 07/11] Input: synaptics-rmi4 - f30/f03: Forward mechanical buttons on buttonpads to PS/2 guest

From: Andrew Duggan
Date: Tue Aug 30 2016 - 15:16:19 EST

Next message: Shardar Mohammed: "RE: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed"
Previous message: Mathieu Maret: "[PATCH] Enable code completion in VIM"
In reply to: Benjamin Tissoires: "Re: [PATCH 07/11] Input: synaptics-rmi4 - f30/f03: Forward mechanical buttons on buttonpads to PS/2 guest"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 08/30/2016 08:13 AM, Benjamin Tissoires wrote:

Hi Andrew,

On Aug 26 2016 or thereabouts, Andrew Duggan wrote:

Resending as plain text

Hi Benjamin,

This patch causes standard clickpads without extended buttons to not work.
I'll explain some more below.

On 08/18/2016 02:24 AM, Benjamin Tissoires wrote:

From: Lyude Paul <thatslyude@xxxxxxxxx>

On the latest series of ThinkPads, the button events for the TrackPoint
are reported through the touchpad itself as opposed to the TrackPoint
device. In order to report these buttons properly, we need to forward
them to the TrackPoint device and send the button presses/releases
through there instead.

Signed-off-by: Lyude Paul <thatslyude@xxxxxxxxx>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
---
drivers/input/rmi4/rmi_driver.h | 13 +++++++++
drivers/input/rmi4/rmi_f03.c | 28 ++++++++++++++++++
drivers/input/rmi4/rmi_f30.c | 64 +++++++++++++++++++++++++++++++++++------
3 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index e4be773..a0b1978 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -99,6 +99,19 @@ struct rmi_function *rmi_find_function(struct rmi_device *rmi_dev, u8 number);

char *rmi_f01_get_product_ID(struct rmi_function *fn);

+#ifdef CONFIG_RMI4_F03
+int rmi_f03_overwrite_button(struct rmi_function *fn, unsigned int button,
+ int value);
+void rmi_f03_commit_buttons(struct rmi_function *fn);
+#else
+static inline int rmi_f03_overwrite_button(struct rmi_function *fn,
+ unsigned int button, int value)
+{
+ return 0;
+}
+static inline void rmi_f03_commit_buttons(struct rmi_function *fn) {}
+#endif
+
extern struct rmi_function_handler rmi_f01_handler;
extern struct rmi_function_handler rmi_f03_handler;
extern struct rmi_function_handler rmi_f11_handler;
diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
index daae1c95..535f426 100644
--- a/drivers/input/rmi4/rmi_f03.c
+++ b/drivers/input/rmi4/rmi_f03.c
@@ -37,6 +37,34 @@ struct f03_data {
u8 rx_queue_length;
};

+int rmi_f03_overwrite_button(struct rmi_function *fn, unsigned int button,
+ int value)
+{
+ struct f03_data *f03 = dev_get_drvdata(&fn->dev);
+ unsigned int bit = BIT(button);
+
+ if (button > 2)
+ return -EINVAL;
+
+ if (value)
+ f03->overwrite_buttons |= bit;
+ else
+ f03->overwrite_buttons &= ~bit;
+
+ return 0;
+}
+
+void rmi_f03_commit_buttons(struct rmi_function *fn)
+{
+ struct f03_data *f03 = dev_get_drvdata(&fn->dev);
+ int i;
+
+ f03->serio->extra_byte = f03->overwrite_buttons;
+
+ for (i = 0; i < 3; i++)
+ serio_interrupt(f03->serio, 0x00, 0x00);
+}
+
static int rmi_f03_pt_write(struct serio *id, unsigned char val)
{
struct f03_data *f03 = id->port_data;
diff --git a/drivers/input/rmi4/rmi_f30.c b/drivers/input/rmi4/rmi_f30.c
index 7990bb0..14e3221 100644
--- a/drivers/input/rmi4/rmi_f30.c
+++ b/drivers/input/rmi4/rmi_f30.c
@@ -74,8 +74,11 @@ struct f30_data {

u8 data_regs[RMI_F30_CTRL_MAX_BYTES];
u16 *gpioled_key_map;
+ u16 *gpio_passthrough_key_map;

struct input_dev *input;
+ bool trackstick_buttons;
+ struct rmi_function *f03;
};

static int rmi_f30_read_control_parameters(struct rmi_function *fn,
@@ -108,6 +111,13 @@ static int rmi_f30_attention(struct rmi_function *fn, unsigned long *irq_bits)
if (!f30->input)
return 0;

+ if (f30->trackstick_buttons && !f30->f03) {
+ f30->f03 = rmi_find_function(rmi_dev, 3);
+
+ if (!f30->f03)
+ return -EBUSY;
+ }
+
/* Read the gpi led data. */
if (rmi_dev->xport->attn_data) {
memcpy(f30->data_regs, rmi_dev->xport->attn_data,
@@ -128,23 +138,29 @@ static int rmi_f30_attention(struct rmi_function *fn, unsigned long *irq_bits)
for (reg_num = 0; reg_num < f30->register_count; ++reg_num) {
for (i = 0; gpiled < f30->gpioled_count && i < 8; ++i,
++gpiled) {
- if (f30->gpioled_key_map[gpiled] != 0) {
- /* buttons have pull up resistors */
- value = (((f30->data_regs[reg_num] >> i) & 0x01)
- == 0);
+ /* buttons have pull up resistors */
+ value = (((f30->data_regs[reg_num] >> i) & 0x01) == 0);

+ if (f30->gpioled_key_map[gpiled] != 0) {
rmi_dbg(RMI_DEBUG_FN, &fn->dev,
"%s: call input report key (0x%04x) value (0x%02x)",
__func__,
f30->gpioled_key_map[gpiled], value);
+
input_report_key(f30->input,
f30->gpioled_key_map[gpiled],
value);
+ } else if (f30->gpio_passthrough_key_map[gpiled]) {
+ rmi_f03_overwrite_button(f30->f03,
+ f30->gpio_passthrough_key_map[gpiled] - BTN_LEFT,
+ value);
}
-
}
}

+ if (f30->trackstick_buttons)
+ rmi_f03_commit_buttons(f30->f03);
+
return 0;
}

@@ -242,10 +258,10 @@ static inline int rmi_f30_initialize(struct rmi_function *fn)
int retval = 0;
int control_address;
int i;
- int button;
+ int button, extra_button;
u8 buf[RMI_F30_QUERY_SIZE];
u8 *ctrl_reg;
- u8 *map_memory;
+ u8 *map_memory, *pt_memory;

f30 = devm_kzalloc(&fn->dev, sizeof(struct f30_data),
GFP_KERNEL);
@@ -343,15 +359,47 @@ static inline int rmi_f30_initialize(struct rmi_function *fn)
map_memory = devm_kzalloc(&fn->dev,
(f30->gpioled_count * (sizeof(u16))),
GFP_KERNEL);
- if (!map_memory) {
+ pt_memory = devm_kzalloc(&fn->dev,
+ (f30->gpioled_count * (sizeof(u16))),
+ GFP_KERNEL);
+ if (!map_memory || !pt_memory) {
dev_err(&fn->dev, "Failed to allocate gpioled map memory.\n");
return -ENOMEM;
}

f30->gpioled_key_map = (u16 *)map_memory;
+ f30->gpio_passthrough_key_map = (u16 *)pt_memory;

pdata = rmi_get_platform_data(rmi_dev);
if (pdata && f30->has_gpio) {

This existing check of pdata is not needed because pdata is embedded in the
transport device and will never be NULL. That means that in this patch the
else case will never be called.

oops. Good catch

+ /*
+ * For touchpads the buttons are mapped as:
+ * - bit 0 = Left, bit 1 = right, bit 2 = middle / clickbutton
+ * - 3, 4, 5 are extended buttons and
+ * - 6 and 7 are other sorts of GPIOs
+ */
+ button = BTN_LEFT;
+ extra_button = BTN_LEFT;
+ for (i = 0; i < f30->gpioled_count - 2 && i < 3; i++) {

Subtracting 2 from gpioled_count does not have the intended affect. The name
gpioled_count comes from the spec, but that byte is really a bit map. On a
typical clickpad bit two is set as mentioned in the new comment. The result

I really doubt this is a bit map. On the T450s, only bit 3 (the 4th bit
then) is set and this corresponds to the numerical value "8". If it were
a bit map, I would expect bits 0-7 to be set -> 0xFF.

Aren't you mixing the gpioled_count and the gpioled_key_map? Because bit
2 set on gpioled_count would be 4, and I can't figure out a proper use
of this number :)

Yes, I was confused and that last email from me needs to be deleted from the internet. I remembered that gpioled_count doesn't work the way I expect it to, but I confused a few things when trying to explain why subtracting 2 from gpioled_count doesn't work. So let me try this again. The gpioled_count is the number of bits used to represent gpios or leds in ctrl 2 and ctrl 3. On a standard clickpad the gpioled_count is set to 3 which means the first 3 bits of ctrl 2 and ctrl 3 need to be checked. This is because the click button is defined in bit 2 (like the comment above).

The Lenovos have more GPIOs so the count is 8. Which means all 8 bits need to be checked since gpios are defined up to bit 7.

is that this for loop only runs once and it only checks the first bit of
ctrl 2 and ctrl 3 for a valid button. Since bit 0 is not set then no valid
buttons are reported for the clickpad.

It looks like the Lenovo systems which this patch is targeting actually have
6 gpios defined (bits 2 - 7 are set) so this code works on those system

Yes, the conditions in the for loops are wrong. I think they could
be fixed easily by having one case for (f30->has_gpio &&
f30->trackstick_buttons) and one other when f30->trackstick_buttons is
not set. In the trackstick button case, I should also only take into
account the first 6 buttons (it's a special case after all :-P ).

since the "count" is sufficiently large. Also, maybe it's time to rename
gpioled_count to something like gpioled_map.

+ if (rmi_f30_is_valid_button(i, f30->ctrl))
+ f30->gpioled_key_map[i] = button++;
+ }
+
+ f30->trackstick_buttons = pdata &&
+ pdata->f30_data.trackstick_buttons;
+
+ if (f30->trackstick_buttons) {
+ for (i = 3; i < f30->gpioled_count - 2; i++) {
+ if (rmi_f30_is_valid_button(i, f30->ctrl))
+ f30->gpio_passthrough_key_map[i] = extra_button++;
+ }
+ } else {
+ for (i = 3; i < f30->gpioled_count - 2; i++) {
+ if (rmi_f30_is_valid_button(i, f30->ctrl))
+ f30->gpioled_key_map[i] = button++;
+ }
+ }
+ } else if (f30->has_gpio) {

As noted above this else block is never called.

Yep :(

Thanks for the other reviews BTW. I amended the patches locally and will
resubmit this week or the week after if there are more reviews coming
in.

I'll take a look at the rest of the patches in the series when I have a chance. But, this was the only patch which caused an issue for me.

Thanks,
Andrew

Cheers,
Benjamin

Andrew

button = BTN_LEFT;
for (i = 0; i < f30->gpioled_count; i++) {
if (rmi_f30_is_valid_button(i, f30->ctrl)) {

Next message: Shardar Mohammed: "RE: [PATCH v8 1/3] i2c: tegra: use readx_poll_timeout after config_load reg programmed"
Previous message: Mathieu Maret: "[PATCH] Enable code completion in VIM"
In reply to: Benjamin Tissoires: "Re: [PATCH 07/11] Input: synaptics-rmi4 - f30/f03: Forward mechanical buttons on buttonpads to PS/2 guest"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]