Skip to content

[TOPIC GPIO] Adapt winc1500 wifi driver to new API #22156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[TOPIC GPIO] Adapt winc1500 wifi driver to new API #22156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 13 additions & 18 deletions drivers/wifi/winc1500/wifi_winc1500_nm_bsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ void (*isr_function)(void);

static inline void chip_isr(struct device *port,
struct gpio_callback *cb,
uint32_t pins)
gpio_port_pins_t pins)
{
if (isr_function) {
isr_function();
Expand All @@ -42,18 +42,18 @@ s8_t nm_bsp_deinit(void)

void nm_bsp_reset(void)
{
gpio_pin_write(winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].pin, 0);
gpio_pin_write(winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].dev,
winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].pin, 0);
gpio_pin_set_raw(winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].pin, 0);
gpio_pin_set_raw(winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].dev,
winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].pin, 0);
nm_bsp_sleep(100);

gpio_pin_write(winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].pin, 1);
gpio_pin_set_raw(winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_CHIP_EN].pin, 1);
nm_bsp_sleep(10);

gpio_pin_write(winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].dev,
winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].pin, 1);
gpio_pin_set_raw(winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].dev,
winc1500.gpios[WINC1500_GPIO_IDX_RESET_N].pin, 1);
nm_bsp_sleep(10);
}

Expand All @@ -76,13 +76,8 @@ void nm_bsp_register_isr(void (*isr_fun)(void))

void nm_bsp_interrupt_ctrl(u8_t enable)
{
if (enable) {
gpio_pin_enable_callback(
winc1500.gpios[WINC1500_GPIO_IDX_IRQN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_IRQN].pin);
} else {
gpio_pin_disable_callback(
winc1500.gpios[WINC1500_GPIO_IDX_IRQN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_IRQN].pin);
}
gpio_pin_interrupt_configure(
winc1500.gpios[WINC1500_GPIO_IDX_IRQN].dev,
winc1500.gpios[WINC1500_GPIO_IDX_IRQN].pin,
enable ? GPIO_INT_EDGE_TO_ACTIVE : GPIO_INT_DISABLE);
}
15 changes: 13 additions & 2 deletions drivers/wifi/winc1500/wifi_winc1500_nm_bsp_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand All @@ -23,6 +21,19 @@ extern tstrNmBusCapabilities egstrNmBusCapabilities;
#define NM_DEBUG CONF_WINC_DEBUG
#define NM_BSP_PRINTF CONF_WINC_PRINTF

enum winc1500_gpio_index {
WINC1500_GPIO_IDX_CHIP_EN = 0,
WINC1500_GPIO_IDX_IRQN,
WINC1500_GPIO_IDX_RESET_N,
Copy link
Member

@mnkp mnkp Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tbursztyka tbursztyka Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pabigot pabigot Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mnkp mnkp Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pabigot pabigot Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


WINC1500_GPIO_IDX_MAX
};

struct winc1500_gpio_configuration {
struct device *dev;
u32_t pin;
Copy link
Member

@mnkp mnkp Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tbursztyka tbursztyka Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pabigot pabigot Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mnkp mnkp Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

struct winc1500_device {
struct winc1500_gpio_configuration *gpios;
struct gpio_callback gpio_cb;
Expand Down
38 changes: 38 additions & 0 deletions drivers/wifi/winc1500/wifi_winc1500_nm_bus_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ LOG_MODULE_REGISTER(winc1500);

#include "wifi_winc1500_config.h"

static
struct winc1500_gpio_configuration winc1500_gpios[WINC1500_GPIO_IDX_MAX] = {
{ .dev = NULL, .pin = DT_INST_0_ATMEL_WINC1500_ENABLE_GPIOS_PIN },
{ .dev = NULL, .pin = DT_INST_0_ATMEL_WINC1500_IRQ_GPIOS_PIN },
{ .dev = NULL, .pin = DT_INST_0_ATMEL_WINC1500_RESET_GPIOS_PIN },
};

#define NM_BUS_MAX_TRX_SZ 256

tstrNmBusCapabilities egstrNmBusCapabilities = {
Expand Down Expand Up @@ -87,6 +94,37 @@ static s8_t spi_rw(u8_t *mosi, u8_t *miso, u16_t size)

#endif

struct winc1500_gpio_configuration *winc1500_configure_gpios(void)
{
struct device *gpio_en, *gpio_irq, *gpio_reset;

gpio_en = device_get_binding(
DT_INST_0_ATMEL_WINC1500_ENABLE_GPIOS_CONTROLLER);
gpio_irq = device_get_binding(
DT_INST_0_ATMEL_WINC1500_IRQ_GPIOS_CONTROLLER);
gpio_reset = device_get_binding(
DT_INST_0_ATMEL_WINC1500_RESET_GPIOS_CONTROLLER);

gpio_pin_configure(gpio_en,
winc1500_gpios[WINC1500_GPIO_IDX_CHIP_EN].pin,
GPIO_OUTPUT |
Copy link
Contributor

@pabigot pabigot Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tbursztyka tbursztyka Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@pabigot pabigot Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mnkp mnkp Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DT_INST_0_ATMEL_WINC1500_ENABLE_GPIOS_FLAGS);
gpio_pin_configure(gpio_irq,
winc1500_gpios[WINC1500_GPIO_IDX_IRQN].pin,
GPIO_INPUT |
DT_INST_0_ATMEL_WINC1500_IRQ_GPIOS_FLAGS);
gpio_pin_configure(gpio_reset,
winc1500_gpios[WINC1500_GPIO_IDX_RESET_N].pin,
GPIO_OUTPUT |
DT_INST_0_ATMEL_WINC1500_RESET_GPIOS_FLAGS);

winc1500_gpios[WINC1500_GPIO_IDX_CHIP_EN].dev = gpio_en;
winc1500_gpios[WINC1500_GPIO_IDX_IRQN].dev = gpio_irq;
winc1500_gpios[WINC1500_GPIO_IDX_RESET_N].dev = gpio_reset;

return winc1500_gpios;
}

s8_t nm_bus_init(void *pvinit)
{
/* configure GPIOs */
Expand Down
27 changes: 0 additions & 27 deletions include/drivers/wifi/winc1500.h
View file
Open in desktop

This file was deleted.

44 changes: 0 additions & 44 deletions samples/net/wifi/src/wifi_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,50 +7,6 @@
#include <zephyr.h>
#include <errno.h>

#ifdef CONFIG_WIFI_WINC1500

#include <device.h>
#include <drivers/gpio.h>
#include <drivers/wifi/winc1500.h>

static
struct winc1500_gpio_configuration winc1500_gpios[WINC1500_GPIO_IDX_MAX] = {
{ .dev = NULL, .pin = DT_INST_0_ATMEL_WINC1500_ENABLE_GPIOS_PIN },
{ .dev = NULL, .pin = DT_INST_0_ATMEL_WINC1500_IRQ_GPIOS_PIN },
{ .dev = NULL, .pin = DT_INST_0_ATMEL_WINC1500_RESET_GPIOS_PIN },
};

struct winc1500_gpio_configuration *winc1500_configure_gpios(void)
{
const int flags_int_in = (GPIO_DIR_IN | GPIO_INT |
GPIO_INT_ACTIVE_LOW | GPIO_INT_DEBOUNCE |
GPIO_INT_EDGE);
const int flags_noint_out = GPIO_DIR_OUT;
struct device *gpio_en, *gpio_irq, *gpio_reset;

gpio_en = device_get_binding(DT_INST_0_ATMEL_WINC1500_ENABLE_GPIOS_CONTROLLER);
gpio_irq = device_get_binding(DT_INST_0_ATMEL_WINC1500_IRQ_GPIOS_CONTROLLER);
gpio_reset = device_get_binding(DT_INST_0_ATMEL_WINC1500_RESET_GPIOS_CONTROLLER);

gpio_pin_configure(gpio_en,
winc1500_gpios[WINC1500_GPIO_IDX_CHIP_EN].pin,
flags_noint_out);
gpio_pin_configure(gpio_irq,
winc1500_gpios[WINC1500_GPIO_IDX_IRQN].pin,
flags_int_in);
gpio_pin_configure(gpio_reset,
winc1500_gpios[WINC1500_GPIO_IDX_RESET_N].pin,
flags_noint_out);

winc1500_gpios[WINC1500_GPIO_IDX_CHIP_EN].dev = gpio_en;
winc1500_gpios[WINC1500_GPIO_IDX_IRQN].dev = gpio_irq;
winc1500_gpios[WINC1500_GPIO_IDX_RESET_N].dev = gpio_reset;

return winc1500_gpios;
}

#endif /* CONFIG_WIFI_WINC1500 */

void main(void)
{

Expand Down