Re: [PATCH v2 2/2] Input: atmel_mxt_ts - wake mXT1386 from deep-sleep mode

[Dmitry Osipenko]

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@xxxxxxxxxx>
>> 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?