[TOPIC GPIO] Adapt winc1500 wifi driver to new API

[tbursztyka]

No description provided.

[carlescufi]

Why aren't the pins being configured with gpio_pin_configure()?
EDIT: I see that is done externally, never mind.

[pabigot]

OK, but one function call with enable ? GPIO_INT_ENABLE : GPIO_INT_DISABLE as the value of the fourth parameter might be easier to maintain.

Has this been tested? Normally we'd want something like GPIO_INT_EDGE_TO_ACTIVE to tell the driver what sort of interrupt to expect. GPIO_INT_ENABLE may not select a valid configuration, since both LOW and HIGH are independent bits.

In fact, I think this would cause an assertion failure against this check:

        __ASSERT(((flags & GPIO_INT_ENABLE) == 0) ||
                 ((flags & (GPIO_INT_LOW_0 | GPIO_INT_HIGH_1)) != 0),
                 "At least one of GPIO_INT_LOW_0, GPIO_INT_HIGH_1 has to be "
                 "enabled.");

so it does have to be changed.

[tbursztyka]

Has this been tested?

No. I don't have the setup anymore and no time to put on that anyway.

[mnkp]

I'm not sure how samples/net/wifi/ sample is supposed to work, however running

west build -b disco_l475_iot1 samples/net/wifi/

or

west build -b cc3220sf_launchxl samples/net/wifi/

leaves CONFIG_WIFI_WINC1500 unset and as a result bulk of the sample code is not compiled. Both boards are white-listed.

It might be cleaner to put the sample code into a separate commit.

[tbursztyka]

leaves CONFIG_WIFI_WINC1500 unset and as a result bulk of the sample code is not compiled. Both boards are white-listed.

cc3220sf_launchxl and disco_l475_iot1 embed a different wifi driver.
winc1500 is not embedded in any board, it's up to the user to wire it up to the board he wants.
So this sample won't build winc1500 directly.

[mnkp]

It would be cleaner if we added here

	gpio_devicetree_flags_t flags;

field, filled it when initializing winc1500_gpios and then referred to the flags field in the rest of the code.

[tbursztyka]

why storing the flags? Aren't the pin configured once and for all, at is used to be or?

[pabigot]

In this case, and because this is being put in RAM and the driver doesn't support multiple instances, I think this is fine as-is.

[mnkp]

That was just a suggestion. Indeed, storing constant flags in RAM is not ideal, but we already keep pin numbers there.

[mnkp]

In case of the reset pin we generally let the user configure its active level in DTS. I propose to follow the convention. In this case the name of the pin should reflect signal logical value. I.e. it should be WINC1500_GPIO_IDX_RESET. Then in the code we would call gpio_pin_set(..., 1) to assert the reset (e.g. pull reset pin low) and gpio_pin_set(..., 0) to de-assert the reset.

[tbursztyka]

No need to change that. Here the point of that PR is just to switch to new API, not to do more work.

[pabigot]

I agree this rework is not necessary given the scope of this work. We have not enforced the convert-to-logic-API in every driver that's already been accepted as converted.

[mnkp]

If we prefer to stick to the old implementation - do not use logical levels - then we should use the gpio_pin_set_raw functions. Otherwise the implementation is inconsistent, also quite confusing to the users reading/using the code.

[pabigot]

I agree, this like #22241 should be using the raw API if it's not going to be updated to use the logical values.

[pabigot]

The motivation for using GPIO_OUTPUT_INACTIVE is that it clearly specifies the initial condition. Without it we have to rely on some other code somewhere initializing the signal to a reasonable state. Presumably that's being done, but it's not being done here, so not providing an initial state makes it harder to understand what the driver's doing.

[tbursztyka]

sure, but that again requires driver logic change which I am not willing to do (mostly because I cannot test). So let's stick to "port it to what logic was being applied".

[pabigot]

That's below my normal minimal bar for Zephyr quality given the risk of the change is low and as-is a known cheap technical debt is left in place.

But since we need to get this in today if you don't want to finish it up I can live with it. @mnkp may or may not insist; I'll wait to approve until there's more feedback. We'll discuss any outstanding issues in the API telecon and resolve them there.

[mnkp]

I also believe we should initialize the output pin to the known value. Otherwise, by choice, we will get driver dependent 'random' value. Some of the Zephyr drivers winc1500 is used with may initialize the pint to low the other to high. Even if we can't test, setting the pin to what we believe is a proper value is better than leaving it to the driver. We should use however physical level flags, e.g. GPIO_OUTPUT_LOW.

[carlescufi]

@mnkp please re-review

[jfischer-no]

Testing it now

[jfischer-no]

I rebased it on the topic-gpio and removed #include <drivers/wifi/winc1500.h>. Then I was able to build the driver.

diff --git a/drivers/wifi/winc1500/wifi_winc1500_nm_bsp_internal.h b/drivers/wifi/winc1500/wifi_winc1500_nm_bsp_internal.h
index 0eff47ddff..90968187a9 100644
--- a/drivers/wifi/winc1500/wifi_winc1500_nm_bsp_internal.h
+++ b/drivers/wifi/winc1500/wifi_winc1500_nm_bsp_internal.h
@@ -11,8 +11,6 @@
 #include <drivers/gpio.h>
 #include <drivers/spi.h>
 
-#include <drivers/wifi/winc1500.h>
-
 #include "wifi_winc1500_config.h"
 #include <bus_wrapper/include/nm_bus_wrapper.h>

The driver starts up and reads the MAC but fails on few settings and does not work, it is exact the same behavior as on master brunch. It could also be a issue with the hardware and I will not open a issue about functionality of this driver.

[00:00:00.000,000] <dbg> w*** Booting Zephyr OS build zephyr-v2.1.0-1756-gd46888432449  ***
inc1500.nm_bus_init: SPI GPIO CS configured on GPIO_1:3
[00:00:00.119,171] <dbg> winc1500.nm_bus_init: NOTICE:DONE
[00:00:00.259,552] <dbg> wifi_winc1500.winc1500_init: WINC1500 MAC Address from OTP (1) f8:f0:05:d0:de:8c
[00:00:00.284,118] <err> wifi_winc1500: Failed set scan region
[00:00:00.308,593] <err> wifi_winc1500: Failed set power profile
[00:00:00.333,312] <err> wifi_winc1500: Failed set tx power
[00:00:00.333,435] <dbg> wifi_winc1500.winc1500_init: WINC1500 driver Initialized
[00:00:00.333,587] <dbg> wifi_winc1500.winc1500_iface_init: eth_init:net_if_set_link_addr:MAC Address f8:f0:05:d0:de:8c

I pushed my brunch with the changes: https://github.com/jfischer-phytec-iot/zephyr/tree/gpio-test-winc1500. At least it should be verified that this driver builds.