Prev: perf tools: Make target to generate self contained source tarball
Next: [PATCH] act_nat: fix the wrong checksum when addr isn't in old_addr/mask
From: Henrik Rydberg on 29 May 2010 04:00
Christopher Heiny wrote:
> This patch adds an initial driver supporting Synaptics ClearPad
> touchscreens that use the RMI4 protocol, as defined here:

Did the actual patch go astray? I cannot find it on the mailing lists.

Henrik

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on 29 May 2010 06:10
On Sat, 29 May 2010 09:54:54 +0200, Henrik Rydberg wrote:
> Christopher Heiny wrote:
> > This patch adds an initial driver supporting Synaptics ClearPad
> > touchscreens that use the RMI4 protocol, as defined here:
>
> Did the actual patch go astray? I cannot find it on the mailing lists.

I received it, at least. It was pretty large, maybe got filtered out on
some lists because of this.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on 29 May 2010 08:40
Hi Christopher,

On Fri, 28 May 2010 17:29:40 -0700, Christopher Heiny wrote:
> Initial driver for Synaptics touchscreens using RMI4 protocol.
>
> Signed-off-by: William Manson <WManson(a)synaptics.com>
> Signed-off-by: Allie Xiong <axiong(a)synaptics.com>
> Signed-off-by: Christopher Heiny <cheiny(a)synaptics.com>

You can add:

Acked-by: Jean Delvare <khali(a)linux-fr.org>

for the i2c parts. I still have a few comments you might be interested
in, maybe for a future incremental patch:

