Reset on Init polarity incorrect for FT5336 driver

[jwoolston]

Describe the bug
The FT5336 input driver appears to have a bug in the reset sequence it uses during init. Per the data sheet of the 5336 and similar family devices, a power on reset should be accomplished by bringing the reset line low, holding for 5ms and then taking the line high.

The existing code contains documentation which appears to indicate that this sequence is known. However, the actual implementation is the inverse - it is brought high for 5ms then returned low, effectively holding the FT5336 in reset.

This is confirmed by a scan of the I2C bus via the I2C shell showing no devices on the bus (in my case there was only the one device), as well as a logic analyzer on boot. This of course prevents the device from being available to the input subsystem.

To Reproduce

1. Configure a device tree with a FT5336 or similar touch controller on an I2C bus with reset pin. For example, on my NUCLEO-F401RE:

&i2c3 {
	pinctrl-0 = <&i2c3_scl_pa8 &i2c3_sda_pc9>;
	pinctrl-names = "default";
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;

	ft6336: ft6336@38 {
		compatible = "focaltech,ft5336";
		reg = <0x38>;
		int-gpios = <&gpiob 9 0>;
		reset-gpios = <&gpiob 8 0>;
	};
};

2. Attach an LCD or similar equipped with said FT5336 family device. In my case I discovered this with this screen from amazon
3. Ensure input subsystem is enabled.
4. Boot and observe: - i2c scan i2c@40005c00 shows no devices connected <---Note of course I2C device may vary
   • If input logging is enabled (DBG) no touch events are registered

Expected behavior

• I2C scan discovers the device attached
• Input logging (DBG) shows touch events

Impact
Minimal/annoyance. Lost a few hours of work trying to diagnose why it was not working and discovered the simple logic bug

Logs and console output
Happy to provide logs or logic analyzer output but I don't think they are necessary. Simple inspection of the code should reveal the necessary changes. I was able to get the driver working with the following patch:

Index: drivers/input/input_ft5336.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/drivers/input/input_ft5336.c b/drivers/input/input_ft5336.c
--- a/drivers/input/input_ft5336.c	(revision c7a19da8ca1abefa8eac41e704e7c678af0282a9)
+++ b/drivers/input/input_ft5336.c	(date 1741482626208)
@@ -180,8 +180,9 @@
 	k_work_init(&data->work, ft5336_work_handler);
 
 	if (config->reset_gpio.port != NULL) {
+		LOG_INF("Configuring FT6336 Reset Pin.");
 		/* Enable reset GPIO and assert reset */
-		r = gpio_pin_configure_dt(&config->reset_gpio, GPIO_OUTPUT_ACTIVE);
+		r = gpio_pin_configure_dt(&config->reset_gpio, GPIO_OUTPUT_LOW);
 		if (r < 0) {
 			LOG_ERR("Could not enable reset GPIO");
 			return r;
@@ -193,7 +194,7 @@
 		 */
 		k_sleep(K_MSEC(5));
 		/* Pull reset pin high to complete reset sequence */
-		r = gpio_pin_set_dt(&config->reset_gpio, 0);
+		r = gpio_pin_set_dt(&config->reset_gpio, 1);
 		if (r < 0) {
 			return r;
 		}

Environment (please complete the following information):

• OS: Windows
• Toolchain: Zephyr SDK
• Commit SHA: c7a19da

[github-actions]

Hi @jwoolston! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[faxe1008]

Not a bug imo you just have to add the appropriate flags (ACTIVE_LOW) to your reset-gpios phandle, then the sequence is as the datasheet wants it.

[fabiobaltieri]

Yeah as @faxe1008 pointed out "active" and "inactive" refer to the logical value of the pin, in this case "active" refer to "reset" so you would read gpio_pin_configure_dt(&config->reset_gpio, GPIO_OUTPUT_ACTIVE); as "activate reset".

In your devicetree reset-gpios = <&gpiob 8 0>; in indeed incorrect since the resets pin is active low.

In general, I recommend avoiding using 0 as a flag on any gpio as that leaves some space for ambiguity, always flag it as either GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW.

[fabiobaltieri]

Same for int-gpios by the way, surprisingly there's quite few incorrect instances in boards/, may have to look into it.

[jwoolston]

@fabiobaltieri Thank you for the explanation. Given that the snippet was taken from a repo provided STM32 board (providing the int-gpios and I simply duplicated for reset-gpios, I believe you are correct that a pass through the boards to eliminate magic number usage would be in order.

[fabiobaltieri]

I'll keep this open as a reminder for myself to check that out, clearly those aren't being tested, probably just ran in polling mode.