Re: [PATCH v3] Input: qt1050 - wait for calibration to complete

[sashiko-bot@kernel.org]

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