diff mbox

Input: mouse: Trackpoint middle button not recognized on Thinkpad E470

Message ID a1682c1c-3894-0439-f224-b6c6fc73eba6@techfak.uni-bielefeld.de (mailing list archive)
State New, archived
Headers show

Commit Message

Julian Exner April 26, 2017, 9:18 p.m. UTC 
On 26/04/17 00:10, Dmitry Torokhov wrote:
> Hmm.. I do not recall seeing real trackpoints with only 2 buttons. Let's
> try defaulting to 3 and see what happens.

This patch assumes three buttons when the trackpoint_read command 
failed. This is just a quick fix. trackpoint_read may fail for other 
reasons than the extended-button-data command not being available, e.g. 
errors in the ps2 communication. A more narrow solution should check if 
a resend request was received, resend, and if the resend failed assume 
three buttons.

Comments

Ulrik De Bie April 28, 2017, 9:05 p.m. UTC | #1 
Hi Julian,

See below ..

On Wed, Apr 26, 2017 at 11:18:44PM +0200, Julian Exner wrote:
> On 26/04/17 00:10, Dmitry Torokhov wrote:
> > Hmm.. I do not recall seeing real trackpoints with only 2 buttons. Let's
> > try defaulting to 3 and see what happens.
> 
> This patch assumes three buttons when the trackpoint_read command failed.
> This is just a quick fix. trackpoint_read may fail for other reasons than
> the extended-button-data command not being available, e.g. errors in the ps2
> communication. A more narrow solution should check if a resend request was
> received, resend, and if the resend failed assume three buttons.

> diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> index 354d47e..70c3a48 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -380,8 +380,8 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
>  		return 0;
>  
>  	if (trackpoint_read(&psmouse->ps2dev, TP_EXT_BTN, &button_info)) {
> -		psmouse_warn(psmouse, "failed to get extended button data\n");
> -		button_info = 0;
> +		psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons are present\n");
> +		button_info = 0x33;
>  	}
>  
>  	psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);

Good, this patch is confirmed to make the trackpoint middle button work on
Thinkpad e570 when applied to vanilla 4.10 kernel.

Now in order to submit the patch for inclusion to the linux kernel,
and before I'll grant a Reviewed-by: tag, it would be best when you
follow the guidelines in Documentation/SubmittingPatches  
(recently moved to : Documentation/process/submitting-patches.rst )

Attention points there are: 
2) Describe your changes 
6) No MIME 
11) Sign your work (Signed-off: line) 
14) The canonical patch format

Thanks,
Kind regards,
Ulrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulrik De Bie May 31, 2017, 7:13 p.m. UTC | #2 
Hi Julian, Oscar,

Peter, you are right and it was even for Lenovo !
Oscar, I reviewed the exact same patch proposed by Julian one month, as
you can read below.

Since he was missing some formalities in the way of submission I pointed
him to submitting-patches.rst .

I was kind of surprised at first that the value was 0x33 instead of 0x3
until I read the documentation pointers Julian provided. 

I really would like this patch to become part of the linux kernel and 
I hope that Julian will go through the formalities for patch submission.

Or alternatively, Oscar, can you confirm that you fully wrote the patch
yourself and just happened to have the same diff as Julian.

Dmitry, what do you suggest as the best way forward?

Thanks,
Kind regards,
Ulrik







On Fri, Apr 28, 2017 at 11:05:32PM +0200, ulrik.debie-os@e2big.org wrote:
> Date:   Fri, 28 Apr 2017 23:05:32 +0200
> From: ulrik.debie-os@e2big.org
> To: Julian Exner <jexner@techfak.uni-bielefeld.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
>  "linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
>  aduggan@synaptics.com
> Subject: Re: Input: mouse: Trackpoint middle button not recognized on
>  Thinkpad E470
> X-Mailing-List: linux-input@vger.kernel.org
> 
> Hi Julian,
> 
> See below ..
> 
> On Wed, Apr 26, 2017 at 11:18:44PM +0200, Julian Exner wrote:
> > On 26/04/17 00:10, Dmitry Torokhov wrote:
> > > Hmm.. I do not recall seeing real trackpoints with only 2 buttons. Let's
> > > try defaulting to 3 and see what happens.
> > 
> > This patch assumes three buttons when the trackpoint_read command failed.
> > This is just a quick fix. trackpoint_read may fail for other reasons than
> > the extended-button-data command not being available, e.g. errors in the ps2
> > communication. A more narrow solution should check if a resend request was
> > received, resend, and if the resend failed assume three buttons.
> 
> > diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> > index 354d47e..70c3a48 100644
> > --- a/drivers/input/mouse/trackpoint.c
> > +++ b/drivers/input/mouse/trackpoint.c
> > @@ -380,8 +380,8 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
> >  		return 0;
> >  
> >  	if (trackpoint_read(&psmouse->ps2dev, TP_EXT_BTN, &button_info)) {
> > -		psmouse_warn(psmouse, "failed to get extended button data\n");
> > -		button_info = 0;
> > +		psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons are present\n");
> > +		button_info = 0x33;
> >  	}
> >  
> >  	psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
> 
> Good, this patch is confirmed to make the trackpoint middle button work on
> Thinkpad e570 when applied to vanilla 4.10 kernel.
> 
> Now in order to submit the patch for inclusion to the linux kernel,
> and before I'll grant a Reviewed-by: tag, it would be best when you
> follow the guidelines in Documentation/SubmittingPatches  
> (recently moved to : Documentation/process/submitting-patches.rst )
> 
> Attention points there are: 
> 2) Describe your changes 
> 6) No MIME 
> 11) Sign your work (Signed-off: line) 
> 14) The canonical patch format
> 
> Thanks,
> Kind regards,
> Ulrik
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julian Exner June 6, 2017, 6:09 p.m. UTC | #3 
Hi all,

