lkml.org 
[lkml]   [2009]   [Dec]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [git pull] Input updates for 2.6.33-rc2


On Wed, 30 Dec 2009, Dmitry Torokhov wrote:
>
> Dmitry Torokhov (7):
> Input: speed up suspend/shutdown for PS/2 mice and keyboards

Btw, the thing I like best about this commit is how it adds some comments
on the keyboard commands.

That said, the naming and the comments aren't all that wonderful. I don't
think you should use the word "RESET" for command f5/f6: the command is
called "Set Default" (and ".. Disable" for f5), and there is no actual
reset involved.

So I suspect the command should be renamed to

#define ATKBD_CMD_SET_DEFAULTS_AND_DISABLE 0x00f5
#define ATKBD_CMD_SET_DEFAULTS 0x00f6

and then the comments wouldn't even be needed (and anybody reading the
code would not have to look them up in the header file).

And then "ATKBD_CMD_RESET_BAT" should probably just be "ATKBD_CMD_RESET"
(I wonder what the "BAT" part is all about?)

Oh, and command F5h is not always a "set defaults and disable". For some
(all?) PS/2 mice it seems to be _just_ a "disable", and I'm not at all
sure you should have used it for the psmouse_cleanup() function. You used
to do a

psmouse_reset(psmouse);

which sent a full reset to the mouse (and waits for the two-byte ACK). It
did _not_ disable the mouse - that happened earlier in
psmouse_deactivate(), when you sent it the PSMOUSE_CMD_DISABLE command.

You changed that 'psmouse_reset()' to

/*
* Reset the mouse to defaults (bare PS/2 protocol).
*/
ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS);

which now disables it again, only to then later do

/*
* Some boxes, such as HP nx7400, get terribly confused if mouse
* is not fully enabled before suspending/shutting down.
*/
ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_ENABLE);

which seems odd. IOW, Why do you do a "set defaults and disable" followed
by a "enable", when you already had it disabled?

I think the PSMOUSE_CMD_RESET that _used_ to be there would re-enable most
mice, but now that you don't even do that any more, I get the feeling that
the "set defaults and disable" should be just a "set defaults".

(You may have good reasons for having picked the "and disable" command. I
just wonder what they are)

Linus


\
 
 \ /
  Last update: 2009-12-31 20:53    [W:0.043 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site