[PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Jiada Wang]

From: Nick Dyer <nick.dyer@itdev.co.uk>

Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
when they are in a sleep state. It must be retried after a delay for the
chip to wake up.

Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
[gdavis: Forward port and fix conflicts.]
Signed-off-by: George G. Davis <george_davis@mentor.com>
[jiada: return exact errno when i2c_transfer & i2c_master_send fails
	rename "retry" to "retried" and keep its order in length
	set "ret" to correct errno before calling dev_err()
	remove reduntant conditional]
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 38 ++++++++++++++++--------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a2189739e30f..bad3ac58503d 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -196,6 +196,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
@@ -624,6 +625,7 @@ static int __mxt_read_reg(struct i2c_client *client,
 			       u16 reg, u16 len, void *val)
 {
 	struct i2c_msg xfer[2];
+	bool retried = false;
 	u8 buf[2];
 	int ret;
 
@@ -642,22 +644,28 @@ 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 (!retried) {
+			dev_dbg(&client->dev, "i2c retry\n");
+			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)
 {
+	bool retried = false;
 	u8 *buf;
 	size_t count;
 	int ret;
@@ -671,14 +679,20 @@ 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 (!retried) {
+			dev_dbg(&client->dev, "i2c retry\n");
+			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);
-- 
2.17.1

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Andy Shevchenko]

On Sat, Sep 12, 2020 at 3:55 AM Jiada Wang <jiada_wang@mentor.com> wrote:
>
> From: Nick Dyer <nick.dyer@itdev.co.uk>
>
> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> when they are in a sleep state. It must be retried after a delay for the
> chip to wake up.
>
> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk>
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> [jiada: return exact errno when i2c_transfer & i2c_master_send fails
>         rename "retry" to "retried" and keep its order in length
>         set "ret" to correct errno before calling dev_err()

>         remove reduntant conditional]

redundant

> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>


...

> +       bool retried = false;

I thought Dmitry wants that to be retry

>         u8 buf[2];
>         int ret;

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

I'm wondering why you can't leave 2 as is and change it to ARRAY_SIZE
in a separate patch?
Also why switch from positive to negative conditional?

> +               if (!retried) {
> +                       dev_dbg(&client->dev, "i2c retry\n");
> +                       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;
>  }

..

> +       bool retried = false;

Same comments here, in this function.

> +retry_write:
>         ret = i2c_master_send(client, buf, count);
> -       if (ret == count) {
> -               ret = 0;
> -       } else {
> -               if (ret >= 0)
> -                       ret = -EIO;
> +       if (ret != count) {
> +               if (!retried) {
> +                       dev_dbg(&client->dev, "i2c retry\n");
> +                       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;
>         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

13.09.2020 11:43, Andy Shevchenko пишет:
> ...
> 
>> +       bool retried = false;

> I thought Dmitry wants that to be retry

In the comment to v2 you suggested to negate the condition, hence I
thought it's YOU who wants it to be retried.

The "retried" is a very common form among kernel drivers, so it's good
to me.

>>         u8 buf[2];
>>         int ret;
> 
>> -       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)) {
...> Also why switch from positive to negative conditional?

This will make code less readable because of the goto, and thus, there
will be two branches for handling of the returned value instead of one +
goto. Hence this part is good to me as-is.

>> +               if (!retried) {
>> +                       dev_dbg(&client->dev, "i2c retry\n");
>> +                       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;
>>  }

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Andy Shevchenko]

On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 13.09.2020 11:43, Andy Shevchenko пишет:
> > ...
> >
> >> +       bool retried = false;
>
> > I thought Dmitry wants that to be retry
>
> In the comment to v2 you suggested to negate the condition,

Where? I just checked a few messages before and I found that I asked
the same question: why is negative conditional used instead of
positive.

> hence I
> thought it's YOU who wants it to be retried.

I see. Let's see how it goes with positive conditionals first.


> The "retried" is a very common form among kernel drivers, so it's good
> to me.
>
> >>         u8 buf[2];
> >>         int ret;
> >
> >> -       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)) {
> ...> Also why switch from positive to negative conditional?
>
> This will make code less readable because of the goto, and thus, there
> will be two branches for handling of the returned value instead of one +
> goto. Hence this part is good to me as-is.

But it's not the purpose of this patch, right?
Style changes should be really separated from the fix.
And since it's a fix it should have a Fixes tag.

>
> >> +               if (!retried) {
> >> +                       dev_dbg(&client->dev, "i2c retry\n");
> >> +                       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;
> >>  }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

14.09.2020 16:49, Andy Shevchenko пишет:
> On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 13.09.2020 11:43, Andy Shevchenko пишет:
>>> ...
>>>
>>>> +       bool retried = false;
>>
>>> I thought Dmitry wants that to be retry
>>
>> In the comment to v2 you suggested to negate the condition,
> 
> Where? I just checked a few messages before and I found that I asked
> the same question: why is negative conditional used instead of
> positive.

I'm reading this as imperative "make it positive", and thus, assumed
that you asked to do so because the "retry" implies a positive
condition, while "retried" implies the negative.

If you've added "Could you please explain why", then I'd read it as a
question.

>> hence I
>> thought it's YOU who wants it to be retried.
> 
> I see. Let's see how it goes with positive conditionals first.
> 
> 
>> The "retried" is a very common form among kernel drivers, so it's good
>> to me.
>>
>>>>         u8 buf[2];
>>>>         int ret;
>>>
>>>> -       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)) {
>> ...> Also why switch from positive to negative conditional?
>>
>> This will make code less readable because of the goto, and thus, there
>> will be two branches for handling of the returned value instead of one +
>> goto. Hence this part is good to me as-is.
> 
> But it's not the purpose of this patch, right?
> Style changes should be really separated from the fix.

This should be up to a particular maintainer to decide. Usually nobody
requires patches to be overly pedantic, this may turn away contributors
because it feels like an unnecessary bikeshedding. It's more preferred
to accept patch as-is if it does right thing and then maintainer could
modify the patch, making cosmetic changes.

> And since it's a fix it should have a Fixes tag.

It shouldn't be a fix, but a new feature because apparently the 1386
controller wasn't ever intended to be properly supported before this patch.

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Andy Shevchenko]

