Re: [PATCH v1 2/3] Input: atmel_mxt_ts - implement I2C retries for mXT1368

[Dmitry Osipenko]

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;