Hi Hn,

On Mon, May 11, 2015 at 01:11:39AM +0800, HungNien Chen wrote:
> Signed-off-by: HungNien Chen <hn.c...@weidahitech.com>

Thank you for making the requested changes. Some more comments below.

> ---
>  drivers/input/touchscreen/Kconfig       |   12 +
>  drivers/input/touchscreen/Makefile      |    1 +
>  drivers/input/touchscreen/wdt87xx_i2c.c | 1509 
> +++++++++++++++++++++++++++++++
>  3 files changed, 1522 insertions(+)
>  create mode 100644 drivers/input/touchscreen/wdt87xx_i2c.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig 
> b/drivers/input/touchscreen/Kconfig
> index 80f6386..0c1a6cc 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -658,6 +658,18 @@ config TOUCHSCREEN_PIXCIR
>         To compile this driver as a module, choose M here: the
>         module will be called pixcir_i2c_ts.
>  
> +config TOUCHSCREEN_WDT87XX_I2C
> +     tristate "Weida HiTech I2C touchscreen"
> +     depends on I2C
> +     help
> +       Say Y here if you have an Weida WDT87XX I2C touchscreen
> +       connected to your system.
> +
> +       If unsure, say N.
> +
> +       To compile this driver as a module, choose M here: the
> +       module will be called wdt87xx_i2c.
> +
>  config TOUCHSCREEN_WM831X
>       tristate "Support for WM831x touchscreen controllers"
>       depends on MFD_WM831X
> diff --git a/drivers/input/touchscreen/Makefile 
> b/drivers/input/touchscreen/Makefile
> index 44deea7..fa3d33b 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_TOUCHSCREEN_TSC2007)   += tsc2007.o
>  obj-$(CONFIG_TOUCHSCREEN_UCB1400)    += ucb1400_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_WACOM_W8001)        += wacom_w8001.o
>  obj-$(CONFIG_TOUCHSCREEN_WACOM_I2C)  += wacom_i2c.o
> +obj-$(CONFIG_TOUCHSCREEN_WDT87XX_I2C)        += wdt87xx_i2c.o
>  obj-$(CONFIG_TOUCHSCREEN_WM831X)     += wm831x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_WM97XX)     += wm97xx-ts.o
>  wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9705)       += wm9705.o
> diff --git a/drivers/input/touchscreen/wdt87xx_i2c.c 
> b/drivers/input/touchscreen/wdt87xx_i2c.c
> new file mode 100644
> index 0000000..e3eef1c
> --- /dev/null
> +++ b/drivers/input/touchscreen/wdt87xx_i2c.c
> @@ -0,0 +1,1509 @@
> +/*
> + * Weida HiTech WDT87xx TouchScreen I2C driver
> + *
> + * Copyright (c) 2014  Weida HiTech Ltd.
> + * HN Chen <hn.c...@weidahitech.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * Note: this is a I2C device driver and report touch events througt the
> + *                   input device
> + */
> +
> +
> +#include <linux/version.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/irq.h>
> +#include <linux/ioc4.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/proc_fs.h>
> +#include <linux/slab.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio.h>
> +#include <linux/input/mt.h>
> +#include <asm/unaligned.h>
> +#include <linux/acpi.h>
> +
> +#define WDT87XX_NAME         "wdt87xx_i2c"
> +#define      WDT87XX_DRV_VER         "0.9.2"
> +#define      WDT87XX_FW_NAME         "wdt87xx_fw.bin"
> +
> +#define      WDT87XX_FW                      1
> +#define      WDT87XX_CFG                     2
> +
> +#define MODE_ACTIVE                  0x01
> +#define MODE_READY                   0x02
> +#define MODE_IDLE                    0x03
> +#define MODE_SLEEP                   0x04
> +#define      MODE_STOP                       0xFF
> +
> +#define WDT_MAX_POINT                        10
> +#define      WDT_RAW_BUF_COUNT               54
> +
> +#define      PG_SIZE                         0x1000
> +#define MAX_RETRIES                  3
> +
> +#define      WDT_UNITS_PER_MM                60
> +
> +/* the definition for one finger */
> +#define      FINGER_EV_OFFSET_ID             0
> +#define      FINGER_EV_OFFSET_X              1
> +#define      FINGER_EV_OFFSET_Y              3
> +#define      FINGER_EV_SIZE                  5
> +
> +/* the definition for a packet */
> +#define      TOUCH_PK_OFFSET_REPORT_ID       0
> +#define      TOUCH_PK_OFFSET_EVENT           1
> +#define TOUCH_PK_OFFSET_SCAN_TIME    51
> +#define      TOUCH_PK_OFFSET_FNGR_NUM        53
> +
> +/* the definition for the controller parameters */
> +#define      CTL_PARAM_OFFSET_FW_ID          0
> +#define      CTL_PARAM_OFFSET_PLAT_ID        2
> +#define      CTL_PARAM_OFFSET_XMLS_ID1       4
> +#define      CTL_PARAM_OFFSET_XMLS_ID2       6
> +#define      CTL_PARAM_OFFSET_PHY_X          8
> +#define      CTL_PARAM_OFFSET_PHY_Y          10
> +
> +struct sys_param {
> +     unsigned short  fw_id;
> +     unsigned short  plat_id;
> +     unsigned short  xmls_id1;
> +     unsigned short  xmls_id2;
> +     unsigned short  phy_x;
> +     unsigned short  phy_y;
> +} __packed;
> +
> +/* the definition for this driver needed */
> +struct wdt87xx_ts_data {
> +     struct i2c_client               *client;
> +     struct input_dev                *input_dev;
> +/* to protect the operation in sysfs */
> +     struct mutex                    sysfs_mutex;
> +     unsigned int                    max_retries;
> +     struct sys_param                sys_param;
> +     u8      finger_last_flag[WDT_MAX_POINT<<1];
> +};
> +
> +/* communacation commands */
> +#define      PACKET_SIZE                     56
> +#define      VND_REQ_READ                    0x06
> +#define      VND_READ_DATA                   0x07
> +#define      VND_REQ_WRITE                   0x08
> +
> +#define VND_CMD_START                        0x00
> +#define VND_CMD_STOP                 0x01
> +#define VND_CMD_RESET                        0x09
> +
> +#define VND_CMD_ERASE                        0x1A
> +
> +#define      VND_GET_CHECKSUM                0x66
> +
> +#define      VND_SET_DATA                    0x83
> +#define      VND_SET_COMMAND_DATA            0x84
> +#define      VND_SET_CHECKSUM_CALC           0x86
> +#define      VND_SET_CHECKSUM_LENGTH         0x87
> +
> +#define VND_CMD_SFLCK                        0xFC
> +#define VND_CMD_SFUNL                        0xFD
> +
> +#define      STRIDX_PLATFORM_ID              0x80
> +#define      STRIDX_PARAMETERS               0x81
> +
> +
> +/* the definition of command structure */
> +union cmd_data {
> +     struct {
> +     unsigned char   report_id;
> +     unsigned char   type;
> +     unsigned short  index;
> +     unsigned int    length;
> +     } defined_data;
> +     unsigned char   buffer[8];
> +};
> +
> +/* the definition of packet structure */
> +union req_data {
> +     struct {
> +     unsigned char   report_id;
> +     unsigned char   type;
> +     unsigned short  index;
> +     unsigned int    length;
> +     unsigned char   data[PACKET_SIZE];
> +     } defined_data;
> +     unsigned char   buffer[64];
> +};
> +
> +/* the definition of firmware data structure */
> +struct chunk_info {
> +     unsigned int    target_start_addr;
> +     unsigned int    length;
> +     unsigned int    source_start_addr;
> +     unsigned int    version_number;
> +     unsigned int    attribute;
> +     unsigned int    temp;
> +};
> +
> +struct chunk_data {
> +     unsigned int    ck_id;
> +     unsigned int    ck_size;
> +     struct chunk_info       chunk_info;
> +     char            *data;
> +};
> +
> +struct format_chunk {
> +     unsigned int    ck_id;
> +     unsigned int    ck_size;
> +     unsigned int    number_chunk;
> +     unsigned int    enable_flag;
> +     unsigned int    checksum;
> +     unsigned int    temp1;
> +     unsigned int    temp2;
> +};
> +
> +struct chunk_info_ex {
> +     struct chunk_info       chunk_info;
> +     char                    *data;
> +     unsigned int            length;
> +};
> +
> +/* the definition of firmware chunk tags */
> +#define              FOURCC_ID_RIFF          0x46464952
> +#define              FOURCC_ID_WHIF          0x46494857
> +#define              FOURCC_ID_FRMT          0x544D5246
> +#define              FOURCC_ID_FRWR          0x52575246
> +#define              FOURCC_ID_CNFG          0x47464E43
> +
> +#define              CHUNK_ID_FRMT           0x00
> +#define              CHUNK_ID_FRWR           0x01
> +#define              CHUNK_ID_CNFG           0x02
> +
> +/* for the touch report */
> +static int   SCREEN_LOGICAL_MAX_X = 0x7FFF;
> +static int   SCREEN_LOGICAL_MAX_Y = 0x7FFF;
> +
> +/* output the error message */
> +#define dev_err(__dev, x...)         dev_err(__dev, x)

