Re: [PATCH 1/2] INPUT/HID: add touch support for SiS touch driver

On Mon, 2015-01-12 at 18:53 +0800, 曾婷葳 (tammy_tseng) wrote:

Why that default? The majority of systems will not feature this
touchscreens.

+	help
+	  This enables support for SiS 9200 family over I2C based touchscreens.
+
+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.

Conditional includes should be avoided if possible.

+#include <linux/hrtimer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include "sis_i2c.h"
+#include <linux/linkage.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+#include <asm/uaccess.h>
+#include <linux/irq.h>
+
+#ifdef _STD_RW_IO

???

+#include <linux/init.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+#define DEVICE_NAME "sis_aegis_touch_device"
+static int sis_char_devs_count = 1;        /* device count */

Why start with 1 ?

+static int sis_char_major = 0;
+static struct cdev sis_char_cdev;
+static struct class *sis_char_class = NULL;
+#endif
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { SIS_SLAVE_ADDR, I2C_CLIENT_END };
+static struct workqueue_struct *sis_wq;
+struct sis_ts_data *ts_bak = 0;
+struct sisTP_driver_data *TPInfo = NULL;

Are you really sure you are limited to a single device?

+static void sis_tpinfo_clear(struct sisTP_driver_data *TPInfo, int max);
+
+#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 = start; i < length; i++ )
+	{
+		printk("%02x ", buf[i]);
+		if (i != 0 && i % 30 == 0)
+			printk("\n");
+	}
+	printk("\n");	
+}
+
+int sis_command_for_write(struct i2c_client *client, int wlength, unsigned char *wdata)
+{
+    int ret = -1;
+    struct i2c_msg msg[1];

Why on earth an array of a single element?

+
+    msg[0].addr = client->addr;
+    msg[0].flags = 0; //Write
+    msg[0].len = wlength;
+    msg[0].buf = (unsigned char *)wdata;
+
+    ret = i2c_transfer(client->adapter, msg, 1);
+
+    return ret;
+}
+
+int sis_command_for_read(struct i2c_client *client, int rlength, unsigned char *rdata)
+{
+    int ret = -1;
+    struct i2c_msg msg[1];

Likewise

+
+    msg[0].addr = client->addr;
+    msg[0].flags = I2C_M_RD; //Read
+    msg[0].len = rlength;
+    msg[0].buf = rdata;
+
+    ret = i2c_transfer(client->adapter, msg, 1);
+
+    return ret;
+}
+
+int sis_cul_unit(uint8_t report_id)
+{
+	int basic = 6;
+	int area = 2;
+	int pressure = 1;
+	int ret = basic;

Why ???

+	
+	if (report_id != ALL_IN_ONE_PACKAGE)
+	{
+		if (IS_AREA(report_id) /*&& IS_TOUCH(report_id)*/)
+		{
+			ret += area;
+		}
+		if (IS_PRESSURE(report_id))
+		{
+			ret += pressure;
+		}
+	}
+	
+	return ret;	
+}
+
+int sis_ReadPacket(struct i2c_client *client, uint8_t cmd, uint8_t* buf)
+{
+	uint8_t tmpbuf[MAX_BYTE] = {0};	//MAX_BYTE = 64;
+#ifdef _CHECK_CRC
+	uint16_t buf_crc = 0;
+	uint16_t package_crc = 0;
+	int l_package_crc = 0;
+	int crc_end = 0;
+#endif
+    int ret = -1;
+    int touchnum = 0;
+    int p_count = 0;
+    int touc_formate_id = 0;
+    int locate = 0;
+    bool read_first = true;
+    
+/*
+		New i2c format 
+	* buf[0] = Low 8 bits of byte count value
+	* buf[1] = High 8 bits of byte counte value
+	* buf[2] = Report ID
+	* buf[touch num * 6 + 2 ] = Touch informations; 1 touch point has 6 bytes, it could be none if no touch 
+	* buf[touch num * 6 + 3] = Touch numbers
+	* 
+	* One touch point information include 6 bytes, the order is 
+	* 
+	* 1. status = touch down or touch up
+	* 2. id = finger id 
+	* 3. x axis low 8 bits
+	* 4. x axis high 8 bits
+	* 5. y axis low 8 bits
+	* 6. y axis high 8 bits
+	* 
+*/
+	do
+	{
+		if (locate >= PACKET_BUFFER_SIZE)
+		{
+			printk(KERN_ERR "sis_ReadPacket: Buf Overflow\n");
+			return -1;
+		}
+		
+		ret = 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);	
+#endif			
+
+		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;	
+		}
+		
+		if (read_first)
+		{
+#ifdef _SUPPORT_BUTTON_TOUCH
+			// access BUTTON TOUCH event and BUTTON NO TOUCH event
+			if (tmpbuf[P_REPORT_ID] ==  BUTTON_FORMAT)
+			{
+				memcpy(&buf[0], &tmpbuf[0], 7);
+				return touchnum; 	//touchnum is 0

So why not return 0 ?

+			}
+#endif 
+			// access NO TOUCH event unless BUTTON NO TOUCH event
+			if (tmpbuf[P_BYTECOUNT] == 0/*NO_TOUCH_BYTECOUNT*/)
+			{
+				return touchnum;	//touchnum is 0
+			}
+		}
+
+		//skip parsing data when two devices are registered at the same slave address
+		//parsing data when P_REPORT_ID && 0xf is TOUCH_FORMAT or P_REPORT_ID is ALL_IN_ONE_PACKAGE
+		touc_formate_id = tmpbuf[P_REPORT_ID] & 0xf;
+		if ((touc_formate_id != TOUCH_FORMAT) && (touc_formate_id != HIDI2C_FORMAT) && (tmpbuf[P_REPORT_ID] != ALL_IN_ONE_PACKAGE))
+		{
+			printk(KERN_ERR "sis_ReadPacket: Error Report_ID\n");
+			return -1;		
+		}
+		
+		p_count = (int) tmpbuf[P_BYTECOUNT] - 1;	//start from 0
+		if (tmpbuf[P_REPORT_ID] != ALL_IN_ONE_PACKAGE)
+		{
+			if (IS_TOUCH(tmpbuf[P_REPORT_ID]))
+			{
+				p_count -= BYTE_CRC_I2C;	//delete 2 byte crc
+			}
+			else if (IS_HIDI2C(tmpbuf[P_REPORT_ID]))
+			{
+				p_count -= 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 -= BYTE_SCANTIME;
+			}
+		}
+		//else {}    						// For ALL_IN_ONE_PACKAGE
+		
+		if (read_first)
+		{
+			touchnum = tmpbuf[p_count]; 	
+		}
+		else
+		{
+			if (tmpbuf[p_count] != 0)
+			{
+				printk(KERN_ERR "sis_ReadPacket: get error package\n");
+				return -1;
+			}
+		}
+
+#ifdef _CHECK_CRC
+		crc_end = p_count + (IS_SCANTIME(tmpbuf[P_REPORT_ID]) * 2);
+		buf_crc = cal_crc(tmpbuf, 2, crc_end); //sub bytecount (2 byte)
+		l_package_crc = p_count + 1 + (IS_SCANTIME(tmpbuf[P_REPORT_ID]) * 2);
+		package_crc = ((tmpbuf[l_package_crc] & 0xff) | ((tmpbuf[l_package_crc + 1] & 0xff) << 8));

