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

[Claudiu.Beznea]

On 21.12.2021 23:25, 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@xxxxxxxxxx>

Also, from v5:
Reviewed-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>

> ---
>  drivers/net/wireless/microchip/wilc1000/spi.c | 62 ++++++++++++++++++-
>  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>  2 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index 5ace9e3a56fc8..2c2ed4b09efd5 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/gpio/consumer.h>
> 
>  #include "netdev.h"
>  #include "cfg80211.h"
> @@ -45,6 +46,10 @@ struct wilc_spi {
>         bool probing_crc;       /* true if we're probing chip's CRC config */
>         bool crc7_enabled;      /* true if crc7 is currently enabled */
>         bool crc16_enabled;     /* true if crc16 is currently enabled */
> +       struct wilc_gpios {
> +               struct gpio_desc *enable;       /* ENABLE GPIO or NULL */
> +               struct gpio_desc *reset;        /* RESET GPIO or NULL */
> +       } gpios;
>  };
> 
>  static const struct wilc_hif_func wilc_hif_spi;
> @@ -152,6 +157,50 @@ struct wilc_spi_special_cmd_rsp {
>         u8 status;
>  } __packed;
> 
> +static int wilc_parse_gpios(struct wilc *wilc)
> +{
> +       struct spi_device *spi = to_spi_device(wilc->dev);
> +       struct wilc_spi *spi_priv = wilc->bus_data;
> +       struct wilc_gpios *gpios = &spi_priv->gpios;
> +
> +       /* get ENABLE pin and deassert it (if it is defined): */
> +       gpios->enable = devm_gpiod_get_optional(&spi->dev,
> +                                               "enable", GPIOD_OUT_LOW);
> +       /* get RESET pin and assert it (if it is defined): */
> +       if (gpios->enable) {
> +               /* if enable pin exists, reset must exist as well */
> +               gpios->reset = devm_gpiod_get(&spi->dev,
> +                                             "reset", GPIOD_OUT_HIGH);
> +               if (IS_ERR(gpios->reset)) {
> +                       dev_err(&spi->dev, "missing reset gpio.\n");
> +                       return PTR_ERR(gpios->reset);
> +               }
> +       } else {
> +               gpios->reset = devm_gpiod_get_optional(&spi->dev,
> +                                                      "reset", GPIOD_OUT_HIGH);
> +       }
> +       return 0;
> +}
> +
> +static void wilc_wlan_power(struct wilc *wilc, bool on)
> +{
> +       struct wilc_spi *spi_priv = wilc->bus_data;
> +       struct wilc_gpios *gpios = &spi_priv->gpios;
> +
> +       if (on) {
> +               /* assert ENABLE: */
> +               gpiod_set_value(gpios->enable, 1);
> +               mdelay(5);
> +               /* 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);
> +       }
> +}
> +
>  static int wilc_bus_probe(struct spi_device *spi)
>  {
>         int ret;
> @@ -171,6 +220,10 @@ static int wilc_bus_probe(struct spi_device *spi)
>         wilc->bus_data = spi_priv;
>         wilc->dev_irq_num = spi->irq;
> 
> +       ret = wilc_parse_gpios(wilc);
> +       if (ret < 0)
> +               goto netdev_cleanup;
> +
>         wilc->rtc_clk = devm_clk_get_optional(&spi->dev, "rtc");
>         if (IS_ERR(wilc->rtc_clk)) {
>                 ret = PTR_ERR(wilc->rtc_clk);
> @@ -983,9 +1036,10 @@ static int wilc_spi_reset(struct wilc *wilc)
> 
>  static int wilc_spi_deinit(struct wilc *wilc)
>  {
> -       /*
> -        * TODO:
> -        */
> +       struct wilc_spi *spi_priv = wilc->bus_data;
> +
> +       spi_priv->isinit = false;
> +       wilc_wlan_power(wilc, false);
>         return 0;
>  }
> 
> @@ -1006,6 +1060,8 @@ static int wilc_spi_init(struct wilc *wilc, bool resume)
>                 dev_err(&spi->dev, "Fail cmd read chip id...\n");
>         }
> 
> +       wilc_wlan_power(wilc, true);
> +
>         /*
>          * configure protocol
>          */
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 3f339c2f46f11..1a37a49fe6477 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -1254,7 +1254,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
>