Re: [PATCH v7 1/4] Input: stmfts - wait for controller ready after reset

On September 25, 2026 1:20:56 PM PDT, David Heidelberg [off-list ref] wrote:

On 25/09/2026 21:27, Dmitry Torokhov wrote:

quoted

Hi David,

On Mon, Sep 07, 2026 at 12:50:19PM +0200, David Heidelberg via B4 Relay wrote:

quoted

  -static void stmfts_reset(struct stmfts_data *sdata)
+static int stmfts_reset(struct stmfts_data *sdata)
  {
  	gpiod_set_value_cansleep(sdata->reset_gpio, 1);
  	msleep(20);
  +	reinit_completion(&sdata->cmd_done);
  	gpiod_set_value_cansleep(sdata->reset_gpio, 0);
-	msleep(50);
+	enable_irq(sdata->client->irq);
+
+	if (!wait_for_completion_timeout(&sdata->cmd_done,
+					 msecs_to_jiffies(STMFTS_RESET_TIMEOUT_MS)))
+		return -ETIMEDOUT;
+
+	return 0;
  }
    static int stmfts_configure(struct stmfts_data *sdata)
  {
  	int err;
    	err = stmfts_command(sdata, STMFTS_SYSTEM_RESET);
  	if (err)

@@ -593,42 +602,47 @@ static int stmfts_power_on(struct stmfts_data *sdata)
  		return err;
    	/*
  	 * The datasheet does not specify the power on time, but considering
  	 * that the reset time is < 10ms, I sleep 20ms to be sure
  	 */
  	msleep(20);
  -	if (sdata->reset_gpio)
-		stmfts_reset(sdata);
+	if (sdata->reset_gpio) {
+		err = stmfts_reset(sdata);
+		if (err) {
+			dev_err(&sdata->client->dev,
+				"controller not ready after reset: %d\n", err);
+			goto err_disable_irq;
+		}
+	} else {
+		enable_irq(sdata->client->irq);
+		msleep(50);
+	}
    	err = stmfts_read_system_info(sdata);
  	if (err)
-		goto err_disable_regulators;
-
-	enable_irq(sdata->client->irq);
-
-	msleep(50);
+		goto err_disable_irq;

I think the logic is becoming quite convoluted here, and factored out
stmfts_reset() does not help. How about we make it look like this:

static int stmfts_power_on(struct stmfts_data *sdata)
{
	int err;

	if (sdata->reset_gpio) {
		gpiod_set_value_cansleep(sdata->reset_gpio, 1);
		/* a short delay before powering up */
		usleep_range(1000, 1500);
	}

	err = regulator_bulk_enable(ARRAY_SIZE(stmfts_supplies),
				    sdata->supplies);
	if (err)
		return err;

	if (sdata->reset_gpio) {
		reinit_completion(&sdata->cmd_done);

		/*
		 * The datasheet does not specify the power on time, but
		 * considering that the reset time is < 10ms, sleep for 20ms
		 * to be sure before releasing reset line.
		 */
		msleep(20);
		gpiod_set_value_cansleep(sdata->reset_gpio, 0);

		enable_irq(sdata->client->irq);

		if (!wait_for_completion_timeout(&sdata->cmd_done,
						 msecs_to_jiffies(STMFTS_RESET_TIMEOUT_MS))) {
			dev_err(&sdata->client->dev, "controller not ready after reset");
			err = -ETIMEDOUT;
			goto err_disable_irq;
		}
	} else {
		/*
		 * We do not know the real controller state (was it powered
		 * off or reset). Let's hope that this is enough time to
		 * initialize.
		 */
		msleep(70);

		enable_irq(sdata->client->irq);
	}

	err = sdata->ops->configure(sdata);
	if (err)
		goto err_disable_irq;

	/*
	 * At this point no one is using the touchscreen
	 * and I don't really care about the return value
	 */
	(void)i2c_smbus_write_byte(sdata->client, STMFTS_SLEEP_IN);

	return 0;

err_disable_irq:
	disable_irq(sdata->client->irq);

	regulator_bulk_disable(ARRAY_SIZE(stmfts_supplies), sdata->supplies);
	return err;
}

Thanks, it seems to be better (thou would be nicer if we didn't have two branches, but... what we can do).

That's that enable_irq() in the middle... We could have more "ifs" but that also looks ugly. 

I added you as Co-developed-by :)

Thanks but I don't think the change was large enough to warrant it.


-- 
Dmitry