Messages in this thread |  | | Subject | Re: [PATCH v2 2/2] Input: atmel_mxt_ts - wake mXT1386 from deep-sleep mode | From | "Wang, Jiada" <> | Date | Sat, 19 Sep 2020 00:55:05 +0900 |
| |
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); >> >
|  |