Message-ID: <20160830151312.GF21864@mail.corp.redhat.com>
Date:   Tue, 30 Aug 2016 17:13:12 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Andrew Duggan <aduggan@...aptics.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Lyude Paul <thatslyude@...il.com>,
        Christopher Heiny <cheiny@...aptics.com>,
        Dennis Wassenberg <dennis.wassenberg@...unet.com>,
        Peter Hutterer <peter.hutterer@...-t.net>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH 07/11] Input: synaptics-rmi4 - f30/f03: Forward
 mechanical buttons on buttonpads to PS/2 guest

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@...il.com>
> >
> >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@...il.com>
> >Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> >---
> > 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 :)

> 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.

Cheers,
Benjamin

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

Powered by blists - more mailing lists