Messages in this thread | | | Subject | Re: [PATCH v2 2/2] Input: atmel_mxt_ts - wake mXT1386 from deep-sleep mode | From | "Wang, Jiada" <> | Date | Sun, 20 Sep 2020 22:13:17 +0900 |
| |
Hi Dmitry
On 2020/09/20 15:02, Dmitry Torokhov wrote: > On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <jiada_wang@mentor.com> 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. > >> >>> 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. >
OK.
Thanks, Jiada > Thanks, >
| |