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

["Jeffrey Lin (林義章)" <Jeffrey.Lin@rad-ic.com>]

[-- Attachment #1: Type: text/plain, Size: 6292 bytes --]

Hi Dmitry:
Answer this mail as below.

From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Thursday, May 05, 2016 6:59 AM
To: jeffrey.lin
Cc: rydberg@euromail.se; grant.likely@linaro.org; robh+dt@kernel.org; jeesw@melfas.com; bleung@chromium.org; Jeffrey Lin (林義章); Roger Yang (楊鎮瑋); KP Li (李昆倍); Albert Shieh (謝欣瑋); linux-kernel@vger.kernel.org; linux-input@vger.kernel.org; devicetree@vger.kernel.org
Subject: Re: [PATCH] driver: input :touchscreen : add Raydium I2C touch driver


Hi Jeffrey,

On Fri, Apr 29, 2016 at 05:45:13PM +0800, jeffrey.lin wrote:
> Raydium I2C touch driver.
>

In my previous e-mail I requested you to enumerate changes that are made to the driver, compared to the previous version(s). There were also a few questions that I did not get answer for.
In order to improve flash read/write security of boot loader. We increase handshake flow and command sequence complexity. Our previous flash write functions are single side action. New version we increase more handshake flow to check data accuracy. Firmware update flow increases as below:
1.      Check firmware file size, if file size is null, return fail.
2.      Check touch MCU run on main firmware or boot loader. Both cases have different return values. In case of flash write function, if touch MCU work on main code, touch driver need issue enter boot loader command to touch MCU and wait for MCU boot mode ready.
3.      Turn off touch MCU watchdog function to prevent interference.
4.      Check boot loader write state path ready.
5.      Send boot mode trigger command to unlock flash-write-protection.
6.      Execute page write command. Each write command will wait touch MCU acknowledge and then execute next step.
7.      Leave boot loader to check firmware work.
8.      Raydium touch driver check touch MCU work on main firmware or not. If touch MCU work on main firmware and return correct acknowledge to kernel, this means flash write command success.
9.      Write checksum information to flash.
Every touch driver flash write command accompany write command, acknowledge byte and wait for acknowledge command. Different sequences have different write commands and different state to return ack.

Previous version as below :
1.      Get firmware data
2.      Force jump to boot loader by soft reset command
3.      Wait for boot loader acknowledge
4.      Flash page write data directly
5.      Force jump to main firmware if page write data finish.

New flow increase more acknowledges for each I2C command to make sure boot loader got the message and finish the actions. After finish flash write data, new flow will le touch MCU jump to main firmware and check main code work well. Then write the total checksum to flash and check the consistence of flash code in every boot time.

New flash write flow is very different form previous version, but more safe. The difference is big. Sorry make you confuse.

Also:

> +static int raydium_i2c_query_ts_info(struct raydium_data *ts) {
> +     struct i2c_client *client = ts->client;
> +     int error, retry_cnt;
> +
> +     for (retry_cnt = 0; retry_cnt < MAX_RETRIES; retry_cnt++) {
> +             error = raydium_i2c_read(client, CMD_DATA_BANK,
> +                     sizeof(ts->obj), (void *)&ts->obj);
> +                     ts->obj.data_bank_addr =
> +                             get_unaligned_le32(&ts->obj.data_bank_addr);
> +
> +             if (!error) {
> +                     error = raydium_i2c_read(client, CMD_QUERY_BANK,
> +                             sizeof(ts->query_bank_info),
> +                             (void *)&ts->query_bank_info);
> +                     if (!error) {
> +                             error = raydium_i2c_read_message(client,
> +                                     ts->query_bank_info, sizeof(ts->info),
> +                                     (void *)&ts->info);
> +
> +                             ts->info.hw_ver =
> +                                     get_unaligned_le32(&ts->info.hw_ver);
> +                             ts->info.ft_ver =
> +                                     get_unaligned_le16(&ts->info.ft_ver);
> +                             ts->info.x_max =
> +                                     get_unaligned_le16(&ts->info.x_max);
> +                             ts->info.y_max =
> +                                     get_unaligned_le16(&ts->info.y_max);
> +                             return 0;
> +                     }
> +             }
> +     }
> +     dev_err(&client->dev, "Get touch data failed: %d\n", error);
> +
> +     return -EINVAL;
> +}
> +
> +static int raydium_i2c_fastboot(struct i2c_client *client) {
> +     static const u8 boot_cmd[] = { 0x50, 0x00, 0x06, 0x20 };
> +     u8 buf[HEADER_SIZE];
> +     int error;
> +
> +     error = raydium_i2c_read_message(client,
> +             get_unaligned_be32(boot_cmd),
> +             sizeof(boot_cmd), buf);

I still can't figure out the logic in read/write mesage handling. Here we see that device is supposedly reporting data_bank_addr as LE integer, but query_bank_info is returned in native CPU endianness. With fastboot command we assume that we are dealing with BE-encoded data and convert it to CPU-endianness before using it.

However in raydium_i2c_read_message() we convert from CPU endianness to BE and this comfuses me (does the device really return data in one endianness but accepts in another endianness?
Raydium MCU support direct access mode by I2C bus to access register or physical buffer directly. In this direct access mode, our touch MCU use BE integer by using commands of raydium_i2c_read_message/ raydium_i2c_send_message. Otherwise, raydum I2C normal mode use LE integer. By the way, the command buffer of soft_rst_cmd[] is upside down.

By the way, why do you have raydium_i2c_read_message() handle reads above MAX_PKG_SIZE? There are no callers that want it.
Generally, I2C bus has maximum packet size but total touch report points maybe exceed this size. Unfortunately my experimental chromebook device exceed this size. So that I’ll split this read command two or more to make sure all data are read.

Thanks.

--
Dmitry


Best Regards
Jeffrey Lin


[-- Attachment #2: Type: text/html, Size: 14693 bytes --]