Re: [PATCH] input:cyapa: Add new I2C-based input touchpad driver for Cypress I2C touchpad devices
Alan Cox
Dudley Du <du...@cypress.com> wrote:
> Dear Linux-Input Maintainers,
>
> Attached patch files are used to add new I2C-based input touchpad device driver for Cypress I2C touchpad devices.
> Please help review them.
Please read Documentation/SubmittingPatches in particular #7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Alan Cox
> + * Status of the cyapa device detection worker.
> + * The worker is started at driver initialization and
> + * resume from system sleep.
> + */
> +enum cyapa_detect_status {
> + CYAPA_DETECT_DONE_SUCCESS,
> + CYAPA_DETECT_DONE_FAILED,
> +};
So a bool is_detected would be much more obvious
> +struct cyapa_reg_data_gen3 {
> + /*
> + * bit 0 - 1: device status
> + * bit 3 - 2: power mode
> + * bit 6 - 4: reserved
> + * bit 7: interrupt valid bit
> + */
> + u8 device_status;
> + /*
> + * bit 7 - 4: number of fingers currently touching pad
> + * bit 3: valid data check bit
> + * bit 2: middle mechanism button state if exists
> + * bit 1: right mechanism button state if exists
> + * bit 0: left mechanism button state if exists
> + */
> + u8 finger_btn;
> + struct cyapa_touch_gen3 touches[CYAPA_MAX_TOUCHES];
> +};
If this is a hardware format you might want to check how different
platforms and compiler settings lay it out - eg if they'll put two bytes
of padding between finger_btn and the struct.
> +
> +union cyapa_reg_data {
> + struct cyapa_reg_data_gen3 gen3_data;
> +};
A union of one struct.. wants cleaning up ....
> +/* The main device structure */
> +struct cyapa_i2c {
> + /* synchronize i2c bus operations. */
> + struct semaphore reg_io_sem;
Use mutexes unless you need to the counting behaviour, otherwise it
messes up real time Linux handling
> +
> +/*
> + * When requested IRQ number is not available, the trackpad driver
> + * falls back to using polling mode.
> + * In this case, do not actually enable/disable irq.
> + */
> +static void cyapa_enable_irq(struct cyapa_i2c *touch)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> + if (!touch->polling_mode_enabled &&
> + touch->bl_irq_enable &&
> + !touch->irq_enabled) {
> + touch->irq_enabled = true;
> + enable_irq(touch->irq);
> + }
> + spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
> +}
For plling mode the kernel supports a polled input device type which will
do most of your polling work for you
> +
> +static void cyapa_disable_irq(struct cyapa_i2c *touch)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> + if (!touch->polling_mode_enabled &&
> + touch->bl_irq_enable &&
> + touch->irq_enabled) {
> + touch->irq_enabled = false;
> + disable_irq(touch->irq);
> + }
> + spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
This doesn't do what you think. disable_irq does not guarantee that the
IRQ is not executing on another CPU core. disable_irq_sync does but then
you will deadlock.
> +}
> +
> +static int cyapa_acquire_i2c_bus(struct cyapa_i2c *touch)
> +{
> + cyapa_disable_irq(touch);
> + if (down_interruptible(&touch->reg_io_sem)) {
> + cyapa_enable_irq(touch);
> + return -ERESTARTSYS;
> + }
> +
> + return 0;
This may be running in many contexts in the workqueue so how does
allowing ^C and other signal handling make sense - especially when you
then don't seem to handle signal triggered returns ?
> + if (ret != length)
> + pr_warning("warning I2C block read bytes" \
> + "[%d] not equal to requested bytes [%d].\n",
> + ret, length);
Prefer dev_warn and friends then the user can relate a message to a
device more easily.
> + * In trackpad device, the memory block allocated for I2C register map
> + * is 256 bytes, so the max write block for I2C bus is 256 bytes.
Which is too big to throw on the stack. But you don't need the buffer
anyway. If you required the caller to leave the firsg byte blank you'd
save a large copy and could just fill that one byte in then send.
> + ret = cyapa_i2c_reg_read_block(touch, 0, BL_HEAD_BYTES, status);
> + if ((ret != BL_HEAD_BYTES) && (tries > 0)) {
And here you don't deal with the signal case you've made yourself need to
handle by using down_interruptible
> +static void cyapa_update_firmware_dispatch(struct cyapa_i2c *touch)
> +{
> + /* do something here to update trackpad firmware. */
> +}
???
> +static void cyapa_get_reg_offset(struct cyapa_i2c *touch)
> +{
> + touch->data_base_offset = GEN3_REG_OFFSET_DATA_BASE;
> + touch->control_base_offset = GEN3_REG_OFFSET_CONTROL_BASE;
> + touch->command_base_offset = GEN3_REG_OFFSET_COMMAND_BASE;
> + touch->query_base_offset = GEN3_REG_OFFSET_QUERY_BASE;
> +}
These seem to be constant so why all the offset variables, and why pick
such incredibly long names to type ?
> +static irqreturn_t cyapa_i2c_irq(int irq, void *dev_id)
> +{
> + struct cyapa_i2c *touch = dev_id;
> +
> + cyapa_i2c_reschedule_work(touch, 0);
> +
> + return IRQ_HANDLED;
> +}
Take a look at request_threaded_irq() I think that will solve a lot of
your problems and mess around this, along with the polled input device
for the non IRQ case
+
> +static int cyapa_i2c_open(struct input_dev *input)
> +{
> + struct cyapa_i2c *touch = input_get_drvdata(input);
> + int ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&touch->lock, flags);
> + if (touch->open_count == 0) {
> + ret = cyapa_i2c_reset_config(touch);
> + if (ret < 0) {
> + pr_err("reset i2c trackpad error code, %d.\n", ret);
> + return ret;
> + }
> + }
> + spin_unlock_irqrestore(&touch->lock, flags);
> +
> + spin_lock_irqsave(&touch->lock, flags);
> + touch->open_count++;
> + spin_unlock_irqrestore(&touch->lock, flags);
You've dropped the lock and taken it and dropped it again in sequential
lines. That's nonsensical and also means you've added a race !
> +static void cyapa_i2c_close(struct input_dev *input)
> +{
> + unsigned long flags;
> + struct cyapa_i2c *touch = input_get_drvdata(input);
> +
> + spin_lock_irqsave(&touch->lock, flags);
> +
> + if (touch->open_count == 0) {
Wouldn't this be an error ?
> + ret = request_irq(touch->irq,
> + cyapa_i2c_irq,
> + IRQF_TRIGGER_FALLING,
> + CYAPA_I2C_NAME,
> + touch);
This IRQ can happen from the moment it is registered which means it can
occur before the variables you set up further down...
> + if (ret) {
> + pr_warning("IRQ request failed: %d, "
> + "falling back to polling mode.\n", ret);
> +
> + spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> + touch->polling_mode_enabled = true;
> + touch->bl_irq_enable = false;
> + touch->irq_enabled = false;
> + spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
> + } else {
> + spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> + touch->polling_mode_enabled = false;
> + touch->bl_irq_enable = false;
> + touch->irq_enabled = true;
> + enable_irq_wake(touch->irq);
> + spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
> + }
> +
> + /* create an input_dev instance for trackpad device. */
> + ret = cyapa_create_input_dev(touch);
> + if (ret) {
> + free_irq(touch->irq, touch);
But what if it was polling ?
In general I think
- use the threaded_irq interfaces for your IRQ paths (if you look at the
current kernel you'll see a lot of drivers do this)
- use the polled input device interface for the no IRQ case, so it does
all the polling work for you
- tidy up the locking (and the fact you've got locking in there is a lot
better than many first submissions we see)
I would think about how the various states and handlers work. Right now
the code is quite convoluted, and maybe a state machine of some kind would
clean it up ?
Dmitry Torokhov
> > +
> > +static void cyapa_disable_irq(struct cyapa_i2c *touch)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&touch->miscdev_spinlock, flags);
> > + if (!touch->polling_mode_enabled &&
> > + touch->bl_irq_enable &&
> > + touch->irq_enabled) {
> > + touch->irq_enabled = false;
> > + disable_irq(touch->irq);
> > + }
> > + spin_unlock_irqrestore(&touch->miscdev_spinlock, flags);
>
> This doesn't do what you think. disable_irq does not guarantee that the
> IRQ is not executing on another CPU core. disable_irq_sync does but then
> you will deadlock.
Actually disable_irq() does guarantee that IRQ is not executing once the
call completes. We have disable_irq_nosync() that does not wait.
> +
> > +static int cyapa_i2c_open(struct input_dev *input)
> > +{
> > + struct cyapa_i2c *touch = input_get_drvdata(input);
> > + int ret;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&touch->lock, flags);
> > + if (touch->open_count == 0) {
> > + ret = cyapa_i2c_reset_config(touch);
> > + if (ret < 0) {
> > + pr_err("reset i2c trackpad error code, %d.\n", ret);
> > + return ret;
> > + }
> > + }
> > + spin_unlock_irqrestore(&touch->lock, flags);
> > +
> > + spin_lock_irqsave(&touch->lock, flags);
> > + touch->open_count++;
> > + spin_unlock_irqrestore(&touch->lock, flags);
>
> You've dropped the lock and taken it and dropped it again in sequential
> lines. That's nonsensical and also means you've added a race !
>
Also please note that input core ensures that open() and close() methods
are called only when needed (first user opens the device or last user
closes it), you do not need to count yourself.
Polling mode for a touchscreen is unusable in real product (unlike
accelerometers/magnetometers/etc touchscreens need constant polling with
relatively high rate) so I'd rather not have it at all.
> - tidy up the locking (and the fact you've got locking in there is a lot
> better than many first submissions we see)
>
> I would think about how the various states and handlers work. Right now
> the code is quite convoluted, and maybe a state machine of some kind would
> clean it up ?
>
Also the miscdevice inteface should go away and be replaced with
sysfs/debugfs solution. For firmware update request_firmware() interface
can be used (see atmel_mxt_ts driver).
Thanks.
--
Dmitry
Henrik Rydberg
Thanks for the driver, please find some comments on the MT implementation below.
> diff --git a/drivers/input/mouse/cypress_i2c.c b/drivers/input/mouse/cypress_i2c.c
> new file mode 100644
> index 0000000..61d5f7f
> --- /dev/null
> +++ b/drivers/input/mouse/cypress_i2c.c
> @@ -0,0 +1,1721 @@
> +/*
> + * Cypress APA trackpad with I2C interface
> + *
> + * Copyright (C) 2009 Compulab, Ltd.
> + * Dudley Du <du...@cypress.com>
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License. See the file COPYING in the main directory of this archive for
> + * more details.
> + */
> +
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/uaccess.h>
> +#include <linux/miscdevice.h>
> +#include <linux/fs.h>
> +#include <linux/input/mt.h>
> +
> +#include <linux/cyapa.h>
> +
> +
> +/* DEBUG: debug switch macro */
> +#define DBG_CYAPA_READ_BLOCK_DATA 0
> +
> +
> +/*
> + * Cypress I2C APA trackpad driver version is defined as below:
> + * CYAPA_MAJOR_VER.CYAPA_MINOR_VER.CYAPA_REVISION_VER
> + */
> +#define CYAPA_MAJOR_VER 1
> +#define CYAPA_MINOR_VER 1
> +#define CYAPA_REVISION_VER 0
Versions are best handled by git.
> +
> +#define CYAPA_MT_MAX_TOUCH 255
> +#define CYAPA_MT_MAX_WIDTH 255
> +
> +#define MAX_FINGERS 5
> +#define CYAPA_TOOL_WIDTH 50
> +#define CYAPA_DEFAULT_TOUCH_PRESSURE 50
> +#define CYAPA_MT_TOUCH_MAJOR 50
Most of the constants above seem unused.
> +/*
> + * In the special case, where a finger is removed and makes contact
> + * between two packets, there will be two touches for that finger,
> + * with different tracking_ids.
> + * Thus, the maximum number of slots must be twice the maximum number
> + * of fingers.
> + */
> +#define MAX_MT_SLOTS (2 * MAX_FINGERS)
Alternatively, it could be equal to the number of tracking ids
recycled by the device, i.e., 16.
> +
> +/* When in IRQ mode read the device every THREAD_IRQ_SLEEP_SECS */
> +#define CYAPA_THREAD_IRQ_SLEEP_SECS 2
> +#define CYAPA_THREAD_IRQ_SLEEP_MSECS (CYAPA_THREAD_IRQ_SLEEP_SECS * MSEC_PER_SEC)
> +/*
> + * When in Polling mode and no data received for CYAPA_NO_DATA_THRES msecs
> + * reduce the polling rate to CYAPA_NO_DATA_SLEEP_MSECS
> + */
> +#define CYAPA_NO_DATA_THRES (MSEC_PER_SEC)
> +#define CYAPA_NO_DATA_SLEEP_MSECS (MSEC_PER_SEC / 4)
As Dmitry already pointed out, polling mode should be skipped altogether.
> + * APA trackpad device states.
> + * Used in register 0x00, bit1-0, DeviceStatus field.
> + */
> +#define CYAPA_MAX_TOUCHES (MAX_FINGERS)
Why two variables with the same meaning?
> +#define CYAPA_ONE_TIME_GESTURES (1)
Gestures should be removed since the are not propagated.
> +
> +struct cyapa_touch {
> + int x;
> + int y;
> + int pressure;
> + int tracking_id;
> +};
There is no need to store the touches separately, cyapa_touch_gen3 will suffice.
> +
> +struct cyapa_gesture {
> + u8 id;
> + u8 param1;
> + u8 param2;
> +};
This one falls out.
> +
> +struct cyapa_touch_gen3 {
> + /*
> + * high bits or x/y position value
> + * bit 7 - 4: high 4 bits of x position value
> + * bit 3 - 0: high 4 bits of y position value
> + */
> + u8 xy;
> + u8 x; /* low 8 bits of x position value. */
> + u8 y; /* low 8 bits of y position value. */
> + u8 pressure;
> + /*
> + * The range of tracking_id is 0 - 15,
> + * it is incremented every time a finger makes contact
> + * with the trackpad.
> + */
> + u8 tracking_id;
> +};
Since the set of tracking ids is so limited, the simplest
implementation is to equate the device tracking ids with the MT
slots. A book-keeping bit field can be used to keep track of what
slots should be cleared in a round.
> +
> +struct cyapa_reg_data_gen3 {
> + /*
> + * bit 0 - 1: device status
> + * bit 3 - 2: power mode
> + * bit 6 - 4: reserved
> + * bit 7: interrupt valid bit
> + */
> + u8 device_status;
> + /*
> + * bit 7 - 4: number of fingers currently touching pad
> + * bit 3: valid data check bit
> + * bit 2: middle mechanism button state if exists
> + * bit 1: right mechanism button state if exists
> + * bit 0: left mechanism button state if exists
> + */
> + u8 finger_btn;
> + struct cyapa_touch_gen3 touches[CYAPA_MAX_TOUCHES];
> +};
> +
> +union cyapa_reg_data {
> + struct cyapa_reg_data_gen3 gen3_data;
> +};
This one goes...
> +
> +struct cyapa_report_data {
> + u8 button;
> + u8 reserved1;
> + u8 reserved2;
> + u8 avg_pressure;
> + int rel_deltaX;
> + int rel_deltaY;
> +
> + int touch_fingers;
> + struct cyapa_touch touches[CYAPA_MAX_TOUCHES];
> +
> + int gesture_count;
> + struct cyapa_gesture gestures[CYAPA_ONE_TIME_GESTURES];
> +};
This struct wont be needed.
> +
> +
> +struct cyapa_mt_slot {
> + struct cyapa_touch contact;
> + bool touch_state; /* true: is touched, false: not touched. */
> + bool slot_updated;
> +};
This struct wont be needed.
> +
> +/* The main device structure */
> +struct cyapa_i2c {
> + /* synchronize i2c bus operations. */
> + struct semaphore reg_io_sem;
> + /* synchronize accessing members of cyapa_i2c data structure. */
> + spinlock_t miscdev_spinlock;
> + /* synchronize accessing and updating file->f_pos. */
> + struct mutex misc_mutex;
> + int misc_open_count;
> + /* indicate interrupt enabled by cyapa driver. */
> + bool irq_enabled;
> + /* indicate interrupt enabled by trackpad device. */
> + bool bl_irq_enable;
> + enum cyapa_work_mode fw_work_mode;
> +
> + struct i2c_client *client;
> + struct input_dev *input;
> + struct delayed_work dwork;
> + struct work_struct detect_work;
> + struct workqueue_struct *detect_wq;
> + enum cyapa_detect_status detect_status;
> + /* synchronize access to dwork. */
> + spinlock_t lock;
> + int no_data_count;
> + int scan_ms;
> + int open_count;
> +
> + int irq;
> + /* driver using polling mode if failed to request irq. */
> + bool polling_mode_enabled;
> + struct cyapa_platform_data *pdata;
> + unsigned short data_base_offset;
> + unsigned short control_base_offset;
> + unsigned short command_base_offset;
> + unsigned short query_base_offset;
> +
> + struct cyapa_mt_slot mt_slots[MAX_MT_SLOTS];
This can be skipped.
> +
> + /* read from query data region. */
> + char product_id[16];
> + unsigned char capability[14];
> + unsigned char fw_maj_ver; /* firmware major version. */
> + unsigned char fw_min_ver; /* firmware minor version. */
> + unsigned char hw_maj_ver; /* hardware major version. */
> + unsigned char hw_min_ver; /* hardware minor version. */
> + int max_abs_x;
> + int max_abs_y;
> + int physical_size_x;
> + int physical_size_y;
> +};
> +
> +#if DBG_CYAPA_READ_BLOCK_DATA
> +#define DUMP_BUF_SIZE (40 * 3 + 20) /* max will dump 40 bytes data. */
> +void cyapa_dump_data_block(const char *func, u8 reg, u8 length, void *data)
> +{
> + char buf[DUMP_BUF_SIZE];
> + unsigned buf_len = sizeof(buf);
> + char *p = buf;
> + int i;
> + int l;
> +
> + l = snprintf(p, buf_len, "reg 0x%04x: ", reg);
> + buf_len -= l;
> + p += l;
> + for (i = 0; i < length && buf_len; i++, p += l, buf_len -= l)
> + l = snprintf(p, buf_len, "%02x ", *((char *)data + i));
> + pr_info("%s: data block length = %d\n", func, length);
> + pr_info("%s: %s\n", func, buf);
> +}
> +
> +void cyapa_dump_report_data(const char *func,
> + struct cyapa_report_data *report_data)
> +{
> + int i;
> +
> + pr_info("%s: ------------------------------------\n", func);
> + pr_info("%s: report_data.button = 0x%02x\n",
> + func, report_data->button);
> + pr_info("%s: report_data.avg_pressure = %d\n",
> + func, report_data->avg_pressure);
> + pr_info("%s: report_data.touch_fingers = %d\n",
> + func, report_data->touch_fingers);
> + for (i = 0; i < report_data->touch_fingers; i++) {
> + pr_info("%s: report_data.touches[%d].x = %d\n",
> + func, i, report_data->touches[i].x);
> + pr_info("%s: report_data.touches[%d].y = %d\n",
> + func, i, report_data->touches[i].y);
> + pr_info("%s: report_data.touches[%d].pressure = %d\n",
> + func, i, report_data->touches[i].pressure);
> + if (report_data->touches[i].tracking_id != -1)
> + pr_info("%s: report_data.touches[%d].tracking_id = %d\n",
> + func, i, report_data->touches[i].tracking_id);
> + }
> + pr_info("%s: report_data.gesture_count = %d\n",
> + func, report_data->gesture_count);
> + for (i = 0; i < report_data->gesture_count; i++) {
> + pr_info("%s: report_data.gestures[%d].id = 0x%02x\n",
> + func, i, report_data->gestures[i].id);
> + pr_info("%s: report_data.gestures[%d].param1 = 0x%02x\n",
> + func, i, report_data->gestures[i].param1);
> + pr_info("%s: report_data.gestures[%d].param2 = 0x%02x\n",
> + func, i, report_data->gestures[i].param2);
> + }
> + pr_info("%s: -------------------------------------\n", func);
> +}
> +#else
> +void cyapa_dump_data_block(const char *func, u8 reg, u8 length, void *data) {}
> +void cyapa_dump_report_data(const char *func,
> + struct cyapa_report_data *report_data) {}
> +#endif
Are these debug functions really necessary?
> +static inline void cyapa_report_fingers(struct input_dev *input, int fingers)
> +{
> + input_report_key(input, BTN_TOOL_FINGER, (fingers == 1));
> + input_report_key(input, BTN_TOOL_DOUBLETAP, (fingers == 2));
> + input_report_key(input, BTN_TOOL_TRIPLETAP, (fingers == 3));
> + input_report_key(input, BTN_TOOL_QUADTAP, (fingers > 3));
> +}
This functionality is found in input_mt_report_finger_count() and
input_mt_report_pointer_emulation().
> +
> +static void cyapa_parse_gen3_data(struct cyapa_i2c *touch,
> + struct cyapa_reg_data_gen3 *reg_data,
> + struct cyapa_report_data *report_data)
> +{
> + int i;
> + int fingers;
> +
> + /* only report left button. */
> + report_data->button = reg_data->finger_btn & OP_DATA_BTN_MASK;
> + report_data->avg_pressure = 0;
> + /* parse number of touching fingers. */
> + fingers = (reg_data->finger_btn >> 4) & 0x0F;
> + report_data->touch_fingers = min(CYAPA_MAX_TOUCHES, fingers);
> +
> + /* parse data for each touched finger. */
> + for (i = 0; i < report_data->touch_fingers; i++) {
> + report_data->touches[i].x =
> + ((reg_data->touches[i].xy & 0xF0) << 4) |
> + reg_data->touches[i].x;
> + report_data->touches[i].y =
> + ((reg_data->touches[i].xy & 0x0F) << 8) |
> + reg_data->touches[i].y;
> + report_data->touches[i].pressure =
> + reg_data->touches[i].pressure;
> + report_data->touches[i].tracking_id =
> + reg_data->touches[i].tracking_id;
> + }
> + report_data->gesture_count = 0;
> +
> + /* DEBUG: dump parsed report data */
> + cyapa_dump_report_data(__func__, report_data);
> +}
This function could be expanded to handle input reporting directly. Something like:
int num_fingers = (reg_data->finger_btn >> 4) & 0x0F;
int mask = 0;
for (i = 0; i < num_fingers; i++) {
const struct cyapa_touch_gen3 *touch = ®_data->touches[i];
int slot = touch->tracking_id;
int x = ((touch->xy & 0xF0) << 4) | touch->x;
int y = ((touch->xy & 0x0F) << 8) | touch->y;
input_mt_slot(input, slot);
input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
input_report_abs(input, ABS_MT_POSITION_X, x);
input_report_abs(input, ABS_MT_POSITION_Y, y);
input_report_abs(input, ABS_MT_PRESSURE, touch->pressure);
mask |= (1 << slot);
}
for (i = 0; i < MAX_MT_SLOTS; i++) {
if (mask & (1 << i))
continue;
input_mt_slot(input, i);
input_mt_report_slot_state(input, MT_TOOL_FINGER, false);
}
input_mt_report_pointer_emulation(input, true);
input_report_key(input, BTN_LEFT, reg_data->finger_btn & OP_DATA_BTN_MASK);
input_sync(input);
> +
> +
> +static int cyapa_find_mt_slot(struct cyapa_i2c *touch,
> + struct cyapa_touch *contact)
> +{
> + int i;
> + int empty_slot = -1;
> +
> + for (i = 0; i < MAX_MT_SLOTS; i++) {
> + if ((touch->mt_slots[i].contact.tracking_id == contact->tracking_id) &&
> + touch->mt_slots[i].touch_state)
> + return i;
> +
> + if (!touch->mt_slots[i].touch_state && empty_slot == -1)
> + empty_slot = i;
> + }
> +
> + return empty_slot;
> +}
This wont be necessary.
> +
> +static void cyapa_update_mt_slots(struct cyapa_i2c *touch,
> + struct cyapa_report_data *report_data)
> +{
> + int i;
> + int slotnum;
> +
> + for (i = 0; i < report_data->touch_fingers; i++) {
> + slotnum = cyapa_find_mt_slot(touch, &report_data->touches[i]);
> + if (slotnum < 0)
> + continue;
> +
> + memcpy(&touch->mt_slots[slotnum].contact,
> + &report_data->touches[i],
> + sizeof(struct cyapa_touch));
> + touch->mt_slots[slotnum].slot_updated = true;
> + touch->mt_slots[slotnum].touch_state = true;
> + }
> +}
This wont be necessary.
> +
> +static void cyapa_send_mtb_event(struct cyapa_i2c *touch,
> + struct cyapa_report_data *report_data)
> +{
> + int i;
> + struct cyapa_mt_slot *slot;
> + struct input_dev *input = touch->input;
> +
> + cyapa_update_mt_slots(touch, report_data);
> +
> + for (i = 0; i < MAX_MT_SLOTS; i++) {
> + slot = &touch->mt_slots[i];
> + if (!slot->slot_updated)
> + slot->touch_state = false;
> +
> + input_mt_slot(input, i);
> + input_mt_report_slot_state(input, MT_TOOL_FINGER, slot->touch_state);
> + if (slot->touch_state) {
> + input_report_abs(input, ABS_MT_POSITION_X, slot->contact.x);
> + input_report_abs(input, ABS_MT_POSITION_Y, slot->contact.y);
> + input_report_abs(input, ABS_MT_PRESSURE, slot->contact.pressure);
> + }
> + slot->slot_updated = false;
> + }
> +
> + input_mt_report_pointer_emulation(input, true);
> + input_report_key(input, BTN_LEFT, report_data->button);
> + input_sync(input);
> +}
This could be folded into the parsing function.
> +
> +static int cyapa_handle_input_report_data(struct cyapa_i2c *touch,
> + struct cyapa_report_data *report_data)
> +{
> + cyapa_send_mtb_event(touch, report_data);
> +
> + return report_data->touch_fingers | report_data->button;
> +}
This wont be necessary.
> +
> +static bool cyapa_i2c_get_input(struct cyapa_i2c *touch)
> +{
> + int ret_read_size;
> + int read_length;
> + union cyapa_reg_data reg_data;
> + struct cyapa_reg_data_gen3 *gen3_data;
> + struct cyapa_report_data report_data;
> +
> + /* read register data from trackpad. */
> + gen3_data = ®_data.gen3_data;
> + read_length = (int)sizeof(struct cyapa_reg_data_gen3);
> +
> + ret_read_size = cyapa_i2c_reg_read_block(touch,
> + DATA_REG_START_OFFSET,
> + read_length,
> + (char *)®_data);
> + if (ret_read_size < 0)
> + return 0;
> +
> + if (cyapa_verify_data_device(touch, ®_data) < 0)
> + return 0;
> +
> + /* process and parse raw data read from Trackpad. */
> + cyapa_parse_gen3_data(touch, gen3_data, &report_data);
Reporting could be done here in a straight-forward fashion.
> +
> + /* report data to input subsystem. */
> + return cyapa_handle_input_report_data(touch, &report_data);
Skipping this abstraction entirely.
> +static int cyapa_create_input_dev(struct cyapa_i2c *touch)
> +{
> + int ret;
> + struct input_dev *input = NULL;
> +
> + input = touch->input = input_allocate_device();
> + if (!touch->input) {
> + pr_err("Allocate memory for Input device failed\n");
> + return -ENOMEM;
> + }
> +
> + input->name = "cyapa_i2c_trackpad";
> + input->phys = touch->client->adapter->name;
> + input->id.bustype = BUS_I2C;
> + input->id.version = 1;
> + input->id.product = 0; /* means any product in eventcomm. */
> + input->dev.parent = &touch->client->dev;
> +
> + input->open = cyapa_i2c_open;
> + input->close = cyapa_i2c_close;
> + input_set_drvdata(input, touch);
> +
> + __set_bit(EV_ABS, input->evbit);
> +
> + /*
> + * set and report not-MT axes to support synaptics X Driver.
> + * When multi-fingers on trackpad, only the first finger touch
> + * will be reported as X/Y axes values.
> + */
> + input_set_abs_params(input, ABS_X, 0, touch->max_abs_x, 0, 0);
> + input_set_abs_params(input, ABS_Y, 0, touch->max_abs_y, 0, 0);
> + input_set_abs_params(input, ABS_PRESSURE, 0, 255, 0, 0);
> + input_set_abs_params(input, ABS_TOOL_WIDTH, 0, 255, 0, 0);
> +
> + /* finger position */
> + input_set_abs_params(input, ABS_MT_POSITION_X, 0, touch->max_abs_x, 0, 0);
> + input_set_abs_params(input, ABS_MT_POSITION_Y, 0, touch->max_abs_y, 0, 0);
> + input_set_abs_params(input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> + ret = input_mt_init_slots(input, MAX_MT_SLOTS);
> + if (ret < 0)
> + return ret;
> +
> + if (touch->physical_size_x && touch->physical_size_y) {
> + input_abs_set_res(input, ABS_X,
> + touch->max_abs_x / touch->physical_size_x);
> + input_abs_set_res(input, ABS_Y,
> + touch->max_abs_y / touch->physical_size_y);
> + input_abs_set_res(input, ABS_MT_POSITION_X,
> + touch->max_abs_x / touch->physical_size_x);
> + input_abs_set_res(input, ABS_MT_POSITION_Y,
> + touch->max_abs_y / touch->physical_size_y);
> + }
> +
> + __set_bit(EV_KEY, input->evbit);
> + __set_bit(BTN_TOUCH, input->keybit);
> + __set_bit(BTN_TOOL_FINGER, input->keybit);
> + __set_bit(BTN_TOOL_DOUBLETAP, input->keybit);
> + __set_bit(BTN_TOOL_TRIPLETAP, input->keybit);
> + __set_bit(BTN_TOOL_QUADTAP, input->keybit);
> +
> + __set_bit(BTN_LEFT, input->keybit);
> +
> + /* Register the device in input subsystem */
> + ret = input_register_device(touch->input);
> + if (ret) {
> + pr_err("Input device register failed, %d\n", ret);
> + input_free_device(input);
> + }
> +
> + return ret;
> +}
> +
Thanks,
Henrik