| Message ID | 20190816082952.17985-4-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | atmel_mxt_ts misc | expand |
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.
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. >
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.
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 --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;