| Message ID | 20200921140054.2389-2-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] dt-bindings: input: atmel: add compatible for mXT1386 | expand |
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@mentor.com> > --- > 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;
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) { + dev_dbg(&client->dev, "i2c retry\n"); + /* TODO: add WAKE-GPIO support */ + msleep(MXT_WAKEUP_TIME); + retried = true; + goto retry_read; + } + ret = ret < 0 ? ret : -EIO; dev_err(&client->dev, "%s: i2c transfer failed (%d)\n", __func__, ret); + return ret; } - return ret; + return 0; } static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len, const void *val) { + struct device_node *np = client->dev.of_node; + bool retried = false; u8 *buf; size_t count; int ret; @@ -675,14 +686,21 @@ static int __mxt_write_reg(struct i2c_client *client, u16 reg, u16 len, buf[1] = (reg >> 8) & 0xff; memcpy(&buf[2], val, len); +retry_write: ret = i2c_master_send(client, buf, count); - if (ret == count) { - ret = 0; - } else { - if (ret >= 0) - ret = -EIO; + if (ret != count) { + if (of_device_is_compatible(np, "atmel,mXT1386") && !retried) { + dev_dbg(&client->dev, "i2c retry\n"); + /* TODO: add WAKE-GPIO support */ + msleep(MXT_WAKEUP_TIME); + retried = true; + goto retry_write; + } + ret = ret < 0 ? ret : -EIO; dev_err(&client->dev, "%s: i2c send failed (%d)\n", __func__, ret); + } else { + ret = 0; } kfree(buf); @@ -3235,6 +3253,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume); static const struct of_device_id mxt_of_match[] = { { .compatible = "atmel,maxtouch", }, + { .compatible = "atmel,mXT1386", }, /* Compatibles listed below are deprecated */ { .compatible = "atmel,qt602240_ts", }, { .compatible = "atmel,atmel_mxt_ts", }, @@ -3259,6 +3278,7 @@ static const struct i2c_device_id mxt_id[] = { { "atmel_mxt_tp", 0 }, { "maxtouch", 0 }, { "mXT224", 0 }, + { "mXT1386", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, mxt_id);
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@mentor.com> --- drivers/input/touchscreen/atmel_mxt_ts.c | 44 +++++++++++++++++------- 1 file changed, 32 insertions(+), 12 deletions(-)