[PATCH v2 0/2] wilc1000-spi: Add reset/enable GPIO support

[PATCH v2 0/2] wilc1000-spi: Add reset/enable GPIO support

[David Mosberger-Tang]

This version of the patch splits the documentation update and the
actual driver changes into two separate patches.  Also, since SDIO
apparently never uses GPIO lines for reset/enable control, make
simplify the patch to be SPI-only.

David Mosberger-Tang (2):
  wilc1000: Add reset/enable GPIO support to SPI driver
  wilc1000: Document enable-gpios and reset-gpios properties

 .../net/wireless/microchip,wilc1000.yaml      | 15 ++++++++
 drivers/net/wireless/microchip/wilc1000/spi.c | 36 +++++++++++++++++--
 .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
 3 files changed, 49 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] wilc1000: Add reset/enable GPIO support to SPI driver

[David Mosberger-Tang]

For the SDIO driver, the RESET/ENABLE pins of WILC1000 are controlled
through the SDIO power sequence driver.  This commit adds analogous
support for the SPI driver.  Specifically, during initialization, the
chip will be ENABLEd and taken out of RESET and during
deinitialization, the chip will be placed back into RESET and disabled
(both to reduce power consumption and to ensure the WiFi radio is
off).

Both RESET and ENABLE GPIOs are optional.  However, if the ENABLE GPIO
is specified, then the RESET GPIO should normally also be specified as
otherwise there is no way to ensure proper timing of the ENABLE/RESET
sequence.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 drivers/net/wireless/microchip/wilc1000/spi.c | 36 +++++++++++++++++--
 .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
index 640850f989dd..37215fcc27e0 100644
--- a/drivers/net/wireless/microchip/wilc1000/spi.c
+++ b/drivers/net/wireless/microchip/wilc1000/spi.c
@@ -8,6 +8,7 @@
 #include <linux/spi/spi.h>
 #include <linux/crc7.h>
 #include <linux/crc-itu-t.h>
+#include <linux/of_gpio.h>
 
 #include "netdev.h"
 #include "cfg80211.h"
@@ -152,6 +153,31 @@ struct wilc_spi_special_cmd_rsp {
 	u8 status;
 } __packed;
 
+static void wilc_set_enable(struct spi_device *spi, bool on)
+{
+	int enable_gpio, reset_gpio;
+
+	enable_gpio = of_get_named_gpio(spi->dev.of_node, "chip_en-gpios", 0);
+	reset_gpio = of_get_named_gpio(spi->dev.of_node, "reset-gpios", 0);
+
+	if (on) {
+		if (gpio_is_valid(enable_gpio))
+			/* assert ENABLE */
+			gpio_direction_output(enable_gpio, 1);
+		mdelay(5);	/* 5ms delay required by WILC1000 */
+		if (gpio_is_valid(reset_gpio))
+			/* deassert RESET */
+			gpio_direction_output(reset_gpio, 1);
+	} else {
+		if (gpio_is_valid(reset_gpio))
+			/* assert RESET */
+			gpio_direction_output(reset_gpio, 0);
+		if (gpio_is_valid(enable_gpio))
+			/* deassert ENABLE */
+			gpio_direction_output(enable_gpio, 0);
+	}
+}
+
 static int wilc_bus_probe(struct spi_device *spi)
 {
 	int ret;
@@ -977,9 +1003,11 @@ static int wilc_spi_reset(struct wilc *wilc)
 
 static int wilc_spi_deinit(struct wilc *wilc)
 {
-	/*
-	 * TODO:
-	 */
+	struct spi_device *spi = to_spi_device(wilc->dev);
+	struct wilc_spi *spi_priv = wilc->bus_data;
+
+	spi_priv->isinit = false;
+	wilc_set_enable(spi, false);
 	return 0;
 }
 
@@ -1000,6 +1028,8 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
 		dev_err(&spi->dev, "Fail cmd read chip id...\n");
 	}
 
+	wilc_set_enable(spi, true);
+
 	/*
 	 * configure protocol
 	 */
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 82566544419a..f1e4ac3a2ad5 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -1253,7 +1253,7 @@ void wilc_wlan_cleanup(struct net_device *dev)
 	wilc->rx_buffer = NULL;
 	kfree(wilc->tx_buffer);
 	wilc->tx_buffer = NULL;
-	wilc->hif_func->hif_deinit(NULL);
+	wilc->hif_func->hif_deinit(wilc);
 }
 
 static int wilc_wlan_cfg_commit(struct wilc_vif *vif, int type,
-- 
2.25.1

[PATCH v2 2/2] wilc1000: Document enable-gpios and reset-gpios properties

[David Mosberger-Tang]

Add documentation for the ENABLE and RESET GPIOs that may be needed by
wilc1000-spi.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 .../bindings/net/wireless/microchip,wilc1000.yaml | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
index 6c35682377e6..e4da2a58fcb2 100644
--- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
@@ -32,6 +32,19 @@ properties:
   clock-names:
     const: rtc
 
+  enable-gpios:
+    maxItems: 1
+    description: Used by wilc1000-spi to determine the GPIO line
+      connected to the ENABLE line.  Unless special external circuitry
+      is used, reset-gpios must be specified when enable-gpios is
+      specified as otherwise the driver cannot ensure the proper
+      ENABLE/RESET sequence when enabling the chip.
+
+  reset-gpios:
+    maxItems: 1
+    description: Used by wilc1000-spi to determine the GPIO line
+      connected to the RESET line.
+
 required:
   - compatible
   - interrupts
@@ -51,6 +64,8 @@ examples:
         interrupts = <27 0>;
         clocks = <&pck1>;
         clock-names = "rtc";
+        enable-gpios = <&pioA 5 0>;
+        reset-gpios = <&pioA 6 0>;
       };
     };
 
