From: Nick Dyer <[email protected]>
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 <[email protected]>
[gdavis: Forward port and fix conflicts.]
Signed-off-by: George G. Davis <[email protected]>
[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 <[email protected]>
Reviewed-by: Dmitry Osipenko <[email protected]>
Tested-by: Dmitry Osipenko <[email protected]>
---
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
On Sat, Sep 12, 2020 at 3:55 AM Jiada Wang <[email protected]> wrote:
>
> From: Nick Dyer <[email protected]>
>
> 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 <[email protected]>
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <[email protected]>
> [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 <[email protected]>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Tested-by: Dmitry Osipenko <[email protected]>
...
> + 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
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;
>> }
Hi Jiada,
On Sat, Sep 12, 2020 at 09:55:21AM +0900, Jiada Wang wrote:
> From: Nick Dyer <[email protected]>
>
> 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
On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <[email protected]> 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
14.09.2020 16:49, Andy Shevchenko пишет:
> On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <[email protected]> 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.
On Mon, Sep 14, 2020 at 6:26 PM Dmitry Osipenko <[email protected]> wrote:
> 14.09.2020 16:49, Andy Shevchenko пишет:
> > On Sun, Sep 13, 2020 at 3:57 PM Dmitry Osipenko <[email protected]> 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
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.
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 <[email protected]>
>>
>> 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.
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 <[email protected]>
> >>
> >> 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
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 <[email protected]>
> > >>
> > >> 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
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 <[email protected]>
>>>>>
>>>>> 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.
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 <[email protected]>
> >>>>>
> >>>>> 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
On Mon, Sep 14, 2020 at 4:32 PM Dmitry Torokhov
<[email protected]> 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 <[email protected]>
> > >>>>>
> > >>>>> 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
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 <[email protected]>
>>>>>>
>>>>>> 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
>
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