* [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs @ 2018-04-20 17:24 Sebastian Reichel 2018-04-20 17:44 ` Ezequiel Garcia 2018-04-23 11:58 ` [PATCHv2] " Sebastian Reichel 0 siblings, 2 replies; 10+ messages in thread From: Sebastian Reichel @ 2018-04-20 17:24 UTC (permalink / raw) To: Nick Dyer, Dmitry Torokhov, linux-input Cc: linux-kernel, kernel, Sebastian Reichel The current reset-gpio support triggers an interrupt storm on platforms using the maxtouch with level based interrupt. The Motorola Droid 4, which I used for some of the tests is not affected, since it uses a level based interrupt. This change avoids the interrupt storm by enabling the device while its interrupt is disabled. The following mxt_initialize() requires, that the device is responsive (at least mxt224E is unresponsive for ~22ms), so we wait some time. We don't wait for leaving bootloader mode anymore, since mxt_initialize() checks for it anyways. This fixes a boot problem on GE PPD (watchdog kills device) and also has been tested on the Droid 4. Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> --- drivers/input/touchscreen/atmel_mxt_ts.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 5d9699fe1b55..f8a9f2a47e78 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -194,6 +194,7 @@ enum t100_type { /* Delay times */ #define MXT_BACKUP_TIME 50 /* msec */ +#define MXT_RESET_GPIO_TIME 20 /* msec */ #define MXT_RESET_TIME 200 /* msec */ #define MXT_RESET_TIMEOUT 3000 /* msec */ #define MXT_CRC_TIMEOUT 1000 /* msec */ @@ -3167,20 +3168,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) return error; } + disable_irq(client->irq); + if (data->reset_gpio) { - data->in_bootloader = true; - msleep(MXT_RESET_TIME); - reinit_completion(&data->bl_completion); + msleep(MXT_RESET_GPIO_TIME); gpiod_set_value(data->reset_gpio, 1); - error = mxt_wait_for_completion(data, &data->bl_completion, - MXT_RESET_TIMEOUT); - if (error) - return error; - data->in_bootloader = false; + msleep(MXT_RESET_TIME); } - disable_irq(client->irq); - error = mxt_initialize(data); if (error) return error; -- 2.17.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-20 17:24 [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs Sebastian Reichel @ 2018-04-20 17:44 ` Ezequiel Garcia 2018-04-20 19:42 ` Sebastian Reichel 2018-04-23 11:58 ` [PATCHv2] " Sebastian Reichel 1 sibling, 1 reply; 10+ messages in thread From: Ezequiel Garcia @ 2018-04-20 17:44 UTC (permalink / raw) To: Sebastian Reichel, Nick Dyer, Dmitry Torokhov, linux-input Cc: linux-kernel, kernel Hi Sebastian, On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > The current reset-gpio support triggers an interrupt storm on platforms > using the maxtouch with level based interrupt. The Motorola Droid 4, > which I used for some of the tests is not affected, since it uses a level > based interrupt. > I found this confusing. Interrupt storm happen with level-based interrupts, but the Droid4 is not affected? > This change avoids the interrupt storm by enabling the device while > its interrupt is disabled. The following mxt_initialize() requires, > that the device is responsive (at least mxt224E is unresponsive for > ~22ms), so we wait some time. We don't wait for leaving bootloader > mode anymore, since mxt_initialize() checks for it anyways. > IMHO, having some more or less arbritrary sleeps is almost always a problem. This value might be enough for some platform, might be too short for some other, and then it might get too large for someone else. > This fixes a boot problem on GE PPD (watchdog kills device) and also > has been tested on the Droid 4. > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 5d9699fe1b55..f8a9f2a47e78 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -194,6 +194,7 @@ enum t100_type { > > /* Delay times */ > #define MXT_BACKUP_TIME 50 /* msec */ > +#define MXT_RESET_GPIO_TIME 20 /* msec */ > #define MXT_RESET_TIME 200 /* msec */ > #define MXT_RESET_TIMEOUT 3000 /* msec */ > #define MXT_CRC_TIMEOUT 1000 /* msec */ > @@ -3167,20 +3168,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > return error; > } > > + disable_irq(client->irq); > + > if (data->reset_gpio) { > - data->in_bootloader = true; > - msleep(MXT_RESET_TIME); > - reinit_completion(&data->bl_completion); > + msleep(MXT_RESET_GPIO_TIME); > gpiod_set_value(data->reset_gpio, 1); Can't we enable the IRQ here, just before the wait... > - error = mxt_wait_for_completion(data, &data->bl_completion, > - MXT_RESET_TIMEOUT); ... and then disable it back afterwards? > - if (error) > - return error; > - data->in_bootloader = false; > + msleep(MXT_RESET_TIME); > } > > - disable_irq(client->irq); > - > error = mxt_initialize(data); > if (error) > return error; > -- > 2.17.0 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-20 17:44 ` Ezequiel Garcia @ 2018-04-20 19:42 ` Sebastian Reichel 2018-04-21 20:11 ` Nick Dyer 0 siblings, 1 reply; 10+ messages in thread From: Sebastian Reichel @ 2018-04-20 19:42 UTC (permalink / raw) To: Ezequiel Garcia Cc: Nick Dyer, Dmitry Torokhov, linux-input, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 3719 bytes --] On Fri, Apr 20, 2018 at 02:44:02PM -0300, Ezequiel Garcia wrote: > Hi Sebastian, > > On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > > The current reset-gpio support triggers an interrupt storm on platforms > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > which I used for some of the tests is not affected, since it uses a level > > based interrupt. > > > > I found this confusing. Interrupt storm happen with level-based interrupts, > but the Droid4 is not affected? This is a typo. Droid 4 has an edge based interrupt defined, PPD has an level based interrupt defined. PPD got an interrupt storm, Droid 4 did not. > > This change avoids the interrupt storm by enabling the device while > > its interrupt is disabled. The following mxt_initialize() requires, > > that the device is responsive (at least mxt224E is unresponsive for > > ~22ms), so we wait some time. We don't wait for leaving bootloader > > mode anymore, since mxt_initialize() checks for it anyways. > > > > IMHO, having some more or less arbritrary sleeps is almost > always a problem. This value might be enough for some platform, > might be too short for some other, and then it might get too large > for someone else. The 22ms chip-being-unresponsive are not newly introduced. The same 22ms are also required for soft-reset. I did introduce a new time (MXT_RESET_GPIO_TIME) for the "chip being reset" state, since my randomly chosen 200ms from before were exaggerated considering all mxt datasheets I checked stated only a few nano seconds. > > This fixes a boot problem on GE PPD (watchdog kills device) and also > > has been tested on the Droid 4. > > > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > --- > > drivers/input/touchscreen/atmel_mxt_ts.c | 15 +++++---------- > > 1 file changed, 5 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > > index 5d9699fe1b55..f8a9f2a47e78 100644 > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > > @@ -194,6 +194,7 @@ enum t100_type { > > > > /* Delay times */ > > #define MXT_BACKUP_TIME 50 /* msec */ > > +#define MXT_RESET_GPIO_TIME 20 /* msec */ > > #define MXT_RESET_TIME 200 /* msec */ > > #define MXT_RESET_TIMEOUT 3000 /* msec */ > > #define MXT_CRC_TIMEOUT 1000 /* msec */ > > @@ -3167,20 +3168,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > > return error; > > } > > > > + disable_irq(client->irq); > > + > > if (data->reset_gpio) { > > - data->in_bootloader = true; > > - msleep(MXT_RESET_TIME); > > - reinit_completion(&data->bl_completion); > > + msleep(MXT_RESET_GPIO_TIME); > > gpiod_set_value(data->reset_gpio, 1); > > Can't we enable the IRQ here, just before the wait... > > > - error = mxt_wait_for_completion(data, &data->bl_completion, > > - MXT_RESET_TIMEOUT); > > ... and then disable it back afterwards? Yes and no. We need the wait, since the interrupt pin does random stuff for 100ms after reset. We can do the wait-for-completion after that time (see mxt_soft_reset function). I kept it simple, since the following init does another soft reset anyways. > > - if (error) > > - return error; > > - data->in_bootloader = false; > > + msleep(MXT_RESET_TIME); > > > } > > > > - disable_irq(client->irq); > > - > > error = mxt_initialize(data); > > if (error) > > return error; -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-20 19:42 ` Sebastian Reichel @ 2018-04-21 20:11 ` Nick Dyer 2018-04-23 11:43 ` Sebastian Reichel 0 siblings, 1 reply; 10+ messages in thread From: Nick Dyer @ 2018-04-21 20:11 UTC (permalink / raw) To: Sebastian Reichel, Ezequiel Garcia Cc: Dmitry Torokhov, linux-input, linux-kernel, kernel On Fri, Apr 20, 2018 at 09:42:07PM +0200, Sebastian Reichel wrote: > On Fri, Apr 20, 2018 at 02:44:02PM -0300, Ezequiel Garcia wrote: > > Hi Sebastian, > > > > On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > > > The current reset-gpio support triggers an interrupt storm on platforms > > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > > which I used for some of the tests is not affected, since it uses a level > > > based interrupt. > > > > > > > I found this confusing. Interrupt storm happen with level-based interrupts, > > but the Droid4 is not affected? Can I ask what happens during the interrupt storm. Are you getting lots of the "failed to read T44 and T5" message, or something else? > > > This change avoids the interrupt storm by enabling the device while > > > its interrupt is disabled. The following mxt_initialize() requires, > > > that the device is responsive (at least mxt224E is unresponsive for > > > ~22ms), so we wait some time. We don't wait for leaving bootloader > > > mode anymore, since mxt_initialize() checks for it anyways. > > > > > > > IMHO, having some more or less arbritrary sleeps is almost > > always a problem. This value might be enough for some platform, > > might be too short for some other, and then it might get too large > > for someone else. > > The 22ms chip-being-unresponsive are not newly introduced. The > same 22ms are also required for soft-reset. I did introduce a > new time (MXT_RESET_GPIO_TIME) for the "chip being reset" state, > since my randomly chosen 200ms from before were exaggerated > considering all mxt datasheets I checked stated only a few nano > seconds. According to the data sheets there is a period after a reset where the CHG line is temporarily set as an input, during which the host should ignore it. If you don't, you might get a stray interrupt and try and communicate with the device, which might leave it in a bad state. I think you mentioned that later in your email. The reset time varies per chip, but the 100ms in mxt_soft_reset() was based on discussions with app support at Atmel, so should be correct in most cases. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-21 20:11 ` Nick Dyer @ 2018-04-23 11:43 ` Sebastian Reichel 0 siblings, 0 replies; 10+ messages in thread From: Sebastian Reichel @ 2018-04-23 11:43 UTC (permalink / raw) To: Nick Dyer Cc: Ezequiel Garcia, Dmitry Torokhov, linux-input, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 2784 bytes --] Hi, On Sat, Apr 21, 2018 at 09:11:37PM +0100, Nick Dyer wrote: > On Fri, Apr 20, 2018 at 09:42:07PM +0200, Sebastian Reichel wrote: > > On Fri, Apr 20, 2018 at 02:44:02PM -0300, Ezequiel Garcia wrote: > > > Hi Sebastian, > > > > > > On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > > > > The current reset-gpio support triggers an interrupt storm on platforms > > > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > > > which I used for some of the tests is not affected, since it uses a level > > > > based interrupt. > > > > > > > > > > I found this confusing. Interrupt storm happen with level-based interrupts, > > > but the Droid4 is not affected? > > Can I ask what happens during the interrupt storm. Are you getting lots > of the "failed to read T44 and T5" message, or something else? I don't get any message (with DEBUG enabled for the driver). The boot process hangs and after a few seconds the watchdog kicks in. > > > > This change avoids the interrupt storm by enabling the device while > > > > its interrupt is disabled. The following mxt_initialize() requires, > > > > that the device is responsive (at least mxt224E is unresponsive for > > > > ~22ms), so we wait some time. We don't wait for leaving bootloader > > > > mode anymore, since mxt_initialize() checks for it anyways. > > > > > > > > > > IMHO, having some more or less arbritrary sleeps is almost > > > always a problem. This value might be enough for some platform, > > > might be too short for some other, and then it might get too large > > > for someone else. > > > > The 22ms chip-being-unresponsive are not newly introduced. The > > same 22ms are also required for soft-reset. I did introduce a > > new time (MXT_RESET_GPIO_TIME) for the "chip being reset" state, > > since my randomly chosen 200ms from before were exaggerated > > considering all mxt datasheets I checked stated only a few nano > > seconds. > > According to the data sheets there is a period after a reset where the > CHG line is temporarily set as an input, during which the host should > ignore it. If you don't, you might get a stray interrupt and try and > communicate with the device, which might leave it in a bad state. I > think you mentioned that later in your email. > > The reset time varies per chip, but the 100ms in mxt_soft_reset() was > based on discussions with app support at Atmel, so should be correct in > most cases. Right. This sleep time is missing for the gpio based reset. The other checks are not required, since the chip initialization routine is called directly afterwards. I will send a PATCHv2, that fixes the patch description and uses the same timeout value for both resets. -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-20 17:24 [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs Sebastian Reichel 2018-04-20 17:44 ` Ezequiel Garcia @ 2018-04-23 11:58 ` Sebastian Reichel 2018-05-03 9:15 ` Sebastian Reichel ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Sebastian Reichel @ 2018-04-23 11:58 UTC (permalink / raw) To: Nick Dyer, Dmitry Torokhov, linux-input Cc: linux-kernel, Ezequiel Garcia, kernel, Sebastian Reichel The current reset-gpio support triggers an interrupt storm on platforms using the maxtouch with level based interrupt. The Motorola Droid 4, which I used for some of the tests is not affected, since it uses a edge based interrupt. This change avoids the interrupt storm by enabling the device while its interrupt is disabled. Afterwards we wait 100ms. This is important for two reasons: The device is unresponsive for some time (~22ms for mxt224E) and the CHG (interrupt) line is not working properly for 100ms. We don't need to wait for any following interrupts, since the following mxt_initialize() checks for bootloader mode anyways. This fixes a boot issue on GE PPD (watchdog kills device due to interrupt storm) and does not cause regression on Motorola Droid 4. Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> --- Changes since PATCHv2: * fix patch description * use same timeout value for soft and gpio-reset --- drivers/input/touchscreen/atmel_mxt_ts.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 5d9699fe1b55..f97fbb0af020 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -194,6 +194,8 @@ enum t100_type { /* Delay times */ #define MXT_BACKUP_TIME 50 /* msec */ +#define MXT_RESET_GPIO_TIME 20 /* msec */ +#define MXT_RESET_INVALID_CHG 100 /* msec */ #define MXT_RESET_TIME 200 /* msec */ #define MXT_RESET_TIMEOUT 3000 /* msec */ #define MXT_CRC_TIMEOUT 1000 /* msec */ @@ -1206,7 +1208,7 @@ static int mxt_soft_reset(struct mxt_data *data) return ret; /* Ignore CHG line for 100ms after reset */ - msleep(100); + msleep(MXT_RESET_INVALID_CHG); mxt_acquire_irq(data); @@ -3167,20 +3169,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) return error; } + disable_irq(client->irq); + if (data->reset_gpio) { - data->in_bootloader = true; - msleep(MXT_RESET_TIME); - reinit_completion(&data->bl_completion); + msleep(MXT_RESET_GPIO_TIME); gpiod_set_value(data->reset_gpio, 1); - error = mxt_wait_for_completion(data, &data->bl_completion, - MXT_RESET_TIMEOUT); - if (error) - return error; - data->in_bootloader = false; + msleep(MXT_RESET_INVALID_CHG); } - disable_irq(client->irq); - error = mxt_initialize(data); if (error) return error; -- 2.17.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-23 11:58 ` [PATCHv2] " Sebastian Reichel @ 2018-05-03 9:15 ` Sebastian Reichel 2018-05-09 14:25 ` Pavel Machek 2018-05-24 16:35 ` Dmitry Torokhov 2 siblings, 0 replies; 10+ messages in thread From: Sebastian Reichel @ 2018-05-03 9:15 UTC (permalink / raw) To: Nick Dyer, Dmitry Torokhov, linux-input Cc: linux-kernel, Ezequiel Garcia, kernel [-- Attachment #1: Type: text/plain, Size: 2874 bytes --] Hi, On Mon, Apr 23, 2018 at 01:58:44PM +0200, Sebastian Reichel wrote: > The current reset-gpio support triggers an interrupt storm on platforms > using the maxtouch with level based interrupt. The Motorola Droid 4, > which I used for some of the tests is not affected, since it uses a edge > based interrupt. > > This change avoids the interrupt storm by enabling the device while its > interrupt is disabled. Afterwards we wait 100ms. This is important for > two reasons: The device is unresponsive for some time (~22ms for > mxt224E) and the CHG (interrupt) line is not working properly for 100ms. > We don't need to wait for any following interrupts, since the following > mxt_initialize() checks for bootloader mode anyways. > > This fixes a boot issue on GE PPD (watchdog kills device due to > interrupt storm) and does not cause regression on Motorola Droid 4. > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > Changes since PATCHv2: > * fix patch description > * use same timeout value for soft and gpio-reset > --- ping. -- Sebastian > drivers/input/touchscreen/atmel_mxt_ts.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 5d9699fe1b55..f97fbb0af020 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -194,6 +194,8 @@ enum t100_type { > > /* Delay times */ > #define MXT_BACKUP_TIME 50 /* msec */ > +#define MXT_RESET_GPIO_TIME 20 /* msec */ > +#define MXT_RESET_INVALID_CHG 100 /* msec */ > #define MXT_RESET_TIME 200 /* msec */ > #define MXT_RESET_TIMEOUT 3000 /* msec */ > #define MXT_CRC_TIMEOUT 1000 /* msec */ > @@ -1206,7 +1208,7 @@ static int mxt_soft_reset(struct mxt_data *data) > return ret; > > /* Ignore CHG line for 100ms after reset */ > - msleep(100); > + msleep(MXT_RESET_INVALID_CHG); > > mxt_acquire_irq(data); > > @@ -3167,20 +3169,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > return error; > } > > + disable_irq(client->irq); > + > if (data->reset_gpio) { > - data->in_bootloader = true; > - msleep(MXT_RESET_TIME); > - reinit_completion(&data->bl_completion); > + msleep(MXT_RESET_GPIO_TIME); > gpiod_set_value(data->reset_gpio, 1); > - error = mxt_wait_for_completion(data, &data->bl_completion, > - MXT_RESET_TIMEOUT); > - if (error) > - return error; > - data->in_bootloader = false; > + msleep(MXT_RESET_INVALID_CHG); > } > > - disable_irq(client->irq); > - > error = mxt_initialize(data); > if (error) > return error; > -- > 2.17.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-23 11:58 ` [PATCHv2] " Sebastian Reichel 2018-05-03 9:15 ` Sebastian Reichel @ 2018-05-09 14:25 ` Pavel Machek 2018-05-09 14:40 ` Sebastian Reichel 2018-05-24 16:35 ` Dmitry Torokhov 2 siblings, 1 reply; 10+ messages in thread From: Pavel Machek @ 2018-05-09 14:25 UTC (permalink / raw) To: Sebastian Reichel Cc: Nick Dyer, Dmitry Torokhov, linux-input, linux-kernel, Ezequiel Garcia, kernel [-- Attachment #1: Type: text/plain, Size: 1241 bytes --] On Mon 2018-04-23 13:58:44, Sebastian Reichel wrote: > The current reset-gpio support triggers an interrupt storm on platforms > using the maxtouch with level based interrupt. The Motorola Droid 4, > which I used for some of the tests is not affected, since it uses a edge > based interrupt. > > This change avoids the interrupt storm by enabling the device while its > interrupt is disabled. Afterwards we wait 100ms. This is important for > two reasons: The device is unresponsive for some time (~22ms for > mxt224E) and the CHG (interrupt) line is not working properly for 100ms. > We don't need to wait for any following interrupts, since the following > mxt_initialize() checks for bootloader mode anyways. > > This fixes a boot issue on GE PPD (watchdog kills device due to > interrupt storm) and does not cause regression on Motorola Droid 4. Out of curiosity, what is "GE PPD"? > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Reviewed-by: Pavel Machek <pavel@ucw.cz> -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-05-09 14:25 ` Pavel Machek @ 2018-05-09 14:40 ` Sebastian Reichel 0 siblings, 0 replies; 10+ messages in thread From: Sebastian Reichel @ 2018-05-09 14:40 UTC (permalink / raw) To: Pavel Machek Cc: Nick Dyer, Dmitry Torokhov, linux-input, linux-kernel, Ezequiel Garcia, kernel [-- Attachment #1: Type: text/plain, Size: 1349 bytes --] Hi, On Wed, May 09, 2018 at 04:25:45PM +0200, Pavel Machek wrote: > On Mon 2018-04-23 13:58:44, Sebastian Reichel wrote: > > The current reset-gpio support triggers an interrupt storm on platforms > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > which I used for some of the tests is not affected, since it uses a edge > > based interrupt. > > > > This change avoids the interrupt storm by enabling the device while its > > interrupt is disabled. Afterwards we wait 100ms. This is important for > > two reasons: The device is unresponsive for some time (~22ms for > > mxt224E) and the CHG (interrupt) line is not working properly for 100ms. > > We don't need to wait for any following interrupts, since the following > > mxt_initialize() checks for bootloader mode anyways. > > > > This fixes a boot issue on GE PPD (watchdog kills device due to > > interrupt storm) and does not cause regression on Motorola Droid 4. > > Out of curiosity, what is "GE PPD"? PPD is a product from GE Healthcare to monitor vital biometric signals: arch/arm/boot/dts/imx53-ppd.dts > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > Reviewed-by: Pavel Machek <pavel@ucw.cz> Thanks, -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] Input: atmel_mxt_ts - fix reset-gpio for level based irqs 2018-04-23 11:58 ` [PATCHv2] " Sebastian Reichel 2018-05-03 9:15 ` Sebastian Reichel 2018-05-09 14:25 ` Pavel Machek @ 2018-05-24 16:35 ` Dmitry Torokhov 2 siblings, 0 replies; 10+ messages in thread From: Dmitry Torokhov @ 2018-05-24 16:35 UTC (permalink / raw) To: Sebastian Reichel Cc: Nick Dyer, linux-input, linux-kernel, Ezequiel Garcia, kernel On Mon, Apr 23, 2018 at 01:58:44PM +0200, Sebastian Reichel wrote: > The current reset-gpio support triggers an interrupt storm on platforms > using the maxtouch with level based interrupt. The Motorola Droid 4, > which I used for some of the tests is not affected, since it uses a edge > based interrupt. > > This change avoids the interrupt storm by enabling the device while its > interrupt is disabled. Afterwards we wait 100ms. This is important for > two reasons: The device is unresponsive for some time (~22ms for > mxt224E) and the CHG (interrupt) line is not working properly for 100ms. > We don't need to wait for any following interrupts, since the following > mxt_initialize() checks for bootloader mode anyways. > > This fixes a boot issue on GE PPD (watchdog kills device due to > interrupt storm) and does not cause regression on Motorola Droid 4. > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Applied, thank you. > --- > Changes since PATCHv2: > * fix patch description > * use same timeout value for soft and gpio-reset > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 5d9699fe1b55..f97fbb0af020 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -194,6 +194,8 @@ enum t100_type { > > /* Delay times */ > #define MXT_BACKUP_TIME 50 /* msec */ > +#define MXT_RESET_GPIO_TIME 20 /* msec */ > +#define MXT_RESET_INVALID_CHG 100 /* msec */ > #define MXT_RESET_TIME 200 /* msec */ > #define MXT_RESET_TIMEOUT 3000 /* msec */ > #define MXT_CRC_TIMEOUT 1000 /* msec */ > @@ -1206,7 +1208,7 @@ static int mxt_soft_reset(struct mxt_data *data) > return ret; > > /* Ignore CHG line for 100ms after reset */ > - msleep(100); > + msleep(MXT_RESET_INVALID_CHG); > > mxt_acquire_irq(data); > > @@ -3167,20 +3169,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > return error; > } > > + disable_irq(client->irq); > + > if (data->reset_gpio) { > - data->in_bootloader = true; > - msleep(MXT_RESET_TIME); > - reinit_completion(&data->bl_completion); > + msleep(MXT_RESET_GPIO_TIME); > gpiod_set_value(data->reset_gpio, 1); > - error = mxt_wait_for_completion(data, &data->bl_completion, > - MXT_RESET_TIMEOUT); > - if (error) > - return error; > - data->in_bootloader = false; > + msleep(MXT_RESET_INVALID_CHG); > } > > - disable_irq(client->irq); > - > error = mxt_initialize(data); > if (error) > return error; > -- > 2.17.0 > -- Dmitry ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-24 16:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-20 17:24 [PATCH] Input: atmel_mxt_ts - fix reset-gpio for level based irqs Sebastian Reichel 2018-04-20 17:44 ` Ezequiel Garcia 2018-04-20 19:42 ` Sebastian Reichel 2018-04-21 20:11 ` Nick Dyer 2018-04-23 11:43 ` Sebastian Reichel 2018-04-23 11:58 ` [PATCHv2] " Sebastian Reichel 2018-05-03 9:15 ` Sebastian Reichel 2018-05-09 14:25 ` Pavel Machek 2018-05-09 14:40 ` Sebastian Reichel 2018-05-24 16:35 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).