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

[Dmitry Osipenko]

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