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 ...
[ message continues ]

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

[ message continues ]

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

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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

--

[ message continues ]