Why is this needed?

> +
> +static int wdt87xx_i2c_txrxdata(struct i2c_client *client, char *txdata,
> +             int txlen, char *rxdata, int rxlen);
> +static int wdt87xx_i2c_rxdata(struct i2c_client *client, char *rxdata,
> +             int length);
> +static int wdt87xx_i2c_txdata(struct i2c_client *client, char *txdata,
> +             int length);
> +static int wdt87xx_set_feature(struct i2c_client *client, unsigned char *buf,
> +             unsigned int buf_size);
> +static int wdt87xx_get_feature(struct i2c_client *client, unsigned char *buf,
> +             unsigned int buf_size);
> +static int wdt87xx_get_string(struct i2c_client *client, unsigned char 
> str_idx,
> +             unsigned char *buf, unsigned int buf_size);
> +
> +static unsigned int get_chunk_fourcc(unsigned int chunk_index)
> +{
> +     switch (chunk_index) {
> +     case    CHUNK_ID_FRMT:
> +             return FOURCC_ID_FRMT;
> +     case    CHUNK_ID_FRWR:
> +             return FOURCC_ID_FRWR;
> +     case    CHUNK_ID_CNFG:
> +             return FOURCC_ID_CNFG;
> +     default:
> +             return 0;
> +     }
> +     return 0;

Why do we need this conversion from CHUNK_ID_* to FOURCC_ID_*? Why can't
callers operate on original values?

> +}
> +
> +static int get_chunk_info(const struct firmware *fw, unsigned int 
> chunk_index,
> +     struct chunk_info_ex *fw_chunk_info,
> +     struct format_chunk *wif_format_chunk)
> +{
> +     unsigned int    chunk_four_cc;
> +     const char      *data = 0;
> +     unsigned int    data_len = 0;
> +     unsigned int    is_found = 0;

bool

> +
> +     /* check the pointer is null */
> +     if (!fw || !fw_chunk_info || !wif_format_chunk)
> +             return -EINVAL;
> +
> +     chunk_four_cc = get_chunk_fourcc(chunk_index);
> +
> +     if (!chunk_four_cc)
> +             return -EBADRQC;

-EINVAL

> +
> +     data = fw->data;
> +     data_len = fw->size;
> +
> +     /* check if the chunk is existed */
> +     if (wif_format_chunk->enable_flag | chunk_index) {
> +             unsigned int    start_pos = 12 + sizeof(struct format_chunk);
> +             struct chunk_data       chunk;
> +             unsigned int    ck_id, ck_size;
> +
> +             while (start_pos < data_len && !is_found)       {
> +                     ck_id = get_unaligned_le32(&data[start_pos]);
> +                     ck_size = get_unaligned_le32(&data[start_pos+4]);
> +
> +                     /* the chunk is found */
> +                     if (ck_id == chunk_four_cc) {
> +                             chunk.ck_id = ck_id;
> +                             chunk.ck_size = ck_size;
> +
> +                             chunk.data = (char *) &data[start_pos + 8
> +                                     + sizeof(struct chunk_info)];
> +                             chunk.chunk_info.target_start_addr =
> +                                     get_unaligned_le32(&data[start_pos+8]);
> +                             chunk.chunk_info.length =
> +                                     get_unaligned_le32(&data[start_pos+12]);
> +                             chunk.chunk_info.source_start_addr =
> +                                     get_unaligned_le32(&data[start_pos+16]);
> +                             chunk.chunk_info.version_number =
> +                                     get_unaligned_le32(&data[start_pos+20]);
> +                             chunk.chunk_info.attribute =
> +                                     get_unaligned_le32(&data[start_pos+24]);
> +                             chunk.chunk_info.temp =
> +                                     get_unaligned_le32(&data[start_pos+28]);
> +
> +                             memcpy(&fw_chunk_info->chunk_info,
> +                                     &chunk.chunk_info,
> +                                     sizeof(struct chunk_info));
> +
> +                             fw_chunk_info->length = chunk.chunk_info.length;
> +                             fw_chunk_info->data = chunk.data;
> +
> +                             is_found = 1;
> +                     } else
> +                             start_pos = start_pos + ck_size + 8;
> +             }
> +     }
> +
> +     if (is_found)
> +             return 0;
> +
> +     return -ENODATA;
> +}
> +
> +static int wdt87xx_get_sysparam(struct i2c_client *client,
> +             struct wdt87xx_ts_data *dev_wdt87xx)
> +{
> +     struct sys_param *ctr_param = &dev_wdt87xx->sys_param;
> +     unsigned char   buffer[72];

Please add a #define for 72. In general, please use symbolic names
instead of numeric constants.

> +     int             err = 0;

Please do not initialize variables unless actually needed. For example
you assign this variable a value 2 lines below. Gratuitous assignments
suppress compiler warnings about variables used while uninitialized
which may alert you to code paths where you forgot to assign them proper
value.

> +
> +     err = wdt87xx_get_string(client, STRIDX_PARAMETERS, buffer, 32);
> +     if (err) {
> +             dev_err(&client->dev, "get parameters error (%d)\n", err);
> +             return err;
> +     }
> +
> +     ctr_param->xmls_id1 =
> +             get_unaligned_le16(buffer + CTL_PARAM_OFFSET_XMLS_ID1);
> +     ctr_param->xmls_id2 =
> +             get_unaligned_le16(buffer + CTL_PARAM_OFFSET_XMLS_ID2);
> +     ctr_param->phy_x =
> +             get_unaligned_le16(buffer + CTL_PARAM_OFFSET_PHY_X);
> +     ctr_param->phy_y =
> +             get_unaligned_le16(buffer + CTL_PARAM_OFFSET_PHY_Y);
> +
> +     err = wdt87xx_get_string(client, STRIDX_PLATFORM_ID, buffer, 8);
> +     if (err) {
> +             dev_err(&client->dev, "get platform id error (%d)\n", err);
> +             return err;
> +     }
> +
> +     ctr_param->plat_id = buffer[1];
> +
> +     buffer[0] = 0xf2;
> +     err = wdt87xx_get_feature(client, buffer, 16);
> +     if (err) {
> +             dev_err(&client->dev, "get firmware id error (%d)\n", err);
> +             return err;
> +     }
> +
> +     if (buffer[0] != 0xf2) {
> +             dev_err(&client->dev, "fw id packet error: %x\n", buffer[0]);
> +             return -EBADRQC;
> +     }
> +
> +     ctr_param->fw_id = get_unaligned_le16(&buffer[1]);
> +
> +     dev_dbg(&client->dev, "fw_id: 0x%x, plat_id: 0x%x\n",
> +             ctr_param->fw_id, ctr_param->plat_id);
> +     dev_dbg(&client->dev, "xml_id1: %4x, xml_id2: %4x\n",
> +             ctr_param->xmls_id1, ctr_param->xmls_id2);
> +
> +     return 0;
> +}
> +
> +int  process_fw_data(struct i2c_client *client, const struct firmware *fw,
> +                             struct format_chunk *wif_format_chunk)

