2020-02-12 08:46:47

by Jiada Wang

[permalink] [raw]
Subject: [PATCH v7 03/48] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary

From: Nick Dyer <[email protected]>

The workaround of reading all messages until an invalid is received is a
way of forcing the CHG line high, which means that when using
edge-triggered interrupts the interrupt can be acquired.

With level-triggered interrupts the workaround is unnecessary.

Also, most recent maXTouch chips have a feature called RETRIGEN which, when
enabled, reasserts the interrupt line every cycle if there are messages
waiting. This also makes the workaround unnecessary.

Note: the RETRIGEN feature is only in some firmware versions/chips, it's
not valid simply to enable the bit.

Signed-off-by: Nick Dyer <[email protected]>
Acked-by: Benson Leung <[email protected]>
Acked-by: Yufeng Shen <[email protected]>
(cherry picked from ndyer/linux/for-upstream commit 1ae4e8281e491b22442cd5acdfca1862555f8ecb)
[gdavis: Fix conflicts due to v4.6-rc7 commit eb43335c4095 ("Input:
atmel_mxt_ts - use mxt_acquire_irq in mxt_soft_reset").]
Signed-off-by: George G. Davis <[email protected]>
[jiada: reset use_retrigen_workaround at beginning of mxt_check_retrigen()]
Signed-off-by: Jiada Wang <[email protected]>
---
drivers/input/touchscreen/atmel_mxt_ts.c | 51 ++++++++++++++++++++++--
1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 550caec1018e..e184d0e67ef8 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -20,6 +20,7 @@
#include <linux/i2c.h>
#include <linux/input/mt.h>
#include <linux/interrupt.h>
+#include <linux/irq.h>
#include <linux/of.h>
#include <linux/property.h>
#include <linux/slab.h>
@@ -129,6 +130,7 @@ struct t9_range {
/* MXT_SPT_COMMSCONFIG_T18 */
#define MXT_COMMS_CTRL 0
#define MXT_COMMS_CMD 1
+#define MXT_COMMS_RETRIGEN BIT(6)

/* MXT_DEBUG_DIAGNOSTIC_T37 */
#define MXT_DIAGNOSTIC_PAGEUP 0x01
@@ -308,6 +310,7 @@ struct mxt_data {
struct t7_config t7_cfg;
struct mxt_dbg dbg;
struct gpio_desc *reset_gpio;
+ bool use_retrigen_workaround;

/* Cached parameters from object table */
u16 T5_address;
@@ -318,6 +321,7 @@ struct mxt_data {
u16 T71_address;
u8 T9_reportid_min;
u8 T9_reportid_max;
+ u16 T18_address;
u8 T19_reportid;
u16 T44_address;
u8 T100_reportid_min;
@@ -1190,9 +1194,11 @@ static int mxt_acquire_irq(struct mxt_data *data)

enable_irq(data->irq);

- error = mxt_process_messages_until_invalid(data);
- if (error)
- return error;
+ if (data->use_retrigen_workaround) {
+ error = mxt_process_messages_until_invalid(data);
+ if (error)
+ return error;
+ }

return 0;
}
@@ -1282,6 +1288,33 @@ static u32 mxt_calculate_crc(u8 *base, off_t start_off, off_t end_off)
return crc;
}

+static int mxt_check_retrigen(struct mxt_data *data)
+{
+ struct i2c_client *client = data->client;
+ int error;
+ int val;
+
+ data->use_retrigen_workaround = false;
+
+ if (irq_get_trigger_type(data->irq) & IRQF_TRIGGER_LOW)
+ return 0;
+
+ if (data->T18_address) {
+ error = __mxt_read_reg(client,
+ data->T18_address + MXT_COMMS_CTRL,
+ 1, &val);
+ if (error)
+ return error;
+
+ if (val & MXT_COMMS_RETRIGEN)
+ return 0;
+ }
+
+ dev_warn(&client->dev, "Enabling RETRIGEN workaround\n");
+ data->use_retrigen_workaround = true;
+ return 0;
+}
+
static int mxt_prepare_cfg_mem(struct mxt_data *data, struct mxt_cfg *cfg)
{
struct device *dev = &data->client->dev;
@@ -1561,6 +1594,10 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *fw)

mxt_update_crc(data, MXT_COMMAND_BACKUPNV, MXT_BACKUP_VALUE);

+ ret = mxt_check_retrigen(data);
+ if (ret)
+ goto release_mem;
+
ret = mxt_soft_reset(data);
if (ret)
goto release_mem;
@@ -1604,6 +1641,7 @@ static void mxt_free_object_table(struct mxt_data *data)
data->T71_address = 0;
data->T9_reportid_min = 0;
data->T9_reportid_max = 0;
+ data->T18_address = 0;
data->T19_reportid = 0;
data->T44_address = 0;
data->T100_reportid_min = 0;
@@ -1678,6 +1716,9 @@ static int mxt_parse_object_table(struct mxt_data *data,
object->num_report_ids - 1;
data->num_touchids = object->num_report_ids;
break;
+ case MXT_SPT_COMMSCONFIG_T18:
+ data->T18_address = object->start_address;
+ break;
case MXT_SPT_MESSAGECOUNT_T44:
data->T44_address = object->start_address;
break;
@@ -2141,6 +2182,10 @@ static int mxt_initialize(struct mxt_data *data)
msleep(MXT_FW_RESET_TIME);
}

+ error = mxt_check_retrigen(data);
+ if (error)
+ return error;
+
error = mxt_acquire_irq(data);
if (error)
return error;
--
2.17.1


2020-03-17 22:47:18

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v7 03/48] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary

12.02.2020 11:41, Jiada Wang пишет:
> From: Nick Dyer <[email protected]>
>
> The workaround of reading all messages until an invalid is received is a
> way of forcing the CHG line high, which means that when using
> edge-triggered interrupts the interrupt can be acquired.
>
> With level-triggered interrupts the workaround is unnecessary.
>
> Also, most recent maXTouch chips have a feature called RETRIGEN which, when
> enabled, reasserts the interrupt line every cycle if there are messages
> waiting. This also makes the workaround unnecessary.
>
> Note: the RETRIGEN feature is only in some firmware versions/chips, it's
> not valid simply to enable the bit.

...
> +static int mxt_check_retrigen(struct mxt_data *data)
> +{
> + struct i2c_client *client = data->client;
> + int error;
> + int val;
> +
> + data->use_retrigen_workaround = false;
> +
> + if (irq_get_trigger_type(data->irq) & IRQF_TRIGGER_LOW)
> + return 0;
> +
> + if (data->T18_address) {
> + error = __mxt_read_reg(client,
> + data->T18_address + MXT_COMMS_CTRL,
> + 1, &val);
> + if (error)
> + return error;
> +
> + if (val & MXT_COMMS_RETRIGEN)
> + return 0;
> + }
> +
> + dev_warn(&client->dev, "Enabling RETRIGEN workaround\n");
> + data->use_retrigen_workaround = true;
> + return 0;
> +}

Hello Jiada,

I'm seeing "Enabling RETRIGEN workaround" message with the following
device-tree entry:

touchscreen@4c {
compatible = "atmel,maxtouch";
reg = <0x4c>;

interrupt-parent = <&gpio>;
interrupts = <TEGRA_GPIO(V, 6) IRQ_TYPE_LEVEL_LOW>;

reset-gpios = <&gpio TEGRA_GPIO(Q, 7) GPIO_ACTIVE_HIGH>;
};

This happens because data->irq is NULL. Please fix it, thanks in advance.

2020-03-18 13:04:26

by Jiada Wang

[permalink] [raw]
Subject: Re: [PATCH v7 03/48] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary

Hello Dmitry

On 2020/03/18 7:44, Dmitry Osipenko wrote:
> 12.02.2020 11:41, Jiada Wang пишет:
>> From: Nick Dyer <[email protected]>
>>
>> The workaround of reading all messages until an invalid is received is a
>> way of forcing the CHG line high, which means that when using
>> edge-triggered interrupts the interrupt can be acquired.
>>
>> With level-triggered interrupts the workaround is unnecessary.
>>
>> Also, most recent maXTouch chips have a feature called RETRIGEN which, when
>> enabled, reasserts the interrupt line every cycle if there are messages
>> waiting. This also makes the workaround unnecessary.
>>
>> Note: the RETRIGEN feature is only in some firmware versions/chips, it's
>> not valid simply to enable the bit.
>
> ...
>> +static int mxt_check_retrigen(struct mxt_data *data)
>> +{
>> + struct i2c_client *client = data->client;
>> + int error;
>> + int val;
>> +
>> + data->use_retrigen_workaround = false;
>> +
>> + if (irq_get_trigger_type(data->irq) & IRQF_TRIGGER_LOW)
>> + return 0;
>> +
>> + if (data->T18_address) {
>> + error = __mxt_read_reg(client,
>> + data->T18_address + MXT_COMMS_CTRL,
>> + 1, &val);
>> + if (error)
>> + return error;
>> +
>> + if (val & MXT_COMMS_RETRIGEN)
>> + return 0;
>> + }
>> +
>> + dev_warn(&client->dev, "Enabling RETRIGEN workaround\n");
>> + data->use_retrigen_workaround = true;
>> + return 0;
>> +}
>
> Hello Jiada,
>
> I'm seeing "Enabling RETRIGEN workaround" message with the following
> device-tree entry:
>
> touchscreen@4c {
> compatible = "atmel,maxtouch";
> reg = <0x4c>;
>
> interrupt-parent = <&gpio>;
> interrupts = <TEGRA_GPIO(V, 6) IRQ_TYPE_LEVEL_LOW>;
>
> reset-gpios = <&gpio TEGRA_GPIO(Q, 7) GPIO_ACTIVE_HIGH>;
> };
>
> This happens because data->irq is NULL. Please fix it, thanks in advance.
Thanks for reporting this issue,
I will fix this issue in v8 patch-set

Thanks,
Jiada
>