On Wed, 2021-12-15 at 06:41 +0000, Claudiu.Beznea@microchip.com wrote:

On 15.12.2021 05:05, David Mosberger-Tang wrote:

quoted

+static int wilc_parse_gpios(struct wilc *wilc)

quoted

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

quoted

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

quoted

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