static. In fact every one of the functions and module-global structs in
this file should be static.

> +{
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     unsigned int    length = 0;
> +     struct chunk_info_ex            fw_chunk_info;
> +     int             err = 0;
> +     unsigned int    *u32_data;
> +     unsigned char   fw_id;
> +     unsigned char   chip_id;
> +     unsigned int    data1, data2;
> +
> +     /* check the pointer is null */
> +     if (!fw || !wif_format_chunk)
> +             return -EINVAL;
> +
> +     u32_data = (unsigned int *) fw->data;
> +     length = fw->size;
> +
> +     data1 = get_unaligned_le32(&u32_data[0]);
> +     data2 = get_unaligned_le32(&u32_data[2]);
> +     if (data1 != FOURCC_ID_RIFF || data2 != FOURCC_ID_WHIF) {
> +             dev_err(&client->dev, "Check data tag failed !\n");
> +             return -EBADRQC;
> +     }
> +
> +     /* the length should be equal */
> +     data1 = get_unaligned_le32(&u32_data[1]);
> +     if (data1 != length) {
> +             dev_err(&client->dev, "Check data length failed !\n");
> +             return -EINVAL;
> +     }
> +
> +     wif_format_chunk->ck_id = get_unaligned_le32(&u32_data[3]);
> +     wif_format_chunk->ck_size = get_unaligned_le32(&u32_data[4]);
> +     wif_format_chunk->number_chunk = get_unaligned_le32(&u32_data[5]);
> +     wif_format_chunk->enable_flag = get_unaligned_le32(&u32_data[6]);
> +     wif_format_chunk->checksum = get_unaligned_le32(&u32_data[7]);
> +     wif_format_chunk->temp1 = get_unaligned_le32(&u32_data[8]);
> +     wif_format_chunk->temp2 = get_unaligned_le32(&u32_data[9]);
> +
> +     dev_dbg(&client->dev, "version checking\n");
> +
> +     /* get the version number from the firmware */
> +     err = get_chunk_info(fw, CHUNK_ID_FRWR, &fw_chunk_info,
> +             wif_format_chunk);
> +     if (err) {
> +             dev_err(&client->dev, "can not extract data !\n");
> +             return -EBADR;
> +     }
> +
> +     fw_id = ((fw_chunk_info.chunk_info.version_number >> 12) & 0xF);
> +     chip_id = (((dev_wdt87xx->sys_param.fw_id) >> 12) & 0xF);
> +
> +     if (fw_id != chip_id) {
> +             wdt87xx_get_sysparam(client, dev_wdt87xx);

Why would it be different now?

> +             chip_id = (((dev_wdt87xx->sys_param.fw_id) >> 12) & 0xF);
> +
> +             if (fw_id != chip_id) {
> +                     dev_err(&client->dev, "FW is not match: ");
> +                     dev_err(&client->dev, "fw(%d), chip(%d)\n",
> +                             fw_id, chip_id);
> +                     return -ENODEV;
> +             }
> +     }
> +
> +     return 0;
> +}
> +
> +/* functions for the sysfs implementation */
> +static int wdt87xx_check_firmware(struct chunk_info_ex *fw_chunk_info,
> +                     int ck_id)
> +{
> +     unsigned int    data;
> +
> +     /* check the pointer is null */
> +     if (!fw_chunk_info)
> +             return -EINVAL;

Why would it be? This is an internal function in one driver, not general
purpose API. I think you can rely on callers passing you reasonable
data.

> +
> +     if (ck_id == CHUNK_ID_FRWR) {
> +             unsigned int    is_found = 0;
> +             unsigned int firmware_id = 0xa9e368f5;
 +              data = get_unaligned_le32(fw_chunk_info->data);
> +             if (data == firmware_id)
> +                     is_found = 1;
> +             else
> +                     return -EFAULT;
> +
> +             if (is_found)
> +                     return 0;

This is too complicated...

                fw_id = get_unaligned_le32(fw_chunk_info->data);
                if (fw_id != WDT78XX_FIRMWARE_ID)
                        return -EINVAL;

> +
>
> +     }
> +
> +     return 0;
> +}
> +
> +static int wdt87xx_set_feature(struct i2c_client *client, unsigned char *buf,
> +             unsigned int buf_size)
> +{
> +     int     err = 0;
> +     union req_data  *req_data_set = (union req_data *) buf;
> +     int             data_len = 0;
> +     /* for set/get packets used */
> +     unsigned char           xfer_buffer[80];
> +
> +     /* buffer size must be smaller than 64 */
> +     if (buf_size > 64)
> +             buf_size = 64;

Then you need to return error if you see it larger.
> +
> +     /* set feature command packet */
> +     xfer_buffer[data_len++] = 0x22;
> +     xfer_buffer[data_len++] = 0x00;
> +     if (req_data_set->defined_data.report_id > 0xF) {
> +             xfer_buffer[data_len++] = 0x30;
> +             xfer_buffer[data_len++] = 0x03;
> +             xfer_buffer[data_len++] = req_data_set->defined_data.report_id;
> +     } else {
> +             xfer_buffer[data_len++] = 0x30 |
> +                     req_data_set->defined_data.report_id;
> +             xfer_buffer[data_len++] = 0x03;
> +     }
> +     xfer_buffer[data_len++] = 0x23;
> +     xfer_buffer[data_len++] = 0x00;
> +     xfer_buffer[data_len++] = (buf_size & 0xFF);
> +     xfer_buffer[data_len++] = ((buf_size & 0xFF00) >> 8);
> +
> +     memcpy(&xfer_buffer[data_len], buf, buf_size);
> +
> +     err = wdt87xx_i2c_txdata(client, xfer_buffer, data_len + buf_size);
> +
> +     if (err < 0) {
> +             dev_err(&client->dev, "error no: (%d)\n", err);
> +             return err;
> +     }
> +
> +     mdelay(2);
> +
> +     return 0;
> +}
> +
> +static int wdt87xx_get_feature(struct i2c_client *client, unsigned char *buf,
> +             unsigned int buf_size)
> +{
> +     int     err = 0;
> +     unsigned char   tx_buffer[8];
> +     unsigned char   xfer_buffer[80];
> +     union req_data *req_data_get = (union req_data *) buf;
> +     int             data_len = 0;
> +     unsigned int    xfer_length = 0;
> +
> +     if (buf_size > 64)
> +             buf_size = 64;
> +
> +     /* get feature command packet */
> +     tx_buffer[data_len++] = 0x22;
> +     tx_buffer[data_len++] = 0x00;
> +     if (req_data_get->defined_data.report_id > 0xF) {
> +             tx_buffer[data_len++] = 0x30;
> +             tx_buffer[data_len++] = 0x02;
> +             tx_buffer[data_len++] = req_data_get->defined_data.report_id;
> +     } else {
> +             tx_buffer[data_len++] = 0x30 |
> +                     req_data_get->defined_data.report_id;
> +             tx_buffer[data_len++] = 0x02;
> +     }
> +     tx_buffer[data_len++] = 0x23;
> +     tx_buffer[data_len++] = 0x00;
> +
> +     err = wdt87xx_i2c_txrxdata(client, tx_buffer, data_len, xfer_buffer,
> +                     buf_size + 2);
> +
> +     if (err < 0) {
> +             dev_err(&client->dev, "error no: (%d)\n", err);
> +             return err;
> +     }
> +
> +     /* check size and copy the return data */
> +     xfer_length = get_unaligned_le16(xfer_buffer);
> +
> +     if (buf_size < xfer_length)
> +             xfer_length = buf_size;
> +
> +     memcpy(buf, &xfer_buffer[2], xfer_length);
> +
> +     mdelay(2);
> +
> +     return 0;
> +}
> +
> +static int wdt87xx_get_string(struct i2c_client *client, unsigned char 
> str_idx,
> +             unsigned char *buf, unsigned int buf_size)
> +{
> +     int     err = 0;
> +     unsigned char   tx_buffer[8] = { 0x22, 0x00, 0x13, 0x0E,
> +             0x00, 0x23, 0x00, 0x00 };
> +     unsigned char   xfer_buffer[80];
> +     unsigned int    xfer_length;
> +
> +     if (buf_size > 64)
> +             buf_size = 64;
> +
> +     tx_buffer[4] = str_idx;
> +
> +     err = wdt87xx_i2c_txrxdata(client, tx_buffer, 7, xfer_buffer,
> +             buf_size + 2);
> +
> +     if (err < 0) {
> +             dev_err(&client->dev, "error no: (%d)\n", err);
> +             return err;
> +     }
> +
> +     if (xfer_buffer[1] != 0x03) {
> +             dev_err(&client->dev, "error str id: (%d)\n", xfer_buffer[1]);
> +             return -EBADRQC;
> +     }
> +
> +     xfer_length = xfer_buffer[0];
> +
> +     if (buf_size < xfer_length)
> +             xfer_length = buf_size;
> +
> +     memcpy(buf, &xfer_buffer[2], xfer_length);
> +
> +     mdelay(2);
> +
> +     return 0;
> +}
> +
> +
> +static int wdt87xx_send_command(struct i2c_client *client, int cmd, int 
> value)
> +{
> +     union cmd_data  cmd_data_send;
> +
> +     /* set the command packet */
> +     cmd_data_send.defined_data.report_id = VND_REQ_WRITE;
> +     cmd_data_send.defined_data.type = VND_SET_COMMAND_DATA;
> +     put_unaligned_le16((unsigned short) cmd,
> +             &cmd_data_send.defined_data.index);
> +
> +     switch (cmd)    {
> +     case    VND_CMD_START:
> +     case    VND_CMD_STOP:
> +     case    VND_CMD_RESET:
> +             /* mode selector */
> +             put_unaligned_le32((value & 0xFF),
> +                     &cmd_data_send.defined_data.length);
> +             break;
> +     case    VND_CMD_SFLCK:
> +             put_unaligned_le16(0xC39B, &cmd_data_send.buffer[3]);
> +             break;
> +     case    VND_CMD_SFUNL:
> +             put_unaligned_le16(0x95DA, &cmd_data_send.buffer[3]);
> +             break;
> +     case    VND_CMD_ERASE:
> +     case    VND_SET_CHECKSUM_CALC:
> +     case    VND_SET_CHECKSUM_LENGTH:
> +             put_unaligned_le32(value, &cmd_data_send.buffer[3]);
> +             break;
> +     default:
> +             cmd_data_send.defined_data.report_id = 0;
> +             dev_err(&client->dev, "Invalid command: (%d)", cmd);
> +             return -EINVAL;
> +     }
> +
> +     return wdt87xx_set_feature(client, &cmd_data_send.buffer[0],
> +             sizeof(cmd_data_send));
> +}
> +
> +static int wdt87xx_write_data(struct i2c_client *client,
> +             const char *data, unsigned int address, int length)
> +{
> +     unsigned int    addr_start, data_len;
> +     unsigned short  packet_size;
> +     int             count = 0;
> +     int             err = 0;
> +     union req_data  req_data_set;
> +     const char      *source_data = 0;
> +
> +     source_data = data;
> +     data_len = length;
> +     addr_start = address;
> +
> +     /* address and length should be 4 bytes alignment */
> +     if ((addr_start & 0x3) != 0 || (data_len & 0x3) != 0)   {
> +             dev_err(&client->dev, "addr & len must be aligned %x, %x\n",
> +                     addr_start, data_len);
> +             return -EFAULT;
> +     }
> +
> +     packet_size = PACKET_SIZE;
> +
> +     req_data_set.defined_data.report_id = VND_REQ_WRITE;
> +     req_data_set.defined_data.type = VND_SET_DATA;
> +
> +     while (data_len) {
> +             if (data_len < PACKET_SIZE)
> +                     packet_size = data_len;
> +
> +             put_unaligned_le16(packet_size,
> +                     &req_data_set.defined_data.index);
> +             put_unaligned_le32(addr_start,
> +                     &req_data_set.defined_data.length);
> +
> +             memcpy(req_data_set.defined_data.data,
> +                     source_data, packet_size);
> +
> +             err = wdt87xx_set_feature(client, req_data_set.buffer, 64);
> +
> +             if (err)
> +                     break;
> +
> +             data_len = data_len - packet_size;
> +             source_data = source_data + packet_size;
> +             addr_start = addr_start + packet_size;
> +
> +             count++;
> +
> +             mdelay(4);
> +
> +             if ((count % 64) == 0)  {
> +                     count = 0;
> +                     dev_dbg(&client->dev, "#");
> +             }
> +     }
> +
> +     dev_dbg(&client->dev, "#\n");
> +
> +     return err;
> +}
> +
> +static unsigned short  misr(unsigned short currentValue,
> +             unsigned char newValue)
> +{
> +     unsigned int a, b;
> +     unsigned int bit0;
> +     unsigned int y;
> +
> +     a = currentValue;
> +     b = newValue;
> +     bit0 = a^(b&1);
> +     bit0 ^= a>>1;
> +     bit0 ^= a>>2;
> +     bit0 ^= a>>4;
> +     bit0 ^= a>>5;
> +     bit0 ^= a>>7;
> +     bit0 ^= a>>11;
> +     bit0 ^= a>>15;
> +     y = (a<<1)^b;
> +     y = (y&~1) | (bit0&1);
> +
> +     return (unsigned short) y;

I think I might have asked this before, by anyway: are we sure that we
don't already have this checksum implementation in lib/? And if we don't
maybe we should?

> +}
> +
> +static int wdt87xx_get_checksum(struct i2c_client *client,
> +             unsigned int *checksum, unsigned int address, int length)
> +{
> +     int             err = 0;
> +     int             time_delay = 0;
> +     union req_data  req_data_get;
> +     union cmd_data  cmd_data_get;
> +
> +     err = wdt87xx_send_command(client, VND_SET_CHECKSUM_LENGTH, length);
> +     if (err) {
> +             dev_err(&client->dev, "set checksum length fail (%d)\n", err);
> +             return err;
> +     }
> +
> +     err = wdt87xx_send_command(client, VND_SET_CHECKSUM_CALC, address);
> +     if (err) {
> +             dev_err(&client->dev, "calc checksum fail (%d)\n", err);
> +             return err;
> +     }
> +
> +     time_delay = (length + 1023) / 1024;
> +     /* to wait the operation compeletion */
> +     msleep(time_delay * 10);
> +
> +     cmd_data_get.defined_data.report_id = VND_REQ_READ;
> +     cmd_data_get.defined_data.type = VND_GET_CHECKSUM;
> +     cmd_data_get.defined_data.index = 0;
> +     cmd_data_get.defined_data.length = 0;
> +
> +     err = wdt87xx_set_feature(client, cmd_data_get.buffer, 8);
> +     if (err) {
> +             dev_err(&client->dev, "checksum set read fail (%d)\n", err);
> +             return err;
> +     }
> +
> +     memset(req_data_get.buffer, 0, 64);
> +     req_data_get.defined_data.report_id = VND_READ_DATA;
> +     err = wdt87xx_get_feature(client, req_data_get.buffer, 64);
> +     if (err) {
> +             dev_err(&client->dev, "read checksum fail (%d)\n", err);
> +             return err;
> +     }
> +
> +     *checksum = get_unaligned_le16(&req_data_get.buffer[8]);
> +
> +     return err;
> +}
> +
> +static unsigned short fw_checksum(const unsigned char *data,
> +             unsigned int length)
> +{
> +     unsigned int    i;
> +     unsigned short  checksum = 0;
> +
> +     checksum = 0x0000;
> +
> +     for (i = 0; i < length; i++)
> +             checksum = misr(checksum, data[i]);
> +
> +     return checksum;
> +}
> +
> +static int wdt87xx_write_firmware(struct i2c_client *client,
> +             struct chunk_info_ex *fw_chunk_info, int type)
> +{
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     int             err = 0;
> +     int             err1 = 0;
> +     int             size;
> +     int             start_addr = 0;
> +     int             page_size;
> +     int             retry_count = 0;
> +     int             is_equal = 0;
> +     int             max_retries;
> +     unsigned int    calc_checksum = 0;
> +     unsigned int    read_checksum = 0;
> +     const char      *data;
> +
> +     dev_info(&client->dev, "start 4k page program\n");
> +
> +     err = wdt87xx_send_command(client, VND_CMD_STOP, MODE_STOP);
> +     if (err) {
> +             dev_err(&client->dev, "command stop fail (%d)\n", err);
> +             return err;
> +     }
> +
> +     err = wdt87xx_send_command(client, VND_CMD_SFUNL, 0);
> +     if (err) {
> +             dev_err(&client->dev, "command sfunl fail (%d)\n", err);
> +             goto write_fail;
> +     }
> +
> +     mdelay(10);
> +
> +     start_addr = fw_chunk_info->chunk_info.target_start_addr;
> +     size = fw_chunk_info->chunk_info.length;
> +     data = fw_chunk_info->data;
> +
> +     max_retries = dev_wdt87xx->max_retries;
> +
> +     dev_info(&client->dev, "%x, %x, %d\n", start_addr, size, max_retries);
> +
> +     while (size && !err) {
> +             is_equal = 0;
> +             if (size > PG_SIZE) {
> +                     page_size = PG_SIZE;
> +                     size = size - PG_SIZE;
> +             } else {
> +                     page_size = size;
> +                     size = 0;
> +             }
> +
> +             for (retry_count = 0; retry_count < max_retries && !is_equal;
> +                     retry_count++) {
> +                     err = wdt87xx_send_command(client, VND_CMD_ERASE,
> +                             start_addr);
> +                     if (err) {
> +                             dev_err(&client->dev, "erase fail (%d)\n",
> +                                     err);
> +                             break;
> +                     }
> +
> +                     msleep(50);
> +
> +                     err = wdt87xx_write_data(client, data, start_addr,
> +                             page_size);
> +                     if (err) {
> +                             dev_err(&client->dev, "write data fail (%d)\n",
> +                                     err);
> +                             break;
> +                     }
> +
> +                     read_checksum = 0;
> +                     err = wdt87xx_get_checksum(client, &read_checksum,
> +                             start_addr, page_size);
> +                     if (err)
> +                             break;
> +
> +                     calc_checksum = fw_checksum(data, page_size);
> +
> +                     if (read_checksum == calc_checksum)
> +                             is_equal = 1;
> +                     else {
> +                             dev_err(&client->dev, "checksum fail: (%d)",
> +                                     retry_count);
> +                             dev_err(&client->dev, ",(%d), (%d)\n",
> +                                     read_checksum, calc_checksum);

Please do not split error messages in 2.

> +                     }
> +             }
> +
> +             if (retry_count == MAX_RETRIES) {
> +                     dev_err(&client->dev, "write page fail\n");
> +                     err = -EIO;
> +             }
> +
> +             start_addr = start_addr + page_size;
> +             data = data + page_size;
> +             dev_info(&client->dev, "%x, %x\n", start_addr, size);
> +     }
> +write_fail:
> +     err1 = wdt87xx_send_command(client, VND_CMD_SFLCK, 0);
> +     if (err1)
> +             dev_err(&client->dev, "command sflck fail (%d)\n", err1);
> +
> +     mdelay(10);
> +
> +     err1 = wdt87xx_send_command(client, VND_CMD_START, 0);
> +     if (err1)
> +             dev_err(&client->dev, "command start fail (%d)\n", err1);
> +
> +     dev_info(&client->dev, "stop 4k page program : ");
> +
> +     if (err || err1)
> +             dev_info(&client->dev, "fail\n");
> +     else
> +             dev_info(&client->dev, "pass\n");
> +
> +     if (err1)
> +             return err1;
> +
> +     return err;
> +}
> +
> +static int wdt87xx_sw_reset(struct i2c_client *client)
> +{
> +     int err = 0;
> +
> +     dev_info(&client->dev, "reset device now\n");
> +
> +     err = wdt87xx_send_command(client, VND_CMD_RESET, 0);
> +     if (err) {
> +             dev_err(&client->dev, "command reset fail (%d)\n", err);
> +             return err;
> +     }
> +
> +     msleep(100);
> +
> +     return 0;
> +}
> +
> +static int wdt87xx_load_chunk(struct i2c_client *client,
> +             const struct firmware *fw,
> +             struct format_chunk *wif_format_chunk, unsigned int ck_id)
> +{
> +     int err = 0;
> +     struct chunk_info_ex    fw_chunk_info;
> +
> +     err = get_chunk_info(fw, ck_id, &fw_chunk_info, wif_format_chunk);
> +     if (err) {
> +             dev_err(&client->dev, "can not get the fw !\n");
> +             goto failed;
> +     }
> +
> +     /* Check for incorrect bin file */
> +     err = wdt87xx_check_firmware(&fw_chunk_info, ck_id);
> +     if (err) {
> +             dev_err(&client->dev, "invalid chunk : (%d)\n", ck_id);
> +             goto failed;
> +     }
> +
> +     err = wdt87xx_write_firmware(client, &fw_chunk_info, ck_id);
> +     if (err)
> +             dev_err(&client->dev, "write firmware failed : (%d)\n",
> +                     ck_id);
> +
> +failed:
> +     return err;
> +}
> +
> +static int wdt87xx_load_fw(struct device *dev, const char *fn,
> +             unsigned char type)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     const struct firmware *fw = 0;
> +     int err = 0;
> +     struct format_chunk     wif_format_chunk;
> +
> +     err = request_firmware(&fw, fn, dev);
> +     if (err) {
> +             dev_err(&client->dev, "unable to open firmware %s\n", fn);
> +             return err;
> +     }
> +
> +     disable_irq(client->irq);
> +
> +     err = process_fw_data(client, fw, &wif_format_chunk);
> +     if (err) {
> +             dev_err(&client->dev, "bad fw file !\n");
> +             goto release_firmware;
> +     }
> +
> +     if (type & WDT87XX_FW)  {
> +             err = wdt87xx_load_chunk(client, fw, &wif_format_chunk,
> +                     CHUNK_ID_FRWR);
> +             if (err) {
> +                     dev_err(&client->dev, "load fw chunk failed !\n");
> +                     goto release_firmware;
> +             }
> +     }
> +
> +     if (type & WDT87XX_CFG) {
> +             err = wdt87xx_load_chunk(client, fw, &wif_format_chunk,
> +                     CHUNK_ID_CNFG);
> +             if (err) {
> +                     dev_err(&client->dev, "load cfg chunk failed !\n");
> +                     goto release_firmware;
> +             }
> +     }
> +
> +     err = wdt87xx_sw_reset(client);
> +
> +     if (err)
> +             dev_err(&client->dev, "software reset failed !\n");
> +
> +release_firmware:
> +     enable_irq(client->irq);
> +
> +     mdelay(10);
> +     release_firmware(fw);
> +     return err;
> +}
> +
> +static ssize_t wdt87xx_update_fw(struct device *dev,
> +             struct device_attribute *attr, const char *buf, size_t count)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     int err;
> +     unsigned char option = 0;
> +
> +     if (count <= 0)
> +             return -EINVAL;
> +
> +     err = kstrtou8(buf, 0, &option);
> +     if (err)
> +             return err;
> +
> +     dev_info(dev, "update option (%d)\n", option);
> +     if (option < 1 || option > 3)   {
> +             dev_err(&client->dev, "option is not supported\n");
> +             return -1;
> +     }
> +
> +     err = mutex_lock_interruptible(&dev_wdt87xx->sysfs_mutex);
> +     if (err)
> +             return err;
> +
> +     err = wdt87xx_load_fw(dev, WDT87XX_FW_NAME, option);
> +     if (err) {
> +             dev_err(&client->dev, "the firmware update failed(%d)\n",
> +                     err);
> +             count = err;
> +     }
> +
> +     mutex_unlock(&dev_wdt87xx->sysfs_mutex);
> +
> +     return count;
> +}
> +
> +static ssize_t wdt87xx_fw_id(struct device *dev,
> +             struct device_attribute *attr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     struct sys_param *sys_param = &dev_wdt87xx->sys_param;
> +     int err;
> +
> +     err = mutex_lock_interruptible(&dev_wdt87xx->sysfs_mutex);
> +     if (err)
> +             return err;
> +
> +     wdt87xx_get_sysparam(client, dev_wdt87xx);

Do we expect this to change (unless we flash new config/firmware)?

> +
> +     mutex_unlock(&dev_wdt87xx->sysfs_mutex);
> +
> +     return scnprintf(buf, PAGE_SIZE, "%x\n", sys_param->fw_id);
> +}
> +
> +static ssize_t wdt87xx_plat_id(struct device *dev,
> +             struct device_attribute *attr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     struct sys_param *sys_param = &dev_wdt87xx->sys_param;
> +     int err;
> +
> +     err = mutex_lock_interruptible(&dev_wdt87xx->sysfs_mutex);
> +     if (err)
> +             return err;
> +
> +     wdt87xx_get_sysparam(client, dev_wdt87xx);
> +
> +     mutex_unlock(&dev_wdt87xx->sysfs_mutex);
> +
> +     return scnprintf(buf, PAGE_SIZE, "%x\n", sys_param->plat_id);
> +}
> +
> +static ssize_t wdt87xx_xmls_id1(struct device *dev,
> +             struct device_attribute *attr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     struct sys_param *sys_param = &dev_wdt87xx->sys_param;
> +     int err;
> +
> +     err = mutex_lock_interruptible(&dev_wdt87xx->sysfs_mutex);
> +     if (err)
> +             return err;
> +
> +     wdt87xx_get_sysparam(client, dev_wdt87xx);
> +
> +     mutex_unlock(&dev_wdt87xx->sysfs_mutex);
> +
> +     return scnprintf(buf, PAGE_SIZE, "%x\n", sys_param->xmls_id1);
> +}
> +
> +static ssize_t wdt87xx_xmls_id2(struct device *dev,
> +             struct device_attribute *attr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct wdt87xx_ts_data *dev_wdt87xx = i2c_get_clientdata(client);
> +     struct sys_param *sys_param = &dev_wdt87xx->sys_param;
> +     int err;
> +
> +     err = mutex_lock_interruptible(&dev_wdt87xx->sysfs_mutex);
> +     if (err)
> +             return err;
> +
> +     wdt87xx_get_sysparam(client, dev_wdt87xx);
> +
> +     mutex_unlock(&dev_wdt87xx->sysfs_mutex);
> +
> +     return scnprintf(buf, PAGE_SIZE, "%x\n", sys_param->xmls_id2);
> +}
> +
> +
> +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, wdt87xx_update_fw);
> +static DEVICE_ATTR(fw_id, S_IRUGO, wdt87xx_fw_id, NULL);
> +static DEVICE_ATTR(plat_id, S_IRUGO, wdt87xx_plat_id, NULL);
> +static DEVICE_ATTR(xmls_id1, S_IRUGO, wdt87xx_xmls_id1, NULL);
> +static DEVICE_ATTR(xmls_id2, S_IRUGO, wdt87xx_xmls_id2, NULL);
> +
> +static struct attribute *wdt87xx_attrs[] = {
> +     &dev_attr_update_fw.attr,
> +     &dev_attr_fw_id.attr,
> +     &dev_attr_plat_id.attr,
> +     &dev_attr_xmls_id1.attr,
> +     &dev_attr_xmls_id2.attr,
> +     NULL
> +};
> +
> +static const struct attribute_group wdt87xx_attr_group = {
> +     .attrs = wdt87xx_attrs,
> +};
> +
> +static int wdt87xx_i2c_txrxdata(struct i2c_client *client, char *txdata,
> +                             int txlen, char *rxdata, int rxlen)
> +{
> +     int err;
> +
> +     struct i2c_msg msgs[] = {
> +             {
> +                     .addr   = client->addr,
> +                     .flags  = 0,
> +                     .len    = txlen,
> +                     .buf    = txdata,
> +             },
> +             {
> +                     .addr   = client->addr,
> +                     .flags  = I2C_M_RD,
> +                     .len    = rxlen,
> +                     .buf    = rxdata,
> +             },
> +     };
> +
> +     err = i2c_transfer(client->adapter, msgs, 2);
> +
> +     if (err < 0)
> +             dev_err(&client->dev, "%s: i2c read error (%d)\n",
> +                     __func__, err);
> +
> +     return err < 0 ? err : (err != ARRAY_SIZE(msgs) ? -EIO : 0);
> +}
> +
> +static int wdt87xx_i2c_rxdata(struct i2c_client *client, char *rxdata,
> +                             int length)
> +{
> +     int err;
> +
> +     err = i2c_master_recv(client, rxdata, length);
> +
> +     if (err < 0)
> +             dev_err(&client->dev, "%s: i2c read error (%d)\n",
> +                     __func__, err);
> +
> +     return err;
> +}
> +
> +static int wdt87xx_i2c_txdata(struct i2c_client *client, char *txdata,
> +                     int length)
> +{
> +     int err;
> +
> +     err = i2c_master_send(client, txdata, length);
> +     if (err < 0)
> +             dev_err(&client->dev, "%s: i2c write error (%d)\n",
> +                     __func__, err);
> +
> +     return err;
> +}
> +
> +static irqreturn_t wdt87xx_ts_interrupt(int irq, void *dev_id)
> +{
> +     struct wdt87xx_ts_data *dev_wdt87xx =
> +             (struct wdt87xx_ts_data *) dev_id;
> +     int err;
> +     int i, points;
> +     struct i2c_client *client = dev_wdt87xx->client;
> +     u8 raw_buf[64] = {0};
> +     u8 *ptr_raw_buf = 0;
> +
> +     err = wdt87xx_i2c_rxdata(client, raw_buf, WDT_RAW_BUF_COUNT);
> +
> +     if (err < 0) {
> +             dev_err(&client->dev, "read raw data fail (%d)\n", err);
> +             goto leave_irq;
> +     }
> +
> +     /* touch finger count */
> +     points = raw_buf[TOUCH_PK_OFFSET_FNGR_NUM];
> +
> +     dev_dbg(&client->dev, "point: (%d)\n", points);
> +
> +     if (points == 0) {
> +             for (i = 0; i < WDT_MAX_POINT; i++) {
> +                     /* force to send event of tip off */
> +                     if (dev_wdt87xx->finger_last_flag[i] != 0) {
> +                             dev_dbg(&client->dev, "tip off (%d)\n", i);
> +                             input_mt_slot(dev_wdt87xx->input_dev, i);
> +                             input_mt_report_slot_state(
> +                                     dev_wdt87xx->input_dev,
> +                                     MT_TOOL_FINGER, false);
> +                     }
> +             }
> +
> +             memset(dev_wdt87xx->finger_last_flag, 0, WDT_MAX_POINT<<1);
> +
> +             input_mt_sync_frame(dev_wdt87xx->input_dev);
> +             input_sync(dev_wdt87xx->input_dev);
> +             goto leave_irq;
> +     }
> +
> +     dev_dbg(&client->dev, "+++++++++\n");
> +
> +     ptr_raw_buf = &raw_buf[TOUCH_PK_OFFSET_EVENT];
> +     for (i = 0; i < WDT_MAX_POINT; i++) {
> +             int point_id = (*ptr_raw_buf >> 3) - 1;
> +
> +             if (point_id < 0)
> +                     continue;

So if we get point_id < 0 in the first event block we'll skip all the
rest since "continue" would not advance ptr_raw_buf. If that was
intended then "break" would be more appropriate.

Can you tell me a bit more about the way device sends touch data? Does
it always send all 10 contacts regardless of data in
raw_buf[TOUCH_PK_OFFSET_FNGR_NUM]?

> +
> +             /* tip off */
> +             if (((*ptr_raw_buf & 0x1) == 0) &&
> +                     (dev_wdt87xx->finger_last_flag[point_id] != 0)) {
> +                     dev_dbg(&client->dev, "tip on (%d)\n", point_id);
> +                     input_mt_slot(dev_wdt87xx->input_dev, point_id);
> +                     input_mt_report_slot_state(dev_wdt87xx->input_dev,
> +                             MT_TOOL_FINGER, false);

If you are using INPUT_MT_DROP_UNUSED then I don't think you should
explicitly release slots here and above.

> +             } else  /* tip on */
> +             if (*ptr_raw_buf & 0x1) {
> +                     unsigned int    point_x, point_y;
> +
> +                     point_x = get_unaligned_le16(ptr_raw_buf +
> +                             FINGER_EV_OFFSET_X);
> +                     point_y = get_unaligned_le16(ptr_raw_buf +
> +                             FINGER_EV_OFFSET_Y);
> +
> +                     /* incorrect coordinate */
> +                     if (point_x > 0x7FFF || point_y > 0x7FFF)
> +                             continue;
> +
> +                     dev_dbg(&client->dev, "tip on (%d), x(%d), y(%d)\n",
> +                             i, point_x, point_y);
> +
> +                     input_mt_slot(dev_wdt87xx->input_dev, point_id);
> +                     input_mt_report_slot_state(dev_wdt87xx->input_dev,
> +                             MT_TOOL_FINGER, true);
> +                     input_report_abs(dev_wdt87xx->input_dev,
> +                             ABS_MT_TOUCH_MAJOR, 1);
> +                     input_report_abs(dev_wdt87xx->input_dev,
> +                             ABS_MT_POSITION_X, point_x);
> +                     input_report_abs(dev_wdt87xx->input_dev,
> +                             ABS_MT_POSITION_Y, point_y);
> +             }
> +             dev_wdt87xx->finger_last_flag[point_id] = (*ptr_raw_buf & 0x1);
> +             ptr_raw_buf += FINGER_EV_SIZE;
> +     }
> +
> +     input_mt_sync_frame(dev_wdt87xx->input_dev);
> +     input_sync(dev_wdt87xx->input_dev);
> +
> +leave_irq:
> +     return IRQ_HANDLED;
> +}
> +
> +
> +static int wdt87xx_ts_request_irq(struct i2c_client *client)
> +{
> +     int err = 0;
> +     struct wdt87xx_ts_data *dev_wdt87xx;
> +
> +     dev_wdt87xx = i2c_get_clientdata(client);
> +
> +     err = request_threaded_irq(client->irq, NULL, wdt87xx_ts_interrupt,
> +             IRQF_ONESHOT, client->name, dev_wdt87xx);
> +
> +     if (err < 0) {
> +             dev_err(&client->dev, "%s: request threaded irq fail (%d)\n",
> +                     __func__, err);
> +             return err;
> +     }
> +
> +     disable_irq_nosync(client->irq);

This is still racy as far as I can see. You can get interrupt
immediately after requesting it and input device has not been allocated
yet. Please allocate it first, then request IRQ, then register it. By
the way, consider using devm_request_threaded_irq and
devm_input_allocate_device. 

> +
> +     return 0;
> +}
> +
> +static int wdt87xx_ts_create_input_device(struct i2c_client *client)
> +{
> +     int err = 0;
> +     struct wdt87xx_ts_data *dev_wdt87xx;
> +     struct input_dev        *input_dev;
> +
> +     dev_wdt87xx = (struct wdt87xx_ts_data *) i2c_get_clientdata(client);
> +
> +     input_dev = input_allocate_device();
> +     if (!input_dev) {
> +             dev_err(&client->dev, "%s: failed to allocate input device\n",
> +                     __func__);
> +             return -ENOMEM;
> +     }
> +
> +     dev_wdt87xx->input_dev = input_dev;
> +
> +     input_dev->name = "WDT87xx Touchscreen";
> +     input_dev->id.bustype = BUS_I2C;
> +
> +     __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +
> +     /* for single touch */
> +     set_bit(ABS_X, input_dev->absbit);
> +     set_bit(ABS_Y, input_dev->absbit);
> +     input_set_abs_params(input_dev, ABS_X, 0, SCREEN_LOGICAL_MAX_X, 0, 0);
> +     input_set_abs_params(input_dev, ABS_Y, 0, SCREEN_LOGICAL_MAX_Y, 0, 0);
> +     input_abs_set_res(input_dev, ABS_X, WDT_UNITS_PER_MM);
> +     input_abs_set_res(input_dev, ABS_Y, WDT_UNITS_PER_MM);
> +
> +     input_mt_init_slots(input_dev, WDT_MAX_POINT,
> +             INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> +
> +     set_bit(ABS_MT_TOUCH_MAJOR, input_dev->absbit);
> +     set_bit(ABS_MT_POSITION_X, input_dev->absbit);
> +     set_bit(ABS_MT_POSITION_Y, input_dev->absbit);
> +     set_bit(ABS_MT_WIDTH_MAJOR, input_dev->absbit);
> +
> +     input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
> +                             SCREEN_LOGICAL_MAX_X, 0, 0);
> +     input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
> +                             SCREEN_LOGICAL_MAX_Y, 0, 0);
> +     input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 0xFF, 0, 0);
> +     input_set_abs_params(input_dev, ABS_MT_WIDTH_MAJOR, 0, 200, 0, 0);
> +     input_abs_set_res(input_dev, ABS_MT_POSITION_X, WDT_UNITS_PER_MM);
> +     input_abs_set_res(input_dev, ABS_MT_POSITION_Y, WDT_UNITS_PER_MM);
> +
> +     set_bit(ABS_PRESSURE, input_dev->absbit);
> +     input_set_abs_params(input_dev, ABS_PRESSURE, 0, 0xFF, 0, 0);

