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

On 07/12/21 15:41, Claudiu Beznea - M18063 wrote:

On 02.12.2021 05:55, David Mosberger-Tang wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

This patch is based on similar code from the linux4sam/linux-at91 GIT
repository.

For the SDIO driver, the RESET/ENABLE pins of WILC1000 may be
controlled through the SDIO power sequence driver.  This commit adds
analogous support for the SPI driver.  Specifically, during bus
probing, the chip will be reset-cycled and during unloading, the chip
will be placed 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 also must be specified as otherwise
there is no way to ensure proper timing of the ENABLE/RESET sequence.

Signed-off-by: David Mosberger-Tang <redacted>
---
  .../net/wireless/microchip,wilc1000.yaml      | 11 +++
  .../net/wireless/microchip/wilc1000/Makefile  |  2 +-
  drivers/net/wireless/microchip/wilc1000/hif.h |  2 +
  .../net/wireless/microchip/wilc1000/netdev.h  | 10 +++
  .../net/wireless/microchip/wilc1000/power.c   | 73 +++++++++++++++++++
  drivers/net/wireless/microchip/wilc1000/spi.c | 15 +++-
  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
  7 files changed, 110 insertions(+), 5 deletions(-)
  create mode 100644 drivers/net/wireless/microchip/wilc1000/power.c

diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
index 6c35682377e6..2ce316f5e353 100644
--- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml

@@ -32,6 +32,15 @@ properties:
    clock-names:
      const: rtc

+  enable-gpios:
+    maxItems: 1
+    description: A GPIO line connected to the ENABLE line.  If this
+      is specified, then reset-gpios also must be specified.
+
+  reset-gpios:
+    maxItems: 1
+    description: A GPIO line connected to the RESET line.
+

Bindings should go through a different patch.

quoted

  required:
    - compatible
    - interrupts

@@ -51,6 +60,8 @@ examples:
          interrupts = <27 0>;
          clocks = <&pck1>;
          clock-names = "rtc";
+        enable-gpios = <&pioA 5 0>;
+        reset-gpios = <&pioA 6 0>;
        };
      };

diff --git a/drivers/net/wireless/microchip/wilc1000/Makefile b/drivers/net/wireless/microchip/wilc1000/Makefile
index c3c9e34c1eaa..baf9f021a1d6 100644
--- a/drivers/net/wireless/microchip/wilc1000/Makefile
+++ b/drivers/net/wireless/microchip/wilc1000/Makefile

@@ -2,7 +2,7 @@
  obj-$(CONFIG_WILC1000) += wilc1000.o

  wilc1000-objs := cfg80211.o netdev.o mon.o \
-                       hif.o wlan_cfg.o wlan.o
+                       hif.o wlan_cfg.o wlan.o power.o

  obj-$(CONFIG_WILC1000_SDIO) += wilc1000-sdio.o
  wilc1000-sdio-objs += sdio.o

diff --git a/drivers/net/wireless/microchip/wilc1000/hif.h b/drivers/net/wireless/microchip/wilc1000/hif.h
index cccd54ed0518..a57095d8088e 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.h
+++ b/drivers/net/wireless/microchip/wilc1000/hif.h

@@ -213,4 +213,6 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length);
  void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length);
  void *wilc_parse_join_bss_param(struct cfg80211_bss *bss,
                                 struct cfg80211_crypto_settings *crypto);
+int wilc_of_parse_power_pins(struct wilc *wilc);
+void wilc_wlan_power(struct wilc *wilc, bool on);
  #endif

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index b9a88b3e322f..b95a247322a6 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h

@@ -197,6 +197,15 @@ struct wilc_vif {
         struct cfg80211_bss *bss;
  };

+struct wilc_power_gpios {
+       int reset;
+       int chip_en;
+};
+
+struct wilc_power {
+       struct wilc_power_gpios gpios;
+};
+
  struct wilc_tx_queue_status {
         u8 buffer[AC_BUFFER_SIZE];
         u16 end_index;

@@ -265,6 +274,7 @@ struct wilc {
         bool suspend_event;

         struct workqueue_struct *hif_workqueue;
+       struct wilc_power power;

For SDIO power should be controlled though MMC pwrseq driver. Thus I would
keep this code for SPI only and move this member to struct wilc_spi.

quoted

         struct wilc_cfg cfg;
         void *bus_data;
         struct net_device *monitor_dev;

diff --git a/drivers/net/wireless/microchip/wilc1000/power.c b/drivers/net/wireless/microchip/wilc1000/power.c
new file mode 100644
index 000000000000..d26a39b7698d
--- /dev/null
+++ b/drivers/net/wireless/microchip/wilc1000/power.c

@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2012 - 2018 Microchip Technology Inc., and its subsidiaries.
+ * All rights reserved.

This is not in the GIT repo. It should have been:
"Copyright (c) 2021 Microchip Technology Inc. and its subsidiaries"
		^
		current year

quoted

+ */
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/version.h>
+
+#include "netdev.h"
+
+/**
+ * wilc_of_parse_power_pins() - parse power sequence pins
+ *
+ * @wilc:      wilc data structure
+ *
+ * Returns:     0 on success, negative error number on failures.
+ */
+int wilc_of_parse_power_pins(struct wilc *wilc)
+{
+       struct device_node *of = wilc->dev->of_node;
+       struct wilc_power *power = &wilc->power;
+       int ret;
+
+       power->gpios.reset = of_get_named_gpio_flags(of, "reset-gpios", 0,
+                                                    NULL);
+       power->gpios.chip_en = of_get_named_gpio_flags(of, "chip_en-gpios", 0,
+                                                      NULL);

Consider using gpio descriptors and devm_gpiod_get().

quoted

+       if (!gpio_is_valid(power->gpios.reset))
+               return 0;       /* assume SDIO power sequence driver is used */

In case of SDIO we should rely on mmc pwrseq all the time. It keeps things
cleaner. I would keep the code in this file only for SPI.

quoted

+
+       if (gpio_is_valid(power->gpios.chip_en)) {
+               ret = devm_gpio_request(wilc->dev, power->gpios.chip_en,
+                                       "CHIP_EN");
+               if (ret)
+                       return ret;
+       }
+       return devm_gpio_request(wilc->dev, power->gpios.reset, "RESET");
+}
+EXPORT_SYMBOL_GPL(wilc_of_parse_power_pins);
+
+/**
+ * wilc_wlan_power() - handle power on/off commands
+ *
+ * @wilc:      wilc data structure
+ * @on:                requested power status
+ *
+ * Returns:    none
+ */
+void wilc_wlan_power(struct wilc *wilc, bool on)
+{
+       if (!gpio_is_valid(wilc->power.gpios.reset)) {
+               /* In case SDIO power sequence driver is used to power this
+                * device then the powering sequence is handled by the bus
+                * via pm_runtime_* functions. */

I see this function is called anyway only in SPI context, so, don't think
this handling for SDIO is necessary. Maybe these is something I miss with
regards to it. Ajay, please correct me if I'm wrong.

Yes, 'wilc_wlan_power' function is called for SPI bus. The comments 
(/**/) specific to SDIO can be removed, though the GPIO's validity check 
is required to verify GPIO entries.


Regards,

Ajay