| 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
| ||
|
Message-ID: <d55a2558-b05d-5995-b0f0-f234cb3b50aa@microchip.com>
Date: Wed, 15 Dec 2021 06:41:22 +0000
From: <Claudiu.Beznea@...rochip.com>
To: <davidm@...uge.net>, <Ajay.Kathat@...rochip.com>
CC: <adham.abozaeid@...rochip.com>, <davem@...emloft.net>,
<devicetree@...r.kernel.org>, <kuba@...nel.org>,
<kvalo@...eaurora.org>, <linux-kernel@...r.kernel.org>,
<linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>,
<robh+dt@...nel.org>
Subject: Re: [PATCH v5 1/2] wilc1000: Add reset/enable GPIO support to SPI
driver
On 15.12.2021 05:05, 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>
> ---
> drivers/net/wireless/microchip/wilc1000/spi.c | 58 ++++++++++++++++++-
> .../net/wireless/microchip/wilc1000/wlan.c | 2 +-
> 2 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/spi.c b/drivers/net/wireless/microchip/wilc1000/spi.c
> index 6e7fd18c14e7..0b4425e56bfa 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"
> @@ -43,6 +44,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,46 @@ 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);
As far as I can tell form gpiolib code the difference b/w GPIOD_OUT_HIGH
and GPIOD_OUT_LOW in gpiolib is related to the initial value for the GPIO.
Did you used GPIOD_OUT_HIGH for reset to have the chip out of reset at this
point?
> + 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) {
> + gpiod_set_value(gpios->enable, 1); /* assert ENABLE */
> + mdelay(5);
> + gpiod_set_value(gpios->reset, 0); /* deassert RESET */
From what I can tell from gpiolib code, requesting the pin from device tree
with:
+ reset-gpios = <&pioA 6 GPIO_ACTIVE_LOW>;
makes the value written with gpiod_set_value() to be negated, thus the 0
written here is translated to a 1 on the pin. Is there a reason you did it
like this? Would it have been simpler to have both pins requested with
GPIO_ACTIVE_HIGH and here to do gpiod_set_value(gpio, 1) for both of the
pin. In this way, at the first read of the code one one would have been
telling that it does what datasheet specifies: for power on toggle enable
and reset gpios from 0 to 1 with a delay in between.
> + } else {
> + gpiod_set_value(gpios->reset, 1); /* assert RESET */
> + gpiod_set_value(gpios->enable, 0); /* deassert ENABLE */
I don't usually see comments near the code line in kernel. Maybe move them
before the actual code line or remove them at all as the code is impler enough?
> + }
> +}
> +
> static int wilc_bus_probe(struct spi_device *spi)
> {
> int ret;
> @@ -171,6 +216,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);
> @@ -984,9 +1033,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;
> }
>
> @@ -1007,6 +1057,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 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
>
Powered by blists - more mailing lists