Document the mXT1386 compatible string.
Signed-off-by: Jiada Wang <[email protected]>
---
Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index c88919480d37..c13fc0f3f00b 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -3,6 +3,7 @@ Atmel maXTouch touchscreen/touchpad
Required properties:
- compatible:
atmel,maxtouch
+ atmel,mXT1386
The following compatibles have been used in various products but are
deprecated:
--
2.17.1
According to datasheet, mXT1386 chip has a WAKE line, it is used
to wake the chip up from deep sleep mode before communicating with
it via the I2C-compatible interface.
if the WAKE line is connected to a GPIO line, the line must be
asserted 25 ms before the host attempts to communicate with the mXT1386.
If the WAKE line is connected to the SCL pin, the mXT1386 will send a
NACK on the first attempt to address it, the host must then retry 25 ms
later.
This patch introduces mxt_wake() which does a dummy i2c read, follows
with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to
mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any
actual i2c transfer, mxt_wake() can be executed.
Added new compatible string "atmel,mXT1386".
Signed-off-by: Jiada Wang <[email protected]>
Suggested-by: Dmitry Osipenko <[email protected]>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a2189739e30f..d580050a237f 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -196,6 +196,7 @@ enum t100_type {
#define MXT_CRC_TIMEOUT 1000 /* msec */
#define MXT_FW_RESET_TIME 3000 /* msec */
#define MXT_FW_CHG_TIMEOUT 300 /* msec */
+#define MXT_WAKEUP_TIME 25 /* msec */
/* Command to unlock bootloader */
#define MXT_UNLOCK_CMD_MSB 0xaa
@@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
release_firmware(cfg);
}
+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;
+
+ /* TODO: add WAKE-GPIO support */
+
+ i2c_smbus_xfer(client->adapter, client->addr,
+ 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
+ &dummy);
+
+ msleep(MXT_WAKEUP_TIME);
+}
+
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) {
error = mxt_read_info_block(data);
if (!error)
@@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn)
if (ret)
goto release_firmware;
+ mxt_wake(data);
+
if (!data->in_bootloader) {
/* Change to the bootloader mode */
data->in_bootloader = true;
@@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = {
static void mxt_start(struct mxt_data *data)
{
+ mxt_wake(data);
switch (data->suspend_mode) {
case MXT_SUSPEND_T9_CTRL:
mxt_soft_reset(data);
@@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
static const struct of_device_id mxt_of_match[] = {
{ .compatible = "atmel,maxtouch", },
+ { .compatible = "atmel,mXT1386", },
/* Compatibles listed below are deprecated */
{ .compatible = "atmel,qt602240_ts", },
{ .compatible = "atmel,atmel_mxt_ts", },
@@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = {
{ "atmel_mxt_tp", 0 },
{ "maxtouch", 0 },
{ "mXT224", 0 },
+ { "mXT1386", 0 },
{ }
};
MODULE_DEVICE_TABLE(i2c, mxt_id);
--
2.17.1
18.09.2020 15:56, Jiada Wang пишет:
> According to datasheet, mXT1386 chip has a WAKE line, it is used
> to wake the chip up from deep sleep mode before communicating with
> it via the I2C-compatible interface.
>
> if the WAKE line is connected to a GPIO line, the line must be
> asserted 25 ms before the host attempts to communicate with the mXT1386.
> If the WAKE line is connected to the SCL pin, the mXT1386 will send a
> NACK on the first attempt to address it, the host must then retry 25 ms
> later.
>
> This patch introduces mxt_wake() which does a dummy i2c read, follows
> with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to
> mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any
> actual i2c transfer, mxt_wake() can be executed.
>
> Added new compatible string "atmel,mXT1386".
>
> Signed-off-by: Jiada Wang <[email protected]>
Hello, Jiada!
This not critical, but yours suggested-by tag always should be the last
line of the commit message. It's like you're signing all the words that
were written by you.
> Suggested-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f..d580050a237f 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -196,6 +196,7 @@ enum t100_type {
> #define MXT_CRC_TIMEOUT 1000 /* msec */
> #define MXT_FW_RESET_TIME 3000 /* msec */
> #define MXT_FW_CHG_TIMEOUT 300 /* msec */
> +#define MXT_WAKEUP_TIME 25 /* msec */
>
> /* Command to unlock bootloader */
> #define MXT_UNLOCK_CMD_MSB 0xaa
> @@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
> release_firmware(cfg);
> }
>
> +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.
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.
Perhaps we could even skip the GPIO method entirely by not mentioning it
anywhere, since this method isn't needed for now.
Nevertheless, I think it will be good to add DT compatible for the
"atmel,mXT1386", hence this part is good to me.
> + /* TODO: add WAKE-GPIO support */
> +
> + i2c_smbus_xfer(client->adapter, client->addr,
> + 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
> + &dummy);
> +
There is no need to sleep if there was no I2C error, meaning that device
wasn't in a deep-sleep mode. Please add a check for the value returned
by i2c_smbus_xfer() and invoke the msleep() only in a case of the I2C
error condition.
> + msleep(MXT_WAKEUP_TIME);
> +}
> +
> 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.
> error = mxt_read_info_block(data);
> if (!error)
> @@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn)
> if (ret)
> goto release_firmware;
>
> + mxt_wake(data);
> +
> if (!data->in_bootloader) {
> /* Change to the bootloader mode */
> data->in_bootloader = true;
> @@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = {
>
> static void mxt_start(struct mxt_data *data)
> {
> + mxt_wake(data);
Shouldn't the mxt_wake() be under the MXT_SUSPEND_DEEP_SLEEP switch? The
wake-up should be needed only for waking from deep-sleep mode.
> switch (data->suspend_mode) {
> case MXT_SUSPEND_T9_CTRL:
> mxt_soft_reset(data);
> @@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
>
> static const struct of_device_id mxt_of_match[] = {
> { .compatible = "atmel,maxtouch", },
> + { .compatible = "atmel,mXT1386", },
> /* Compatibles listed below are deprecated */
> { .compatible = "atmel,qt602240_ts", },
> { .compatible = "atmel,atmel_mxt_ts", },
> @@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = {
> { "atmel_mxt_tp", 0 },
> { "maxtouch", 0 },
> { "mXT224", 0 },
> + { "mXT1386", 0 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, mxt_id);
>
Hi Dmitry
Thanks for your quick comments
On 2020/09/18 22:32, Dmitry Osipenko wrote:
> 18.09.2020 15:56, Jiada Wang пишет:
>> According to datasheet, mXT1386 chip has a WAKE line, it is used
>> to wake the chip up from deep sleep mode before communicating with
>> it via the I2C-compatible interface.
>>
>> if the WAKE line is connected to a GPIO line, the line must be
>> asserted 25 ms before the host attempts to communicate with the mXT1386.
>> If the WAKE line is connected to the SCL pin, the mXT1386 will send a
>> NACK on the first attempt to address it, the host must then retry 25 ms
>> later.
>>
>> This patch introduces mxt_wake() which does a dummy i2c read, follows
>> with a 25 ms sleep for mXT1386 chip. mxt_wake() is added to
>> mxt_initialize(), mxt_load_fw() and mxt_start() to ensure before any
>> actual i2c transfer, mxt_wake() can be executed.
>>
>> Added new compatible string "atmel,mXT1386".
>>
>> Signed-off-by: Jiada Wang <[email protected]>
>
> Hello, Jiada!
>
> This not critical, but yours suggested-by tag always should be the last
> line of the commit message. It's like you're signing all the words that
> were written by you.
>
>> Suggested-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/input/touchscreen/atmel_mxt_ts.c | 27 ++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index a2189739e30f..d580050a237f 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -196,6 +196,7 @@ enum t100_type {
>> #define MXT_CRC_TIMEOUT 1000 /* msec */
>> #define MXT_FW_RESET_TIME 3000 /* msec */
>> #define MXT_FW_CHG_TIMEOUT 300 /* msec */
>> +#define MXT_WAKEUP_TIME 25 /* msec */
>>
>> /* Command to unlock bootloader */
>> #define MXT_UNLOCK_CMD_MSB 0xaa
>> @@ -2099,12 +2100,33 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
>> release_firmware(cfg);
>> }
>>
>> +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.
> 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.
> Perhaps we could even skip the GPIO method entirely by not mentioning it
> anywhere, since this method isn't needed for now.
>
> Nevertheless, I think it will be good to add DT compatible for the
> "atmel,mXT1386", hence this part is good to me.
>
>> + /* TODO: add WAKE-GPIO support */
>> +
>> + i2c_smbus_xfer(client->adapter, client->addr,
>> + 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE,
>> + &dummy);
>> +
>
> There is no need to sleep if there was no I2C error, meaning that device
> wasn't in a deep-sleep mode. Please add a check for the value returned
> by i2c_smbus_xfer() and invoke the msleep() only in a case of the I2C
> error condition.
>
OK. will do this
>> + msleep(MXT_WAKEUP_TIME);
>> +}
>> +
>> 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.
>> error = mxt_read_info_block(data);
>> if (!error)
>> @@ -2787,6 +2809,8 @@ static int mxt_load_fw(struct device *dev, const char *fn)
>> if (ret)
>> goto release_firmware;
>>
>> + mxt_wake(data);
>> +
>> if (!data->in_bootloader) {
>> /* Change to the bootloader mode */
>> data->in_bootloader = true;
>> @@ -2928,6 +2952,7 @@ static const struct attribute_group mxt_attr_group = {
>>
>> static void mxt_start(struct mxt_data *data)
>> {
>> + mxt_wake(data);
>
> Shouldn't the mxt_wake() be under the MXT_SUSPEND_DEEP_SLEEP switch? The
> wake-up should be needed only for waking from deep-sleep mode.
OK, I will move it under MXT_SUSPEND_DEEP_SLEEP
Thanks,
Jiada
>
>> switch (data->suspend_mode) {
>> case MXT_SUSPEND_T9_CTRL:
>> mxt_soft_reset(data);
>> @@ -3185,6 +3210,7 @@ static SIMPLE_DEV_PM_OPS(mxt_pm_ops, mxt_suspend, mxt_resume);
>>
>> static const struct of_device_id mxt_of_match[] = {
>> { .compatible = "atmel,maxtouch", },
>> + { .compatible = "atmel,mXT1386", },
>> /* Compatibles listed below are deprecated */
>> { .compatible = "atmel,qt602240_ts", },
>> { .compatible = "atmel,atmel_mxt_ts", },
>> @@ -3209,6 +3235,7 @@ static const struct i2c_device_id mxt_id[] = {
>> { "atmel_mxt_tp", 0 },
>> { "maxtouch", 0 },
>> { "mXT224", 0 },
>> + { "mXT1386", 0 },
>> { }
>> };
>> MODULE_DEVICE_TABLE(i2c, mxt_id);
>>
>
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.
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.
>> 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.
...
>>> 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.
The datasheet says that there are 2 seconds since the time of the last
I2C access before TS is put back into auto-sleep if deep-sleep mode is
enabled. The wait-loop has msleep(3000).
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,
if I understand you correctly,
compatible combine with property is what you are suggesting, right?
> 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.
>
> The datasheet says that there are 2 seconds since the time of the last
> I2C access before TS is put back into auto-sleep if deep-sleep mode is
> enabled. The wait-loop has msleep(3000).
OK, thanks for the clarification
Thanks,
Jiada
>
On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <[email protected]> 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
Hi Dmitry
On 2020/09/20 15:02, Dmitry Torokhov wrote:
> On Sat, Sep 19, 2020 at 10:28 PM Wang, Jiada <[email protected]> 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,
>
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 <[email protected]>
>> 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?
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 <[email protected]>
>>> 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...
Thanks,
Jiada
>
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 <[email protected]>
>>>> 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!