Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6905092:
cyapa: support MT-A protocol, and add cyapa_misc_dev interface
|
Created:
9 years, 8 months ago by dudl Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, vb+kernel_google.com, Mandeep Singh Baines, Olof Johansson, Micah C, dudl, dso_cypress.com, singh_google.com Visibility:
Public. |
Descriptioncyapa: support MT-A protocol, and add cyapa_misc_dev interface
1. Remove codes that support relative report data, this mode is no longer required for trackpads
2. Modify cyapa driver to support MT-A protocol to support multitouch X input driver;
3. Add cyapa_misc_dev interface, so applications can get driver, firmware and hardware version of the trackpad device.
Change-Id: I8c70391a581bd1f62803f25fe3a52589b56aeed7
Signed-off-by: Du, Dudley <dudl@cypress.com>;
R= olofj@chromium.org, micahc@chromium.org, djkurtz@chromium.org
BUG=chrome-os-partner:3549
TEST=test trackpad on kaen with multitouch X driver
Patch Set 1 #
Total comments: 44
Messages
Total messages: 5 (0 generated)
nits http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c.c File drivers/input/mouse/cypress_i2c.c (right): http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:43: /* Cypress I2C APA trackpad driver version is defined as bellow: s/bellow/below http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:44: ** CYAPA_MAJOR_VER.CYAPA_MINOR_VER.CYAPA_REVISION_VER . */ IMHO, it sounds better as: CYAPA_VER_MAJOR.CYAPA_VER_MINOR.CYAPA_VER_REVISION And no need for **, and trailing '.' http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:47: #define CYAPA_REVISION_VER 7 Just one tab http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:60: #define GESTURE_DECODE_INDEX(x) ((x) & 0x1F) two tabs http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:64: #define GESTURE_PALM_REJECTIOIN GESTURE_ID_CODE(0, 1) 2 tabs http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:69: ** one finger click and hold for more than definitioin time, s/definitioin/defined s/**/ * http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:87: #define GESTURE_2F_SCROLL_DOWN GESTURE_ID_CODE(2, 3) next three lines: two tabs http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:95: #define GESTURE_2F_DRAG GESTURE_ID_CODE(2, 9) 2 tabs http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1647: .resume = cyapa_i2c_resume, align
Hi, I have a handful of comments below, mostly drive-by simple nits. Main worry is that the new locking is getting pretty complex. Overall it's a pretty clean change, it'd be nice to iterate quickly on it a few times for even more cleanup though. One thing I commented on a number of times, but might have missed a few instances of, are local variables that are initialized without needing to be. Please go through the file and check for other occurrances too, don't assume I spotted them all. :-) Thanks! -Olof http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c.c File drivers/input/mouse/cypress_i2c.c (right): http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:227: /* in gen1 and gen2, only 1 gesture one time supported. */ "only 1 gesture at a time"? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:236: struct mutex misc_mutex; You now have three new locking primitives. Please add a comment in the structure which parts of the structure is locked by which primitive and why. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:315: printk(KERN_INFO "%s: ------------------------------------\n", func); You can use pr_info here, it's a little shorter. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:354: */ This comment was changed away from kernel comment style to a different style. Please go back to the one with single *'s. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:433: DBGPRINTK(("%s: i2c_master_send error, retval=%d\n", pr_debug instead? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:532: int count = 0; This assignment is unneeded. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:553: int count = 0; This assignment is unneeded. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:568: struct cyapa_i2c *touch = NULL; Assignment to NULL not needed, or just assign to file->private_data directly here. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:632: struct cyapa_i2c *touch = NULL; Assignment to NULL not needed -- or just assign to file->private_data here instead. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:635: __func__, (int)count, (int)*offset); Printing this on every call is too verbose. pr_debug() instead? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:684: struct cyapa_i2c *touch = NULL; Again, this can be assigned to file->private_data here instead. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:687: __func__, (int)count, (int)*offset); pr_debug() http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:697: return -EINVAL; This code is duplicated with the read function. Breaking it out to a shared function or define could be good. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:712: if (ret < 0) { Braces not needed http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:728: struct cyapa_i2c *touch = NULL; Again, can be assigned directly. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:744: return -EINVAL; The above is a little hard to read. Making the if() fit on one line would help, such as: if (copy_from_user(&ioctl_data, (u8 *)arg, sizeof(ioctl_data)) would be easier to read. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:770: ioctl_data.buf + 2)) The above is hard to read. Since it's just 3 bytes, stage them in a local buffer and copy that out instead. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:837: int ret = 0; No need to initialize. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:841: if (ret) { No need for braces. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:980: printk(KERN_INFO "Cypress Trackpad Information:\n"); pr_info(), please. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1277: cyapa_update_firmware_dispatch(touch); The above would be better to be a return at the end of the if, and save a level of indentation below. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1294: */ Please don't add dead code. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1439: 0, CYAPA_MT_MAX_TOUCH, 0, 0); This is a typical case where it's OK to go avove 80 characters for readability reasons. It's easier to read this on one line even if it's a slightly longer line. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1474: return retval; No need to return this here, just change the code below to be return retval and fall through. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1656: "cypress_i2c: Initialize Cypress multi-touch trackpad.\n"); pr_info() http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1683: "cypress_i2c: Cypress multi-touch trackpad driver exited.\n"); pr_info(). http://codereview.chromium.org/6905092/diff/1/include/linux/cyapa.h File include/linux/cyapa.h (right): http://codereview.chromium.org/6905092/diff/1/include/linux/cyapa.h#newcode8 include/linux/cyapa.h:8: #define CYAPA_MISC_NAME "cyapa_misc_dev" This is unneccessarily long. It means udev will create /dev/cyapa_misc_dev. "cyapa" should be sufficient.
Some more nits. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c.c File drivers/input/mouse/cypress_i2c.c (right): http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:266: int max_absolution_y; max_abs_x / max_abs_y absolution means something completely different! http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1343: ** be pull up. I'm not sure what this comment means. Perhaps: /* * In polling mode, initialize the polling interval to * CYAPA_NO_DATA_SLEEP_MSECS. * Once data is read, the polling rate will be automatically increased. */ And please use kernel multi-line comment style, here, and everywhere else in this driver. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1365: ** then cancel the delayed work routine. /* * Since the mouse, wheel and kbd input_dev all use the same open and close * routines, only cancel the delayed work routine when all three drivers are * closed. */ http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1433: input_set_abs_params(input, ABS_Y, 0, touch->max_absolution_y, 0, 0); Do you really still want to report non-MT axes in a MT driver? When multiple contacts are present, which finger is reported by these axes? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1446: 0, CYAPA_MT_MAX_WIDTH, 0, 0); Do you really report both axes for both touch and approach elipses, and orientation? Or is this just for future expansion? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1451: 0, touch->max_absolution_x, 0, 0); absolution and absolute have completely different meanings. :) I think you mean "max_absolute_x", or maybe just use "max_abs_x" and "max_abs_y". http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1493: /* do platfrom initialize firstly. */ /* First, initialize platform_data */ http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1506: * when special paltfrom that do not support slave interrupt. s/normallly/normally s/paltfrom/platform 'platform' is misspelled often... Please verify every where in this file. And, please use kernel multi-line comment style: /* * Comment comment * comment comment */
Hi All, Attached is the patch file of cyapa driver that modified depending on http://codereview.chromium.org/6905092/ comments based on chromeos-2.6.38. Because I encounter some issue when submit code in gerrit system, so send out the patch file firstly. Please help review it. Thanks. Best Wishes, Dudley Du 86-21-61648950 -----Original Message----- From: djkurtz@chromium.org [mailto:djkurtz@chromium.org] Sent: Monday, May 02, 2011 8:44 PM To: Dudley Du; olofj@chromium.org; micahc@chromium.org Cc: chromium-os-reviews@chromium.org; vb+kernel@google.com; msb@chromium.org; olofj@chromium.org; micahc@google.com; Dudley Du; dso@cypress.com; singh@google.com Subject: Re: Update cyapa driver support MT type-A protocol, add cyapa_misc_dev interface (issue6905092) Some more nits. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c.c File drivers/input/mouse/cypress_i2c.c (right): http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:266: int max_absolution_y; max_abs_x / max_abs_y absolution means something completely different! http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1343: ** be pull up. I'm not sure what this comment means. Perhaps: /* * In polling mode, initialize the polling interval to * CYAPA_NO_DATA_SLEEP_MSECS. * Once data is read, the polling rate will be automatically increased. */ And please use kernel multi-line comment style, here, and everywhere else in this driver. http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1365: ** then cancel the delayed work routine. /* * Since the mouse, wheel and kbd input_dev all use the same open and close * routines, only cancel the delayed work routine when all three drivers are * closed. */ http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1433: input_set_abs_params(input, ABS_Y, 0, touch->max_absolution_y, 0, 0); Do you really still want to report non-MT axes in a MT driver? When multiple contacts are present, which finger is reported by these axes? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1446: 0, CYAPA_MT_MAX_WIDTH, 0, 0); Do you really report both axes for both touch and approach elipses, and orientation? Or is this just for future expansion? http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1451: 0, touch->max_absolution_x, 0, 0); absolution and absolute have completely different meanings. :) I think you mean "max_absolute_x", or maybe just use "max_abs_x" and "max_abs_y". http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1493: /* do platfrom initialize firstly. */ /* First, initialize platform_data */ http://codereview.chromium.org/6905092/diff/1/drivers/input/mouse/cypress_i2c... drivers/input/mouse/cypress_i2c.c:1506: * when special paltfrom that do not support slave interrupt. s/normallly/normally s/paltfrom/platform 'platform' is misspelled often... Please verify every where in this file. And, please use kernel multi-line comment style: /* * Comment comment * comment comment */ http://codereview.chromium.org/6905092/ |
|||||||||||||||||||||||||||||||||||||||||||||||
