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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <0eba4b4c-7c00-291a-e9a3-c8c45fe4be86@microchip.com>
Date:   Mon, 13 Dec 2021 06:32:26 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <davidm@...uge.net>, <Ajay.Kathat@...rochip.com>
CC:     <kvalo@...eaurora.org>, <davem@...emloft.net>, <kuba@...nel.org>,
        <linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <adham.abozaeid@...rochip.com>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] wilc1000: Add reset/enable GPIO support to SPI
 driver

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@...uge.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
>