diff mbox series

[v1,03/63] Input: atmel_mxt_ts - only read messages in mxt_acquire_irq() when necessary

Message ID 20190816082952.17985-4-jiada_wang@mentor.com (mailing list archive)
State Superseded
Headers show
Series atmel_mxt_ts misc | expand

Commit Message

Wang, Jiada Aug. 16, 2019, 8:28 a.m. UTC
From: Nick Dyer <nick.dyer@itdev.co.uk>

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 <nick.dyer@itdev.co.uk>
Acked-by: Benson Leung <bleung@chromium.org>
Acked-by: Yufeng Shen <miletus@chromium.org>
(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 <george_davis@mentor.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 49 ++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 3 deletions(-)

Comments

Dmitry Torokhov Aug. 16, 2019, 5:16 p.m. UTC | #1
On Fri, Aug 16, 2019 at 05:28:52PM +0900, Jiada Wang wrote:
> From: Nick Dyer <nick.dyer@itdev.co.uk>
> 
> 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.

Instead of trying to work around of misconfiguration for IRQ/firmware,
can we simply error out of probe if we see a level interrupt with
!RETRIGEN firmware?

Thanks.
Wang, Jiada Aug. 21, 2019, 1:26 p.m. UTC | #2
Hi Dmitry

On 2019/08/17 2:16, Dmitry Torokhov wrote:
> On Fri, Aug 16, 2019 at 05:28:52PM +0900, Jiada Wang wrote:
>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>
>> 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.
> 
> Instead of trying to work around of misconfiguration for IRQ/firmware,
> can we simply error out of probe if we see a level interrupt with
> !RETRIGEN firmware?
> 
I think for old firmwares, which doesn't support RETRIGEN feature, this 
workaround is needed, otherwise we will break all old firmwares, which 
configured with edge-triggered IRQ

for recent firmwares which support RETRIGEN feature, we can fail probe, 
if RETRIGEN is not enabled, and configured with edge-triggered IRQ.

what is your thought?

Thanks,
Jiada
> Thanks.
>
Dmitry Torokhov Aug. 21, 2019, 5:54 p.m. UTC | #3
On Wed, Aug 21, 2019 at 10:26:31PM +0900, Jiada Wang wrote:
> Hi Dmitry
> 
> On 2019/08/17 2:16, Dmitry Torokhov wrote:
> > On Fri, Aug 16, 2019 at 05:28:52PM +0900, Jiada Wang wrote:
> > > From: Nick Dyer <nick.dyer@itdev.co.uk>
> > > 
> > > 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.
> > 
> > Instead of trying to work around of misconfiguration for IRQ/firmware,
> > can we simply error out of probe if we see a level interrupt with
> > !RETRIGEN firmware?
> > 
> I think for old firmwares, which doesn't support RETRIGEN feature, this
> workaround is needed, otherwise we will break all old firmwares, which
> configured with edge-triggered IRQ

Do you know if there are any? I know Chrome OS firmware have RETRIGEN
activated and they are pretty old (original Pixel is from 2013). But if
we indeed have devices with edge interrupt and old not firmware that
does not retrigger, I guess we'll have to keep it... 

Thanks.
Wang, Jiada Aug. 22, 2019, 3:37 a.m. UTC | #4
Hi

On 2019/08/22 2:54, Dmitry Torokhov wrote:
> On Wed, Aug 21, 2019 at 10:26:31PM +0900, Jiada Wang wrote:
>> Hi Dmitry
>>
>> On 2019/08/17 2:16, Dmitry Torokhov wrote:
>>> On Fri, Aug 16, 2019 at 05:28:52PM +0900, Jiada Wang wrote:
>>>> From: Nick Dyer <nick.dyer@itdev.co.uk>
>>>>
>>>> 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.
>>>
>>> Instead of trying to work around of misconfiguration for IRQ/firmware,
>>> can we simply error out of probe if we see a level interrupt with
>>> !RETRIGEN firmware?
>>>
>> I think for old firmwares, which doesn't support RETRIGEN feature, this
>> workaround is needed, otherwise we will break all old firmwares, which
>> configured with edge-triggered IRQ
> 
> Do you know if there are any? I know Chrome OS firmware have RETRIGEN
> activated and they are pretty old (original Pixel is from 2013). But if
> we indeed have devices with edge interrupt and old not firmware that
> does not retrigger, I guess we'll have to keep it...
> 

Honestly I don't know firmwares/chips which don't support RETRIGEN feature.

BUT Dyer originally authored this patch in 2012, I assume here "old" 
firmware/chips means, those before 2012.


Thanks,
Jiada

> Thanks.
>
diff mbox series

Patch

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 79e35c929857..9b165d23e092 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,31 @@  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;
+
+	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 +1592,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 +1639,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 +1714,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;
@@ -2140,6 +2179,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;