Re: [RFC PATCH 0/1] input/touchscreen: Synaptics Touchscreen Driver

Previous thread: shutdown bugs, since about 2.6.31 or so by Gene Heskett on Monday, March 22, 2010 - 5:50 pm. (4 messages)

Next thread: linux-next: manual merge of the kgdb tree with the input tree by Stephen Rothwell on Monday, March 22, 2010 - 8:09 pm. (3 messages)
From: Christopher Heiny
Date: Monday, March 22, 2010 - 7:07 pm

[Empty message]
From: Christopher Heiny
Date: Monday, March 22, 2010 - 7:07 pm

Initial driver for Synaptics touchscreens using RMI4 protocol.

Signed-off-by: William Manson <WManson@synaptics.com>
Signed-off-by: Allie Xiong <axiong@synaptics.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
---

 drivers/input/touchscreen/Kconfig            |   13 +
 drivers/input/touchscreen/Makefile           |    1 +
 drivers/input/touchscreen/rmi.h              |  196 ++++++++
 drivers/input/touchscreen/rmi_app_touchpad.c |  355 +++++++++++++++
 drivers/input/touchscreen/rmi_core.c         |  632 ++++++++++++++++++++++++++
 drivers/input/touchscreen/rmi_core.h         |   57 +++
 drivers/input/touchscreen/rmi_function_11.c  |  352 ++++++++++++++
 drivers/input/touchscreen/rmi_function_11.h  |   39 ++
 drivers/input/touchscreen/rmi_functions.h    |  109 +++++
 drivers/input/touchscreen/rmi_i2c.h          |   54 +++
 drivers/input/touchscreen/rmi_i2c_gta01.c    |  122 +++++
 drivers/input/touchscreen/rmi_phys_i2c.c     |  545 ++++++++++++++++++++++
 12 files changed, 2475 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 8a8fa4d..ebd3abf 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -307,6 +307,19 @@ config TOUCHSCREEN_MIGOR
 	  To compile this driver as a module, choose M here: the
 	  module will be called migor_ts.
 
+config TOUCHSCREEN_SYNAPTICS_RMI4_I2C
+	tristate "Synaptics RMI4 I2C touchscreens"
+	depends on I2C
+	help
+	  Say Y here if you have a Synaptics RMI4 I2C touchscreen connected to
+	  your system. This enables support for Synaptics RMI4 over I2C based
+	  touchscreens.
+
+	  If unsure, say N.
+
+	  To compile this driver as a set of modules, choose M here: the
+	  modules will be called rmi, rmi_app_touchpad, rmi_phys_i2c.
+
 config TOUCHSCREEN_TOUCHRIGHT
 	tristate "Touchright serial touchscreen"
 	select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index ...
From: Jean Delvare
Date: Friday, April 2, 2010 - 5:50 am

Hi Christopher,

Sorry for the delay. Here finally come my comments on your code. I'm
only commenting on the i2c side of things.


This name is unfortunate. "client data" usually refers to run-time
state data of an i2c client. For configuration data, we rather use the

This is already included in the i2c client struct itself, so you


This is incorrect. What goes in device ID tables is _device_ names, nor
_driver_ names. It might be the same string, but you don't want to use

You don't need a copy of the i2c client in your own structure. It is
allocated by the i2c core, if you need a reference to it, then just
store a pointer thereto.

This is the only blocker point in your driver as far as I am concerned.

This works, but I would encourage you to use i2c_smbus_write_byte_data

Using i2c_smbus_read_byte_data() would look better, be more portable,

Likewise, i2c_smbus_read_i2c_block_data() would be better, if you never
need to read more than 32 bytes at once. If you need to, then
i2c_transfer() would still be better than i2c_master_send +
i2c_master_recv, as you avoid the downtime between the 2 messages on


Whould be <= 15, if I'm not mistaken. Using <= sizeof(txbuf_most) + 1

If you were using i2c_smbus_write_i2c_block_data(), you wouldn't have
to. Unless you must write more than 32 bytes sometimes? In that case,

If you keep that code, please just define "i" earlier in your function

That's what you get when hard-coding function names in log messages ;)

I am curious why your read functions have a retry mechanism and your






Now I am confused. Instead of attaching the proper platform data to
each I2C device, you have a single object with all the platform data
for all devices in it, and you pass that big object as the platform
data of all devices, and they have to find out which part is meant for
them? This is odd. What's the point of doing that? Why don't you just
set the platform data pointer to the right subset of data for every

What if no matching ...
From: Christopher Heiny
Date: Monday, April 5, 2010 - 4:04 pm

Hi Jean,

Thanks for the input!  It's quite helpful and clears up some things we 
were a bit puzzled about.  We'll fold most of your suggestions into the 
next patch.  Answers to some questions you raised will be coming shortly.

				Thanks again,
					Chris
--

From: =?ISO-8859-1?Q?Arve_Hj=F8nnev=E5g?=
Date: Monday, March 22, 2010 - 8:04 pm

On Mon, Mar 22, 2010 at 7:07 PM, Christopher Heiny <cheiny@synaptics.com> wrote:

Do you plan to add platform data to align the reported touchscreen
data with the screen behind it, or do the new hardware allow the the
firmware handle this? In the past we even needed separate parameters
for different firmware versions (seen in
drivers/staging/dream/synaptics_i2c_rmi.h).

-- 
Arve Hjønnevåg
--

From: Christopher Heiny
Date: Tuesday, March 23, 2010 - 12:18 pm

Hi Arve,

RMI4 touchscreens allow adjustment of the reported coordinate range (see 
the F11_2D_Ctrl6..9 registers, page 48 of the current version of the 
spec at http://www.synaptics.com/developers/manuals).  Using this 
feature, the device can be configured to report the same number of 
positions on each axis as there are pixels on the display.

We plan to make these settings accessible via sysfs, so that it can be 
dynamically tweaked if the user changes the display resolution (not 
likely on a phone, probable on a tablet/slate/ereader type device). 
Assuming there are no significant issues with our current patch, we plan 
to include that in the next one.  We're holding off that implementation 
because we're still finding our feet on the submission process, and 
wanted to keep the initial submits small so changes would be more 
manageable.

Coordinate rotation/reflection settings will be handled at the driver 
level, again via sysfs.

These features should be independent of the touchscreen firmware level. 
  Initial settings for these features should probably be done at the 
platform level.

					Chris
--

From: =?ISO-8859-1?Q?Arve_Hj=F8nnev=E5g?=
Date: Tuesday, March 23, 2010 - 3:35 pm

This does not help aligning the touchscreen values with the screen
behind it. It just moves the linear scaling from userspace (which can
use fixed or floating point values to preserve subpixel precision) to

Do you also have a plan to let the userspace know that touchscreen
coordinate x1,y1 correspond to screen coordinate 0,0 and x2,y2
correspond to screen coordinate xmax,ymax? The android driver sets
absmin/max to the values reported when touching the display edges (not
the actual min and max that the touchscreen can report), but other

In the past they have depended on the firmware version for two
reasons. On one product, firmware changes to improve the edges of the
screen completely changed the relationship between values reported and
the physical touch location. On another product, the physical size of
the sensor changed, and the firmware version was used to detect this.
If all RMI4 based product allow field updates of the firmware the
first case it less important, but we still need to cover the second



-- 
Arve Hjønnevåg
--

From: Christopher Heiny
Date: Tuesday, March 23, 2010 - 6:17 pm

Hi Arve,

It sounds like your concern is for cases when the origin of the 
touchscreen coordinates does not correspond to a corner of the pixel 
area.  Is that correct?

In any case, it's a perfectly valid issue - not all manufacturers take 
care to map the touchscreen to the display screen that way (though most 
do).  Adding a translation control to the driver would be easy - we'll 

It's more the other direction - we were concerned (validly, it turned 
out) that some extensive changes might be required as a result of 
feedback on the initial submissions, and wanted to keep the codebase 
we'd have to refactor small.

As the codebase grows, we'll switch to using patch series.  Probably 

We are not planning on that, since it would require the driver to know 
the orientation (standard? rot 90? rot -90? rot 180?) and resolution of 
the display and track whenever that changes.  It is better to handle 
that information at a higher level, which can then tell the touchscreen 


Hmmmm.  I can see a lot of other cases where it might be desirable to 
know the size of the touchscreen in a platform independent manner. 
Certainly the firmware version is not a reliable way to do this going 
forward.  I will contact the spec maintainer and see if we can have the 
device report the relative information in a query.

				Thanks,
					Chris
--

Previous thread: shutdown bugs, since about 2.6.31 or so by Gene Heskett on Monday, March 22, 2010 - 5:50 pm. (4 messages)

Next thread: linux-next: manual merge of the kgdb tree with the input tree by Stephen Rothwell on Monday, March 22, 2010 - 8:09 pm. (3 messages)