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

From: jeffrey.lin
Date: Fri May 13 2016 - 13:08:50 EST


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);