-- 
2.25.1

Re: [PATCH v2 1/2] wilc1000: Add reset/enable GPIO support to SPI driver

[Claudiu.Beznea]

On 08.12.2021 08:16, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> For the SDIO driver, the RESET/ENABLE pins of WILC1000 are controlled
> through the SDIO power sequence driver.  This commit adds analogous
> support for the SPI driver.  Specifically, during initialization, the
> chip will be ENABLEd and taken out of RESET and during
> deinitialization, the chip will be placed back into RESET and disabled
> (both to reduce power consumption and to ensure the WiFi radio is
> off).
> 
> Both RESET and ENABLE GPIOs are optional.  However, if the ENABLE GPIO
> is specified, then the RESET GPIO should normally also be specified as
> otherwise there is no way to ensure proper timing of the ENABLE/RESET
> sequence.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---

Please specify what have been changed since previous version either here
under "---" or in cover letter.

>  drivers/net/wireless/microchip/wilc1000/spi.c | 36 +++++++++++++++++--
>  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index 640850f989dd..37215fcc27e0 100644
> --- a/drivers/net/wireless/microchip/wilc1000/spi.c
> +++ b/drivers/net/wireless/microchip/wilc1000/spi.c
> @@ -8,6 +8,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/crc7.h>
>  #include <linux/crc-itu-t.h>
> +#include <linux/of_gpio.h>

GPIO descriptors are covered by <linux/gpio/consumer.h>

> 
>  #include "netdev.h"
>  #include "cfg80211.h"
> @@ -152,6 +153,31 @@ struct wilc_spi_special_cmd_rsp {
>         u8 status;
>  } __packed;
> 
> +static void wilc_set_enable(struct spi_device *spi, bool on)

I liked better wilc_wlan_power().

> +{
> +       int enable_gpio, reset_gpio;
> +
> +       enable_gpio = of_get_named_gpio(spi->dev.of_node, "chip_en-gpios", 0);
> +       reset_gpio = of_get_named_gpio(spi->dev.of_node, "reset-gpios", 0);

The equivalent of of_get_named_gpio(), device managed, is
devm_gpiod_get_from_of_node() and could be used on behalf of spi device.
But I presume devm_gpiod_get()/devm_gpiod_get_optional() could also be used
for your use case.

And I would keep the parsing just one time, at probe.

> +
> +       if (on) {
> +               if (gpio_is_valid(enable_gpio))
> +                       /* assert ENABLE */
> +                       gpio_direction_output(enable_gpio, 1);
> +               mdelay(5);      /* 5ms delay required by WILC1000 */
> +               if (gpio_is_valid(reset_gpio))
> +                       /* deassert RESET */
> +                       gpio_direction_output(reset_gpio, 1);
> +       } else {
> +               if (gpio_is_valid(reset_gpio))
> +                       /* assert RESET */
> +                       gpio_direction_output(reset_gpio, 0);
> +               if (gpio_is_valid(enable_gpio))
> +                       /* deassert ENABLE */
> +                       gpio_direction_output(enable_gpio, 0);
> +       }

With gpio descriptors as far as I can tell you don't have to explicitly
check the validity of gpio as it is embedded in gpiod_direction_output()
specific function thus the above code may become:

	if (on) {
		gpiod_direction_output(enable_gpio, on);
		mdelay(5);
		gpiod_direction_output(reset_gpio, on);
	} else {
		gpiod_direction_output(reset_gpio, on);
		gpiod_direction_output(enable_gpio, on);
	}
> +}
> +
>  static int wilc_bus_probe(struct spi_device *spi)
>  {
>         int ret;
> @@ -977,9 +1003,11 @@ static int wilc_spi_reset(struct wilc *wilc)
> 
>  static int wilc_spi_deinit(struct wilc *wilc)
>  {
> -       /*
> -        * TODO:
> -        */
> +       struct spi_device *spi = to_spi_device(wilc->dev);
> +       struct wilc_spi *spi_priv = wilc->bus_data;
> +
> +       spi_priv->isinit = false;
> +       wilc_set_enable(spi, false);
>         return 0;
>  }
> 
> @@ -1000,6 +1028,8 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
>                 dev_err(&spi->dev, "Fail cmd read chip id...\n");
>         }
> 
> +       wilc_set_enable(spi, true);
> +
>         /*
>          * configure protocol
>          */
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 82566544419a..f1e4ac3a2ad5 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -1253,7 +1253,7 @@ void wilc_wlan_cleanup(struct net_device *dev)
>         wilc->rx_buffer = NULL;
>         kfree(wilc->tx_buffer);
>         wilc->tx_buffer = NULL;
> -       wilc->hif_func->hif_deinit(NULL);
> +       wilc->hif_func->hif_deinit(wilc);
>  }
> 
>  static int wilc_wlan_cfg_commit(struct wilc_vif *vif, int type,
> --
> 2.25.1
>