> (...)
> --- /dev/null
> +++ b/drivers/input/touchscreen/rmi_i2c_gta01.c
> @@ -0,0 +1,117 @@
> (...)
> +static void
> +rmi_i2c_release(struct device *dev)
> +{
> + struct platform_device *pd = container_of(dev,
> + struct platform_device, dev);

You could use to_platform_device(dev) instead, it's more readable.

> (...)
> --- /dev/null
> +++ b/drivers/input/touchscreen/rmi_phys_i2c.c
> (...)
> +/* TODO: for multiple device support will need a per-device mutex */

This comment would be better placed below, where page_mutex is declared.

> +#define DRIVER_NAME "rmi4-i2c"
> +
> +/* TODO: for multiple device support will need a per-device device name */

This comment is confusing... you would need different names if you were
supporting different device _types_ in the same driver. But you
definitely do _not_ need different names to support several devices of
the same type in a given system.

> +#define DEVICE_NAME "rmi4-i2c"

The use of dashes in i2c device names is strongly discouraged.
Including "i2c" in these names is discouraged as well, as it is
redundant. "rmi4_ts" would be a better name IMHO.

> (...)
> +static int
> +rmi_i2c_probe(struct i2c_client *client, const struct i2c_device_id *dev_id)
> +{
> + struct instance_data *id;
> + int retval = 0;
> + int i;
> + bool found = false;
> +
> + struct rmi_i2c_data *rmii2cdata;
> + struct rmi_i2c_platformdata *platformdata;
> +
> + pr_debug("Probing i2c RMI device\n");
> +
> + /* Allocate and initialize the instance data for this client */
> + id = kzalloc(sizeof(*id) * 2, GFP_KERNEL);

I still don't get the * 2.

> (...)
> + /* cast to our struct rmi_i2c_data so we know
> + the fields (see rmi_ic2.h) */
> + rmii2cdata = ((struct rmi_i2c_data *)(client->dev.platform_data));

Explicit cast still not needed, you can just write:

rmii2cdata = client->dev.platform_data;

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Henrik Rydberg on 29 May 2010 10:50
Jean Delvare wrote:
> On Sat, 29 May 2010 09:54:54 +0200, Henrik Rydberg wrote:
>> Christopher Heiny wrote:
>>> This patch adds an initial driver supporting Synaptics ClearPad
>>> touchscreens that use the RMI4 protocol, as defined here:
>> Did the actual patch go astray? I cannot find it on the mailing lists.
>
> I received it, at least. It was pretty large, maybe got filtered out on
> some lists because of this.
>

Yes, probably. Got it via mail, thank you Christopher.

Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Henrik Rydberg on 29 May 2010 16:40
Christopher Heiny wrote:
> Initial driver for Synaptics touchscreens using RMI4 protocol.
>
> Signed-off-by: William Manson <WManson(a)synaptics.com>
> Signed-off-by: Allie Xiong <axiong(a)synaptics.com>
> Signed-off-by: Christopher Heiny <cheiny(a)synaptics.com>
> ---

Thank you for this driver. Some scattered comments on the MT parts below.

Henrik

[...]
> +int FN_11_report(struct rmi_application *app,
> + struct rmi_function_info *rfi, struct input_dev *input)
> +{
> + unsigned char values[2] = {0, 0};
> + unsigned char data[12] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
> + /* number of touch points - fingers down in this case */
> + int fingerDownCount;
> + int X, Y, Z, W, Wy, Wx;
> + int finger;
> + int fn11FingersSupported;
> + int fn11FingerRegisters;
> + unsigned short fn11DataBaseAddr;
> + unsigned char fn11DataRegBlockSize;
> + static bool wasdown;
> +
> + fingerDownCount = 0;
> +
> + /* get 2D sensor finger data */
> +
> + /* First get the finger status field - the size of the finger status
> + field is determined by the number of finger supporte - 2 bits per
> + finger, so the number of registers to read is:
> + registerCount = ceil(numberOfFingers/4).
> + Read the required number of registers and check each 2 bit field to
> + determine if a finger is down:
> + 00 = finger not present,
> + 01 = finger present and data accurate,
> + 10 = finger present but data may not be accurate,
> + 11 = reserved for product use.
> + */
> + fn11FingersSupported = rfi->numDataPoints;
> + fn11FingerRegisters = (fn11FingersSupported + 3)/4;
> +
> + fn11DataBaseAddr = rfi->funcDescriptor.dataBaseAddr;
> +
> + if (rmi_read_multiple(app, fn11DataBaseAddr, values,
> + fn11FingerRegisters)) {
> + printk(KERN_ERR "%s: RMI4 function $11 report: "
> + "Could not read finger status registers 0x%x\n",
> + __func__, fn11DataBaseAddr);
> + return 0;

The in-kernel rmi interface seems to expect the number of touches to be returned
here, but it does not seem to be used anywhere. Would it not be more useful to
return zero on success, and error codes on failure?

> + }
> +
> + /* For each finger present, read the proper number of registers
> + to get absolute data. */
> + fn11DataRegBlockSize = rfi->dataRegBlockSize;
> +
> + for (finger = 0; finger < fn11FingersSupported; finger++) {
> + int reg;
> + int fingerShift;
> + int fingerStatus;
> +
> + /* determine which data byte the finger status is in */
> + reg = finger/4;
> + /* bit shift to get finger's status */
> + fingerShift = (finger % 4) * 2;
> + fingerStatus = (values[reg] >> fingerShift) & 3;
> +
> + /* if finger status indicates a finger is present then
> + read the finger data and report it */
> + if (fingerStatus == 1 || fingerStatus == 2) {
> + /* number of active touch points not same as
> + number of supported fingers */
> + fingerDownCount++;
> +
> + /* Read the finger data */
> + if (rmi_read_multiple(app, fn11DataBaseAddr +
> + ((finger * fn11DataRegBlockSize) +
> + fn11FingerRegisters),
> + data, fn11DataRegBlockSize)) {
> + printk(KERN_ERR "%s: RMI4 function $11 report: "
> + "Could not read finger data registers "
> + "0x%x\n", __func__,
> + fn11DataBaseAddr +
> + ((finger * fn11DataRegBlockSize) +
> + fn11FingerRegisters));
> + return 0;

Bailing out without finishing off the input packet...

> + } else {
> + X = (data[0] & 0x1f) << 4;
> + X |= data[2] & 0xf;
> + Y = (data[1] & 0x1f) << 4;
> + Y |= (data[2] >> 4) & 0xf;
> + W = data[3];
> +
> + /* upper 4 bits of W are Wy,
> + lower 4 of W are Wx */
> + Wy = (W >> 4) & 0x0f;
> + Wx = W & 0x0f;
> +
> + Z = data[4];
> +
> + /* if this is the first finger report normal
> + ABS_X, ABS_Y, PRESSURE, TOOL_WIDTH events for
> + non-MT apps. Apps that support Multi-touch
> + will ignore these events and use the MT events.
> + Apps that don't support Multi-touch will still
> + function.
> + */
> + if (fingerDownCount == 1) {
> + input_report_abs(input, ABS_X, X);
> + input_report_abs(input, ABS_Y, Y);
> + input_report_abs(input, ABS_PRESSURE,
> + Z);
> + input_report_abs(input, ABS_TOOL_WIDTH,
> + max(Wx, Wy));
> + input_report_key(input, BTN_TOUCH, 1);
> + wasdown = true;
> + }
> +
> + /* Report Multi-Touch events for each finger */
> + /* major axis of touch area ellipse */
> + input_report_abs(input, ABS_MT_TOUCH_MAJOR,
> + max(Wx, Wy));
> + /* minor axis of touch area ellipse */
> + input_report_abs(input, ABS_MT_TOUCH_MINOR,
> + min(Wx, Wy));
> + /* Currently only 2 supported - 1 or 0 */
> + input_report_abs(input, ABS_MT_ORIENTATION,
> + (Wx > Wy ? 1 : 0));
> + input_report_abs(input, ABS_MT_POSITION_X, X);
> + input_report_abs(input, ABS_MT_POSITION_Y, Y);
> + /* Tracking ID reported but not used yet */
> + input_report_abs(input, ABS_MT_TRACKING_ID,
> + finger+1);

The tracking id used here is not entirely proper, since the id seems to be
reused too often.

Does the position in the finger array (0..fn11FingersSupported) really track an
identified contact, as suggested by the code? If so, a proper tracking id could
be formed by keeping an id per position, and assigning a new id when the
fingerStatus changes for that position.

> + /* MT sync between fingers */
> + input_mt_sync(input);
> + }
> + }
> + }
> +
> + if (fingerDownCount) {
> + /* touch will be non-zero if we had any reported events */
> + input_sync(input); /* sync after groups of events */
> + } else {
> + /* if we had a finger down before and now we don't have
> + any we need to send a button up and a sync. */
> + if (wasdown) {
> + wasdown = false;
> + input_report_key(input, BTN_TOUCH, 0);
> + input_sync(input); /* sync after groups of events */
> + }
> + }

input_sync() should always be called at the end, regardless. If nothing changed,
the input core will filter it out.

> +
> + /* return the number of touch points: fingers down or buttons pressed */
> + return fingerDownCount;
> +}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: perf tools: Make target to generate self contained source tarball
Next: [PATCH] act_nat: fix the wrong checksum when addr isn't in old_addr/mask