[PATCH v5 0/2] wilc1000: Add reset/enable GPIO support to SPI driver

[PATCH v5 0/2] wilc1000: Add reset/enable GPIO support to SPI driver

[David Mosberger-Tang]

The only change in this version is to fix a dt_binding_check error by
including <dt-bindings/gpio/gpio.h> in microchip,wilc1000.yaml.

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      | 19 ++++++
 drivers/net/wireless/microchip/wilc1000/spi.c | 58 ++++++++++++++++++-
 .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
 3 files changed, 75 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v5 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 | 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);
+		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 */
+	} else {
+		gpiod_set_value(gpios->reset, 1);	/* assert RESET */
+		gpiod_set_value(gpios->enable, 0);	/* deassert ENABLE */
+	}
+}
+
 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

[PATCH v5 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>
---
 .../net/wireless/microchip,wilc1000.yaml      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
index 6c35682377e6..60de78f1bc7b 100644
--- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
@@ -32,6 +32,21 @@ properties:
   clock-names:
     const: rtc
 
+  enable-gpios:
+    maxItems: 1
+    description: Used by wilc1000-spi to determine the GPIO line
+      connected to the ENABLE line.  If specified, reset-gpios
+      must be specified as well as otherwise the driver cannot
+      ensure the timing required between asserting ENABLE
+      and deasserting RESET.  This should be declared as an
+      active-high signal.
+
+  reset-gpios:
+    maxItems: 1
+    description: Used by wilc1000-spi to determine the GPIO line
+      connected to the RESET line.  This should be declared as an
+      active-low signal.
+
 required:
   - compatible
   - interrupts
@@ -40,6 +55,8 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
+
     spi {
       #address-cells = <1>;
       #size-cells = <0>;
@@ -51,6 +68,8 @@ examples:
         interrupts = <27 0>;
         clocks = <&pck1>;
         clock-names = "rtc";
+        enable-gpios = <&pioA 5 GPIO_ACTIVE_HIGH>;
+        reset-gpios = <&pioA 6 GPIO_ACTIVE_LOW>;
       };
     };
 
-- 
2.25.1

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

[Claudiu.Beznea]

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
> 
> The only change in this version is to fix a dt_binding_check error by
> including <dt-bindings/gpio/gpio.h> in microchip,wilc1000.yaml.

For future patches, here you should have the full changelog b/w version,
something like:

Changes in v5:
- this
- that
- etc

Changes in v4:
- this, that

...

Changes in v2:
- this, that

> 
> 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      | 19 ++++++
>  drivers/net/wireless/microchip/wilc1000/spi.c | 58 ++++++++++++++++++-
>  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>  3 files changed, 75 insertions(+), 4 deletions(-)
> 
> --
> 2.25.1
> 

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

[Claudiu.Beznea]

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

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

[David Mosberger-Tang]

On Wed, 2021-12-15 at 05:37 +0000, Claudiu.Beznea@microchip.com wrote:
> 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
> > 
> > The only change in this version is to fix a dt_binding_check error by
> > including <dt-bindings/gpio/gpio.h> in microchip,wilc1000.yaml.
> 
> For future patches, here you should have the full changelog b/w version,
> something like:
> 
> Changes in v5:
> - this
> - that
> - etc
> 
> Changes in v4:
> - this, that
> 
> ...
> 
> Changes in v2:
> - this, that

Sure, I can do that going forward.

Thanks,

  --david

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

[David Mosberger-Tang]

On Wed, 2021-12-15 at 06:41 +0000, Claudiu.Beznea@microchip.com wrote:
> On 15.12.2021 05:05, David Mosberger-Tang wrote:
> > 
> +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.

Yes.

> Did you used GPIOD_OUT_HIGH for reset to have the chip out of reset at this
> point?

No, ~RESET is an active-low signal.  GPIOD_OUT_LOW should really be
called GPIOD_OUT_DEASSERTED or something like that.  The code ensures
that the chip is in RESET and ~ENABLEd after parsing the GPIOs.

> > +               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?

Yes, of course.  RESET is an active-low signal, as defined in the
datasheet.

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

