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

[Dmitry Torokhov]

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?

> 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.

>
> > 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.

Thanks,

-- 
Dmitry