lkml.org 
[lkml]   [2020]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/2] Input: atmel_mxt_ts - wake mXT1386 from deep-sleep mode
From
Date
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);
>>
>

\
 
 \ /
  Last update: 2020-09-18 17:55    [W:0.057 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site