| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20121104215335.GA1060@polaris.bitmath.org>
Date: Sun, 4 Nov 2012 22:53:35 +0100
From: "Henrik Rydberg" <rydberg@...omail.se>
To: Alexandra Chin <Alexandra.Chin@...synaptics.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Linux Kernel <linux-kernel@...r.kernel.org>,
Linux Input <linux-input@...r.kernel.org>,
Linus Walleij <linus.walleij@...ricsson.com>,
Naveen Kumar Gaddipati <naveen.gaddipati@...ricsson.com>,
Mahesh Srinivasan <msrinivasan@...aptics.com>,
Alex Chang <alex.chang@...synaptics.com>,
Scott Lin <scott.lin@...synaptics.com>,
Christopher Heiny <Cheiny@...aptics.com>
Subject: Re: [PATCH v3] staging: ste_rmi4: Convert to Type-B support
Hi Alexandra,
Thanks for making changes.
> This patch converts to MT-B because Synaptics touch devices are
> capable of tracking identifiable fingers
>
> This patch was tested on pandaboard, except input_mt_sync_frame(),
> which is a quite new function.
I am not sure how to interpret this. Is the patch untested, or tested
on something different from a pandaboard?
> I changed to use sylpheed as my mail client. Please let me know
> if there is any problem.
> Greatly appreciate your time.
As mentioned, the commit message of the patch should not contain mail conversation.
> drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c | 122 ++++++++++++-------------
> 1 files changed, 57 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> index 277491a..7876f6b 100644
> --- a/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> +++ b/drivers/staging/ste_rmi4/synaptics_i2c_rmi4.c
> @@ -1,7 +1,7 @@
> /**
> *
> - * Synaptics Register Mapped Interface (RMI4) I2C Physical Layer Driver.
> - * Copyright (c) 2007-2010, Synaptics Incorporated
> + * Synaptics Register Mapped Interface (RMI4) I2C Touchscreen Driver.
> + * Copyright (c) 2007-2012, Synaptics Incorporated
> *
> * Author: Js HA <js.ha@...ricsson.com> for ST-Ericsson
> * Author: Naveen Kumar G <naveen.gaddipati@...ricsson.com> for ST-Ericsson
> @@ -31,6 +31,7 @@
> #include <linux/interrupt.h>
> #include <linux/regulator/consumer.h>
> #include <linux/module.h>
> +#include <linux/input/mt.h>
> #include "synaptics_i2c_rmi4.h"
>
> /* TODO: for multiple device support will need a per-device mutex */
> @@ -63,12 +64,11 @@
> #define MASK_4BIT 0x0F
> #define MASK_3BIT 0x07
> #define MASK_2BIT 0x03
> -#define TOUCHPAD_CTRL_INTR 0x8
> +#define TOUCHSCREEN_CTRL_INTR 0x8
> #define PDT_START_SCAN_LOCATION (0x00E9)
> #define PDT_END_SCAN_LOCATION (0x000A)
> #define PDT_ENTRY_SIZE (0x0006)
> -#define RMI4_NUMBER_OF_MAX_FINGERS (8)
> -#define SYNAPTICS_RMI4_TOUCHPAD_FUNC_NUM (0x11)
> +#define SYNAPTICS_RMI4_TOUCHSCREEN_FUNC_NUM (0x11)
> #define SYNAPTICS_RMI4_DEVICE_CONTROL_FUNC_NUM (0x01)
>
> /**
> @@ -164,6 +164,7 @@ struct synaptics_rmi4_device_info {
> * @regulator: pointer to the regulator structure
> * @wait: wait queue structure variable
> * @touch_stopped: flag to stop the thread function
> + * @fingers_supported: maximum supported fingers
> *
> * This structure gives the device data information.
> */
> @@ -184,6 +185,7 @@ struct synaptics_rmi4_data {
> struct regulator *regulator;
> wait_queue_head_t wait;
> bool touch_stopped;
> + unsigned char fingers_supported;
> };
>
> /**
> @@ -291,34 +293,33 @@ exit:
> }
>
> /**
> - * synpatics_rmi4_touchpad_report() - reports for the rmi4 touchpad device
> + * synpatics_rmi4_touchscreen_report() - reports for the rmi4 touchscreen device
> * @pdata: pointer to synaptics_rmi4_data structure
> * @rfi: pointer to synaptics_rmi4_fn structure
> *
> - * This function calls to reports for the rmi4 touchpad device
> + * This function calls to reports for the rmi4 touchscreen device
> */
> -static int synpatics_rmi4_touchpad_report(struct synaptics_rmi4_data *pdata,
> +static int synpatics_rmi4_touchscreen_report(struct synaptics_rmi4_data *pdata,
> struct synaptics_rmi4_fn *rfi)
> {
> /* number of touch points - fingers down in this case */
> int touch_count = 0;
> int finger;
> - int fingers_supported;
> int finger_registers;
> int reg;
> int finger_shift;
> int finger_status;
> int retval;
> + int x, y;
> + int wx, wy;
> unsigned short data_base_addr;
> unsigned short data_offset;
> unsigned char data_reg_blk_size;
> unsigned char values[2];
> unsigned char data[DATA_LEN];
> - int x[RMI4_NUMBER_OF_MAX_FINGERS];
> - int y[RMI4_NUMBER_OF_MAX_FINGERS];
> - int wx[RMI4_NUMBER_OF_MAX_FINGERS];
> - int wy[RMI4_NUMBER_OF_MAX_FINGERS];
> + unsigned char fingers_supported = pdata->fingers_supported;
> struct i2c_client *client = pdata->i2c_client;
> + struct input_dev *input_dev = pdata->input_dev;
The patch is definitely an improvement over the existing code, but I
would very much recommend a follow-up patch which splits this
function.
>
> /* get 2D sensor finger data */
> /*
> @@ -333,7 +334,6 @@ static int synpatics_rmi4_touchpad_report(struct synaptics_rmi4_data *pdata,
> * 10 = finger present but data may not be accurate,
> * 11 = reserved for product use.
> */
> - fingers_supported = rfi->num_of_data_points;
> finger_registers = (fingers_supported + 3)/4;
> data_base_addr = rfi->fn_desc.data_base_addr;
> retval = synaptics_rmi4_i2c_block_read(pdata, data_base_addr, values,
> @@ -353,12 +353,16 @@ static int synpatics_rmi4_touchpad_report(struct synaptics_rmi4_data *pdata,
> reg = finger/4;
> /* bit shift to get finger's status */
> finger_shift = (finger % 4) * 2;
> - finger_status = (values[reg] >> finger_shift) & 3;
> + finger_status = (values[reg] >> finger_shift) & MASK_2BIT;
> /*
> * if finger status indicates a finger is present then
> * read the finger data and report it
> */
> - if (finger_status == 1 || finger_status == 2) {
> + input_mt_slot(input_dev, finger);
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
> + finger_status != 0);
> +
> + if (finger_status) {
> /* Read the finger data */
> data_offset = data_base_addr +
> ((finger * data_reg_blk_size) +
> @@ -367,50 +371,33 @@ static int synpatics_rmi4_touchpad_report(struct synaptics_rmi4_data *pdata,
> data_offset, data,
> data_reg_blk_size);
> if (retval != data_reg_blk_size) {
> - printk(KERN_ERR "%s:read data failed\n",
> + dev_err(&client->dev, "%s:read data failed\n",
> __func__);
> return 0;
> - } else {
> - x[touch_count] =
> - (data[0] << 4) | (data[2] & MASK_4BIT);
> - y[touch_count] =
> - (data[1] << 4) |
> - ((data[2] >> 4) & MASK_4BIT);
> - wy[touch_count] =
> - (data[3] >> 4) & MASK_4BIT;
> - wx[touch_count] =
> - (data[3] & MASK_4BIT);
> -
> - if (pdata->board->x_flip)
> - x[touch_count] =
> - pdata->sensor_max_x -
> - x[touch_count];
> - if (pdata->board->y_flip)
> - y[touch_count] =
> - pdata->sensor_max_y -
> - y[touch_count];
> }
> + x = (data[0] << 4) | (data[2] & MASK_4BIT);
> + y = (data[1] << 4) | ((data[2] >> 4) & MASK_4BIT);
> + wy = (data[3] >> 4) & MASK_4BIT;
> + wx = (data[3] & MASK_4BIT);
> +
> + if (pdata->board->x_flip)
> + x = pdata->sensor_max_x - x;
> + if (pdata->board->y_flip)
> + y = pdata->sensor_max_y - y;
> +
> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> + max(wx, wy));
> + input_report_abs(input_dev, ABS_MT_POSITION_X, x);
> + input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> +
> /* number of active touch points */
> touch_count++;
> }
> }
>
> - /* report to input subsystem */
> - if (touch_count) {
> - for (finger = 0; finger < touch_count; finger++) {
> - input_report_abs(pdata->input_dev, ABS_MT_TOUCH_MAJOR,
> - max(wx[finger] , wy[finger]));
> - input_report_abs(pdata->input_dev, ABS_MT_POSITION_X,
> - x[finger]);
> - input_report_abs(pdata->input_dev, ABS_MT_POSITION_Y,
> - y[finger]);
> - input_mt_sync(pdata->input_dev);
> - }
> - } else
> - input_mt_sync(pdata->input_dev);
> -
> /* sync after groups of events */
> - input_sync(pdata->input_dev);
> + input_mt_sync_frame(input_dev);
> + input_sync(input_dev);
> /* return the number of touch points */
> return touch_count;
> }
> @@ -428,13 +415,13 @@ static int synaptics_rmi4_report_device(struct synaptics_rmi4_data *pdata,
> int touch = 0;
> struct i2c_client *client = pdata->i2c_client;
> static int num_error_reports;
> - if (rfi->fn_number != SYNAPTICS_RMI4_TOUCHPAD_FUNC_NUM) {
> + if (rfi->fn_number != SYNAPTICS_RMI4_TOUCHSCREEN_FUNC_NUM) {
> num_error_reports++;
> if (num_error_reports < MAX_ERROR_REPORT)
> dev_err(&client->dev, "%s:report not supported\n",
> __func__);
> } else
> - touch = synpatics_rmi4_touchpad_report(pdata, rfi);
> + touch = synpatics_rmi4_touchscreen_report(pdata, rfi);
> return touch;
> }
> /**
> @@ -510,15 +497,15 @@ static irqreturn_t synaptics_rmi4_irq(int irq, void *data)
> }
>
> /**
> - * synpatics_rmi4_touchpad_detect() - detects the rmi4 touchpad device
> + * synpatics_rmi4_touchscreen_detect() - detects the rmi4 touchscreen device
> * @pdata: pointer to synaptics_rmi4_data structure
> * @rfi: pointer to synaptics_rmi4_fn structure
> * @fd: pointer to synaptics_rmi4_fn_desc structure
> * @interruptcount: count the number of interrupts
> *
> - * This function calls to detects the rmi4 touchpad device
> + * This function calls to detects the rmi4 touchscreen device
> */
> -static int synpatics_rmi4_touchpad_detect(struct synaptics_rmi4_data *pdata,
> +static int synpatics_rmi4_touchscreen_detect(struct synaptics_rmi4_data *pdata,
> struct synaptics_rmi4_fn *rfi,
> struct synaptics_rmi4_fn_desc *fd,
> unsigned int interruptcount)
> @@ -575,6 +562,7 @@ static int synpatics_rmi4_touchpad_detect(struct synaptics_rmi4_data *pdata,
> if ((queries[1] & MASK_3BIT) == 5)
> rfi->num_of_data_points = 10;
> }
> + pdata->fingers_supported = rfi->num_of_data_points;
> /* Need to get interrupt info for handling interrupts */
> rfi->index_to_intr_reg = (interruptcount + 7)/8;
> if (rfi->index_to_intr_reg != 0)
> @@ -655,13 +643,13 @@ static int synpatics_rmi4_touchpad_detect(struct synaptics_rmi4_data *pdata,
> }
>
> /**
> - * synaptics_rmi4_touchpad_config() - configures the rmi4 touchpad device
> + * synaptics_rmi4_touchscreen_config() - configures the rmi4 touchscreen device
> * @pdata: pointer to synaptics_rmi4_data structure
> * @rfi: pointer to synaptics_rmi4_fn structure
> *
> - * This function calls to configures the rmi4 touchpad device
> + * This function calls to configures the rmi4 touchscreen device
> */
> -int synaptics_rmi4_touchpad_config(struct synaptics_rmi4_data *pdata,
> +int synaptics_rmi4_touchscreen_config(struct synaptics_rmi4_data *pdata,
> struct synaptics_rmi4_fn *rfi)
> {
> /*
> @@ -751,7 +739,7 @@ static int synaptics_rmi4_i2c_query_device(struct synaptics_rmi4_data *pdata)
> pdata->fn01_data_base_addr =
> rmi_fd.data_base_addr;
> break;
> - case SYNAPTICS_RMI4_TOUCHPAD_FUNC_NUM:
> + case SYNAPTICS_RMI4_TOUCHSCREEN_FUNC_NUM:
> if (rmi_fd.intr_src_count) {
> rfi = kmalloc(sizeof(*rfi),
> GFP_KERNEL);
> @@ -761,7 +749,8 @@ static int synaptics_rmi4_i2c_query_device(struct synaptics_rmi4_data *pdata)
> __func__);
> return -ENOMEM;
> }
> - retval = synpatics_rmi4_touchpad_detect
> + retval =
> + synpatics_rmi4_touchscreen_detect
> (pdata, rfi,
> &rmi_fd,
> intr_count);
Odd line break is a clear sign that something could be broken out into its own function.
> @@ -854,8 +843,9 @@ static int synaptics_rmi4_i2c_query_device(struct synaptics_rmi4_data *pdata)
> list_for_each_entry(rfi, &rmi->support_fn_list, link) {
> if (rfi->num_of_data_sources) {
> if (rfi->fn_number ==
> - SYNAPTICS_RMI4_TOUCHPAD_FUNC_NUM) {
> - retval = synaptics_rmi4_touchpad_config
> + SYNAPTICS_RMI4_TOUCHSCREEN_FUNC_NUM) {
> + retval =
> + synaptics_rmi4_touchscreen_config
> (pdata, rfi);
> if (retval < 0)
> return retval;
Same here.
> @@ -988,6 +978,8 @@ static int __devinit synaptics_rmi4_probe
> rmi4_data->sensor_max_y, 0, 0);
> input_set_abs_params(rmi4_data->input_dev, ABS_MT_TOUCH_MAJOR, 0,
> MAX_TOUCH_MAJOR, 0, 0);
> + input_mt_init_slots(rmi4_data->input_dev,
> + rmi4_data->fingers_supported, INPUT_MT_DIRECT);
>
> /* Clear interrupts */
> synaptics_rmi4_i2c_block_read(rmi4_data,
> @@ -1076,7 +1068,7 @@ static int synaptics_rmi4_suspend(struct device *dev)
>
> retval = synaptics_rmi4_i2c_byte_write(rmi4_data,
> rmi4_data->fn01_ctrl_base_addr + 1,
> - (intr_status & ~TOUCHPAD_CTRL_INTR));
> + (intr_status & ~TOUCHSCREEN_CTRL_INTR));
> if (retval < 0)
> return retval;
>
> @@ -1112,7 +1104,7 @@ static int synaptics_rmi4_resume(struct device *dev)
>
> retval = synaptics_rmi4_i2c_byte_write(rmi4_data,
> rmi4_data->fn01_ctrl_base_addr + 1,
> - (intr_status | TOUCHPAD_CTRL_INTR));
> + (intr_status | TOUCHSCREEN_CTRL_INTR));
> if (retval < 0)
> return retval;
>
> --
> 1.7.5.4
>
Ok, with a final polish and a proper commit message, this looks like it will work.
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists