[Quantal][PULL] Cypress PS/2 Trackpad driver
kamal at canonical.com
Tue Aug 14 17:38:02 UTC 2012
In reference to ...
Thanks for reviewing this Seth!...
On Tue, 2012-08-14 at 12:03 -0500, Seth Forshee wrote:
> The driver is mostly isolated to its own source file, so I don't see it
> causing a lot of maintenance headaches. The code overall doesn't look
> too bad, but there are several small problems (and one bigger one). Even
> after your cleanup the driver still doesn't conform to kernel coding
> style (it's using 4 spaces for indentation instead of tabs).
I had fixed all the checkpatch.pl whitespace "errors", but didn't try to
address the "warnings" (the 4-space indent is a warning). I'll do that,
and resubmit the pull request.
> Probably it
> ought to be using the psmouse_dbg, psmouse_info, etc. for debug messages
> as well.
I'll add that to my to-do list, but I'm hoping we can accept it in
Quantal as it is for now.
> One possible problem for us is that the way you've taken their original
> patch and then applied fixes on top of it, we could end up with commits
> that won't build under certain configurations (i.e.
> CONFIG_MOUSE_PS2_CYPRESS=n up to "UBUNTU: SAUCE: input: Cypress PS/2
> Trackpad fix no-config stubs"). We'd probably always have it enabled of
> course, but I thought it was worth mentioning.
Ah, but I don't think that's so. I think it will build okay at every
commit in any configuration, because I don't actually add cypress_ps2.o
to the Makefile until "UBUNTU: SAUCE: input: Cypress PS/2 Trackpad link
driver into psmouse-base". Before that point, cypress_ps2.[ch] won't
even try to be compiled so there's no problem there, and its nice that
we can keep commits of their original code as they delivered it. Yes?
> The most objectionable thing though is the addition of the
> PSMOUSE_CMD_CYTP state. I suspect upstream won't respond favorably to
> this, and I find the co-opting of ps2dev->wait troublesome (even though
> I don't think it's going to cause problems). Honestly I'm really not
> sure why they can't just use ps2_command(), but if there's a good reason
> they can't I still don't see why they can't do all of this internally
> from the protocol_handler callback.
I'll feed that information back to Cypress and/or add it to my to-do
list to investigate further. Again hoping its acceptable as is for
> On the one hand it would be nice to have these issues fixed, but on the
> other hand I don't see any of them causing us major problems either. So
> I won't personally object to carrying this temporarily as a sauce patch,
> with the understanding that you'll be working to get it upstream. It
> would be nice to get the remaining coding style issues fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 836 bytes
Desc: This is a digitally signed message part
More information about the kernel-team