Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver

[jeffrey.lin]

Hi Dmitry:
This patch you submitted had some problems. I still debug in progress. Should I comment the issues in this patch or create a new patch if I finish the issues?

 >static int raydium_i2c_fastboot(struct i2c_client *client)
 > {
 >-	static const u8 boot_cmd[] = { 0x50, 0x00, 0x06, 0x20 };
 >-	u8 buf[HEADER_SIZE];
 >+	u8 buf[4]; // XXX FIXME why size 4 and not 1?
Correct.Raydium direct access mode is word alignment, so that 4 bytes reading is correct but only lower bytes show the message I need.

 >static int raydium_i2c_check_fw_status(struct raydium_data *ts)
 >{
 >	struct i2c_client *client = ts->client;
 >-	static const u8 bl_area[] = {0x62, 0x6f, 0x6f, 0x74};
 >-	static const u8 main_area[] = {0x66, 0x69, 0x72, 0x6d};
 >-	u8 buf[HEADER_SIZE];
 >+	static const u8 bl_area[] = { 0x62, 0x6f, 0x6f, 0x74 };
 >+	static const u8 main_area[] = { 0x66, 0x69, 0x72, 0x6d };
 >+	u8 buf[4];
 > 	int error;
 > 
 >-	error = raydium_i2c_read(client, CMD_BOOT_READ, HEADER_SIZE,
 >-		(void *)buf);
 >+	error = raydium_i2c_read(client, RM_CMD_BOOT_READ, buf, sizeof(buf));
 > 	if (!error) {
 >+		// XXX FIXME: why do we compare only 1st bytes? Do we even
 >+		// need 4-byte constants?
One bytes comparison is fine. I'll change as below:

static int raydium_i2c_check_fw_status(struct raydium_data *ts)
{
	struct i2c_client *client = ts->client;
	static const u8 bl_ack = 0x62;
	static const u8 main_ack = 0x66;
	u8 buf[4];
	int error;

	error = raydium_i2c_read(client, RM_CMD_BOOT_READ, buf, sizeof(buf));
	if (!error) {
		// XXX FIXME: why do we compare only 1st bytes? Do we even
		// need 4-byte constants?
		if (buf[0] == bl_ack)
			ts->boot_mode = RAYDIUM_TS_BLDR;
		else if (buf[0] == main_ack)
			ts->boot_mode = RAYDIUM_TS_MAIN;
		return 0;

>+	while (len) {
>+		xfer_len = min_t(size_t, len, RM_BL_WRT_PKG_SIZE);
> 
>-		memcpy(&buf[DATA_STR], page + DATA_STR +
>-			u8_idx*RAYDIUM_TRANS_BUFSIZE,
>-			RAYDIUM_TRANS_BUFSIZE);
>+		buf[BL_HEADER] = 0x0b;
>+		// XXX FIXME: is this correct? Do we really want all pages
>+		// after 1st to have 0xff? Should it be a counter?
>+		// Why do we need both pages and packages within pages?
>+		buf[BL_PAGE_STR] = page_idx ? 0 : 0xff;
This is correct. Touch MCU need this index as start page.
>+		buf[BL_PKG_IDX] = pkg_idx;
This should be:
		buf[BL_PKG_IDX] = pkg_idx++;
>+		memcpy(&buf[BL_DATA_STR], data, xfer_len);