On Mon, Sep 14, 2020 at 6:26 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 14.09.2020 16:49, Andy Shevchenko пишет:
> > On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <digetx@gmail.com> wrote:

...

> >>>> +       ret = i2c_transfer(client->adapter, xfer, ARRAY_SIZE(xfer));
> >>>> +       if (ret != ARRAY_SIZE(xfer)) {
> >> ...> Also why switch from positive to negative conditional?
> >>
> >> This will make code less readable because of the goto, and thus, there
> >> will be two branches for handling of the returned value instead of one +
> >> goto. Hence this part is good to me as-is.
> >
> > But it's not the purpose of this patch, right?
> > Style changes should be really separated from the fix.
>
> This should be up to a particular maintainer to decide. Usually nobody
> requires patches to be overly pedantic, this may turn away contributors
> because it feels like an unnecessary bikeshedding.

Let's see what Wolfram thinks about this.

> It's more preferred
> to accept patch as-is if it does right thing and then maintainer could
> modify the patch, making cosmetic changes.

It depends on the maintainer's workflow (which may be different from
maintainer to maintainer).

> > And since it's a fix it should have a Fixes tag.
>
> It shouldn't be a fix, but a new feature because apparently the 1386
> controller wasn't ever intended to be properly supported before this patch.

Thanks for clarification. Indeed in this case no tag is needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

14.09.2020 18:50, Andy Shevchenko пишет:
...
>> It's more preferred
>> to accept patch as-is if it does right thing and then maintainer could
>> modify the patch, making cosmetic changes.
> 
> It depends on the maintainer's workflow (which may be different from
> maintainer to maintainer).

Sure!

It's awesome that you're pointing out it all in the reviews because it's
important to have such things explained and definitely should help to
improve quality of further of patches! But it shouldn't be necessary to
demand a very minor changes, IMO.

In particular Jiada was submitting this and lots of other atmel_mxt_ts
patches for about a year now without much progress yet, and you probably
should know how a frustrating experience this could be for a contributor
since you're a longtime kernel developer.

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Torokhov]

Hi Jiada,

On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> when they are in a sleep state. It must be retried after a delay for the
> chip to wake up.

Do we know when the chip is in sleep state? Can we do a quick I2C
transaction in this case instead of adding retry logic to everything? Or
there is another benefit for having such retry logic?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

