Received-SPF: pass (google.com: domain of linux-wireless+bounces-3552-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?utf-8?q?Alexis_Lothor=C3=A9?= <alexis.lothore@bootlin.com>
Date: Tue, 13 Feb 2024 16:22:44 +0100
Subject: [PATCH RFC] wifi: wilc1000: fix reset line assert/deassert
 polarity
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
Message-Id: <20240213-wilc_1000_reset_line-v1-1-e01da2b23fed@bootlin.com>
To: linux-wireless@vger.kernel.org
Cc: Ajay Singh <ajay.kathat@microchip.com>, 
 Claudiu Beznea <claudiu.beznea@tuxon.dev>, Kalle Valo <kvalo@kernel.org>, 
 David Mosberger-Tang <davidm@egauge.net>, 
 Thomas Petazzoni <thomas.petazzoni@bootlin.com>, 
 linux-kernel@vger.kernel.org, 
 =?utf-8?q?Alexis_Lothor=C3=A9?= <alexis.lothore@bootlin.com>

When using a wilc1000 chip over a spi bus, users can optionally define a
reset gpio and a chip enable gpio. The reset line of wilc1000 is active
low, so to hold the chip in reset, a low (physical) value must be applied.

The corresponding device tree binding documentation was introduced by
commit f31ee3c0a555 ("wilc1000: Document enable-gpios and reset-gpios
properties") and correctly indicates that the reset line is an active-low
signal. However, the corresponding driver part, brought by commit
ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver"), is
misusing the gpiod APIs and apply an inverted logic when powering up/down
the chip (for example, setting the reset line to a logic "1" during power
up, which in fact asserts the reset line when device tree describes the
reset line as GPIO_ACTIVE_LOW). As a consequence, any platform currently
using the driver in SPI mode must use a faulty reset line description in
device tree, or else chip will be maintained in reset and will not even
allow to bring up the chip.

Fix reset line usage by inverting back the gpiod APIs usage, setting the
reset line to the logic value "0" when powering the chip, and the logic
value "1" when powering off the chip.

Fixes: ec031ac4792c ("wilc1000: Add reset/enable GPIO support to SPI driver")
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
This issue was detected because I struggled a bit to setup a WILC-over-SPI
setup, and eventually realized that it was due to chip being hold in reset.

This patch, if accepted, will force any WILC-over-SPI user to update its
device tree description: any platform currently working correctly in this
setup likely have a wrong GPIO_ACTIVE_HIGH used on the reset line device
tree description, contrary to what the documentation says. I am not sure
whether this is tolerable ? If not, what would be the proper way to fix
this ? Make the driver manually parse this flag and somehow make it able
to deal with both versions (so basically: ignoring the setting) ? Just live
with it, and possibly document the issue somewhere ?
---
 drivers/net/wireless/microchip/wilc1000/spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index c92ee4b73a74..30eed2ea523d 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -192,11 +192,11 @@ static void wilc_wlan_power(struct wilc *wilc, bool on)
 		/* assert ENABLE: */
 		gpiod_set_value(gpios->enable, 1);
 		mdelay(5);
-		/* assert RESET: */
-		gpiod_set_value(gpios->reset, 1);
-	} else {
 		/* deassert RESET: */
 		gpiod_set_value(gpios->reset, 0);
+	} else {
+		/* assert RESET: */
+		gpiod_set_value(gpios->reset, 1);
 		/* deassert ENABLE: */
 		gpiod_set_value(gpios->enable, 0);
 	}

---
base-commit: 246a8c611ace197f43ecc6ea4936c6ca363b8aaa
change-id: 20240119-wilc_1000_reset_line-393270fc474e

Best regards,
-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com