Re: [PATCH 1/3] input: alps: Reset mouse before identifying it

[Pali RohÃr]

On Thursday 23 October 2014 17:44:04 Dmitry Torokhov wrote:
> On Sun, Oct 19, 2014 at 01:07:41PM +0200, Pali RohÃr wrote:
> > On Wednesday 15 October 2014 20:22:56 Dmitry Torokhov wrote:
> > > On Wed, Oct 15, 2014 at 08:10:39PM +0200, Pali RohÃr wrote:
> > > > On Wednesday 15 October 2014 20:00:11 Dmitry Torokhov 
wrote:
> > > > > On Wed, Oct 15, 2014 at 07:57:37PM +0200, Pali RohÃr 
wrote:
> > > > > > On Wednesday 15 October 2014 19:43:15 Dmitry
> > > > > > Torokhov
> > 
> > wrote:
> > > > > > > On Wed, Oct 15, 2014 at 02:53:11PM +0200, Pali
> > > > > > > RohÃr
> > 
> > wrote:
> > > > > > > > On Tuesday 14 October 2014 08:08:34 Dmitry
> > > > > > > > Torokhov
> > > > 
> > > > wrote:
> > > > > > > > > On Fri, Oct 03, 2014 at 11:47:59AM +0200, Hans
> > > > > > > > > de Goede
> > > > > > 
> > > > > > wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > Thanks for working on this!
> > > > > > > > > > 
> > > > > > > > > > On 10/03/2014 11:43 AM, Pali RohÃr wrote:
> > > > > > > > > > > On some systems after starting computer
> > > > > > > > > > > function alps_identify() does not detect
> > > > > > > > > > > dual ALPS touchpad+trackstick device
> > > > > > > > > > > correctly and detect only touchpad.
> > > > > > > > > > > 
> > > > > > > > > > > Resetting ALPS device before identifiying
> > > > > > > > > > > it fixing this problem and both parts
> > > > > > > > > > > touchpad and trackstick are detected.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Pali RohÃr
> > > > > > > > > > > <pali.rohar@xxxxxxxxx> Tested-by: Pali
> > > > > > > > > > > RohÃr <pali.rohar@xxxxxxxxx>
> > > > > > > > > > 
> > > > > > > > > > Looks good and seems sensible:
> > > > > > > > > > 
> > > > > > > > > > Acked-by: Hans de Goede
> > > > > > > > > > <hdegoede@xxxxxxxxxx>
> > > > > > > > > 
> > > > > > > > > *sigh* I am not really happy about this, as we
> > > > > > > > > making boot longer and longer for people
> > > > > > > > > without ALPS touchpads. It would be better if
> > > > > > > > > we only reset the mouse when we knew we are
> > > > > > > > > dealing with ALPS, and even better if we only
> > > > > > > > > reset it when we suspected that we missed
> > > > > > > > > trackstick. Any chance of doing this?
> > > > > > > > > 
> > > > > > > > > Thanks.
> > > > > > > > 
> > > > > > > > Dmitry, problem is that function check which
> > > > > > > > detecting trackstick does not working when I
> > > > > > > > start my laptop from power-off state and do not
> > > > > > > > reset PS/2 device. But detecting ALPS touchpad
> > > > > > > > looks like working. So if do not like this
> > > > > > > > idea, what about doing something like this in
> > > > > > > > alps_dectect function?
> > > > > > > > 
> > > > > > > > int alps_detect(...)
> > > > > > > > {
> > > > > > > > ...
> > > > > > > > /* detect if device is ALPS */
> > > > > > > > if (alps_identify(...) < 0)
> > > > > > > > return -1;
> > > > > > > > /* now we know that device is ALPS */
> > > > > > > > if (!(flags & ALPS_DUALPOINT)) {
> > > > > > > > /* reset it and identify again, maybe there is
> > > > > > > > trackstick */ psmouse_reset(...);
> > > > > > > > alps_identify(...);
> > > > > > > > }
> > > > > > > > ...
> > > > > > > > }
> > > > > > > > 
> > > > > > > > It will does not affect non ALPS devices
> > > > > > > > (because first identify call will fail), but
> > > > > > > > will affect ALPS devices without trackstick
> > > > > > > > (because identify will be called twice and
> > > > > > > > reset too).
> > > > > > > 
> > > > > > > I think this is a step in right direction. Do you
> > > > > > > know what exactly fails in alps_identify() on
> > > > > > > your box if you do not call psmouse_reset?
> > > > > > > 
> > > > > > > Thanks.
> > > > > > 
> > > > > > Yes, I know. It is failing in
> > > > > > alps_probe_trackstick_v3(). It calls
> > > > > > alps_command_mode_read_reg(...) and it returns 0
> > > > > > which means trackstick is not there.
> > > > > 
> > > > > OK, so can we try sticking psmouse_reset() there? This
> > > > > will limit the exposure of the new delay.
> > > > > 
> > > > > Thanks.
> > > > 
> > > > Sorry, but I think this is not safe. Function
> > > > psmouse_reset will reset device (set it to relative
> > > > mode, etc...) and before and after
> > > > alps_probe_trackstick_v3() are called other functions.
> > > > So it could break something else.
> > > 
> > > We might need to repeat bits of alps_identify() after
> > > resetting the mouse, you are right. It should still be
> > > doable though.
> > 
> > What about checking "E6 report" and if that pass reset
> > device and do full alps_identify? With check for "E6
> > report" we can filter probably all PS/2 devices which are
> > not ALPS.
> 
> Why don't you pull alps_probe_trackstick_v3() from
> alps_identify(), rename it into __alps_identify() and then
> have real alps_identify be:
> 
> static int alps_identfy(struct psmouse *psmouse, struct
> alps_data *priv) {
> 	int error;
> 
> 	error = __alps_identify(psmouse, priv);
> 	if (error)
> 		return error;
> 
> 	if (priv->proto_version == ALPS_PROTO_V3 &&
> 	    (priv->flags & ALPS_IS_RUSHMORE)) {
> 		/*
> 		 * Some Dell Lattitudes may not always recognize
> 		 * tracksticks without resetting the device.
> 		 */
> 		psmouse_reset(psmouse);
> 
> 		error =  __alps_identify(psmouse, priv);
> 		if (error)
> 			return error;
> 
> 		if (alps_probe_trackstick_v3(psmouse,
> 					     ALPS_REG_BASE_RUSHMORE))
> 			priv->flags &= ~ALPS_DUALPOINT;
> 	}
> 
> 	return 0;
> }
> 
> This way you minimize number of devices exposed to extra
> reset.
> 
> Thanks.

Hello Dmitry,

I sent new patch series v3 with another solution for fixing this 
problem. It just effectively move more parts of alps code and 
trackstick detection should work. It does not introduce new reset 
call (like in this version) so it is better.

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.