13.09.2020 19:56, Dmitry Torokhov пишет:
> Hi Jiada,
> 
> On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>
>> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
>> when they are in a sleep state. It must be retried after a delay for the
>> chip to wake up.
> 
> Do we know when the chip is in sleep state? Can we do a quick I2C
> transaction in this case instead of adding retry logic to everything? Or
> there is another benefit for having such retry logic?

Hello!

Please take a look at page 29 of:

https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf

It says that the retry is needed after waking up from a deep-sleep mode.

There are at least two examples when it's needed:

1. Driver probe. Controller could be in a deep-sleep mode at the probe
time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
hardware info.

2. Touchscreen input device is opened. The touchscreen is in a
deep-sleep mode at the time when input device is opened, hence first
__mxt_write_reg() invoked from mxt_start() returns I2C NACK.

I think placing the retries within __mxt_read() / write_reg() should be
the most universal option.

Perhaps it should be possible to add mxt_wake() that will read out some
generic register and then this helper should be invoked after HW
resetting (before mxt_read_info_block()) and from mxt_start() (before
mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.

AFAIK, there shouldn't be any extra benefits from the retrying other
than to handle the deep-sleep of mxt1386.

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Torokhov]

On Mon, Sep 14, 2020 at 08:29:44PM +0300, Dmitry Osipenko wrote:
> 13.09.2020 19:56, Dmitry Torokhov пишет:
> > Hi Jiada,
> > 
> > On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
> >> From: Nick Dyer <nick.dyer@itdev.co.uk>
> >>
> >> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> >> when they are in a sleep state. It must be retried after a delay for the
> >> chip to wake up.
> > 
> > Do we know when the chip is in sleep state? Can we do a quick I2C
> > transaction in this case instead of adding retry logic to everything? Or
> > there is another benefit for having such retry logic?
> 
> Hello!
> 
> Please take a look at page 29 of:
> 
> https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf
> 
> It says that the retry is needed after waking up from a deep-sleep mode.
> 
> There are at least two examples when it's needed:
> 
> 1. Driver probe. Controller could be in a deep-sleep mode at the probe
> time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
> hardware info.
> 
> 2. Touchscreen input device is opened. The touchscreen is in a
> deep-sleep mode at the time when input device is opened, hence first
> __mxt_write_reg() invoked from mxt_start() returns I2C NACK.
> 
> I think placing the retries within __mxt_read() / write_reg() should be
> the most universal option.
> 
> Perhaps it should be possible to add mxt_wake() that will read out some
> generic register

I do not think we need to read a particular register, just doing a quick
read:

	i2c_smbus_xfer(client->adapter, client->addr,
			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy)

should suffice.

> and then this helper should be invoked after HW
> resetting (before mxt_read_info_block()) and from mxt_start() (before
> mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.
>

Actually, reading the spec, it all depends on how the WAKE pin is wired
up on a given board. In certain setups retrying transaction is the right
approach, while in others explicit control is needed. So indeed, we need
a "wake" helper that we should call in probe and resume paths.

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Torokhov]

