login
Search
From: Fred <fwk@ubuntu.linuxcertified.com> This is a new touchscreen driver for the Cypress Semiconductor cyttsp family of devices. This updated driver is for both the i2c and spi versions of the devices. The driver code consists of common core code for both i2c and spi driver. This submission is in response to review comments from the initial submission. Signed-off-by: Kevin McNeely <kev@cypress.com> --- drivers/input/touchscreen/Kconfig | 33 + drivers/input/touchscreen/Makefile | 3 + drivers/input/touchscreen/cyttsp_board-xxx.c | 110 ++ drivers/input/touchscreen/cyttsp_core.c | 1778 ++++++++++++++++++++++++++ drivers/input/touchscreen/cyttsp_core.h | 49 + drivers/input/touchscreen/cyttsp_i2c.c | 183 +++ drivers/input/touchscreen/cyttsp_spi.c | 339 +++++ include/linux/cyttsp.h | 104 ++ 8 files changed, 2599 insertions(+), 0 deletions(-) create mode 100644 drivers/input/touchscreen/cyttsp_board-xxx.c create mode 100755 drivers/input/touchscreen/cyttsp_core.c create mode 100755 drivers/input/touchscreen/cyttsp_core.h create mode 100755 drivers/input/touchscreen/cyttsp_i2c.c create mode 100755 drivers/input/touchscreen/cyttsp_spi.c create mode 100755 include/linux/cyttsp.h diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig index 3b9d5e2..b923379 100644 --- a/drivers/input/touchscreen/Kconfig +++ b/drivers/input/touchscreen/Kconfig @@ -603,4 +603,37 @@ config TOUCHSCREEN_TPS6507X To compile this driver as a module, choose M here: the module will be called tps6507x_ts. +config TOUCHSCREEN_CYTTSP_I2C + default n + tristate "Cypress TTSP i2c touchscreen" + depends on I2C + help + Say Y here if you have a Cypress TTSP touchscreen + connected to your system's i2c bus. + + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called cyttsp_i2c. + +config TOUCHSCREEN_CYTTSP_SPI + default ...
Hi Kevin, E-mail address is is wrong again it seems. Please fix. You may want to divide this whole patch into three patches: 1. core driver 2. i2c driver This file is good as example only but not for check in. This is a board specific code and the board-xxx.c We always support and base our code from latest kernel only. Please remove the code Most of the time kernel people understands "pdev == platform device and not just device", so better rename this variable Un-necessary debug macros and abstractions. This is not allowed. Please use mutext_lock and unlock Please don't do like DBG(...) like things. As it is strictly for debugging purpose only please remove this function from the code itself. Developer in need of such debugging routines will write down their own when they sit for debugging the problem. I don't see any need Please use dev_dbg(...) or pr_debug(...) macros directly instead of putting them under DBG(...). It is better you remove DBG(..) from whole driver and replace them with functions should be "static". I would leave a decision to Dmitry if he wants the driver to support old single touch protocol hacked up with HAT_xx bits so that driver can support two touches with the old single touch protocol itself. I would prefer not to support this single touch because we are hacking up this I would really prefer that you remove the polling method from this code as it will simplify a code lot. We can delete the whole workqueue because as you will be using request_threaded_irq(...), I don't see any need of this timer. Better remove the polling method all together to simplify the code. This kind of custom interface may not be allowed in the kernel driver. Please use Please add __init like I will comment on spi driver interface later. ---Trilok Soni -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. --
The ABS_HAT* bits should go away. They were gross abuse even when we did not have MT protocol and shoudl not be used at all now that we do have it. I will be cleaning up older drivers (like Elantech) to get rid of them. Multitouch devices shouls support either A or B MT protocol. SInce this device seems to be supporting tracking in hardware it shoudl simply adhere to protocol B to limit kernel->userspace traffict and be done with it. Seconded. Polling is hardly useful on real production setup as it will And lose printk as well. The boot is exremely noisy as it is... return i2c_add_driver(&cyttsp_i2c_driver); is all that is needed. -- Dmitry --
I have responded to Henrik's review about protocol A and B. I would like to keep protocol A for now and just change the default I will make this change. Thank you for your review. --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- --
HI Trilok, Thank you for your review. I am collecting your comments below and starting Thank you for your review Trilok. --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- --
General impression: There is a lot of useful code in here, but as already pointed out, well over half of it should not be part of the kernel driver. Suggestion 1: Clean out the internal state code and use MT protocol B instead. Suggestion 2: Cut out the single touch calculation, as already pointed out by Trilok. Why not submit it to the mtdev project instead? The problem is generic to all new MT drivers, so a common solution while waiting for full MT support in userspace could be useful. Thanks, Henrik --
I will remove the ST code and polling completely. However, I would like to keep the MT Protocol A. Our solution allows The platform builder to select to use MT protocol B or not as part of platform_data in the board configuration. If it makes more sense, I can reverse the code to default to protocol B and allow the platform builder developer to select protocol A. Thank you for the invitation to submit to the mtdev project. I defer to Dmitry if I should make the next update into the mtdev project. Thank you for your review. --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- --
On 08/07/2010 02:32 AM, Kevin McNeely wrote: There is nothing preventing you from keeping say a dkms package somewhere with all options intact. However, for the kernel, it is a question of maintainability. If the driver can produce prefectly valid data using protocol B, and by doing so several hundred lines of code can be removed, that is very much preferred. Since both protocols can be translated to protocol B via mtdev, which is already very much in use, there is little reason to support protocol A when the device can do tracking. Thanks, Henrik --
I will remove the Protocol A. Thank you again for your review. --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- --
Hi Kevin, Use kernel style for multi-line comments. /* * You should use this format * for multi-line commenting You may want to move this to include/linux/input/cyttsp.h ---Trilok Soni -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. --
Will do. --------------------------------------------------------------- This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. --------------------------------------------------------------- --