21.09.2020 17:00, Jiada Wang пишет:
> According to datasheet, mXT1386 chip has a WAKE line, it is used
> to wake the chip up from deep sleep mode before communicating with
> it via the I2C-compatible interface.
>
> if the WAKE line is connected to a GPIO line, the line must be
> asserted 25 ms before the host attempts to communicate with the mXT1386.
> If the WAKE line is connected to the SCL pin, the mXT1386 will send a
> NACK on the first attempt to address it, the host must then retry 25 ms
> later.
>
> This patch adds compatible string "atmel,mXT1386" for mXT1386 controller,
> when I2C transfer on mXT1386 fails, retry the transfer once after a 25 ms
> sleep.
>
> Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 44 +++++++++++++++++-------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 98f17fa3a892..96d5f4d64cb0 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -198,6 +198,7 @@ enum t100_type {
> #define MXT_CRC_TIMEOUT 1000 /* msec */
> #define MXT_FW_RESET_TIME 3000 /* msec */
> #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> +#define MXT_WAKEUP_TIME 25 /* msec */
>
> /* Command to unlock bootloader */
> #define MXT_UNLOCK_CMD_MSB 0xaa
> @@ -627,7 +628,9 @@ static int mxt_send_bootloader_cmd(struct mxt_data *data, bool unlock)
> static int __mxt_read_reg(struct i2c_client *client,
> u16 reg, u16 len, void *val)
> {
> + struct device_node *np = client->dev.of_node;
> struct i2c_msg xfer[2];
> + bool retried = false;
> u8 buf[2];
> int ret;
>
> @@ -646,22 +649,30 @@ static int __mxt_read_reg(struct i2c_client *client,
> xfer[1].len = len;
> xfer[1].buf = val;
>
> - ret = i2c_transfer(client->adapter, xfer, 2);
> - if (ret == 2) {
> - ret = 0;
> - } else {
> - if (ret >= 0)
> - ret = -EIO;
> +retry_read:
> + ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> + if (ret != ARRAY_SIZE(xfer)) {
> + if (of_device_is_compatible(np, "atmel,mXT1386") && !retried) {
Hello, Jiada!
This looks almost good to me! But I think we should add "bool
retry_i2c_transfers" to the struct mxt_data and then set it to true for
atmel,mXT1386 because of_device_is_compatible() looks a bit too bulky
and this is usually discouraged to have in the code.
Secondly, we should also add a clarifying comment to the code, telling
why retries are needed for 1386, something like this:
static int mxt_probe(struct i2c_client *client, const struct
i2c_device_id *id)
{
...
/*
* The mXT1386 has a dedicated WAKE-line that could be connected to a
* dedicated GPIO, or to the I2C SCL pin, or permanently asserted LOW.
* It's used for waking controller from a deep-sleep and it needs to be
* asserted LOW for 25 milliseconds before issuing I2C transfer if
controller
* was in a deep-sleep mode. If WAKE-line is connected to I2C SCL pin, then
* the first I2C transfer will get an instant NAK and transfer needs to be
* retried after 25ms. There are too many places in the code where the
wake-up
* needs to be inserted, hence it's much easier to add a retry to the common
* I2C accessors, also given that the WAKE-GPIO is unsupported by the
driver.
*/
if (of_device_is_compatible(np, "atmel,mXT1386")
data->retry_i2c_transfers = true;