On Mon, Sep 14, 2020 at 12:33:40PM -0700, Dmitry Torokhov wrote:
> On Mon, Sep 14, 2020 at 08:29:44PM +0300, Dmitry Osipenko wrote:
> > 13.09.2020 19:56, Dmitry Torokhov пишет:
> > > Hi Jiada,
> > > 
> > > On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
> > >> From: Nick Dyer <nick.dyer@itdev.co.uk>
> > >>
> > >> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> > >> when they are in a sleep state. It must be retried after a delay for the
> > >> chip to wake up.
> > > 
> > > Do we know when the chip is in sleep state? Can we do a quick I2C
> > > transaction in this case instead of adding retry logic to everything? Or
> > > there is another benefit for having such retry logic?
> > 
> > Hello!
> > 
> > Please take a look at page 29 of:
> > 
> > https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf
> > 
> > It says that the retry is needed after waking up from a deep-sleep mode.
> > 
> > There are at least two examples when it's needed:
> > 
> > 1. Driver probe. Controller could be in a deep-sleep mode at the probe
> > time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
> > hardware info.
> > 
> > 2. Touchscreen input device is opened. The touchscreen is in a
> > deep-sleep mode at the time when input device is opened, hence first
> > __mxt_write_reg() invoked from mxt_start() returns I2C NACK.
> > 
> > I think placing the retries within __mxt_read() / write_reg() should be
> > the most universal option.
> > 
> > Perhaps it should be possible to add mxt_wake() that will read out some
> > generic register
> 
> I do not think we need to read a particular register, just doing a quick
> read:
> 
> 	i2c_smbus_xfer(client->adapter, client->addr,
> 			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy)
> 
> should suffice.
> 
> > and then this helper should be invoked after HW
> > resetting (before mxt_read_info_block()) and from mxt_start() (before
> > mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.
> >
> 
> Actually, reading the spec, it all depends on how the WAKE pin is wired
> up on a given board. In certain setups retrying transaction is the right
> approach, while in others explicit control is needed. So indeed, we need
> a "wake" helper that we should call in probe and resume paths.

By the way, I would like to avoid the unnecessary retries in probe paths
if possible. I.e. on Chrome OS we really keep an eye on boot times and
in case of multi-sourced touchscreens we may legitimately not have
device at given address.

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

14.09.2020 22:36, Dmitry Torokhov пишет:
> On Mon, Sep 14, 2020 at 12:33:40PM -0700, Dmitry Torokhov wrote:
>> On Mon, Sep 14, 2020 at 08:29:44PM +0300, Dmitry Osipenko wrote:
>>> 13.09.2020 19:56, Dmitry Torokhov пишет:
>>>> Hi Jiada,
>>>>
>>>> On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
>>>>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>>>>
>>>>> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
>>>>> when they are in a sleep state. It must be retried after a delay for the
>>>>> chip to wake up.
>>>>
>>>> Do we know when the chip is in sleep state? Can we do a quick I2C
>>>> transaction in this case instead of adding retry logic to everything? Or
>>>> there is another benefit for having such retry logic?
>>>
>>> Hello!
>>>
>>> Please take a look at page 29 of:
>>>
>>> https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf
>>>
>>> It says that the retry is needed after waking up from a deep-sleep mode.
>>>
>>> There are at least two examples when it's needed:
>>>
>>> 1. Driver probe. Controller could be in a deep-sleep mode at the probe
>>> time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
>>> hardware info.
>>>
>>> 2. Touchscreen input device is opened. The touchscreen is in a
>>> deep-sleep mode at the time when input device is opened, hence first
>>> __mxt_write_reg() invoked from mxt_start() returns I2C NACK.
>>>
>>> I think placing the retries within __mxt_read() / write_reg() should be
>>> the most universal option.
>>>
>>> Perhaps it should be possible to add mxt_wake() that will read out some
>>> generic register
>>
>> I do not think we need to read a particular register, just doing a quick
>> read:
>>
>> 	i2c_smbus_xfer(client->adapter, client->addr,
>> 			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy)
>>
>> should suffice.
>>
>>> and then this helper should be invoked after HW
>>> resetting (before mxt_read_info_block()) and from mxt_start() (before
>>> mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.
>>>
>>
>> Actually, reading the spec, it all depends on how the WAKE pin is wired
>> up on a given board. In certain setups retrying transaction is the right
>> approach, while in others explicit control is needed. So indeed, we need
>> a "wake" helper that we should call in probe and resume paths.

The WAKE-GPIO was never supported and I'm not sure whether anyone
actually needs it. I think we could ignore this case until anyone would
really need and could test it.

> By the way, I would like to avoid the unnecessary retries in probe paths
> if possible. I.e. on Chrome OS we really keep an eye on boot times and
> in case of multi-sourced touchscreens we may legitimately not have
> device at given address.

We could add a new MXT1386 DT compatible and then do:

static void mxt_wake(struct mxt_data *data)
{
	struct i2c_client *client = data->client;
	struct device *dev = &data->client->dev;
	union i2c_smbus_data dummy;

	if (!of_device_is_compatible(dev, "atmel,mXT1386"))
		return;

	/* TODO: add WAKE-GPIO support */

	i2c_smbus_xfer(client->adapter, client->addr,
			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
			&dummy);

	msleep(MXT_WAKEUP_TIME);
}

Jiada, will you be able to re-work this patch? Please note that the new
"atmel,mXT1386" DT compatible needs to be added into the
atmel,maxtouch.txt binding.

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Torokhov]

On Tue, Sep 15, 2020 at 12:33:18AM +0300, Dmitry Osipenko wrote:
> 14.09.2020 22:36, Dmitry Torokhov пишет:
> > On Mon, Sep 14, 2020 at 12:33:40PM -0700, Dmitry Torokhov wrote:
> >> On Mon, Sep 14, 2020 at 08:29:44PM +0300, Dmitry Osipenko wrote:
> >>> 13.09.2020 19:56, Dmitry Torokhov пишет:
> >>>> Hi Jiada,
> >>>>
> >>>> On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
> >>>>> From: Nick Dyer <nick.dyer@itdev.co.uk>
> >>>>>
> >>>>> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> >>>>> when they are in a sleep state. It must be retried after a delay for the
> >>>>> chip to wake up.
> >>>>
> >>>> Do we know when the chip is in sleep state? Can we do a quick I2C
> >>>> transaction in this case instead of adding retry logic to everything? Or
> >>>> there is another benefit for having such retry logic?
> >>>
> >>> Hello!
> >>>
> >>> Please take a look at page 29 of:
> >>>
> >>> https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf
> >>>
> >>> It says that the retry is needed after waking up from a deep-sleep mode.
> >>>
> >>> There are at least two examples when it's needed:
> >>>
> >>> 1. Driver probe. Controller could be in a deep-sleep mode at the probe
> >>> time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
> >>> hardware info.
> >>>
> >>> 2. Touchscreen input device is opened. The touchscreen is in a
> >>> deep-sleep mode at the time when input device is opened, hence first
> >>> __mxt_write_reg() invoked from mxt_start() returns I2C NACK.
> >>>
> >>> I think placing the retries within __mxt_read() / write_reg() should be
> >>> the most universal option.
> >>>
> >>> Perhaps it should be possible to add mxt_wake() that will read out some
> >>> generic register
> >>
> >> I do not think we need to read a particular register, just doing a quick
> >> read:
> >>
> >> 	i2c_smbus_xfer(client->adapter, client->addr,
> >> 			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy)
> >>
> >> should suffice.
> >>
> >>> and then this helper should be invoked after HW
> >>> resetting (before mxt_read_info_block()) and from mxt_start() (before
> >>> mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.
> >>>
> >>
> >> Actually, reading the spec, it all depends on how the WAKE pin is wired
> >> up on a given board. In certain setups retrying transaction is the right
> >> approach, while in others explicit control is needed. So indeed, we need
> >> a "wake" helper that we should call in probe and resume paths.
> 
> The WAKE-GPIO was never supported and I'm not sure whether anyone
> actually needs it. I think we could ignore this case until anyone would
> really need and could test it.

Right, I am not advocating adding GPIO control right away, I was simply
arguing why I believe we should separate wakeup handling from normal
communication handling.

> 
> > By the way, I would like to avoid the unnecessary retries in probe paths
> > if possible. I.e. on Chrome OS we really keep an eye on boot times and
> > in case of multi-sourced touchscreens we may legitimately not have
> > device at given address.
> 
> We could add a new MXT1386 DT compatible and then do:
> 
> static void mxt_wake(struct mxt_data *data)
> {
> 	struct i2c_client *client = data->client;
> 	struct device *dev = &data->client->dev;
> 	union i2c_smbus_data dummy;
> 
> 	if (!of_device_is_compatible(dev, "atmel,mXT1386"))
> 		return;
> 
> 	/* TODO: add WAKE-GPIO support */
> 
> 	i2c_smbus_xfer(client->adapter, client->addr,
> 			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
> 			&dummy);
> 
> 	msleep(MXT_WAKEUP_TIME);
> }
> 
> Jiada, will you be able to re-work this patch? Please note that the new
> "atmel,mXT1386" DT compatible needs to be added into the
> atmel,maxtouch.txt binding.

Another option is to have a device property "atmel,wakeup-type" or
something, in case there are more Atmel variants needing this.

Rob, any preferences here?

Thanks.

-- 
Dmitry

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Rob Herring]

On Mon, Sep 14, 2020 at 4:32 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Sep 15, 2020 at 12:33:18AM +0300, Dmitry Osipenko wrote:
> > 14.09.2020 22:36, Dmitry Torokhov пишет:
> > > On Mon, Sep 14, 2020 at 12:33:40PM -0700, Dmitry Torokhov wrote:
> > >> On Mon, Sep 14, 2020 at 08:29:44PM +0300, Dmitry Osipenko wrote:
> > >>> 13.09.2020 19:56, Dmitry Torokhov пишет:
> > >>>> Hi Jiada,
> > >>>>
> > >>>> On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
> > >>>>> From: Nick Dyer <nick.dyer@itdev.co.uk>
> > >>>>>
> > >>>>> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
> > >>>>> when they are in a sleep state. It must be retried after a delay for the
> > >>>>> chip to wake up.
> > >>>>
> > >>>> Do we know when the chip is in sleep state? Can we do a quick I2C
> > >>>> transaction in this case instead of adding retry logic to everything? Or
> > >>>> there is another benefit for having such retry logic?
> > >>>
> > >>> Hello!
> > >>>
> > >>> Please take a look at page 29 of:
> > >>>
> > >>> https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf
> > >>>
> > >>> It says that the retry is needed after waking up from a deep-sleep mode.
> > >>>
> > >>> There are at least two examples when it's needed:
> > >>>
> > >>> 1. Driver probe. Controller could be in a deep-sleep mode at the probe
> > >>> time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
> > >>> hardware info.
> > >>>
> > >>> 2. Touchscreen input device is opened. The touchscreen is in a
> > >>> deep-sleep mode at the time when input device is opened, hence first
> > >>> __mxt_write_reg() invoked from mxt_start() returns I2C NACK.
> > >>>
> > >>> I think placing the retries within __mxt_read() / write_reg() should be
> > >>> the most universal option.
> > >>>
> > >>> Perhaps it should be possible to add mxt_wake() that will read out some
> > >>> generic register
> > >>
> > >> I do not think we need to read a particular register, just doing a quick
> > >> read:
> > >>
> > >>    i2c_smbus_xfer(client->adapter, client->addr,
> > >>                    0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy)
> > >>
> > >> should suffice.
> > >>
> > >>> and then this helper should be invoked after HW
> > >>> resetting (before mxt_read_info_block()) and from mxt_start() (before
> > >>> mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.
> > >>>
> > >>
> > >> Actually, reading the spec, it all depends on how the WAKE pin is wired
> > >> up on a given board. In certain setups retrying transaction is the right
> > >> approach, while in others explicit control is needed. So indeed, we need
> > >> a "wake" helper that we should call in probe and resume paths.
> >
> > The WAKE-GPIO was never supported and I'm not sure whether anyone
> > actually needs it. I think we could ignore this case until anyone would
> > really need and could test it.
>
> Right, I am not advocating adding GPIO control right away, I was simply
> arguing why I believe we should separate wakeup handling from normal
> communication handling.
>
> >
> > > By the way, I would like to avoid the unnecessary retries in probe paths
> > > if possible. I.e. on Chrome OS we really keep an eye on boot times and
> > > in case of multi-sourced touchscreens we may legitimately not have
> > > device at given address.
> >
> > We could add a new MXT1386 DT compatible and then do:
> >
> > static void mxt_wake(struct mxt_data *data)
> > {
> >       struct i2c_client *client = data->client;
> >       struct device *dev = &data->client->dev;
> >       union i2c_smbus_data dummy;
> >
> >       if (!of_device_is_compatible(dev, "atmel,mXT1386"))
> >               return;
> >
> >       /* TODO: add WAKE-GPIO support */
> >
> >       i2c_smbus_xfer(client->adapter, client->addr,
> >                       0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
> >                       &dummy);
> >
> >       msleep(MXT_WAKEUP_TIME);
> > }
> >
> > Jiada, will you be able to re-work this patch? Please note that the new
> > "atmel,mXT1386" DT compatible needs to be added into the
> > atmel,maxtouch.txt binding.
>
> Another option is to have a device property "atmel,wakeup-type" or
> something, in case there are more Atmel variants needing this.
>
> Rob, any preferences here?

If device specific, then I prefer to be implied by the compatible. If
board specific, then a property is appropriate. Seems like this is the
former case.

Rob

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Wang, Jiada]

Hi Dmitry

On 2020/09/15 6:33, Dmitry Osipenko wrote:
> 14.09.2020 22:36, Dmitry Torokhov пишет:
>> On Mon, Sep 14, 2020 at 12:33:40PM -0700, Dmitry Torokhov wrote:
>>> On Mon, Sep 14, 2020 at 08:29:44PM +0300, Dmitry Osipenko wrote:
>>>> 13.09.2020 19:56, Dmitry Torokhov пишет:
>>>>> Hi Jiada,
>>>>>
>>>>> On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
>>>>>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>>>>>
>>>>>> Some maXTouch chips (eg mXT1386) will not respond on the first I2C request
>>>>>> when they are in a sleep state. It must be retried after a delay for the
>>>>>> chip to wake up.
>>>>>
>>>>> Do we know when the chip is in sleep state? Can we do a quick I2C
>>>>> transaction in this case instead of adding retry logic to everything? Or
>>>>> there is another benefit for having such retry logic?
>>>>
>>>> Hello!
>>>>
>>>> Please take a look at page 29 of:
>>>>
>>>> https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf
>>>>
>>>> It says that the retry is needed after waking up from a deep-sleep mode.
>>>>
>>>> There are at least two examples when it's needed:
>>>>
>>>> 1. Driver probe. Controller could be in a deep-sleep mode at the probe
>>>> time, and then first __mxt_read_reg() returns I2C NACK on reading out TS
>>>> hardware info.
>>>>
>>>> 2. Touchscreen input device is opened. The touchscreen is in a
>>>> deep-sleep mode at the time when input device is opened, hence first
>>>> __mxt_write_reg() invoked from mxt_start() returns I2C NACK.
>>>>
>>>> I think placing the retries within __mxt_read() / write_reg() should be
>>>> the most universal option.
>>>>
>>>> Perhaps it should be possible to add mxt_wake() that will read out some
>>>> generic register
>>>
>>> I do not think we need to read a particular register, just doing a quick
>>> read:
>>>
>>> 	i2c_smbus_xfer(client->adapter, client->addr,
>>> 			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &dummy)
>>>
>>> should suffice.
>>>
>>>> and then this helper should be invoked after HW
>>>> resetting (before mxt_read_info_block()) and from mxt_start() (before
>>>> mxt_set_t7_power_cfg()). But this approach feels a bit fragile to me.
>>>>
>>>
>>> Actually, reading the spec, it all depends on how the WAKE pin is wired
>>> up on a given board. In certain setups retrying transaction is the right
>>> approach, while in others explicit control is needed. So indeed, we need
>>> a "wake" helper that we should call in probe and resume paths.
> 
> The WAKE-GPIO was never supported and I'm not sure whether anyone
> actually needs it. I think we could ignore this case until anyone would
> really need and could test it.
> 
>> By the way, I would like to avoid the unnecessary retries in probe paths
>> if possible. I.e. on Chrome OS we really keep an eye on boot times and
>> in case of multi-sourced touchscreens we may legitimately not have
>> device at given address.
> 
> We could add a new MXT1386 DT compatible and then do:
> 
> static void mxt_wake(struct mxt_data *data)
> {
> 	struct i2c_client *client = data->client;
> 	struct device *dev = &data->client->dev;
> 	union i2c_smbus_data dummy;
> 
> 	if (!of_device_is_compatible(dev, "atmel,mXT1386"))
> 		return;
> 
> 	/* TODO: add WAKE-GPIO support */
> 
> 	i2c_smbus_xfer(client->adapter, client->addr,
> 			0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
> 			&dummy);
> 
> 	msleep(MXT_WAKEUP_TIME);
> }
> 
> Jiada, will you be able to re-work this patch? Please note that the new
> "atmel,mXT1386" DT compatible needs to be added into the
> atmel,maxtouch.txt binding.

Yes, I can re-work this patch, and add one more change to dts-binding.

to summarize long discussion in this thread,
I think what I need to do are:
1) since the change will be different from current one, I will need to 
start a new patch
2) call mxt_wake() in mxt_probe() and mxt_resume()
3) update atmel,maxtouch.txt binding

please correct me if I am wrong.

Thanks,
Jiada
> 

Re: [PATCH v4 1/1] Input: atmel_mxt_ts - implement I2C retries

[Dmitry Osipenko]

15.09.2020 18:55, Wang, Jiada пишет:
...
>> Jiada, will you be able to re-work this patch? Please note that the new
>> "atmel,mXT1386" DT compatible needs to be added into the
>> atmel,maxtouch.txt binding.
> 
> Yes, I can re-work this patch, and add one more change to dts-binding.
> 
> to summarize long discussion in this thread,
> I think what I need to do are:
> 1) since the change will be different from current one, I will need to
> start a new patch
> 2) call mxt_wake() in mxt_probe() and mxt_resume()

in mxt_initialize(), mxt_load_fw() and mxt_start()

> 3) update atmel,maxtouch.txt binding
> 
> please correct me if I am wrong.

sounds good