On September 25, 2026 1:20:56 PM PDT, David Heidelberg <[email protected]> wrote: >On 25/09/2026 21:27, Dmitry Torokhov wrote: >> Hi David, >> >> On Mon, Sep 07, 2026 at 12:50:19PM +0200, David Heidelberg via B4 Relay >> wrote: >>> -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