sorry for neglecting this thread for so long. And thanks Ulrik for 
pointing me to the formalities.

> I was kind of surprised at first that the value was 0x33 instead of 0x3
> until I read the documentation pointers Julian provided.

It seems like some kind of softswitch or masking for the individual 
buttons. One of the 4-bit nibbles indicates the number of available 
buttons in hardware, the other one the number of activated buttons.

> I really would like this patch to become part of the linux kernel and
> I hope that Julian will go through the formalities for patch submission.

I'm willing to do so, but I'm very short on time currently. 
Additionally, I checked on a Thinkpad forum and was told that there are 
Thinkpads from the 90s with two buttons (e.g. ThinkPad 760 and those 
before ThinkPad 390/600/770). So I'm a bit afraid that simply setting 
the default to three buttons may break something for these models and a 
more elaborate patch may be necessary.


Kind regards,
Julian
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Hutterer June 6, 2017, 11:22 p.m. UTC | #4 
On Tue, Jun 06, 2017 at 08:09:50PM +0200, Julian Exner wrote:
> Hi all,
> 
> sorry for neglecting this thread for so long. And thanks Ulrik for pointing
> me to the formalities.
> 
> > I was kind of surprised at first that the value was 0x33 instead of 0x3
> > until I read the documentation pointers Julian provided.
> 
> It seems like some kind of softswitch or masking for the individual buttons.
> One of the 4-bit nibbles indicates the number of available buttons in
> hardware, the other one the number of activated buttons.
> 
> > I really would like this patch to become part of the linux kernel and
> > I hope that Julian will go through the formalities for patch submission.
> 
> I'm willing to do so, but I'm very short on time currently. Additionally, I
> checked on a Thinkpad forum and was told that there are Thinkpads from the
> 90s with two buttons (e.g. ThinkPad 760 and those before ThinkPad
> 390/600/770). So I'm a bit afraid that simply setting the default to three
> buttons may break something for these models and a more elaborate patch may
> be necessary.

how many of these do you expect to still be alive *and* in need of a new
kernel? wikipedia says the 760 was produced 1995 to 1998 which would make
the newest of them 19 years old now. I'd rather have a hwdb quirk for those
in userspace, because disabling event codes is trivial.

Cheers,
   Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulrik De Bie June 15, 2017, 7:51 p.m. UTC | #5 
Hi,

On Wed, Jun 07, 2017 at 09:22:09AM +1000, Peter Hutterer wrote:
> 
> On Tue, Jun 06, 2017 at 08:09:50PM +0200, Julian Exner wrote:
> > Hi all,
> > 
> > sorry for neglecting this thread for so long. And thanks Ulrik for pointing
> > me to the formalities.
> > 
> > > I was kind of surprised at first that the value was 0x33 instead of 0x3
> > > until I read the documentation pointers Julian provided.
> > 
> > It seems like some kind of softswitch or masking for the individual buttons.
> > One of the 4-bit nibbles indicates the number of available buttons in
> > hardware, the other one the number of activated buttons.
> > 
> > > I really would like this patch to become part of the linux kernel and
> > > I hope that Julian will go through the formalities for patch submission.
> > 
> > I'm willing to do so, but I'm very short on time currently. Additionally, I
> > checked on a Thinkpad forum and was told that there are Thinkpads from the
> > 90s with two buttons (e.g. ThinkPad 760 and those before ThinkPad
> > 390/600/770). So I'm a bit afraid that simply setting the default to three
> > buttons may break something for these models and a more elaborate patch may
> > be necessary.
> 
> how many of these do you expect to still be alive *and* in need of a new
> kernel? wikipedia says the 760 was produced 1995 to 1998 which would make
> the newest of them 19 years old now. I'd rather have a hwdb quirk for those
> in userspace, because disabling event codes is trivial.

And next to that, question is if it actually would be using this specific
driver AND would fail to read out the amount of buttons via the extended 
button request.

Up until commit from Mon Nov 16 22:12:21 2009

315eb996d550 Input: psmouse - rework setting of BTN_MIDDLE capability

it was default middle button for all trackpoints; after that the
middle button was disabled when < 3 buttons reported or the readout
of the buttons failed.

Ulrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 354d47e..70c3a48 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -380,8 +380,8 @@  int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
 		return 0;
 
 	if (trackpoint_read(&psmouse->ps2dev, TP_EXT_BTN, &button_info)) {
-		psmouse_warn(psmouse, "failed to get extended button data\n");
-		button_info = 0;
+		psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons are present\n");
+		button_info = 0x33;
 	}
 
 	psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);