Re: PATCH 2.6.11-rc4]: IBM TrackPoint configuration support

[Dmitry Torokhov]

Hi,

sorry, couple of more things (and I promise I will shut up ;))

>  /*
> + * Try to initialize the IBM TrackPoint
> + */
> +	if (max_proto > PSMOUSE_PS2 && trackpoint_init(psmouse) == 0) {
> +		psmouse->vendor = "IBM";
> +		psmouse->name = "TrackPoint";
> + 
> +		return PSMOUSE_PS2;
> +	}

I would like you to change the code so that psmouse structure only
changed when set_properties is set. Probably moving these into 
trackpoint_init is a good idea since _init should not override 
psmouse->private unless told to do so. It is important when the device
was not identified as a trackpoint but later (let's say after resume)
it is - in this case generic reconnect will cause psmouse_extensions
with set_properties = 0 to verify protocol and we dont' want to change
anything in psmouse to give the original disconnect handler change to
clean up properly.

Thinking about it some more I am pretty sure that you need a special
protocol number for the trackpoint, because protocol number is used
by psmouse_reconnect to check whether reconnect can be done or rescan
is needed. You can reuse the standard protocol handler, but you need
the new number. This way if trackpad was not identified as such first
time around, but is identified on reconnect psmouse-base would notice
that it is a new type of device and schedule reconnect and will proper
cleanup/initialization.

Does this make sense?

> +
> +#define MAKE_ATTR_WRITE(_item, command) \
> +        static ssize_t psmouse_attr_set_##_item(struct psmouse
> *psmouse, const char *buf, size_t count) \

It looks like the patch got mangled on it's way, please check your mail
client.

> +        { \
> +		char *rest; \
> +                unsigned long value; \
> +                struct trackpoint_data *tp = psmouse->private; \
> +                value = simple_strtoul(buf, &rest, 10); \

Whitespace damage (tabs vs. spaces). Also there are some trailing
whitespace. If you are using vi I find the following helpful to 
highlight the problem spots:

set listchars=tab:\|-
highlight WhitespaceEOL ctermbg=red guibg=red
match WhitespaceEOL /\s\+$/

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/