| Message ID | 20200918125601.8210-2-jiada_wang@mentor.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] dt-bindings: input: atmel: add compatible for mXT1386 | expand |
18.09.2020 15:56, Jiada Wang пишет: > According to datasheet, mXT1386 chip has a WAKE line, it is used > to wake the chip up from deep sleep mode before communicating with > it via the I2C-compatible interface. > > if the WAKE line is connected to a GPIO line, the line must be > asserted 25 ms before the host attempts to communicate with the mXT1386. > If the WAKE line is connected to the SCL pin, the mXT1386 will send a > NACK on the first attempt to address it, the host must then retry 25 ms > later. > > This patch introduces mxt_wake() which does a dummy i2c read, follows > with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to > mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any > actual i2c transfer, mxt_wake() can be executed. > > Added new compatible string "atmel,mXT1386". > > Signed-off-by: Jiada Wang <jiada_wang@mentor.com> Hello, Jiada! This not critical, but yours suggested-by tag always should be the last line of the commit message. It's like you're signing all the words that were written by you. > Suggested-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index a2189739e30f..d580050a237f 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 > @@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx) > release_firmware(cfg); > } > > +static void mxt_wake(struct mxt_data *data) > +{ > + struct i2c_client *client = data->client; > + struct device *dev = &data->client->dev; > + struct device_node *np = dev->of_node; > + union i2c_smbus_data dummy; > + > + if (!of_device_is_compatible(np, "atmel,mXT1386")) > + return; I'm not sure whether you misses the previous answers from Dmitry Torokhov and Rob Herring, but they suggested to add a new device-tree property which should specify the atmel,wakeup-method. There are 3 possible variants: - NONE - GPIO - I2C-SCL Hence we should bail out from mxt_wake() if method is set to NONE or GPIO. Perhaps we could even skip the GPIO method entirely by not mentioning it anywhere, since this method isn't needed for now. Nevertheless, I think it will be good to add DT compatible for the "atmel,mXT1386", hence this part is good to me. > + /* TODO: add WAKE-GPIO support */ > + > + i2c_smbus_xfer(client->adapter, client->addr, > + 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, > + &dummy); > + There is no need to sleep if there was no I2C error, meaning that device wasn't in a deep-sleep mode. Please add a check for the value returned by i2c_smbus_xfer() and invoke the msleep() only in a case of the I2C error condition. > + msleep(MXT_WAKEUP_TIME); > +} > + > static int mxt_initialize(struct mxt_data *data) > { > struct i2c_client *client = data->client; > int recovery_attempts = 0; > int error; > > + mxt_wake(data); > + > while (1) { I assume the mxt_wake() should be placed here, since there is a 3 seconds timeout in the end of the while-loop, meaning that device may get back into deep-sleep on a retry. > error = mxt_read_info_block(data); > if (!error) > @@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn) > if (ret) > goto release_firmware; > > + mxt_wake(data); > + > if (!data->in_bootloader) { > /* Change to the bootloader mode */ > data->in_bootloader = true; > @@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = { > > static void mxt_start(struct mxt_data *data) > { > + mxt_wake(data); Shouldn't the mxt_wake() be under the MXT_SUSPEND_DEEP_SLEEP switch? The wake-up should be needed only for waking from deep-sleep mode. > switch (data->suspend_mode) { > case MXT_SUSPEND_T9_CTRL: > mxt_soft_reset(data); > @@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume); > > static const struct of_device_id mxt_of_match[] = { > { .compatible = "atmel,maxtouch", }, > + { .compatible = "atmel,mXT1386", }, > /* Compatibles listed below are deprecated */ > { .compatible = "atmel,qt602240_ts", }, > { .compatible = "atmel,atmel_mxt_ts", }, > @@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = { > { "atmel_mxt_tp", 0 }, > { "maxtouch", 0 }, > { "mXT224", 0 }, > + { "mXT1386", 0 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, mxt_id); >
Hi Dmitry Thanks for your quick comments On 2020/09/18 22:32, Dmitry Osipenko wrote: > 18.09.2020 15:56, Jiada Wang пишет: >> According to datasheet, mXT1386 chip has a WAKE line, it is used >> to wake the chip up from deep sleep mode before communicating with >> it via the I2C-compatible interface. >> >> if the WAKE line is connected to a GPIO line, the line must be >> asserted 25 ms before the host attempts to communicate with the mXT1386. >> If the WAKE line is connected to the SCL pin, the mXT1386 will send a >> NACK on the first attempt to address it, the host must then retry 25 ms >> later. >> >> This patch introduces mxt_wake() which does a dummy i2c read, follows >> with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to >> mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any >> actual i2c transfer, mxt_wake() can be executed. >> >> Added new compatible string "atmel,mXT1386". >> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.com> > > Hello, Jiada! > > This not critical, but yours suggested-by tag always should be the last > line of the commit message. It's like you're signing all the words that > were written by you. > >> Suggested-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c >> index a2189739e30f..d580050a237f 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 >> @@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx) >> release_firmware(cfg); >> } >> >> +static void mxt_wake(struct mxt_data *data) >> +{ >> + struct i2c_client *client = data->client; >> + struct device *dev = &data->client->dev; >> + struct device_node *np = dev->of_node; >> + union i2c_smbus_data dummy; >> + >> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >> + return; > I'm not sure whether you misses the previous answers from Dmitry > Torokhov and Rob Herring, but they suggested to add a new device-tree > property which should specify the atmel,wakeup-method. > I think Rob Herring prefers for the compatible solution than property. > There are 3 possible variants: > > - NONE > - GPIO > - I2C-SCL > > Hence we should bail out from mxt_wake() if method is set to NONE or GPIO. > for "GPIO", we still need 25 ms sleep. but rather than a dummy read, WAKE line need to be asserted before sleep. > Perhaps we could even skip the GPIO method entirely by not mentioning it > anywhere, since this method isn't needed for now. > > Nevertheless, I think it will be good to add DT compatible for the > "atmel,mXT1386", hence this part is good to me. > >> + /* TODO: add WAKE-GPIO support */ >> + >> + i2c_smbus_xfer(client->adapter, client->addr, >> + 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, >> + &dummy); >> + > > There is no need to sleep if there was no I2C error, meaning that device > wasn't in a deep-sleep mode. Please add a check for the value returned > by i2c_smbus_xfer() and invoke the msleep() only in a case of the I2C > error condition. > OK. will do this >> + msleep(MXT_WAKEUP_TIME); >> +} >> + >> static int mxt_initialize(struct mxt_data *data) >> { >> struct i2c_client *client = data->client; >> int recovery_attempts = 0; >> int error; >> >> + mxt_wake(data); >> + >> while (1) { > > I assume the mxt_wake() should be placed here, since there is a 3 > seconds timeout in the end of the while-loop, meaning that device may > get back into deep-sleep on a retry. > Can you elaborate a little more why exit from bootload mode after sleep for 3 second could enter deep-sleep mode. >> error = mxt_read_info_block(data); >> if (!error) >> @@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn) >> if (ret) >> goto release_firmware; >> >> + mxt_wake(data); >> + >> if (!data->in_bootloader) { >> /* Change to the bootloader mode */ >> data->in_bootloader = true; >> @@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = { >> >> static void mxt_start(struct mxt_data *data) >> { >> + mxt_wake(data); > > Shouldn't the mxt_wake() be under the MXT_SUSPEND_DEEP_SLEEP switch? The > wake-up should be needed only for waking from deep-sleep mode. OK, I will move it under MXT_SUSPEND_DEEP_SLEEP Thanks, Jiada > >> switch (data->suspend_mode) { >> case MXT_SUSPEND_T9_CTRL: >> mxt_soft_reset(data); >> @@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume); >> >> static const struct of_device_id mxt_of_match[] = { >> { .compatible = "atmel,maxtouch", }, >> + { .compatible = "atmel,mXT1386", }, >> /* Compatibles listed below are deprecated */ >> { .compatible = "atmel,qt602240_ts", }, >> { .compatible = "atmel,atmel_mxt_ts", }, >> @@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = { >> { "atmel_mxt_tp", 0 }, >> { "maxtouch", 0 }, >> { "mXT224", 0 }, >> + { "mXT1386", 0 }, >> { } >> }; >> MODULE_DEVICE_TABLE(i2c, mxt_id); >> >
18.09.2020 18:55, Wang, Jiada пишет: ... >>> +static void mxt_wake(struct mxt_data *data) >>> +{ >>> + struct i2c_client *client = data->client; >>> + struct device *dev = &data->client->dev; >>> + struct device_node *np = dev->of_node; >>> + union i2c_smbus_data dummy; >>> + >>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >>> + return; >> I'm not sure whether you misses the previous answers from Dmitry >> Torokhov and Rob Herring, but they suggested to add a new device-tree >> property which should specify the atmel,wakeup-method. >> > I think Rob Herring prefers for the compatible solution than property. Actually, seems you're right. But I'm not sure now whether he just made a typo, because it's actually a board-specific option. It could be more preferred to skip the i2c_smbus_xfer() for the NONE variant, but it also should be harmless in practice. I guess we indeed could keep the current variant of yours patch and then add a clarifying comment to the commit message and to the code, telling that i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. >> There are 3 possible variants: >> >> - NONE >> - GPIO >> - I2C-SCL >> >> Hence we should bail out from mxt_wake() if method is set to NONE or >> GPIO. >> > for "GPIO", we still need 25 ms sleep. but rather than a dummy read, > WAKE line need to be asserted before sleep. Correct, I just meant to bail out because GPIO is currently unsupported. ... >>> static int mxt_initialize(struct mxt_data *data) >>> { >>> struct i2c_client *client = data->client; >>> int recovery_attempts = 0; >>> int error; >>> + mxt_wake(data); >>> + >>> while (1) { >> >> I assume the mxt_wake() should be placed here, since there is a 3 >> seconds timeout in the end of the while-loop, meaning that device may >> get back into deep-sleep on a retry. >> > Can you elaborate a little more why exit from bootload mode after sleep > for 3 second could enter deep-sleep mode. The loop attempts to exit from bootload mode and then I suppose mxt_read_info_block() may fail if I2C "accidentally" fails, hence the deep-sleep mode still will be enabled on a retry. The datasheet says that there are 2 seconds since the time of the last I2C access before TS is put back into auto-sleep if deep-sleep mode is enabled. The wait-loop has msleep(3000).
Hi Dmitry On 2020/09/20 4:49, Dmitry Osipenko wrote: > 18.09.2020 18:55, Wang, Jiada пишет: > ... >>>> +static void mxt_wake(struct mxt_data *data) >>>> +{ >>>> + struct i2c_client *client = data->client; >>>> + struct device *dev = &data->client->dev; >>>> + struct device_node *np = dev->of_node; >>>> + union i2c_smbus_data dummy; >>>> + >>>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >>>> + return; >>> I'm not sure whether you misses the previous answers from Dmitry >>> Torokhov and Rob Herring, but they suggested to add a new device-tree >>> property which should specify the atmel,wakeup-method. >>> >> I think Rob Herring prefers for the compatible solution than property. > > Actually, seems you're right. But I'm not sure now whether he just made > a typo, because it's actually a board-specific option. > Right, I think since it is a board specific issue, so "property" is the preferred way, if I understand you correctly, compatible combine with property is what you are suggesting, right? > It could be more preferred to skip the i2c_smbus_xfer() for the NONE > variant, but it also should be harmless in practice. I guess we indeed > could keep the current variant of yours patch and then add a clarifying > comment to the commit message and to the code, telling that > i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. > I will skip dummy read for "NONE" variant. >>> There are 3 possible variants: >>> >>> - NONE >>> - GPIO >>> - I2C-SCL >>> >>> Hence we should bail out from mxt_wake() if method is set to NONE or >>> GPIO. >>> >> for "GPIO", we still need 25 ms sleep. but rather than a dummy read, >> WAKE line need to be asserted before sleep. > > Correct, I just meant to bail out because GPIO is currently unsupported. > OK > ... >>>> static int mxt_initialize(struct mxt_data *data) >>>> { >>>> struct i2c_client *client = data->client; >>>> int recovery_attempts = 0; >>>> int error; >>>> + mxt_wake(data); >>>> + >>>> while (1) { >>> >>> I assume the mxt_wake() should be placed here, since there is a 3 >>> seconds timeout in the end of the while-loop, meaning that device may >>> get back into deep-sleep on a retry. >>> >> Can you elaborate a little more why exit from bootload mode after sleep >> for 3 second could enter deep-sleep mode. > > The loop attempts to exit from bootload mode and then I suppose > mxt_read_info_block() may fail if I2C "accidentally" fails, hence the > deep-sleep mode still will be enabled on a retry. > > The datasheet says that there are 2 seconds since the time of the last > I2C access before TS is put back into auto-sleep if deep-sleep mode is > enabled. The wait-loop has msleep(3000). OK, thanks for the clarification Thanks, Jiada >
On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <jiada_wang@mentor.com> wrote: > > Hi Dmitry > > On 2020/09/20 4:49, Dmitry Osipenko wrote: > > 18.09.2020 18:55, Wang, Jiada пишет: > > ... > >>>> +static void mxt_wake(struct mxt_data *data) > >>>> +{ > >>>> + struct i2c_client *client = data->client; > >>>> + struct device *dev = &data->client->dev; > >>>> + struct device_node *np = dev->of_node; > >>>> + union i2c_smbus_data dummy; > >>>> + > >>>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) > >>>> + return; > >>> I'm not sure whether you misses the previous answers from Dmitry > >>> Torokhov and Rob Herring, but they suggested to add a new device-tree > >>> property which should specify the atmel,wakeup-method. > >>> > >> I think Rob Herring prefers for the compatible solution than property. > > > > Actually, seems you're right. But I'm not sure now whether he just made > > a typo, because it's actually a board-specific option. > > > Right, I think since it is a board specific issue, > so "property" is the preferred way, Why are you saying it is a board-specific issue? It seems to me that it is behavior of a given controller, not behavior of a board that happens to use such a controller? > if I understand you correctly, > compatible combine with property is what you are suggesting, right? We should gate the behavior either off a compatible or a new property, but not both. > > > It could be more preferred to skip the i2c_smbus_xfer() for the NONE > > variant, but it also should be harmless in practice. I guess we indeed > > could keep the current variant of yours patch and then add a clarifying > > comment to the commit message and to the code, telling that > > i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. > > > I will skip dummy read for "NONE" variant. > > >>> There are 3 possible variants: > >>> > >>> - NONE > >>> - GPIO > >>> - I2C-SCL > >>> > >>> Hence we should bail out from mxt_wake() if method is set to NONE or > >>> GPIO. > >>> > >> for "GPIO", we still need 25 ms sleep. but rather than a dummy read, > >> WAKE line need to be asserted before sleep. > > > > Correct, I just meant to bail out because GPIO is currently unsupported. > > > > OK > > > ... > >>>> static int mxt_initialize(struct mxt_data *data) > >>>> { > >>>> struct i2c_client *client = data->client; > >>>> int recovery_attempts = 0; > >>>> int error; > >>>> + mxt_wake(data); > >>>> + > >>>> while (1) { > >>> > >>> I assume the mxt_wake() should be placed here, since there is a 3 > >>> seconds timeout in the end of the while-loop, meaning that device may > >>> get back into deep-sleep on a retry. > >>> > >> Can you elaborate a little more why exit from bootload mode after sleep > >> for 3 second could enter deep-sleep mode. > > > > The loop attempts to exit from bootload mode and then I suppose > > mxt_read_info_block() may fail if I2C "accidentally" fails, hence the > > deep-sleep mode still will be enabled on a retry. If the controller is in bootloader mode it will not be in a deep sleep mode. If the controller was just reset via reset GPIO it will not be in deep sleep mode. The controller can only be in deep sleep mode if someone requested deep sleep mode. I'd recommend moving the mxt_wake in the "else" case of handling reset GPIO. Thanks,
Hi Dmitry On 2020/09/20 15:02, Dmitry Torokhov wrote: > On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <jiada_wang@mentor.com> wrote: >> >> Hi Dmitry >> >> On 2020/09/20 4:49, Dmitry Osipenko wrote: >>> 18.09.2020 18:55, Wang, Jiada пишет: >>> ... >>>>>> +static void mxt_wake(struct mxt_data *data) >>>>>> +{ >>>>>> + struct i2c_client *client = data->client; >>>>>> + struct device *dev = &data->client->dev; >>>>>> + struct device_node *np = dev->of_node; >>>>>> + union i2c_smbus_data dummy; >>>>>> + >>>>>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >>>>>> + return; >>>>> I'm not sure whether you misses the previous answers from Dmitry >>>>> Torokhov and Rob Herring, but they suggested to add a new device-tree >>>>> property which should specify the atmel,wakeup-method. >>>>> >>>> I think Rob Herring prefers for the compatible solution than property. >>> >>> Actually, seems you're right. But I'm not sure now whether he just made >>> a typo, because it's actually a board-specific option. >>> >> Right, I think since it is a board specific issue, >> so "property" is the preferred way, > > Why are you saying it is a board-specific issue? It seems to me that > it is behavior of a given controller, not behavior of a board that > happens to use such a controller? > the issue only occurs on mXT1386 controller, but with same mXT1386 soc, behavior differs from how WAKE line is connected, (left low, connect to GPIO or connect to SCL), so I think the issue also is board-specific? >> if I understand you correctly, >> compatible combine with property is what you are suggesting, right? > > We should gate the behavior either off a compatible or a new property, > but not both. > >> >>> It could be more preferred to skip the i2c_smbus_xfer() for the NONE >>> variant, but it also should be harmless in practice. I guess we indeed >>> could keep the current variant of yours patch and then add a clarifying >>> comment to the commit message and to the code, telling that >>> i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. >>> >> I will skip dummy read for "NONE" variant. >> >>>>> There are 3 possible variants: >>>>> >>>>> - NONE >>>>> - GPIO >>>>> - I2C-SCL >>>>> >>>>> Hence we should bail out from mxt_wake() if method is set to NONE or >>>>> GPIO. >>>>> >>>> for "GPIO", we still need 25 ms sleep. but rather than a dummy read, >>>> WAKE line need to be asserted before sleep. >>> >>> Correct, I just meant to bail out because GPIO is currently unsupported. >>> >> >> OK >> >>> ... >>>>>> static int mxt_initialize(struct mxt_data *data) >>>>>> { >>>>>> struct i2c_client *client = data->client; >>>>>> int recovery_attempts = 0; >>>>>> int error; >>>>>> + mxt_wake(data); >>>>>> + >>>>>> while (1) { >>>>> >>>>> I assume the mxt_wake() should be placed here, since there is a 3 >>>>> seconds timeout in the end of the while-loop, meaning that device may >>>>> get back into deep-sleep on a retry. >>>>> >>>> Can you elaborate a little more why exit from bootload mode after sleep >>>> for 3 second could enter deep-sleep mode. >>> >>> The loop attempts to exit from bootload mode and then I suppose >>> mxt_read_info_block() may fail if I2C "accidentally" fails, hence the >>> deep-sleep mode still will be enabled on a retry. > > If the controller is in bootloader mode it will not be in a deep sleep > mode. If the controller was just reset via reset GPIO it will not be > in deep sleep mode. The controller can only be in deep sleep mode if > someone requested deep sleep mode. I'd recommend moving the mxt_wake > in the "else" case of handling reset GPIO. > OK. Thanks, Jiada > Thanks, >
20.09.2020 16:13, Wang, Jiada пишет: > Hi Dmitry > > On 2020/09/20 15:02, Dmitry Torokhov wrote: >> On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <jiada_wang@mentor.com> >> wrote: >>> >>> Hi Dmitry >>> >>> On 2020/09/20 4:49, Dmitry Osipenko wrote: >>>> 18.09.2020 18:55, Wang, Jiada пишет: >>>> ... >>>>>>> +static void mxt_wake(struct mxt_data *data) >>>>>>> +{ >>>>>>> + struct i2c_client *client = data->client; >>>>>>> + struct device *dev = &data->client->dev; >>>>>>> + struct device_node *np = dev->of_node; >>>>>>> + union i2c_smbus_data dummy; >>>>>>> + >>>>>>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >>>>>>> + return; >>>>>> I'm not sure whether you misses the previous answers from Dmitry >>>>>> Torokhov and Rob Herring, but they suggested to add a new device-tree >>>>>> property which should specify the atmel,wakeup-method. >>>>>> >>>>> I think Rob Herring prefers for the compatible solution than property. >>>> >>>> Actually, seems you're right. But I'm not sure now whether he just made >>>> a typo, because it's actually a board-specific option. >>>> >>> Right, I think since it is a board specific issue, >>> so "property" is the preferred way, >> >> Why are you saying it is a board-specific issue? It seems to me that >> it is behavior of a given controller, not behavior of a board that >> happens to use such a controller? >> > > the issue only occurs on mXT1386 controller, > but with same mXT1386 soc, behavior differs from how WAKE line is > connected, (left low, connect to GPIO or connect to SCL), > so I think the issue also is board-specific? > >>> if I understand you correctly, >>> compatible combine with property is what you are suggesting, right? >> >> We should gate the behavior either off a compatible or a new property, >> but not both. Both variants will work. But if other controller models have a similar need, then a wakeup-method property should be more universal since potentially it could be reused by other TS models without much changes to the code. To be honest, I'm not familiar with other Atmel TS controllers, so have no clue what variant should be more preferred. The wakeup-method should be a safer variant, but it also will be a bit more invasive code change. >>>> It could be more preferred to skip the i2c_smbus_xfer() for the NONE >>>> variant, but it also should be harmless in practice. I guess we indeed >>>> could keep the current variant of yours patch and then add a clarifying >>>> comment to the commit message and to the code, telling that >>>> i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. >>>> >>> I will skip dummy read for "NONE" variant. >>> >>>>>> There are 3 possible variants: >>>>>> >>>>>> - NONE >>>>>> - GPIO >>>>>> - I2C-SCL >>>>>> >>>>>> Hence we should bail out from mxt_wake() if method is set to NONE or >>>>>> GPIO. >>>>>> >>>>> for "GPIO", we still need 25 ms sleep. but rather than a dummy read, >>>>> WAKE line need to be asserted before sleep. >>>> >>>> Correct, I just meant to bail out because GPIO is currently >>>> unsupported. >>>> >>> >>> OK >>> >>>> ... >>>>>>> static int mxt_initialize(struct mxt_data *data) >>>>>>> { >>>>>>> struct i2c_client *client = data->client; >>>>>>> int recovery_attempts = 0; >>>>>>> int error; >>>>>>> + mxt_wake(data); >>>>>>> + >>>>>>> while (1) { >>>>>> >>>>>> I assume the mxt_wake() should be placed here, since there is a 3 >>>>>> seconds timeout in the end of the while-loop, meaning that device may >>>>>> get back into deep-sleep on a retry. >>>>>> >>>>> Can you elaborate a little more why exit from bootload mode after >>>>> sleep >>>>> for 3 second could enter deep-sleep mode. >>>> >>>> The loop attempts to exit from bootload mode and then I suppose >>>> mxt_read_info_block() may fail if I2C "accidentally" fails, hence the >>>> deep-sleep mode still will be enabled on a retry. >> >> If the controller is in bootloader mode it will not be in a deep sleep >> mode. If the controller was just reset via reset GPIO it will not be >> in deep sleep mode. The controller can only be in deep sleep mode if >> someone requested deep sleep mode. I'd recommend moving the mxt_wake >> in the "else" case of handling reset GPIO. My observation on Acer A500 shows that first I2C transfer after the reset via GPIO could easily get a NAK, hence mxt_wake() definitely must be placed before the mxt_read_info_block(). Apparently reset doesn't wake controller. What's even more interesting is that I now spotted that the NAK could happen in mxt_interrupt() after mxt_initialize(). I'm also now seeing that both mxt_set_t7_power_cfg() and mxt_t6_command() in mxt_start() need the mxt_wake()! Because both 100% get a NAK without the wakes. @@ -3005,9 +3022,11 @@ static void mxt_start(struct mxt_data *data) case MXT_SUSPEND_DEEP_SLEEP: default: + mxt_wake(data); mxt_set_t7_power_cfg(data, MXT_POWER_CFG_RUN); /* Recalibrate since chip has been in deep sleep */ + mxt_wake(data); mxt_t6_command(data, MXT_COMMAND_CALIBRATE, 1, false); break; } Maybe adding I2C retries still isn't a bad idea?
Hi Dmitry On 2020/09/20 23:21, Dmitry Osipenko wrote: > 20.09.2020 16:13, Wang, Jiada пишет: >> Hi Dmitry >> >> On 2020/09/20 15:02, Dmitry Torokhov wrote: >>> On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <jiada_wang@mentor.com> >>> wrote: >>>> >>>> Hi Dmitry >>>> >>>> On 2020/09/20 4:49, Dmitry Osipenko wrote: >>>>> 18.09.2020 18:55, Wang, Jiada пишет: >>>>> ... >>>>>>>> +static void mxt_wake(struct mxt_data *data) >>>>>>>> +{ >>>>>>>> + struct i2c_client *client = data->client; >>>>>>>> + struct device *dev = &data->client->dev; >>>>>>>> + struct device_node *np = dev->of_node; >>>>>>>> + union i2c_smbus_data dummy; >>>>>>>> + >>>>>>>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >>>>>>>> + return; >>>>>>> I'm not sure whether you misses the previous answers from Dmitry >>>>>>> Torokhov and Rob Herring, but they suggested to add a new device-tree >>>>>>> property which should specify the atmel,wakeup-method. >>>>>>> >>>>>> I think Rob Herring prefers for the compatible solution than property. >>>>> >>>>> Actually, seems you're right. But I'm not sure now whether he just made >>>>> a typo, because it's actually a board-specific option. >>>>> >>>> Right, I think since it is a board specific issue, >>>> so "property" is the preferred way, >>> >>> Why are you saying it is a board-specific issue? It seems to me that >>> it is behavior of a given controller, not behavior of a board that >>> happens to use such a controller? >>> >> >> the issue only occurs on mXT1386 controller, >> but with same mXT1386 soc, behavior differs from how WAKE line is >> connected, (left low, connect to GPIO or connect to SCL), >> so I think the issue also is board-specific? >> >>>> if I understand you correctly, >>>> compatible combine with property is what you are suggesting, right? >>> >>> We should gate the behavior either off a compatible or a new property, >>> but not both. > > Both variants will work. But if other controller models have a similar > need, then a wakeup-method property should be more universal since > potentially it could be reused by other TS models without much changes > to the code. > > To be honest, I'm not familiar with other Atmel TS controllers, so have > no clue what variant should be more preferred. The wakeup-method should > be a safer variant, but it also will be a bit more invasive code change. > >>>>> It could be more preferred to skip the i2c_smbus_xfer() for the NONE >>>>> variant, but it also should be harmless in practice. I guess we indeed >>>>> could keep the current variant of yours patch and then add a clarifying >>>>> comment to the commit message and to the code, telling that >>>>> i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. >>>>> >>>> I will skip dummy read for "NONE" variant. >>>> >>>>>>> There are 3 possible variants: >>>>>>> >>>>>>> - NONE >>>>>>> - GPIO >>>>>>> - I2C-SCL >>>>>>> >>>>>>> Hence we should bail out from mxt_wake() if method is set to NONE or >>>>>>> GPIO. >>>>>>> >>>>>> for "GPIO", we still need 25 ms sleep. but rather than a dummy read, >>>>>> WAKE line need to be asserted before sleep. >>>>> >>>>> Correct, I just meant to bail out because GPIO is currently >>>>> unsupported. >>>>> >>>> >>>> OK >>>> >>>>> ... >>>>>>>> static int mxt_initialize(struct mxt_data *data) >>>>>>>> { >>>>>>>> struct i2c_client *client = data->client; >>>>>>>> int recovery_attempts = 0; >>>>>>>> int error; >>>>>>>> + mxt_wake(data); >>>>>>>> + >>>>>>>> while (1) { >>>>>>> >>>>>>> I assume the mxt_wake() should be placed here, since there is a 3 >>>>>>> seconds timeout in the end of the while-loop, meaning that device may >>>>>>> get back into deep-sleep on a retry. >>>>>>> >>>>>> Can you elaborate a little more why exit from bootload mode after >>>>>> sleep >>>>>> for 3 second could enter deep-sleep mode. >>>>> >>>>> The loop attempts to exit from bootload mode and then I suppose >>>>> mxt_read_info_block() may fail if I2C "accidentally" fails, hence the >>>>> deep-sleep mode still will be enabled on a retry. >>> >>> If the controller is in bootloader mode it will not be in a deep sleep >>> mode. If the controller was just reset via reset GPIO it will not be >>> in deep sleep mode. The controller can only be in deep sleep mode if >>> someone requested deep sleep mode. I'd recommend moving the mxt_wake >>> in the "else" case of handling reset GPIO. > > My observation on Acer A500 shows that first I2C transfer after the > reset via GPIO could easily get a NAK, hence mxt_wake() definitely must > be placed before the mxt_read_info_block(). Apparently reset doesn't > wake controller. > > What's even more interesting is that I now spotted that the NAK could > happen in mxt_interrupt() after mxt_initialize(). > > I'm also now seeing that both mxt_set_t7_power_cfg() and > mxt_t6_command() in mxt_start() need the mxt_wake()! Because both 100% > get a NAK without the wakes. > > @@ -3005,9 +3022,11 @@ static void mxt_start(struct mxt_data *data) > > case MXT_SUSPEND_DEEP_SLEEP: > default: > + mxt_wake(data); > mxt_set_t7_power_cfg(data, MXT_POWER_CFG_RUN); > > /* Recalibrate since chip has been in deep sleep */ > + mxt_wake(data); > mxt_t6_command(data, MXT_COMMAND_CALIBRATE, 1, false); > break; > } > > Maybe adding I2C retries still isn't a bad idea? Yes, by working on find out where need to place mxt_wake(), I am having feeling, we must over look somewhere which needs mxt_wake(), also it will introduce lots of difficulty, later someone needs add some new routines. probably based on retries idea, we can add "compatible" check, to only narrow the retry mechanism happen on mXT1368 controller, is more easier way... Thanks, Jiada >
20.09.2020 17:36, Wang, Jiada пишет: > Hi Dmitry > > On 2020/09/20 23:21, Dmitry Osipenko wrote: >> 20.09.2020 16:13, Wang, Jiada пишет: >>> Hi Dmitry >>> >>> On 2020/09/20 15:02, Dmitry Torokhov wrote: >>>> On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <jiada_wang@mentor.com> >>>> wrote: >>>>> >>>>> Hi Dmitry >>>>> >>>>> On 2020/09/20 4:49, Dmitry Osipenko wrote: >>>>>> 18.09.2020 18:55, Wang, Jiada пишет: >>>>>> ... >>>>>>>>> +static void mxt_wake(struct mxt_data *data) >>>>>>>>> +{ >>>>>>>>> + struct i2c_client *client = data->client; >>>>>>>>> + struct device *dev = &data->client->dev; >>>>>>>>> + struct device_node *np = dev->of_node; >>>>>>>>> + union i2c_smbus_data dummy; >>>>>>>>> + >>>>>>>>> + if (!of_device_is_compatible(np, "atmel,mXT1386")) >>>>>>>>> + return; >>>>>>>> I'm not sure whether you misses the previous answers from Dmitry >>>>>>>> Torokhov and Rob Herring, but they suggested to add a new >>>>>>>> device-tree >>>>>>>> property which should specify the atmel,wakeup-method. >>>>>>>> >>>>>>> I think Rob Herring prefers for the compatible solution than >>>>>>> property. >>>>>> >>>>>> Actually, seems you're right. But I'm not sure now whether he just >>>>>> made >>>>>> a typo, because it's actually a board-specific option. >>>>>> >>>>> Right, I think since it is a board specific issue, >>>>> so "property" is the preferred way, >>>> >>>> Why are you saying it is a board-specific issue? It seems to me that >>>> it is behavior of a given controller, not behavior of a board that >>>> happens to use such a controller? >>>> >>> >>> the issue only occurs on mXT1386 controller, >>> but with same mXT1386 soc, behavior differs from how WAKE line is >>> connected, (left low, connect to GPIO or connect to SCL), >>> so I think the issue also is board-specific? >>> >>>>> if I understand you correctly, >>>>> compatible combine with property is what you are suggesting, right? >>>> >>>> We should gate the behavior either off a compatible or a new property, >>>> but not both. >> >> Both variants will work. But if other controller models have a similar >> need, then a wakeup-method property should be more universal since >> potentially it could be reused by other TS models without much changes >> to the code. >> >> To be honest, I'm not familiar with other Atmel TS controllers, so have >> no clue what variant should be more preferred. The wakeup-method should >> be a safer variant, but it also will be a bit more invasive code change. >> >>>>>> It could be more preferred to skip the i2c_smbus_xfer() for the NONE >>>>>> variant, but it also should be harmless in practice. I guess we >>>>>> indeed >>>>>> could keep the current variant of yours patch and then add a >>>>>> clarifying >>>>>> comment to the commit message and to the code, telling that >>>>>> i2c_smbus_xfer() is harmless in a case of the hardwired WAKE-LINE. >>>>>> >>>>> I will skip dummy read for "NONE" variant. >>>>> >>>>>>>> There are 3 possible variants: >>>>>>>> >>>>>>>> - NONE >>>>>>>> - GPIO >>>>>>>> - I2C-SCL >>>>>>>> >>>>>>>> Hence we should bail out from mxt_wake() if method is set to >>>>>>>> NONE or >>>>>>>> GPIO. >>>>>>>> >>>>>>> for "GPIO", we still need 25 ms sleep. but rather than a dummy read, >>>>>>> WAKE line need to be asserted before sleep. >>>>>> >>>>>> Correct, I just meant to bail out because GPIO is currently >>>>>> unsupported. >>>>>> >>>>> >>>>> OK >>>>> >>>>>> ... >>>>>>>>> static int mxt_initialize(struct mxt_data *data) >>>>>>>>> { >>>>>>>>> struct i2c_client *client = data->client; >>>>>>>>> int recovery_attempts = 0; >>>>>>>>> int error; >>>>>>>>> + mxt_wake(data); >>>>>>>>> + >>>>>>>>> while (1) { >>>>>>>> >>>>>>>> I assume the mxt_wake() should be placed here, since there is a 3 >>>>>>>> seconds timeout in the end of the while-loop, meaning that >>>>>>>> device may >>>>>>>> get back into deep-sleep on a retry. >>>>>>>> >>>>>>> Can you elaborate a little more why exit from bootload mode after >>>>>>> sleep >>>>>>> for 3 second could enter deep-sleep mode. >>>>>> >>>>>> The loop attempts to exit from bootload mode and then I suppose >>>>>> mxt_read_info_block() may fail if I2C "accidentally" fails, hence the >>>>>> deep-sleep mode still will be enabled on a retry. >>>> >>>> If the controller is in bootloader mode it will not be in a deep sleep >>>> mode. If the controller was just reset via reset GPIO it will not be >>>> in deep sleep mode. The controller can only be in deep sleep mode if >>>> someone requested deep sleep mode. I'd recommend moving the mxt_wake >>>> in the "else" case of handling reset GPIO. >> >> My observation on Acer A500 shows that first I2C transfer after the >> reset via GPIO could easily get a NAK, hence mxt_wake() definitely must >> be placed before the mxt_read_info_block(). Apparently reset doesn't >> wake controller. >> >> What's even more interesting is that I now spotted that the NAK could >> happen in mxt_interrupt() after mxt_initialize(). >> >> I'm also now seeing that both mxt_set_t7_power_cfg() and >> mxt_t6_command() in mxt_start() need the mxt_wake()! Because both 100% >> get a NAK without the wakes. >> >> @@ -3005,9 +3022,11 @@ static void mxt_start(struct mxt_data *data) >> >> case MXT_SUSPEND_DEEP_SLEEP: >> default: >> + mxt_wake(data); >> mxt_set_t7_power_cfg(data, MXT_POWER_CFG_RUN); >> >> /* Recalibrate since chip has been in deep sleep */ >> + mxt_wake(data); >> mxt_t6_command(data, MXT_COMMAND_CALIBRATE, 1, false); >> break; >> } >> >> Maybe adding I2C retries still isn't a bad idea? > > Yes, by working on find out where need to place mxt_wake(), > I am having feeling, we must over look somewhere which needs mxt_wake(), > also it will introduce lots of difficulty, later someone needs add some > new routines. > > probably based on retries idea, we can add "compatible" check, > to only narrow the retry mechanism happen on mXT1368 controller, > is more easier way... Agree, this should be the best option. BTW, could you please add a patch to update the touchscreen@4c entry in arch/arm/boot/dts/tegra20-acer-a500-picasso.dts? Thanks in advance!
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index a2189739e30f..d580050a237f 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 @@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx) release_firmware(cfg); } +static void mxt_wake(struct mxt_data *data) +{ + struct i2c_client *client = data->client; + struct device *dev = &data->client->dev; + struct device_node *np = dev->of_node; + union i2c_smbus_data dummy; + + if (!of_device_is_compatible(np, "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); +} + static int mxt_initialize(struct mxt_data *data) { struct i2c_client *client = data->client; int recovery_attempts = 0; int error; + mxt_wake(data); + while (1) { error = mxt_read_info_block(data); if (!error) @@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn) if (ret) goto release_firmware; + mxt_wake(data); + if (!data->in_bootloader) { /* Change to the bootloader mode */ data->in_bootloader = true; @@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = { static void mxt_start(struct mxt_data *data) { + mxt_wake(data); switch (data->suspend_mode) { case MXT_SUSPEND_T9_CTRL: mxt_soft_reset(data); @@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume); static const struct of_device_id mxt_of_match[] = { { .compatible = "atmel,maxtouch", }, + { .compatible = "atmel,mXT1386", }, /* Compatibles listed below are deprecated */ { .compatible = "atmel,qt602240_ts", }, { .compatible = "atmel,atmel_mxt_ts", }, @@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = { { "atmel_mxt_tp", 0 }, { "maxtouch", 0 }, { "mXT224", 0 }, + { "mXT1386", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, mxt_id);
According to datasheet, mXT1386 chip has a WAKE line, it is used to wake the chip up from deep sleep mode before communicating with it via the I2C-compatible interface. if the WAKE line is connected to a GPIO line, the line must be asserted 25 ms before the host attempts to communicate with the mXT1386. If the WAKE line is connected to the SCL pin, the mXT1386 will send a NACK on the first attempt to address it, the host must then retry 25 ms later. This patch introduces mxt_wake() which does a dummy i2c read, follows with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any actual i2c transfer, mxt_wake() can be executed. Added new compatible string "atmel,mXT1386". Signed-off-by: Jiada Wang <jiada_wang@mentor.com> Suggested-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++ 1 file changed, 27 insertions(+)