From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oliver Neukum Subject: Re: [PATCH 1/2] INPUT/HID: add touch support for SiS touch driver Date: Mon, 12 Jan 2015 12:50:49 +0100 Message-ID: <1421063449.12454.17.camel@linux-0dmf.site> References: <8322374EB97AA24A95D0DDBFC8F1CA1DBF987B@SISMBEV01.sis.com.tw> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <8322374EB97AA24A95D0DDBFC8F1CA1DBF987B@SISMBEV01.sis.com.tw> Sender: linux-kernel-owner@vger.kernel.org To: "=?UTF-8?Q?=E6=9B=BE=E5=A9=B7=E8=91=B3?= (tammy_tseng)" Cc: linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, tammy0524@gmail.com List-Id: linux-input@vger.kernel.org On Mon, 2015-01-12 at 18:53 +0800, =E6=9B=BE=E5=A9=B7=E8=91=B3 (tammy_t= seng) wrote: > Hi, > This package of patch is to add support for multitouch behavior for S= iS touch products. > The patch of SiS i2c multitouch driver is in input/touchscreen. >=20 > Signed-off-by: Tammy Tseng >=20 > --- > diff --git a/linux-3.18.1/drivers/input/touchscreen/Kconfig b/linux-3= =2E18.1/drivers/input/touchscreen/Kconfig > index e1d8003..edc8e27 100644 > --- a/linux-3.18.1/drivers/input/touchscreen/Kconfig > +++ b/linux-3.18.1/drivers/input/touchscreen/Kconfig > @@ -962,4 +962,18 @@ config TOUCHSCREEN_ZFORCE > To compile this driver as a module, choose M here: the > module will be called zforce_ts. > =20 > +config TOUCHSCREEN_SIS_I2C > + tristate "SiS 9200 family I2C touchscreen driver" > + depends on I2C > + default y Why that default? The majority of systems will not feature this touchscreens. > + help > + This enables support for SiS 9200 family over I2C based touchscre= ens. > + > +config FW_SUPPORT_POWERMODE > + default n > + bool "SiS FW support power mode" > + depends on TOUCHSCREEN_SIS_I2C > + help > + This enables support power mode provided by SiS firmwave What does this mode do? The help should be more extensive to be helpful. > + > endif > diff --git a/linux-3.18.1/drivers/input/touchscreen/Makefile b/linux-= 3.18.1/drivers/input/touchscreen/Makefile > index 090e61c..e316477 100644 > --- a/linux-3.18.1/drivers/input/touchscreen/Makefile > +++ b/linux-3.18.1/drivers/input/touchscreen/Makefile > @@ -6,6 +6,10 @@ > =20 > wm97xx-ts-y :=3D wm97xx-core.o > =20 > +ifdef CONFIG_TOUCHSCREEN_SIS_I2C > +obj-$(CONFIG_TOUCHSCREEN_SIS_I2C) +=3D sis_i2c.o > +endif > + > obj-$(CONFIG_OF_TOUCHSCREEN) +=3D of_touchscreen.o > obj-$(CONFIG_TOUCHSCREEN_88PM860X) +=3D 88pm860x-ts.o > obj-$(CONFIG_TOUCHSCREEN_AD7877) +=3D ad7877.o > diff --git a/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c b/linux= -3.18.1/drivers/input/touchscreen/sis_i2c.c > new file mode 100644 > index 0000000..2e6fc1a > --- /dev/null > +++ b/linux-3.18.1/drivers/input/touchscreen/sis_i2c.c > @@ -0,0 +1,1725 @@ > +/* drivers/input/touchscreen/sis_i2c.c - I2C Touch panel driver for = SiS 9200 family > + * > + * Copyright (C) 2011 SiS, Inc. > + * > + * This software is licensed under the terms of the GNU General Publ= ic > + * License version 2, as published by the Free Software Foundation, = and > + * may be copied, distributed, and modified under those terms. > + * > + * 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. > + * > + * Date: 2012/11/13 > + * Version: Android_v2.05.00-A639-1113 > + */ > + > +#include > +#include > +#ifdef CONFIG_HAS_EARLYSUSPEND > +#include > +#endif Conditional includes should be avoided if possible. > +#include > +#include > +#include > +#include > +#include > +#include > +#include "sis_i2c.h" > +#include > +#include > +#include > +#include > +#include > + > +#ifdef _STD_RW_IO ??? > +#include > +#include > +#include > +#define DEVICE_NAME "sis_aegis_touch_device" > +static int sis_char_devs_count =3D 1; /* device count */ Why start with 1 ? > +static int sis_char_major =3D 0; > +static struct cdev sis_char_cdev; > +static struct class *sis_char_class =3D NULL; > +#endif > + > +/* Addresses to scan */ > +static const unsigned short normal_i2c[] =3D { SIS_SLAVE_ADDR, I2C_C= LIENT_END }; > +static struct workqueue_struct *sis_wq; > +struct sis_ts_data *ts_bak =3D 0; > +struct sisTP_driver_data *TPInfo =3D NULL; Are you really sure you are limited to a single device? > +static void sis_tpinfo_clear(struct sisTP_driver_data *TPInfo, int m= ax); > + > +#ifdef CONFIG_HAS_EARLYSUSPEND > +static void sis_ts_early_suspend(struct early_suspend *h); > +static void sis_ts_late_resume(struct early_suspend *h); > +#endif > + > +#ifdef CONFIG_X86 > +//static const struct i2c_client_address_data addr_data; > +/* Insmod parameters */ > +static int sis_ts_detect(struct i2c_client *client, struct i2c_board= _info *info); > +#endif > + > +#ifdef _CHECK_CRC > +uint16_t cal_crc (char* cmd, int start, int end); > +#endif > + > +void PrintBuffer(int start, int length, char* buf) Polluting the name space like this is really nasty. Could you check which of your declarations can be declared "static"? > +{ > + int i; > + for ( i =3D start; i < length; i++ ) > + { > + printk("%02x ", buf[i]); > + if (i !=3D 0 && i % 30 =3D=3D 0) > + printk("\n"); > + } > + printk("\n");=09 > +} > + > +int sis_command_for_write(struct i2c_client *client, int wlength, un= signed char *wdata) > +{ > + int ret =3D -1; > + struct i2c_msg msg[1]; Why on earth an array of a single element? > + > + msg[0].addr =3D client->addr; > + msg[0].flags =3D 0; //Write > + msg[0].len =3D wlength; > + msg[0].buf =3D (unsigned char *)wdata; > + > + ret =3D i2c_transfer(client->adapter, msg, 1); > + > + return ret; > +} > + > +int sis_command_for_read(struct i2c_client *client, int rlength, uns= igned char *rdata) > +{ > + int ret =3D -1; > + struct i2c_msg msg[1]; Likewise > + > + msg[0].addr =3D client->addr; > + msg[0].flags =3D I2C_M_RD; //Read > + msg[0].len =3D rlength; > + msg[0].buf =3D rdata; > + > + ret =3D i2c_transfer(client->adapter, msg, 1); > + > + return ret; > +} > + > +int sis_cul_unit(uint8_t report_id) > +{ > + int basic =3D 6; > + int area =3D 2; > + int pressure =3D 1; > + int ret =3D basic; Why ??? > +=09 > + if (report_id !=3D ALL_IN_ONE_PACKAGE) > + { > + if (IS_AREA(report_id) /*&& IS_TOUCH(report_id)*/) > + { > + ret +=3D area; > + } > + if (IS_PRESSURE(report_id)) > + { > + ret +=3D pressure; > + } > + } > +=09 > + return ret;=09 > +} > + > +int sis_ReadPacket(struct i2c_client *client, uint8_t cmd, uint8_t* = buf) > +{ > + uint8_t tmpbuf[MAX_BYTE] =3D {0}; //MAX_BYTE =3D 64; > +#ifdef _CHECK_CRC > + uint16_t buf_crc =3D 0; > + uint16_t package_crc =3D 0; > + int l_package_crc =3D 0; > + int crc_end =3D 0; > +#endif > + int ret =3D -1; > + int touchnum =3D 0; > + int p_count =3D 0; > + int touc_formate_id =3D 0; > + int locate =3D 0; > + bool read_first =3D true; > + =20 > +/* > + New i2c format=20 > + * buf[0] =3D Low 8 bits of byte count value > + * buf[1] =3D High 8 bits of byte counte value > + * buf[2] =3D Report ID > + * buf[touch num * 6 + 2 ] =3D Touch informations; 1 touch point has= 6 bytes, it could be none if no touch=20 > + * buf[touch num * 6 + 3] =3D Touch numbers > + *=20 > + * One touch point information include 6 bytes, the order is=20 > + *=20 > + * 1. status =3D touch down or touch up > + * 2. id =3D finger id=20 > + * 3. x axis low 8 bits > + * 4. x axis high 8 bits > + * 5. y axis low 8 bits > + * 6. y axis high 8 bits > + *=20 > +*/ > + do > + { > + if (locate >=3D PACKET_BUFFER_SIZE) > + { > + printk(KERN_ERR "sis_ReadPacket: Buf Overflow\n"); > + return -1; > + } > + =09 > + ret =3D sis_command_for_read(client, MAX_BYTE, tmpbuf); > + > +#ifdef _DEBUG_PACKAGE > + printk(KERN_INFO "chaoban test: Buf_Data [0~63] \n"); > + PrintBuffer(0, 64, tmpbuf);=09 > +#endif =09 > + > + if(ret < 0 ) > + { > + printk(KERN_ERR "sis_ReadPacket: i2c transfer error\n"); It would be nicer to use the debug methods defined for devices, so that you get device identification in the log for free. > + return ret; > + } > + // error package length of receiving data > + else if (tmpbuf[P_BYTECOUNT] > MAX_BYTE) This looks like a bug. You are checking only the lower byte. > + { > + printk(KERN_ERR "sis_ReadPacket: Error Bytecount\n"); > + return -1;=09 > + } > + =09 > + if (read_first) > + { > +#ifdef _SUPPORT_BUTTON_TOUCH > + // access BUTTON TOUCH event and BUTTON NO TOUCH event > + if (tmpbuf[P_REPORT_ID] =3D=3D BUTTON_FORMAT) > + { > + memcpy(&buf[0], &tmpbuf[0], 7); > + return touchnum; //touchnum is 0 So why not return 0 ? > + } > +#endif=20 > + // access NO TOUCH event unless BUTTON NO TOUCH event > + if (tmpbuf[P_BYTECOUNT] =3D=3D 0/*NO_TOUCH_BYTECOUNT*/) > + { > + return touchnum; //touchnum is 0 > + } > + } > + > + //skip parsing data when two devices are registered at the same sl= ave address > + //parsing data when P_REPORT_ID && 0xf is TOUCH_FORMAT or P_REPORT= _ID is ALL_IN_ONE_PACKAGE > + touc_formate_id =3D tmpbuf[P_REPORT_ID] & 0xf; > + if ((touc_formate_id !=3D TOUCH_FORMAT) && (touc_formate_id !=3D H= IDI2C_FORMAT) && (tmpbuf[P_REPORT_ID] !=3D ALL_IN_ONE_PACKAGE)) > + { > + printk(KERN_ERR "sis_ReadPacket: Error Report_ID\n"); > + return -1; =09 > + } > + =09 > + p_count =3D (int) tmpbuf[P_BYTECOUNT] - 1; //start from 0 > + if (tmpbuf[P_REPORT_ID] !=3D ALL_IN_ONE_PACKAGE) > + { > + if (IS_TOUCH(tmpbuf[P_REPORT_ID])) > + { > + p_count -=3D BYTE_CRC_I2C; //delete 2 byte crc > + } > + else if (IS_HIDI2C(tmpbuf[P_REPORT_ID])) > + { > + p_count -=3D BYTE_CRC_HIDI2C; > + } > + else //should not be happen > + { > + printk(KERN_ERR "sis_ReadPacket: delete crc error\n"); > + return -1; > + } > + > + if (IS_SCANTIME(tmpbuf[P_REPORT_ID])) > + { > + p_count -=3D BYTE_SCANTIME; > + } > + } > + //else {} // For ALL_IN_ONE_PACKAGE > + =09 > + if (read_first) > + { > + touchnum =3D tmpbuf[p_count]; =09 > + } > + else > + { > + if (tmpbuf[p_count] !=3D 0) > + { > + printk(KERN_ERR "sis_ReadPacket: get error package\n"); > + return -1; > + } > + } > + > +#ifdef _CHECK_CRC > + crc_end =3D p_count + (IS_SCANTIME(tmpbuf[P_REPORT_ID]) * 2); > + buf_crc =3D cal_crc(tmpbuf, 2, crc_end); //sub bytecount (2 byte) > + l_package_crc =3D p_count + 1 + (IS_SCANTIME(tmpbuf[P_REPORT_ID]) = * 2); > + package_crc =3D ((tmpbuf[l_package_crc] & 0xff) | ((tmpbuf[l_packa= ge_crc + 1] & 0xff) << 8)); We have macros to read data in a defined endianness > + =09 > + if (buf_crc !=3D package_crc) > + { > + printk(KERN_ERR "sis_ReadPacket: CRC Error\n"); > + return -1; > + } > +#endif=09 > + memcpy(&buf[locate], &tmpbuf[0], 64); //Buf_Data [0~63] [64~128] > + locate +=3D 64; > + read_first =3D false; > + =09 > + }while(tmpbuf[P_REPORT_ID] !=3D ALL_IN_ONE_PACKAGE && tmpbuf[p_co= unt] > 5); > + > + return touchnum; > +}=09 > + > + > +int check_gpio_interrupt(void) > +{ > + int ret =3D 0; > + //TODO > + //CHECK GPIO INTERRUPT STATUS BY YOUR PLATFORM SETTING. > + ret =3D gpio_get_value(GPIO_IRQ); > + return ret; > +} > + > +void ts_report_key(struct i2c_client *client, uint8_t keybit_state) > +{ > + int i =3D 0; > + uint8_t diff_keybit_state=3D 0x0; //check keybit_state is differenc= e with pre_keybit_state > + uint8_t key_value =3D 0x0; //button location for binary > + uint8_t key_pressed =3D 0x0; //button is up or down > + struct sis_ts_data *ts =3D i2c_get_clientdata(client); > + > + if (!ts) > + { > + printk(KERN_ERR "%s error: Missing Platform Data!\n", __func__); > + return; > + } > + > + diff_keybit_state =3D TPInfo->pre_keybit_state ^ keybit_state; > + > + if (diff_keybit_state) > + { > + for (i =3D 0; i < BUTTON_KEY_COUNT; i++) > + { > + if ((diff_keybit_state >> i) & 0x01) > + { > + key_value =3D diff_keybit_state & (0x01 << i); > + key_pressed =3D (keybit_state >> i) & 0x01; > + switch (key_value) > + { > + case MSK_COMP: > + input_report_key(ts->input_dev, KEY_COMPOSE, key_pressed); > + printk(KERN_ERR "%s : MSK_COMP %d \n", __func__ , key_pressed)= ; > + break; > + case MSK_BACK: > + input_report_key(ts->input_dev, KEY_BACK, key_pressed); > + printk(KERN_ERR "%s : MSK_BACK %d \n", __func__ , key_pressed)= ; > + break; > + case MSK_MENU: > + input_report_key(ts->input_dev, KEY_MENU, key_pressed); > + printk(KERN_ERR "%s : MSK_MENU %d \n", __func__ , key_pressed)= ; > + break; > + case MSK_HOME: > + input_report_key(ts->input_dev, KEY_HOME, key_pressed); > + printk(KERN_ERR "%s : MSK_HOME %d \n", __func__ , key_pressed)= ; > + break; > + case MSK_NOBTN: > + //Release the button if it touched. > + default: > + break; > + } > + } > + } > + TPInfo->pre_keybit_state =3D keybit_state; > + } > +} > + > + > +static void sis_ts_work_func(struct work_struct *work) > +{ > + struct sis_ts_data *ts =3D container_of(work, struct sis_ts_data, w= ork); > + int ret =3D -1; > + int point_unit; =20 > + uint8_t buf[PACKET_BUFFER_SIZE] =3D {0}; > + uint8_t i =3D 0, fingers =3D 0; > + uint8_t px =3D 0, py =3D 0, pstatus =3D 0; > + uint8_t p_area =3D 0; > + uint8_t p_preasure =3D 0; > +#ifdef _SUPPORT_BUTTON_TOUCH=09 > + int button_key; > + uint8_t button_buf[10] =3D {0}; > +#endif > + > +#ifdef _ANDROID_4 > + bool all_touch_up =3D true; > +#endif > +=09 > + mutex_lock(&ts->mutex_wq);=20 > + > + /* I2C or SMBUS block data read */ > + ret =3D sis_ReadPacket(ts->client, SIS_CMD_NORMAL, buf); > +#ifdef _DEBUG_PACKAGE_WORKFUNC > + printk(KERN_INFO "chaoban test: Buf_Data [0~63] \n"); > + PrintBuffer(0, 64, buf); =09 > + if ((buf[P_REPORT_ID] !=3D ALL_IN_ONE_PACKAGE) && (ret > 5)) > + { > + printk(KERN_INFO "chaoban test: Buf_Data [64~125] \n"); > + PrintBuffer(64, 128, buf);=09 > + } > +#endif > + > +// add=20 > +#ifdef _SUPPORT_BUTTON_TOUCH > + sis_ReadPacket(ts->client, SIS_CMD_NORMAL, button_buf); > +#endif > + > + if (ret < 0) // Error Number > + { > + printk(KERN_INFO "chaoban test: ret =3D -1\n"); > + goto err_free_allocate; > + } > +#ifdef _SUPPORT_BUTTON_TOUCH > + // access BUTTON TOUCH event and BUTTON NO TOUCH even > + else if (button_buf[P_REPORT_ID] =3D=3D BUTTON_FORMAT) > + { > + //fingers =3D 0; //modify > + button_key =3D ((button_buf[BUTTON_STATE] & 0xff) | ((button_buf[B= UTTON_STATE + 1] & 0xff)<< 8));=09 Again macros for endianness And the driver has a great number of conditional compilations are they really needed? The driver as is has a number of issues and is hard to review due to the use of "//" for comments and a lot of conditional compilation and unnecessary variables used for constants. Could you fix this up and resubmit? Regards Oliver