[Quantal][PULL] Cypress PS/2 Trackpad driver

Kamal Mostafa kamal at canonical.com
Tue Aug 14 17:38:02 UTC 2012


In reference to ...
  git://kernel.ubuntu.com/kamal/ubuntu-quantal.git cypress-for-quantal
  http://kernel.ubuntu.com/git?p=kamal/ubuntu-quantal.git;a=shortlog;h=refs/heads/cypress-for-quantal

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
Quantal.

> 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.
> 
> Seth
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20120814/25fd65a7/attachment.sig>


More information about the kernel-team mailing list