[1/3] input: alps: Reset mouse before identifying it
diff mbox

Message ID 1412329392-5580-2-git-send-email-pali.rohar@gmail.com
State New, archived
Headers show

Commit Message

Pali Rohár Oct. 3, 2014, 9:43 a.m. UTC
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@gmail.com>
Tested-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |    2 ++
 1 file changed, 2 insertions(+)

Comments

Hans de Goede Oct. 3, 2014, 9:47 a.m. UTC | #1
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@gmail.com>
> Tested-by: Pali Rohár <pali.rohar@gmail.com>

Looks good and seems sensible:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  drivers/input/mouse/alps.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 35a49bf..1bd5aa1 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2403,6 +2403,8 @@ int alps_detect(struct psmouse *psmouse, bool set_properties)
>  {
>  	struct alps_data dummy;
>  
> +	psmouse_reset(psmouse);
> +
>  	if (alps_identify(psmouse, &dummy) < 0)
>  		return -1;
>  
> 
--
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
Dmitry Torokhov Oct. 14, 2014, 6:08 a.m. UTC | #2
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@gmail.com>
> > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Looks good and seems sensible:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

*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.
Pali Rohár Oct. 15, 2014, 12:53 p.m. UTC | #3
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@gmail.com>
> > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > Looks good and seems sensible:
> > 
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> *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).
Dmitry Torokhov Oct. 15, 2014, 5:43 p.m. UTC | #4
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@gmail.com>
> > > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > > 
> > > Looks good and seems sensible:
> > > 
> > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > *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.
Pali Rohár Oct. 15, 2014, 5:57 p.m. UTC | #5
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@gmail.com>
> > > > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > > > 
> > > > Looks good and seems sensible:
> > > > 
> > > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > > 
> > > *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.
Dmitry Torokhov Oct. 15, 2014, 6 p.m. UTC | #6
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@gmail.com>
> > > > > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > 
> > > > > Looks good and seems sensible:
> > > > > 
> > > > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > > > 
> > > > *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.
Pali Rohár Oct. 15, 2014, 6:10 p.m. UTC | #7
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@gmail.com>
> > > > > > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > 
> > > > > > Looks good and seems sensible:
> > > > > > 
> > > > > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > > > > 
> > > > > *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.

Tommy (added To header), what do you think? How could be this 
problem solved? When or where to call psmouse_reset() so that it 
will not affect non ALPS devices and also it call will be safe?
Dmitry Torokhov Oct. 15, 2014, 6:22 p.m. UTC | #8
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@gmail.com>
> > > > > > > > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > > 
> > > > > > > Looks good and seems sensible:
> > > > > > > 
> > > > > > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > 
> > > > > > *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.
Pali Rohár Oct. 19, 2014, 11:07 a.m. UTC | #9
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@gmail.com> Tested-by: Pali Rohár
> > > > > > > > > <pali.rohar@gmail.com>
> > > > > > > > 
> > > > > > > > Looks good and seems sensible:
> > > > > > > > 
> > > > > > > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > > 
> > > > > > > *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.
Dmitry Torokhov Oct. 23, 2014, 3:44 p.m. UTC | #10
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@gmail.com> Tested-by: Pali Rohár
> > > > > > > > > > <pali.rohar@gmail.com>
> > > > > > > > > 
> > > > > > > > > Looks good and seems sensible:
> > > > > > > > > 
> > > > > > > > > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > > > 
> > > > > > > > *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.
Pali Rohár Nov. 1, 2014, 11:29 p.m. UTC | #11
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@gmail.com> Tested-by: Pali
> > > > > > > > > > > Rohár <pali.rohar@gmail.com>
> > > > > > > > > > 
> > > > > > > > > > Looks good and seems sensible:
> > > > > > > > > > 
> > > > > > > > > > Acked-by: Hans de Goede
> > > > > > > > > > <hdegoede@redhat.com>
> > > > > > > > > 
> > > > > > > > > *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.

Patch
diff mbox

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 35a49bf..1bd5aa1 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2403,6 +2403,8 @@  int alps_detect(struct psmouse *psmouse, bool set_properties)
 {
 	struct alps_data dummy;
 
+	psmouse_reset(psmouse);
+
 	if (alps_identify(psmouse, &dummy) < 0)
 		return -1;