Hi Dmitry

On 2020/09/20 15:02, Dmitry Torokhov wrote:

On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada [off-list ref] wrote:

quoted

Hi Dmitry

On 2020/09/20 4:49, Dmitry Osipenko wrote:

quoted

18.09.2020 18:55, Wang, Jiada пишет:
...

quoted

quoted

quoted

    +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?

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

quoted

...

quoted

quoted

quoted

    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,