HI Trilok, Thank you for your review. I am collecting your comments below and starting a new revision to incorporate your requests. > -----Original Message----- > From: Trilok Soni [mailto:tsoni@xxxxxxxxxxxxxx] > Sent: Thursday, August 05, 2010 1:46 PM > To: Kevin McNeely > Cc: Dmitry Torokhov; David Brown; Fred; Samuel Ortiz; Eric Miao; Ben > Dooks; Simtec Linux Team; Todd Fischer; Arnaud Patard; Sascha Hauer; > Henrik Rydberg; linux-input@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] i2c: cyttsp i2c and spi touchscreen driver init > submit > > Hi Kevin, > > On 8/5/2010 11:42 PM, Kevin McNeely wrote: > > From: Fred <fwk@xxxxxxxxxxxxxxxxxxxxxxxxx> > > > > E-mail address is is wrong again it seems. Please fix. Will do. > > You may want to divide this whole patch into three patches: > > 1. core driver > 2. i2c driver > 3. spi driver > > > 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@xxxxxxxxxxx> > > --- > > 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 > > File modes are wrong. > Will fix. > > > > > 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 > > default n is not required. Will remove. > > > + 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 n > > Ditto. > Will remove. > > + tristate "Cypress TTSP spi touchscreen" > > + depends on SPI_MASTER > > + help > > + Say Y here if you have a Cypress TTSP touchscreen > > + connected to your system's spi bus. > > + > > + If unsure, say N. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called cyttsp_spi. > > + > > +config TOUCHSCREEN_CYTTSP_CORE > > + default y > > We don't want anything to be default y unless it is curing a cancer. > Will remove. > > diff --git a/drivers/input/touchscreen/cyttsp_board-xxx.c > b/drivers/input/touchscreen/cyttsp_board-xxx.c > > This file is good as example only but not for check in. This is a board > specific code and the board-xxx.c > code in appropriate arch will carry this code. Please remove this file > from the patch. > > > diff --git a/drivers/input/touchscreen/cyttsp_core.c > b/drivers/input/touchscreen/cyttsp_core.c > > new file mode 100755 > > index 0000000..95019e9 > > --- /dev/null > > +++ b/drivers/input/touchscreen/cyttsp_core.c > > @@ -0,0 +1,1778 @@ > > +/* Core Source for: > > + * Cypress TrueTouch(TM) Standard Product (TTSP) touchscreen > drivers. > > + * For use with Cypress Txx2xx and Txx3xx parts. > > + * Supported parts include: > > + * CY8CTST241 > > + * CY8CTMG240 > > + * CY8CTST341 > > + * CY8CTMA340 > > + * > > + * Copyright (C) 2009, 2010 Cypress Semiconductor, Inc. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * version 2, and only version 2, as published by the > > + * Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > along > > + * with this program; if not, write to the Free Software Foundation, > Inc., > > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. > > + * > > + * Contact Cypress Semiconductor at www.cypress.com > (kev@xxxxxxxxxxx) > > + * > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/init.h> > > +#include <linux/module.h> > > +#include <linux/input.h> > > +#include <linux/slab.h> > > +#include <linux/gpio.h> > > +#include <linux/irq.h> > > +#include <linux/interrupt.h> > > +#include <linux/timer.h> > > +#include <linux/workqueue.h> > > +#include <linux/byteorder/generic.h> > > +#include <linux/bitops.h> > > +#include <linux/cyttsp.h> > > +#include <linux/ctype.h> > > +#include "cyttsp_core.h" > > + > > +#define DBG(x) > > + > > +/* rely on kernel input.h to define Multi-Touch capability */ > > +#ifndef ABS_MT_TRACKING_ID > > +/* define only if not defined already by system; */ > > +/* value based on linux kernel 2.6.30.10 */ > > +#define ABS_MT_TRACKING_ID (ABS_MT_BLOB_ID + 1) > > +#endif /* ABS_MT_TRACKING_ID */ > > We always support and base our code from latest kernel only. Please > remove the code > which relies on supporting older kernels. > Will do. > > + > > +#define TOUCHSCREEN_TIMEOUT (msecs_to_jiffies(28)) > > +/* Bootloader File 0 offset */ > > +#define CY_BL_FILE0 0x00 > > +/* Bootloader command directive */ > > +#define CY_BL_CMD 0xFF > > +/* Bootloader Enter Loader mode */ > > +#define CY_BL_ENTER 0x38 > > +/* Bootloader Write a Block */ > > +#define CY_BL_WRITE_BLK 0x39 > > +/* Bootloader Terminate Loader mode */ > > +#define CY_BL_TERMINATE 0x3B > > +/* Bootloader Exit and Verify Checksum command */ > > +#define CY_BL_EXIT 0xA5 > > +/* Bootloader default keys */ > > +#define CY_BL_KEY0 0 > > +#define CY_BL_KEY1 1 > > +#define CY_BL_KEY2 2 > > +#define CY_BL_KEY3 3 > > +#define CY_BL_KEY4 4 > > +#define CY_BL_KEY5 5 > > +#define CY_BL_KEY6 6 > > +#define CY_BL_KEY7 7 > > + > > +#define CY_DIFF(m, n) ((m) != (n)) > > And why such macro is required? Why can't we just do "if (i != j)"? Will replace. > > > + > > +/* TTSP Bootloader Register Map interface definition */ > > +#define CY_BL_CHKSUM_OK 0x01 > > +struct cyttsp_bootloader_data { > > + u8 bl_file; > > + u8 bl_status; > > + u8 bl_error; > > + u8 blver_hi; > > + u8 blver_lo; > > + u8 bld_blver_hi; > > + u8 bld_blver_lo; > > + u8 ttspver_hi; > > + u8 ttspver_lo; > > + u8 appid_hi; > > + u8 appid_lo; > > + u8 appver_hi; > > + u8 appver_lo; > > + u8 cid_0; > > + u8 cid_1; > > + u8 cid_2; > > +}; > > + > > +#define cyttsp_wake_data cyttsp_xydata > > Why we need to such assignments and introduce these kind of macros? I > don't think it is necessary. Will remove. > > > + > > +struct cyttsp { > > + struct device *pdev; > > Most of the time kernel people understands "pdev == platform device and > not just device", so better rename this variable > to "dev" only. > > > + int irq; > > + struct input_dev *input; > > + struct work_struct work; > > + struct timer_list timer; > > + struct mutex mutex; > > + char phys[32]; > > + struct cyttsp_platform_data *platform_data; > > + struct cyttsp_bootloader_data bl_data; > > + struct cyttsp_sysinfo_data sysinfo_data; > > + u8 num_prv_st_tch; > > + u16 act_trk[CY_NUM_TRK_ID]; > > + u16 prv_mt_tch[CY_NUM_MT_TCH_ID]; > > + u16 prv_st_tch[CY_NUM_ST_TCH_ID]; > > + u16 prv_mt_pos[CY_NUM_TRK_ID][2]; > > + struct cyttsp_bus_ops *bus_ops; > > + unsigned fw_loader_mode:1; > > + unsigned suspended:1; > > +}; > > + > > +struct cyttsp_track_data { > > + u8 prv_tch; > > + u8 cur_tch; > > + u16 tmp_trk[CY_NUM_MT_TCH_ID]; > > + u16 snd_trk[CY_NUM_MT_TCH_ID]; > > + u16 cur_trk[CY_NUM_TRK_ID]; > > + u16 cur_st_tch[CY_NUM_ST_TCH_ID]; > > + u16 cur_mt_tch[CY_NUM_MT_TCH_ID]; > > + /* if NOT CY_USE_TRACKING_ID then only */ > > + /* uses CY_NUM_MT_TCH_ID positions */ > > + u16 cur_mt_pos[CY_NUM_TRK_ID][2]; > > + /* if NOT CY_USE_TRACKING_ID then only */ > > + /* uses CY_NUM_MT_TCH_ID positions */ > > + u8 cur_mt_z[CY_NUM_TRK_ID]; > > + u8 tool_width; > > + u16 st_x1; > > + u16 st_y1; > > + u8 st_z1; > > + u16 st_x2; > > + u16 st_y2; > > + u8 st_z2; > > +}; > > + > > +static const u8 bl_cmd[] = { > > + CY_BL_FILE0, CY_BL_CMD, CY_BL_EXIT, > > + CY_BL_KEY0, CY_BL_KEY1, CY_BL_KEY2, > > + CY_BL_KEY3, CY_BL_KEY4, CY_BL_KEY5, > > + CY_BL_KEY6, CY_BL_KEY7 > > +}; > > + > > +#define LOCK(m) do { \ > > + DBG(printk(KERN_INFO "%s: lock\n", __func__);) \ > > + mutex_lock(&(m)); \ > > +} while (0); > > + > > +#define UNLOCK(m) do { \ > > + DBG(printk(KERN_INFO "%s: unlock\n", __func__);) \ > > + mutex_unlock(&(m)); \ > > +} while (0); > > Un-necessary debug macros and abstractions. This is not allowed. Please > use mutext_lock and unlock > APIs directly wherever they are required. Will change. > > > + > > +DBG( > > +static void print_data_block(const char *func, u8 command, > > + u8 length, void *data) > > +{ > > + char buf[1024]; > > + unsigned buf_len = sizeof(buf); > > + char *p = buf; > > + int i; > > + int l; > > + > > + l = snprintf(p, buf_len, "cmd 0x%x: ", command); > > + 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)); > > + printk(KERN_DEBUG "%s: %s\n", func, buf); > > +}) > > 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 > of such debugging helpers in the mainlined version of the driver. Will remove. > > > + > > +static int ttsp_read_block_data(struct cyttsp *ts, u8 command, > > + u8 length, void *buf) > > +{ > > + int rc; > > + int tries; > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > + > > + if (!buf || !length) { > > + printk(KERN_ERR "%s: Error, buf:%s len:%u\n", > > + __func__, !buf ? "NULL" : "OK", length); > > + return -EIO; > > + } > > + > > + for (tries = 0, rc = 1; tries < CY_NUM_RETRY && rc; tries++) > > + rc = ts->bus_ops->read(ts->bus_ops, command, length, buf); > > + > > + if (rc < 0) > > + printk(KERN_ERR "%s: error %d\n", __func__, rc); > > + DBG(print_data_block(__func__, command, length, buf);) > > 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 > dev_dbg(...) if you have device pointer or use pr_debug(...). Will do. > > > + > > +static int ttsp_tch_ext(struct cyttsp *ts, void *buf) > > +{ > > + int rc; > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > Un-necessary debug statements. Such statements should be removed from > the driver. Will do. > > > + > > +/* > *********************************************************************** > * > > + * The cyttsp_xy_worker function reads the XY coordinates and sends > them to > > + * the input layer. It is scheduled from the interrupt (or timer). > > + * > *********************************************************************** > **/ > > +void handle_single_touch(struct cyttsp_xydata *xy, struct > cyttsp_track_data *t, > > + struct cyttsp *ts) > > +{ Make static. Ok. > > 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 > because the userspace frameworks are not converted or supporting the > new MT based protocols. ST will be removed completely. > > > + > > +void handle_multi_touch(struct cyttsp_track_data *t, struct cyttsp > *ts) > > +{ > > static please. Ok. > > > + > > +void cyttsp_xy_worker(struct cyttsp *ts) > > +{ > > static please. Ok. > > > + > > + DBG( > > + if (trc.cur_tch) { > > + unsigned i; > > + u8 *pdata = (u8 *)&xy_data; > > + > > + printk(KERN_INFO "%s: TTSP data_pack: ", __func__); > > + for (i = 0; i < sizeof(struct cyttsp_xydata); i++) > > + printk(KERN_INFO "[%d]=0x%x ", i, pdata[i]); > > + printk(KERN_INFO "\n"); > > + }) > > I would really like to remove such DBG formatted code. Will remove. > > > + > > + /* Determine if display is tilted */ > > + tilt = !!FLIP_DATA(ts->platform_data->flags); > > + /* Check for switch in origin */ > > + rev_x = !!REVERSE_X(ts->platform_data->flags); > > + rev_y = !!REVERSE_Y(ts->platform_data->flags); > > + > > ... > > > + > > + /* update platform data for the current multi-touch information > */ > > + memcpy(ts->act_trk, trc.cur_trk, CY_NUM_TRK_ID); > > + > > +exit_xy_worker: > > + DBG(printk(KERN_INFO"%s: finished.\n", __func__);) > > + return; > > Do you need this return statment? Will remove. > > > +} > > + > > +/* > *********************************************************************** > * > > + * ISR function. This function is general, initialized in drivers > init > > + * function and disables further IRQs until this IRQ is processed in > worker. > > + * > *********************************************************************** > **/ > > +static irqreturn_t cyttsp_irq(int irq, void *handle) > > +{ > > + struct cyttsp *ts = (struct cyttsp *)handle; > > Casting is not required when the source is void *. > > > + > > + DBG(printk(KERN_INFO"%s: Got IRQ!\n", __func__);) > > Un-necessary debug statements. Will remove. > > > + cyttsp_xy_worker(ts); > > + return IRQ_HANDLED; > > +} > > + > > +/* schedulable worker entry for timer polling method */ > > +void cyttsp_xy_worker_(struct work_struct *work) > > +{ > > + struct cyttsp *ts = container_of(work, struct cyttsp, work); > > + cyttsp_xy_worker(ts); > > +} > > 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(...), > you will not need any workqueue. > Polling code will be removed completely. > > + > > +/* Timer function used as touch interrupt generator for polling > method */ > > +static void cyttsp_timer(unsigned long handle) > > +{ > > + struct cyttsp *ts = (struct cyttsp *)handle; > > + > > + DBG(printk(KERN_INFO"%s: TTSP timer event!\n", __func__);) > > + /* schedule motion signal handling */ > > + if (!work_pending(&ts->work)) > > + schedule_work(&ts->work); > > + mod_timer(&ts->timer, jiffies + TOUCHSCREEN_TIMEOUT); > > + return; > > +} > > I don't see any need of this timer. Better remove the polling method > all together to simplify the code. > Only support interrupt based approach only. > Polling code will be removed completely. > > > + > > + > > +/* > *********************************************************************** > * > > + * Probe initialization functions > > + * > *********************************************************************** > * */ > > +static int cyttsp_load_bl_regs(struct cyttsp *ts) > > +{ > > + int retval; > > + > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > + > > + retval = ttsp_read_block_data(ts, CY_REG_BASE, > > + sizeof(ts->bl_data), &(ts->bl_data)); > > + > > + if (retval < 0) { > > + DBG(printk(KERN_INFO "%s: Failed reading block data, > err:%d\n", > > + __func__, retval);) > > + goto fail; > > + } > > + > > + DBG({ > > + int i; > > + u8 *bl_data = (u8 *)&(ts->bl_data); > > + for (i = 0; i < sizeof(struct cyttsp_bootloader_data); i++) > > + printk(KERN_INFO "%s bl_data[%d]=0x%x\n", > > + __func__, i, bl_data[i]); > > + }) > > Better remove such debug code. Will remove. > > > + > > + return 0; > > +fail: > > + return retval; > > +} > > ... > > > + > > +static ssize_t firmware_write(struct kobject *kobj, > > + struct bin_attribute *bin_attr, > > + char *buf, loff_t pos, size_t size) > > +{ > > + struct device *dev = container_of(kobj, struct device, kobj); > > + struct cyttsp *ts = dev_get_drvdata(dev); > > + LOCK(ts->mutex); > > + DBG({ > > + char str[128]; > > + char *p = str; > > + int i; > > + for (i = 0; i < size; i++, p += 2) > > + sprintf(p, "%02x", (unsigned char)buf[i]); > > + printk(KERN_DEBUG "%s: size %d, pos %ld payload %s\n", > > + __func__, size, (long)pos, str); > > + }) > > + ttsp_write_block_data(ts, CY_REG_BASE, size, buf); > > + UNLOCK(ts->mutex); > > + return size; > > +} > > + > > +static ssize_t firmware_read(struct kobject *kobj, > > + struct bin_attribute *ba, > > + char *buf, loff_t pos, size_t size) > > +{ > > + int count = 0; > > + struct device *dev = container_of(kobj, struct device, kobj); > > + struct cyttsp *ts = dev_get_drvdata(dev); > > + > > + LOCK(ts->mutex); > > + if (!ts->fw_loader_mode) > > + goto exit; > > + if (!cyttsp_load_bl_regs(ts)) { > > + *(unsigned short *)buf = ts->bl_data.bl_status << 8 | > > + ts->bl_data.bl_error; > > + count = sizeof(unsigned short); > > + } else { > > + printk(KERN_ERR "%s: error reading status\n", __func__); > > + count = 0; > > + } > > +exit: > > + UNLOCK(ts->mutex); > > + return count; > > +} > > This kind of custom interface may not be allowed in the kernel driver. > Please use > request_firmware(...) call based interface to support firmware loading. > Will look into. > > + > > +void *cyttsp_core_init(struct cyttsp_bus_ops *bus_ops, struct device > *pdev) > > +{ > > + struct input_dev *input_device; > > + struct cyttsp *ts; > > + int retval = 0; > > + > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > + > > + ts = kzalloc(sizeof(*ts), GFP_KERNEL); > > + if (ts == NULL) { > > + printk(KERN_ERR "%s: Error, kzalloc\n", __func__); > > + goto error_alloc_data_failed; > > + } > > + mutex_init(&ts->mutex); > > + ts->pdev = pdev; > > + ts->platform_data = pdev->platform_data; > > + ts->bus_ops = bus_ops; > > + > > + if (ts->platform_data->init) > > + retval = ts->platform_data->init(1); > > + if (retval) { > > + printk(KERN_ERR "%s: platform init failed!\n", __func__); > > + goto error_init; > > + } > > + > > + if (ts->platform_data->use_timer) > > + ts->irq = -1; > > + else > > + ts->irq = gpio_to_irq(ts->platform_data->irq_gpio); > > + > > + retval = cyttsp_power_on(ts); > > + if (retval < 0) { > > + printk(KERN_ERR "%s: Error, power on failed!\n", __func__); > > + goto error_power_on; > > + } > > + > > + /* Create the input device and register it. */ > > + input_device = input_allocate_device(); > > + if (!input_device) { > > + retval = -ENOMEM; > > + printk(KERN_ERR "%s: Error, failed to allocate input > device\n", > > + __func__); > > + goto error_input_allocate_device; > > + } > > + > > + ts->input = input_device; > > + input_device->name = ts->platform_data->name; > > + input_device->phys = ts->phys; > > + input_device->dev.parent = ts->pdev; > > + /* init the touch structures */ > > + ts->num_prv_st_tch = CY_NTCH; > > + memset(ts->act_trk, CY_NTCH, sizeof(ts->act_trk)); > > + memset(ts->prv_mt_pos, CY_NTCH, sizeof(ts->prv_mt_pos)); > > + memset(ts->prv_mt_tch, CY_IGNR_TCH, sizeof(ts->prv_mt_tch)); > > + memset(ts->prv_st_tch, CY_IGNR_TCH, sizeof(ts->prv_st_tch)); > > + > > + __set_bit(EV_SYN, input_device->evbit); > > + __set_bit(EV_KEY, input_device->evbit); > > + __set_bit(EV_ABS, input_device->evbit); > > + __set_bit(BTN_TOUCH, input_device->keybit); > > + __set_bit(BTN_2, input_device->keybit); > > + if (ts->platform_data->use_gestures) > > + __set_bit(BTN_3, input_device->keybit); > > + > > + input_set_abs_params(input_device, ABS_X, 0, ts->platform_data- > >maxx, > > + 0, 0); > > + input_set_abs_params(input_device, ABS_Y, 0, ts->platform_data- > >maxy, > > + 0, 0); > > + input_set_abs_params(input_device, ABS_TOOL_WIDTH, 0, > > + CY_LARGE_TOOL_WIDTH, 0, 0); > > + input_set_abs_params(input_device, ABS_PRESSURE, 0, CY_MAXZ, 0, > 0); > > + input_set_abs_params(input_device, ABS_HAT0X, 0, > > + ts->platform_data->maxx, 0, 0); > > + input_set_abs_params(input_device, ABS_HAT0Y, 0, > > + ts->platform_data->maxy, 0, 0); > > + if (ts->platform_data->use_gestures) { > > + input_set_abs_params(input_device, ABS_HAT1X, 0, CY_MAXZ, > > + 0, 0); > > + input_set_abs_params(input_device, ABS_HAT1Y, 0, CY_MAXZ, > > + 0, 0); > > + } > > + if (ts->platform_data->use_mt) { > > + input_set_abs_params(input_device, ABS_MT_POSITION_X, 0, > > + ts->platform_data->maxx, 0, 0); > > + input_set_abs_params(input_device, ABS_MT_POSITION_Y, 0, > > + ts->platform_data->maxy, 0, 0); > > + input_set_abs_params(input_device, ABS_MT_TOUCH_MAJOR, 0, > > + CY_MAXZ, 0, 0); > > + input_set_abs_params(input_device, ABS_MT_WIDTH_MAJOR, 0, > > + CY_LARGE_TOOL_WIDTH, 0, 0); > > + if (ts->platform_data->use_trk_id) > > + input_set_abs_params(input_device, > ABS_MT_TRACKING_ID, > > + 0, CY_NUM_TRK_ID, 0, 0); > > + } > > + > > + if (ts->platform_data->use_virtual_keys) > > + input_set_capability(input_device, EV_KEY, KEY_PROG1); > > + > > + retval = input_register_device(input_device); > > + if (retval) { > > + printk(KERN_ERR "%s: Error, failed to register input > device\n", > > + __func__); > > + goto error_input_register_device; > > + } > > + DBG(printk(KERN_INFO "%s: Registered input device %s\n", > > + __func__, input_device->name);) > > + > > + /* Prepare our worker structure prior to setting up the timer ISR > */ > > + INIT_WORK(&ts->work, cyttsp_xy_worker_); > > + > > + /* Timer or Interrupt setup */ > > + if (ts->platform_data->use_timer) { > > + DBG(printk(KERN_INFO "%s: Setting up Timer\n", __func__);) > > + setup_timer(&ts->timer, cyttsp_timer, (unsigned long) ts); > > + mod_timer(&ts->timer, jiffies + TOUCHSCREEN_TIMEOUT); > > + } else { > > + DBG( > > + printk(KERN_INFO "%s: Setting up Interrupt. Device > name=%s\n", > > + __func__, input_device->name);) > > + retval = request_threaded_irq(ts->irq, NULL, cyttsp_irq, > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + input_device->name, ts); > > + > > + if (retval) { > > + printk(KERN_ERR "%s: Error, could not request irq\n", > > + __func__); > > + goto error_free_irq; > > + } else { > > + DBG(printk(KERN_INFO "%s: Interrupt=%d\n", > > + __func__, ts->irq);) > > + } > > + } > > + retval = device_create_file(pdev, &fwloader); > > + if (retval) { > > + printk(KERN_ERR "%s: Error, could not create attribute\n", > > + __func__); > > + goto device_create_error; > > + } > > + dev_set_drvdata(pdev, ts); > > + printk(KERN_INFO "%s: Successful.\n", __func__); > > + return ts; > > + > > +device_create_error: > > +error_free_irq: > > + if (ts->irq >= 0) > > + free_irq(ts->irq, ts); > > + input_unregister_device(input_device); > > +error_input_register_device: > > + input_free_device(input_device); > > +error_input_allocate_device: > > +error_power_on: > > + if (ts->platform_data->init) > > + ts->platform_data->init(0); > > +error_init: > > + kfree(ts); > > +error_alloc_data_failed: > > + return NULL; > > +} > > + > EXPORT_SYMBOL_GPL(...)? > > > +/* registered in driver struct */ > > +void cyttsp_core_release(void *handle) > > +{ > > + struct cyttsp *ts = handle; > > + > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > + cancel_work_sync(&ts->work); > > + if (ts->platform_data->use_timer) > > + del_timer_sync(&ts->timer); > > + else > > + free_irq(ts->irq, ts); > > + input_unregister_device(ts->input); > > + input_free_device(ts->input); > > No need of input_free_device after input_unregister_device. Will remove. > > > + if (ts->platform_data->init) > > + ts->platform_data->init(0); > > + kfree(ts); > > +} > > EXPORT_SYMBOL_GPL(...)? Will add. > > > +MODULE_LICENSE("GPL"); > > +MODULE_DESCRIPTION("Cypress TrueTouch(R) Standard touchscreen driver > core"); > > +MODULE_AUTHOR("Cypress"); > > + > > > > diff --git a/drivers/input/touchscreen/cyttsp_i2c.c > b/drivers/input/touchscreen/cyttsp_i2c.c > > new file mode 100755 > > index 0000000..ef96105 > > --- /dev/null > > +++ b/drivers/input/touchscreen/cyttsp_i2c.c > > @@ -0,0 +1,183 @@ > > +/* Source for: > > + * Cypress TrueTouch(TM) Standard Product (TTSP) I2C touchscreen > driver. > > + * For use with Cypress Txx2xx and Txx3xx parts with I2C interface. > > + * Supported parts include: > > + * CY8CTST241 > > + * CY8CTMG240 > > + * CY8CTST341 > > + * CY8CTMA340 > > + * > > + * Copyright (C) 2009, 2010 Cypress Semiconductor, Inc. > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * version 2, and only version 2, as published by the > > + * Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > along > > + * with this program; if not, write to the Free Software Foundation, > Inc., > > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. > > + * > > + * Contact Cypress Semiconductor at www.cypress.com > (kev@xxxxxxxxxxx) > > + * > > + */ > > + > > +#include <linux/init.h> > > +#include <linux/module.h> > > +#include <linux/i2c.h> > > +#include <linux/cyttsp.h> > > +#include "cyttsp_core.h" > > + > > +#define DBG(x) > > + > > +struct cyttsp_i2c { > > + struct cyttsp_bus_ops ops; > > + struct i2c_client *client; > > + void *ttsp_client; > > +}; > > + > > +static s32 ttsp_i2c_read_block_data(void *handle, u8 addr, > > + u8 length, void *values) > > +{ > > + struct cyttsp_i2c *ts = container_of(handle, struct cyttsp_i2c, > ops); > > + return i2c_smbus_read_i2c_block_data(ts->client, > > + addr, length, values); > > +} > > + > > +static s32 ttsp_i2c_write_block_data(void *handle, u8 addr, > > + u8 length, const void *values) > > +{ > > + struct cyttsp_i2c *ts = container_of(handle, struct cyttsp_i2c, > ops); > > + return i2c_smbus_write_i2c_block_data(ts->client, > > + addr, length, values); > > +} > > + > > +static s32 ttsp_i2c_tch_ext(void *handle, void *values) > > +{ > > + int retval = 0; > > + struct cyttsp_i2c *ts = container_of(handle, struct cyttsp_i2c, > ops); > > + > > + DBG(printk(KERN_INFO "%s: Enter\n", __func__);) > > + > > + /* Add custom touch extension handling code here */ > > + /* set: retval < 0 for any returned system errors, > > + retval = 0 if normal touch handling is required, > > + retval > 0 if normal touch handling is *not* required */ > > + if (!ts || !values) > > + retval = -EIO; > > + > > + return retval; > > +} > > +static int __devinit cyttsp_i2c_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct cyttsp_i2c *ts; > > + int retval; > > + > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > + > > + if (!i2c_check_functionality(client->adapter, > > + I2C_FUNC_SMBUS_READ_WORD_DATA)) > > + return -EIO; > > + > > + /* allocate and clear memory */ > > + ts = kzalloc(sizeof(*ts), GFP_KERNEL); > > + if (ts == NULL) { > > + printk(KERN_ERR "%s: Error, kzalloc.\n", __func__); > > + retval = -ENOMEM; > > + goto error_alloc_data_failed; > > + } > > + > > + /* register driver_data */ > > + ts->client = client; > > + i2c_set_clientdata(client, ts); > > + ts->ops.write = ttsp_i2c_write_block_data; > > + ts->ops.read = ttsp_i2c_read_block_data; > > + ts->ops.ext = ttsp_i2c_tch_ext; > > + > > + ts->ttsp_client = cyttsp_core_init(&ts->ops, &client->dev); > > + if (!ts->ttsp_client) > > + goto ttsp_core_err; > > + > > + printk(KERN_INFO "%s: Successful registration %s\n", > > + __func__, CY_I2C_NAME); > > + return 0; > > + > > +ttsp_core_err: > > + kfree(ts); > > +error_alloc_data_failed: > > + return retval; > > +} > > + > > + > > +/* registered in driver struct */ > > +static int __devexit cyttsp_i2c_remove(struct i2c_client *client) > > +{ > > + struct cyttsp_i2c *ts; > > + > > + DBG(printk(KERN_INFO"%s: Enter\n", __func__);) > > + ts = i2c_get_clientdata(client); > > + cyttsp_core_release(ts->ttsp_client); > > + kfree(ts); > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM > > +static int cyttsp_i2c_suspend(struct i2c_client *client, > pm_message_t message) > > +{ > > + return cyttsp_suspend(dev_get_drvdata(&client->dev)); > > +} > > + > > +static int cyttsp_i2c_resume(struct i2c_client *client) > > +{ > > + return cyttsp_resume(dev_get_drvdata(&client->dev)); > > +} > > +#endif > > + > > +static const struct i2c_device_id cyttsp_i2c_id[] = { > > + { CY_I2C_NAME, 0 }, { } > > +}; > > + > > +static struct i2c_driver cyttsp_i2c_driver = { > > + .driver = { > > + .name = CY_I2C_NAME, > > + .owner = THIS_MODULE, > > + }, > > + .probe = cyttsp_i2c_probe, > > + .remove = __devexit_p(cyttsp_i2c_remove), > > + .id_table = cyttsp_i2c_id, > > +#ifdef CONFIG_PM > > + .suspend = cyttsp_i2c_suspend, > > + .resume = cyttsp_i2c_resume, > > +#endif > > +}; > > + > > +static int cyttsp_i2c_init(void) > > +{ > > Please add __init like > > static int __init cyttsp_i2c_init(void) Will add. > > > + int retval; > > + retval = i2c_add_driver(&cyttsp_i2c_driver); > > + printk(KERN_INFO "%s: Cypress TrueTouch(R) Standard Product I2C " > > + "Touchscreen Driver (Built %s @ %s) returned %d\n", > > + __func__, __DATE__, __TIME__, retval); > > + > > + return retval; > > +} > > + > > +static void cyttsp_i2c_exit(void) > > __exit > Will add. > > +{ > > + return i2c_del_driver(&cyttsp_i2c_driver); > > +} > > + > > +module_init(cyttsp_i2c_init); > > +module_exit(cyttsp_i2c_exit); > > + > > +MODULE_ALIAS("i2c:cyttsp"); > > +MODULE_LICENSE("GPL"); > > +MODULE_DESCRIPTION("Cypress TrueTouch(R) Standard Product (TTSP) I2C > driver"); > > +MODULE_AUTHOR("Cypress"); > > +MODULE_DEVICE_TABLE(i2c, cyttsp_i2c_id); > > diff --git a/drivers/input/touchscreen/cyttsp_spi.c > b/drivers/input/touchscreen/cyttsp_spi.c > > new file mode 100755 > > index 0000000..cb6432a > > --- /dev/null > > +++ b/drivers/input/touchscreen/cyttsp_spi.c > > I will comment on spi driver interface later. Thank you for your review Trilok. -kev > > ---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. --------------------------------------------------------------- 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. --------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