We have macros to read data in a defined endianness

+			
+		if (buf_crc != package_crc)
+		{
+			printk(KERN_ERR "sis_ReadPacket: CRC Error\n");
+			return -1;
+		}
+#endif	
+		memcpy(&buf[locate], &tmpbuf[0], 64);	//Buf_Data [0~63] [64~128]
+		locate += 64;
+		read_first = false;
+		
+	}while(tmpbuf[P_REPORT_ID] != ALL_IN_ONE_PACKAGE  &&  tmpbuf[p_count] > 5);
+
+	return touchnum;
+}	
+
+
+int check_gpio_interrupt(void)
+{
+    int ret = 0;
+    //TODO
+    //CHECK GPIO INTERRUPT STATUS BY YOUR PLATFORM SETTING.
+    ret = gpio_get_value(GPIO_IRQ);
+    return ret;
+}
+
+void ts_report_key(struct i2c_client *client, uint8_t keybit_state)
+{
+	int i = 0;
+	uint8_t diff_keybit_state= 0x0; //check keybit_state is difference with pre_keybit_state
+	uint8_t key_value = 0x0; //button location for binary
+	uint8_t  key_pressed = 0x0; //button is up or down
+	struct sis_ts_data *ts = i2c_get_clientdata(client);
+
+	if (!ts)
+	{
+		printk(KERN_ERR "%s error: Missing Platform Data!\n", __func__);
+		return;
+	}
+
+	diff_keybit_state = TPInfo->pre_keybit_state ^ keybit_state;
+
+	if (diff_keybit_state)
+	{
+		for (i = 0; i < BUTTON_KEY_COUNT; i++)
+		{
+		    if ((diff_keybit_state >> i) & 0x01)
+			{
+				key_value = diff_keybit_state & (0x01 << i);
+				key_pressed = (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 = keybit_state;
+	}
+}
+
+
+static void sis_ts_work_func(struct work_struct *work)
+{
+	struct sis_ts_data *ts = container_of(work, struct sis_ts_data, work);
+    int ret = -1;
+    int point_unit;  
+	uint8_t buf[PACKET_BUFFER_SIZE] = {0};
+	uint8_t i = 0, fingers = 0;
+	uint8_t px = 0, py = 0, pstatus = 0;
+	uint8_t p_area = 0;
+    uint8_t p_preasure = 0;
+#ifdef _SUPPORT_BUTTON_TOUCH	
+	int button_key;
+	uint8_t button_buf[10] = {0};
+#endif
+
+#ifdef _ANDROID_4
+	bool all_touch_up = true;
+#endif
+	
+	mutex_lock(&ts->mutex_wq); 
+
+    /* I2C or SMBUS block data read */
+    ret = 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);			
+	if ((buf[P_REPORT_ID] != ALL_IN_ONE_PACKAGE) && (ret > 5))
+	{
+		printk(KERN_INFO "chaoban test: Buf_Data [64~125] \n");
+		PrintBuffer(64, 128, buf);	
+	}
+#endif
+
+// add 
+#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 = -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] == BUTTON_FORMAT)
+	{
+		//fingers = 0; //modify
+		button_key = ((button_buf[BUTTON_STATE] & 0xff) | ((button_buf[BUTTON_STATE + 1] & 0xff)<< 8));	

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