Your device does not appear to report pressure so do not declare it in
capabilities.

> +
> +     input_set_abs_params(input_dev, ABS_MT_TRACKING_ID, 0,
> +             WDT_MAX_POINT, 0, 0);
> +
> +     set_bit(EV_ABS, input_dev->evbit);
> +     set_bit(EV_SYN, input_dev->evbit);
> +
> +     err = input_register_device(input_dev);
> +     if (err) {
> +             dev_err(&client->dev, "register input device fail (%d)\n",
> +                     err);
> +             input_free_device(input_dev);
> +             dev_wdt87xx->input_dev = 0;
> +             return err;
> +     }
> +
> +     return 0;
> +}
> +
> +static int wdt87xx_ts_probe(struct i2c_client *client,
> +                     const struct i2c_device_id *id)
> +{
> +     struct wdt87xx_ts_data *dev_wdt87xx;
> +     int err = 0;
> +
> +     dev_dbg(&client->dev, "wdt87xx : adapter=(%d), client irq:(%d)\n",
> +             client->adapter->nr, client->irq);
> +
> +     /* check if the I2C function is ok in this adaptor */
> +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +             return -ENODEV;
> +
> +     dev_dbg(&client->dev, "kzalloc\n");
> +     dev_wdt87xx = devm_kzalloc(&client->dev,
> +             sizeof(struct wdt87xx_ts_data), GFP_KERNEL);
> +     if (!dev_wdt87xx) {
> +             err = -ENOMEM;
> +             goto exit_alloc_data_fail;
> +     }
> +
> +     dev_dbg(&client->dev, "i2c_set_clientdata\n");
> +
> +     dev_wdt87xx->client = client;
> +     mutex_init(&dev_wdt87xx->sysfs_mutex);
> +     i2c_set_clientdata(client, dev_wdt87xx);
> +     dev_wdt87xx->max_retries = MAX_RETRIES;
> +
> +     dev_dbg(&client->dev, "wdt87xx_ts_request_irq\n");
> +     err = wdt87xx_ts_request_irq(client);
> +     if (err < 0) {
> +             dev_err(&client->dev, "request irq fail (%d)\n", err);
> +             goto exit_irq_request;
> +     }
> +
> +     dev_dbg(&client->dev, "wdt87xx_ts_create_input_device\n");
> +     err = wdt87xx_ts_create_input_device(client);
> +     if (err < 0) {
> +             dev_err(&client->dev, "create input device fail (%d)\n", err);
> +             goto exit_create_input_device;
> +     }
> +
> +     dev_dbg(&client->dev, "sysfs_create_group\n");
> +     err = sysfs_create_group(&client->dev.kobj, &wdt87xx_attr_group);
> +     if (err) {
> +             dev_err(&client->dev, "create sysfs fail (%d)\n", err);
> +             goto exit_sysfs_group;
> +     }
> +
> +     enable_irq(client->irq);
> +
> +     dev_dbg(&client->dev, "%s leave\n", __func__);
> +
> +     return 0;
> +
> +exit_sysfs_group:
> +     if (dev_wdt87xx->input_dev)
> +             input_unregister_device(dev_wdt87xx->input_dev);
> +exit_create_input_device:
> +     if (client->irq)
> +             free_irq(client->irq, dev_wdt87xx);
> +exit_irq_request:
> +exit_alloc_data_fail:
> +     return err;
> +}
> +
> +static int wdt87xx_ts_remove(struct i2c_client *client)
> +{
> +     struct wdt87xx_ts_data *dev_wdt87xx =
> +             (struct wdt87xx_ts_data *) i2c_get_clientdata(client);
> +
> +     dev_dbg(&client->dev, "==%s==\n", __func__);
> +
> +     sysfs_remove_group(&client->dev.kobj, &wdt87xx_attr_group);
> +
> +     if (client->irq)

This check is not needed.

> +             free_irq(client->irq, dev_wdt87xx);
> +
> +     if (dev_wdt87xx->input_dev)

This check is not needed.

> +             input_unregister_device(dev_wdt87xx->input_dev);
> +
> +     return 0;
> +}
> +
> +static int __maybe_unused wdt87xx_suspend(struct device *dev)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     int err = 0;
> +
> +     dev_dbg(&client->dev, "enter %s\n", __func__);
> +
> +     disable_irq(client->irq);
> +
> +     err = wdt87xx_send_command(client, VND_CMD_STOP, MODE_IDLE);
> +     if (err)
> +             dev_err(&client->dev, "%s: command stop fail (%d)\n",
> +                     __func__, err);
> +
> +     return err;
> +}
> +
> +static int __maybe_unused wdt87xx_resume(struct device *dev)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     int err = 0;
> +
> +     /* once the chip is reset before resume,  */
> +     /* we need some time to wait it is stable */
> +     mdelay(100);
> +
> +     err = wdt87xx_send_command(client, VND_CMD_START, 0);
> +     if (err)
> +             dev_err(&client->dev, "%s: command start fail (%d)\n",
> +                     __func__, err);
> +
> +     enable_irq(client->irq);
> +
> +     dev_dbg(&client->dev, "leave %s\n", __func__);
> +
> +     return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(wdt87xx_pm_ops, wdt87xx_suspend, wdt87xx_resume);
> +
> +static const struct i2c_device_id wdt87xx_dev_id[] = {
> +     { WDT87XX_NAME, 0 },
> +     { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, wdt87xx_dev_id);
> +
> +static const struct acpi_device_id wdt87xx_acpi_id[] = {
> +     { "WDHT0001", 0 },
> +     { }
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, wdt87xx_acpi_id);
> +
> +static struct i2c_driver wdt87xx_driver = {
> +     .probe          = wdt87xx_ts_probe,
> +     .remove         = wdt87xx_ts_remove,
> +     .id_table       = wdt87xx_dev_id,
> +     .driver = {
> +             .name   = WDT87XX_NAME,
> +             .owner  = THIS_MODULE,
> +             .pm     = &wdt87xx_pm_ops,
> +             .acpi_match_table = ACPI_PTR(wdt87xx_acpi_id),
> +     },
> +};
> +
> +module_i2c_driver(wdt87xx_driver);
> +
> +MODULE_AUTHOR("HN Chen <hn.c...@weidahitech.com>");
> +MODULE_DESCRIPTION("WeidaHiTech WDT87XX Touchscreen driver");
> +MODULE_VERSION(WDT87XX_DRV_VER);
> +MODULE_LICENSE("GPL");
> +
> -- 
> 1.9.1
> 

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to