| Message ID | 20200912005521.26319-1-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [v4,1/1] Input: atmel_mxt_ts - implement I2C retries | expand |
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; > }
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 <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.
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; > >> }
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.
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.
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 <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.
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.
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.
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.
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.
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
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 >
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
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);