I think you're confusing 0 and 1 with low-voltage and high-voltage.  0
means de-assert the signal, 1 means assert the signal.  Whether that
translates to a low voltage or a high voltage depends on whether the
signal a active-low or active-high.

> 
> 
> > +       } 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?

You're kidding, right?

  --david

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

[Claudiu.Beznea]

On 15.12.2021 16:59, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, 2021-12-15 at 06:41 +0000, Claudiu.Beznea@microchip.com wrote:
>> On 15.12.2021 05:05, David Mosberger-Tang wrote:
>>>
>> +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.
> 
> Yes.
> 
>> Did you used GPIOD_OUT_HIGH for reset to have the chip out of reset at this
>> point?
> 
> No, ~RESET is an active-low signal.  GPIOD_OUT_LOW should really be
> called GPIOD_OUT_DEASSERTED or something like that.  The code ensures
> that the chip is in RESET and ~ENABLEd after parsing the GPIOs.
> 
>>> +               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?
> 
> Yes, of course.  RESET is an active-low signal, as defined in the
> datasheet.

Right, I missed that.

> 
>> 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.
> 
> I think you're confusing 0 and 1 with low-voltage and high-voltage.  0
> means de-assert the signal, 1 means assert the signal.  Whether that
> translates to a low voltage or a high voltage depends on whether the
> signal a active-low or active-high.
> 
>>
>>
>>> +       } 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?
> 
> You're kidding, right?
> 
>   --david
> 

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

[Claudiu.Beznea]

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
> 
> The only change in this version is to fix a dt_binding_check error by
> including <dt-bindings/gpio/gpio.h> in microchip,wilc1000.yaml.
> 
> 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      | 19 ++++++
>  drivers/net/wireless/microchip/wilc1000/spi.c | 58 ++++++++++++++++++-
>  .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>  3 files changed, 75 insertions(+), 4 deletions(-)
> 
> --
> 2.25.1
> 

Comments are clarified, you can add my:

Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>

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

[Kalle Valo]

David Mosberger-Tang <davidm@egauge.net> writes:

>> > +       } 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?
>
> You're kidding, right?

I agree with Claudiu, the comments are not really providing more
information from what can be seen from the code. And the style of having
the comment in the same line is not commonly used in upstream.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

[David Mosberger-Tang]

On Thu, 2021-12-16 at 10:10 +0200, Kalle Valo wrote:
> David Mosberger-Tang <davidm@egauge.net> writes:
> 
> > > > +       } 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?
> > 
> > You're kidding, right?
> 
> I agree with Claudiu, the comments are not really providing more
> information from what can be seen from the code. And the style of having
> the comment in the same line is not commonly used in upstream.

The code is obvious if you think of 1 as "assert" and 0 as "deassert".  It looks
utterly wrong if you think of 1 as outputting 3.3V and 0 as outputting 0V.

But if you insist, I'll remove the comments.

  --david

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

[Rob Herring]

On Wed, 15 Dec 2021 03:05:12 +0000, David Mosberger-Tang wrote:
> Add documentation for the ENABLE and RESET GPIOs that may be needed by
> wilc1000-spi.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
>  .../net/wireless/microchip,wilc1000.yaml      | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Kalle Valo]

David Mosberger-Tang <davidm@egauge.net> writes:

> On Thu, 2021-12-16 at 10:10 +0200, Kalle Valo wrote:
>> David Mosberger-Tang <davidm@egauge.net> writes:
>> 
>> > > > +       } 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?
>> > 
>> > You're kidding, right?
>> 
>> I agree with Claudiu, the comments are not really providing more
>> information from what can be seen from the code. And the style of having
>> the comment in the same line is not commonly used in upstream.
>
> The code is obvious if you think of 1 as "assert" and 0 as "deassert".  It looks
> utterly wrong if you think of 1 as outputting 3.3V and 0 as outputting 0V.

Yeah, I guess for people who are more hardware orientated that looks
wrong :)

> But if you insist, I'll remove the comments.

I don't insist removing the comments, but please move them to their own
line so that the style is consistent.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches