* Re: [PATCH v3] Input: qt1050 - wait for calibration to complete
2026-07-11 2:52 [PATCH v3] Input: qt1050 - wait for calibration to complete Miles Krause via B4 Relay
@ 2026-07-11 3:05 ` sashiko-bot
2026-07-11 3:18 ` Miles Krause
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-11 3:05 UTC (permalink / raw)
To: Miles Krause; +Cc: linux-input, dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Polling for a transient 'calibration started' state makes the probe highly susceptible to random timeouts due to thread preemption or scheduling delays.
- [High] The calibration is triggered and awaited before the soft reset and firmware data application, rendering the calibration useless and leaving the device uncalibrated.
- [High] Command register `QT1050_RES_CAL` is not volatile, leading to cached commands being erroneously re-written.
- [High] If `regmap_write()` fails during suspend, the IRQ remains disabled indefinitely.
--
--- Patch [1]: [PATCH v3] Input: qt1050 - wait for calibration to complete ---
commit ae3cb773621fc88acc85ed8d8f19253b64a18472
Author: Miles Krause <mileskrause5200@gmail.com>
Input: qt1050 - wait for calibration to complete
The CALIBRATE flag is set when calibration begins and clears when it
finishes. regmap_read_poll_timeout() stops when its condition becomes
true, so the existing condition returns when calibration starts instead
of waiting for it to finish.
First wait for the flag to become set, then wait for it to clear before
continuing with the soft reset. Use a 500 ms completion timeout to leave
margin beyond the usual calibration time.
Link: https://lore.kernel.org/r/20260711024428.ED7FF1F000E9@smtp.kernel.org
> --- a/drivers/input/keyboard/qt1050.c
> +++ b/drivers/input/keyboard/qt1050.c
[ ... ]
> @@ -498,9 +499,18 @@ static int qt1050_probe(struct i2c_client *client)
> return err;
> }
> err = regmap_read_poll_timeout(ts->regmap, QT1050_DET_STATUS, status,
> - status >> 7 == 1, 10000, 200000);
> + status & QT1050_DET_STATUS_CALIBRATE,
> + 10000, 200000);
[Severity: High]
This is a pre-existing issue, but does polling for a transient state here
make the probe susceptible to random timeouts?
If calibration completes very quickly before the first read, or if the
kernel thread is delayed or preempted, could we miss the 1 state entirely
and time out after 200ms? Since this causes probe to fail with -ETIMEDOUT,
could this randomly leave the device unusable on faster buses?
> + if (err) {
> + dev_err(dev, "Calibration did not start: %d\n", err);
> + return err;
> + }
> +
> + err = regmap_read_poll_timeout(ts->regmap, QT1050_DET_STATUS, status,
> + !(status & QT1050_DET_STATUS_CALIBRATE),
> + 10000, 500000);
> + if (err) {
> + dev_err(dev, "Calibration did not complete: %d\n", err);
> return err;
> }
[Severity: High]
This isn't a bug introduced by this patch, but looking at the sequence in
qt1050_probe(), calibration is triggered before the soft reset and custom
firmware data application:
qt1050_probe() {
...
err = regmap_update_bits(ts->regmap, QT1050_RES_CAL, 0x7f,
QT1050_RES_CAL_CALIBRATE);
...
err = regmap_update_bits(ts->regmap, QT1050_RES_CAL,
QT1050_RES_CAL_RESET, QT1050_RES_CAL_RESET);
...
err = qt1050_apply_fw_data(ts);
}
Does this logic render the calibration ineffective? Modifying the physical
capacitive tuning parameters usually requires recalibration after they are
set to establish accurate baselines, rather than before.
[Severity: High]
Also pre-existing, but do these read-modify-write operations modify a cached
command register and accidentally accumulate bits?
QT1050_RES_CAL is omitted from the volatile registers list in
qt1050_volatile_reg(). Triggering calibration caches BIT(1). Later,
triggering the soft reset causes regmap_update_bits() to read BIT(1) from
the cache, set BIT(7), and write BIT(7) | BIT(1) to the hardware.
Could this trigger both a calibration and a soft reset simultaneously and
cause unpredictable firmware behavior?
[Severity: High]
This is a pre-existing issue unrelated to the patch above, but looking at
qt1050_suspend(), if regmap_write() fails, does the IRQ remain permanently
disabled?
qt1050_suspend() {
disable_irq(client->irq);
...
return regmap_write(ts->regmap, QT1050_LPMODE,
device_may_wakeup(dev) ? 125 : 0);
}
If the I2C write fails, the function returns the error directly.
Since the PM core will not call resume for a device that failed to suspend,
enable_irq() is never called. Could this leave the touch panel permanently
dead until the module is reloaded?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-qt1050-calibration-status-bit-v3-1-cc9ae3f0b2ec@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] Input: qt1050 - wait for calibration to complete
2026-07-11 2:52 [PATCH v3] Input: qt1050 - wait for calibration to complete Miles Krause via B4 Relay
2026-07-11 3:05 ` sashiko-bot
@ 2026-07-11 3:18 ` Miles Krause
1 sibling, 0 replies; 3+ messages in thread
From: Miles Krause @ 2026-07-11 3:18 UTC (permalink / raw)
To: mileskrause5200
Cc: Dmitry Torokhov, Rob Herring, Marco Felsch, linux-input,
linux-kernel, Sashiko
Please disregard this series. Further review showed that the
calibration sequence requires hardware validation and a broader
investigation than this patch provides.
Thank you for the review.
On Fri, Jul 10, 2026 at 10:52 PM Miles Krause via B4 Relay
<devnull+mileskrause5200.gmail.com@kernel.org> wrote:
>
> From: Miles Krause <mileskrause5200@gmail.com>
>
> The CALIBRATE flag is set when calibration begins and clears when it
> finishes. regmap_read_poll_timeout() stops when its condition becomes
> true, so the existing condition returns when calibration starts instead
> of waiting for it to finish.
>
> First wait for the flag to become set, then wait for it to clear before
> continuing with the soft reset. Use a 500 ms completion timeout to leave
> margin beyond the usual calibration time.
>
> Fixes: cbebf5addec1 ("Input: qt1050 - add Microchip AT42QT1050 support")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/20260711024428.ED7FF1F000E9@smtp.kernel.org
> Signed-off-by: Miles Krause <mileskrause5200@gmail.com>
> ---
> Compile-tested on x86_64 with:
>
> make O=out allmodconfig
> make O=out W=1 drivers/input/keyboard/qt1050.o
>
> Sparse-tested with:
>
> make O=out-sparse allmodconfig
> make O=out-sparse C=1 CHECK=sparse drivers/input/keyboard/qt1050.o
> ---
> Changes in v3:
> - Wait for calibration to start before waiting for completion.
> - Increase the calibration completion timeout to 500 ms.
> - Link to v2: https://patch.msgid.link/20260710-qt1050-calibration-status-bit-v2-1-a58da64bdf7e@gmail.com
>
> Changes in v2:
> - Invert the polling condition to wait for calibration to finish.
> - Reframe the change as a bug fix based on Sashiko's review.
> - Link to v1: https://patch.msgid.link/20260710-qt1050-calibration-status-bit-v1-1-e4f7d1d797b6@gmail.com
>
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> To: Rob Herring <robh@kernel.org>
> To: Marco Felsch <m.felsch@pengutronix.de>
> Cc: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/input/keyboard/qt1050.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/qt1050.c b/drivers/input/keyboard/qt1050.c
> index f9f480c91032..8cd061587302 100644
> --- a/drivers/input/keyboard/qt1050.c
> +++ b/drivers/input/keyboard/qt1050.c
> @@ -27,7 +27,8 @@
> #define QT1050_FW_VERSION 0x01
>
> /* Detection status */
> -#define QT1050_DET_STATUS 0x02
> +#define QT1050_DET_STATUS 0x02
> +#define QT1050_DET_STATUS_CALIBRATE BIT(7)
>
> /* Key status */
> #define QT1050_KEY_STATUS 0x03
> @@ -498,9 +499,18 @@ static int qt1050_probe(struct i2c_client *client)
> return err;
> }
> err = regmap_read_poll_timeout(ts->regmap, QT1050_DET_STATUS, status,
> - status >> 7 == 1, 10000, 200000);
> + status & QT1050_DET_STATUS_CALIBRATE,
> + 10000, 200000);
> if (err) {
> - dev_err(dev, "Calibration failed: %d\n", err);
> + dev_err(dev, "Calibration did not start: %d\n", err);
> + return err;
> + }
> +
> + err = regmap_read_poll_timeout(ts->regmap, QT1050_DET_STATUS, status,
> + !(status & QT1050_DET_STATUS_CALIBRATE),
> + 10000, 500000);
> + if (err) {
> + dev_err(dev, "Calibration did not complete: %d\n", err);
> return err;
> }
>
>
> ---
> base-commit: c8f174900926d3b58cd048ac33b4cbb3de419bfe
> change-id: 20260710-qt1050-calibration-status-bit-0b2e479ffd4b
>
> Best regards,
> --
> Miles Krause <mileskrause5200@gmail.com>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread