Skip to content

rework GPIO configuration flags #11880

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 7 commits into from
Closed

rework GPIO configuration flags #11880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed85ff7
drivers: gpio: fix misuse of u8_t where bool is intended
pabigot Dec 5, 2018
c3d8936
dt-bindings: gpio: revise flags to support more complete control
pabigot Dec 5, 2018
c85f776
dt-bindings: gpio: revise drive strength flags for generality
pabigot Dec 11, 2018
d5ade36
drivers: gpio: sx1509b: remove ability to configure by port
pabigot Dec 10, 2018
13ff5f8
drivers: gpio: sx1509b: transition to new GPIO configuration
pabigot Dec 11, 2018
e0e3d38
drivers: gpio: esp32: correct constant test for interrupt trigger
pabigot Jan 26, 2019
241f9b4
tests: gpio: basic_api: correct constant test for interrupt trigger
pabigot Jan 26, 2019
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
2 changes: 1 addition & 1 deletion drivers/gpio/gpio_cc2650.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ static int gpio_cc2650_config_pin(int pin, int flags)
if (flags & GPIO_DS_DISCONNECT_LOW) {
disconnect(pin, &gpio_doe31_0_config, &iocfg_config);
}
if (flags & GPIO_DS_ALT_LOW) {
if ((flags & GPIO_DS_LOW_MASK) == GPIO_DS_HI_LOW) {
iocfg_config |= CC2650_IOC_MAX_DRIVE_STRENGTH;
} else {
iocfg_config |= CC2650_IOC_MIN_DRIVE_STRENGTH;
Expand Down
2 changes: 1 addition & 1 deletion drivers/gpio/gpio_dw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ static inline void dw_interrupt_config(struct device *port, int access_op,
struct gpio_dw_runtime *context = port->driver_data;
const struct gpio_dw_config *config = port->config->config_info;
u32_t base_addr = dw_base_to_block_base(context->base_addr);
u8_t flag_is_set;
bool flag_is_set;

ARG_UNUSED(access_op);

Expand Down
4 changes: 2 additions & 2 deletions drivers/gpio/gpio_esp32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ static int convert_int_type(int flags)
return 0; /* Disables interrupt for a pin. */
}

if ((flags & GPIO_INT_EDGE) == GPIO_INT_EDGE) {
if ((flags & GPIO_INT_TRIGGER_MASK) == GPIO_INT_EDGE) {
if ((flags & GPIO_INT_ACTIVE_HIGH) == GPIO_INT_ACTIVE_HIGH) {
return 1;
}
Expand All @@ -64,7 +64,7 @@ static int convert_int_type(int flags)
return 2; /* Defaults to falling edge. */
}

if ((flags & GPIO_INT_LEVEL) == GPIO_INT_LEVEL) {
if ((flags & GPIO_INT_TRIGGER_MASK) == GPIO_INT_LEVEL) {
if ((flags & GPIO_INT_ACTIVE_HIGH) == GPIO_INT_ACTIVE_HIGH) {
return 5;
}
Expand Down
24 changes: 16 additions & 8 deletions drivers/gpio/gpio_nrfx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,31 +134,39 @@ static int gpio_nrfx_config(struct device *port, int access_op,
u8_t from_pin;
u8_t to_pin;

/* Map default selections to Nordic standard (low) */
if ((flags & GPIO_DS_LOW_MASK) == GPIO_DS_DFLT_LOW) {
flags |= GPIO_DS_LO_LOW;
}
if ((flags & GPIO_DS_HIGH_MASK) == GPIO_DS_DFLT_HIGH) {
flags |= GPIO_DS_LO_HIGH;
}

switch (flags & (GPIO_DS_LOW_MASK | GPIO_DS_HIGH_MASK)) {
case GPIO_DS_DFLT_LOW | GPIO_DS_DFLT_HIGH:
case GPIO_DS_LO_LOW | GPIO_DS_LO_HIGH:
drive = NRF_GPIO_PIN_S0S1;
break;
case GPIO_DS_DFLT_LOW | GPIO_DS_ALT_HIGH:
case GPIO_DS_LO_LOW | GPIO_DS_HI_HIGH:
drive = NRF_GPIO_PIN_S0H1;
break;
case GPIO_DS_DFLT_LOW | GPIO_DS_DISCONNECT_HIGH:
case GPIO_DS_LO_LOW | GPIO_DS_DISCONNECT_HIGH:
drive = NRF_GPIO_PIN_S0D1;
break;

case GPIO_DS_ALT_LOW | GPIO_DS_DFLT_HIGH:
case GPIO_DS_HI_LOW | GPIO_DS_LO_HIGH:
drive = NRF_GPIO_PIN_H0S1;
break;
case GPIO_DS_ALT_LOW | GPIO_DS_ALT_HIGH:
case GPIO_DS_HI_LOW | GPIO_DS_HI_HIGH:
drive = NRF_GPIO_PIN_H0H1;
break;
case GPIO_DS_ALT_LOW | GPIO_DS_DISCONNECT_HIGH:
case GPIO_DS_HI_LOW | GPIO_DS_DISCONNECT_HIGH:
drive = NRF_GPIO_PIN_H0D1;
break;

case GPIO_DS_DISCONNECT_LOW | GPIO_DS_DFLT_HIGH:
case GPIO_DS_DISCONNECT_LOW | GPIO_DS_LO_HIGH:
drive = NRF_GPIO_PIN_D0S1;
break;
case GPIO_DS_DISCONNECT_LOW | GPIO_DS_ALT_HIGH:
case GPIO_DS_DISCONNECT_LOW | GPIO_DS_HI_HIGH:
drive = NRF_GPIO_PIN_D0H1;
break;

Expand Down
2 changes: 1 addition & 1 deletion drivers/gpio/gpio_qmsi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ static void gpio_qmsi_callback(void *data, u32_t status)
}
}

static void qmsi_write_bit(u32_t *target, u8_t bit, u8_t value)
static void qmsi_write_bit(u32_t *target, u8_t bit, bool value)
{
if (value) {
sys_set_bit((uintptr_t) target, bit);
Expand Down
2 changes: 1 addition & 1 deletion drivers/gpio/gpio_qmsi_ss.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ static void ss_gpio_qmsi_callback(void *data, uint32_t status)
}
}

static void ss_qmsi_write_bit(u32_t *target, u8_t bit, u8_t value)
static void ss_qmsi_write_bit(u32_t *target, u8_t bit, bool value)
{
if (value) {
sys_set_bit((uintptr_t) target, bit);
Expand Down
210 changes: 102 additions & 108 deletions drivers/gpio/gpio_sx1509b.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@
#include <misc/byteorder.h>
#include <misc/util.h>

/** Cache of the output configuration and data of the pins */
/** Cache of the output configuration and data of the pins. */
struct gpio_sx1509b_pin_state {
u16_t input_disable;
u16_t pull_up;
u16_t pull_down;
u16_t open_drain;
u16_t polarity;
u16_t dir;
u16_t data;
u16_t input_disable; /* 0x00 */
u16_t long_slew; /* 0x02 */
u16_t low_drive; /* 0x04 */
u16_t pull_up; /* 0x06 */
u16_t pull_down; /* 0x08 */
u16_t open_drain; /* 0x0A */
u16_t polarity; /* 0x0C */
u16_t dir; /* 0x0E */
u16_t data; /* 0x10 */
};

/** Runtime driver data */
Expand Down Expand Up @@ -89,12 +91,12 @@ static inline int i2c_reg_write_word_be(struct device *dev, u16_t dev_addr,
}

/**
* @brief Configure pin or port
* @brief Configure pin (port not supported)
*
* @param dev Device struct of the SX1509B
* @param access_op Access operation (pin or port)
* @param access_op Access operation (pin only, port not supported)
* @param pin The pin number
* @param flags Flags of pin or port
* @param flags Flags of pin
*
* @return 0 if successful, failed otherwise
*/
Expand All @@ -104,112 +106,108 @@ static int gpio_sx1509b_config(struct device *dev, int access_op, u32_t pin,
const struct gpio_sx1509b_config *cfg = dev->config->config_info;
struct gpio_sx1509b_drv_data *drv_data = dev->driver_data;
struct gpio_sx1509b_pin_state *pins = &drv_data->pin_state;
struct {
u8_t reg;
struct gpio_sx1509b_pin_state pins;
} __packed outbuf;
int ret = 0;
u16_t bit = BIT(pin);
Copy link
Member

@anangl anangl Jan 7, 2019

Choose a reason for hiding this comment

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

Have I missed something or would replacing it with the following:

u16_t bits = ((access_op == GPIO_ACCESS_BY_PORT) ? 0xFFFF : BIT(pin));

allow you to keep the ability to configure entire port at once (although such feature does not seem to have many use cases, indeed)?

Copy link
Contributor Author

@pabigot pabigot Jan 7, 2019

Choose a reason for hiding this comment

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

In fact configuring the entire port at once is very important, it just can't be done within the existing Zephyr GPIO API, which apparently is designed to configure every pin on the port to the same configuration. Which is indeed useless in most cases and motivated removing the port-level API from this driver in an earlier commit so the rest of this would be simpler.

I've got a more significant update to the SX1509B driver pending, that includes configuring all pins in one go (used in nrf52_pca20020 startup), changing multiple pin values in one transaction, and LED driver support. It's part of what's waiting for this to be merged.

bool data_first = false;

if (access_op != GPIO_ACCESS_BY_PIN) {
return -ENOTSUP;
}

if (flags & GPIO_INT) {
if (flags & GPIO_INT_ENABLE) {
return -ENOTSUP;
}

k_sem_take(&drv_data->lock, K_FOREVER);

switch (access_op) {
case GPIO_ACCESS_BY_PIN:
if ((flags & GPIO_DIR_MASK) == GPIO_DIR_IN) {
pins->dir |= BIT(pin);
pins->input_disable &= ~BIT(pin);
} else {
pins->dir &= ~BIT(pin);
pins->input_disable |= BIT(pin);
}
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_UP) {
pins->pull_up |= BIT(pin);
} else {
pins->pull_up &= ~BIT(pin);
}
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_DOWN) {
pins->pull_down |= BIT(pin);
} else {
pins->pull_down &= ~BIT(pin);
}
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_NORMAL) {
pins->pull_up &= ~BIT(pin);
pins->pull_down &= ~BIT(pin);
}
if (flags & GPIO_DS_DISCONNECT_HIGH) {
pins->open_drain |= BIT(pin);
} else {
pins->open_drain &= ~BIT(pin);
}
if (flags & GPIO_POL_INV) {
pins->polarity |= BIT(pin);
} else {
pins->polarity &= ~BIT(pin);
}
break;
case GPIO_ACCESS_BY_PORT:
if ((flags & GPIO_DIR_MASK) == GPIO_DIR_IN) {
pins->dir = 0xffff;
pins->input_disable = 0x0000;
} else {
pins->dir = 0x0000;
pins->input_disable = 0xffff;
}
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_UP) {
pins->pull_up = 0xffff;
} else {
pins->pull_up = 0x0000;
}
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_DOWN) {
pins->pull_down = 0xffff;
} else {
pins->pull_down = 0x0000;
}
if ((flags & GPIO_PUD_MASK) == GPIO_PUD_NORMAL) {
pins->pull_up = 0x0000;
pins->pull_down = 0x0000;
}
if (flags & GPIO_DS_DISCONNECT_HIGH) {
pins->open_drain = 0xffff;
} else {
pins->open_drain = 0x0000;
}
if (flags & GPIO_POL_INV) {
pins->polarity = 0xffff;
} else {
pins->polarity = 0x0000;
}
break;
if ((flags & GPIO_OPEN_MASK) == GPIO_OPEN_DRAIN) {
pins->open_drain |= bit;
} else {
pins->open_drain &= ~bit;
}

switch (flags & GPIO_PUD_MASK) {
default:
ret = -ENOTSUP;
ret = -EINVAL;
goto out;
case GPIO_PUD_NORMAL:
pins->pull_up &= ~bit;
pins->pull_down &= ~bit;
break;
case GPIO_PUD_PULL_UP:
pins->pull_up |= bit;
pins->pull_down &= ~bit;
break;
case GPIO_PUD_PULL_DOWN:
pins->pull_up &= ~bit;
pins->pull_down |= bit;
break;
}

ret = i2c_reg_write_word_be(drv_data->i2c_master, cfg->i2c_slave_addr,
SX1509B_REG_DIR, pins->dir);
if (ret)
goto out;
if (flags & GPIO_IO_IN_ENABLED) {
pins->input_disable &= ~bit;
} else {
pins->input_disable |= bit;
}

ret = i2c_reg_write_word_be(drv_data->i2c_master, cfg->i2c_slave_addr,
SX1509B_REG_INPUT_DISABLE,
pins->input_disable);
if (ret)
goto out;
if (flags & GPIO_IO_OUT_ENABLED) {
pins->dir &= ~bit;
} else {
pins->dir |= bit;
}

ret = i2c_reg_write_word_be(drv_data->i2c_master, cfg->i2c_slave_addr,
SX1509B_REG_PULL_UP,
pins->pull_up);
if (ret)
goto out;
if (flags & GPIO_IO_INIT_ENABLED) {
data_first = true;
if ((flags & GPIO_IO_INIT_MASK) == GPIO_IO_INIT_HIGH) {
pins->data |= bit;
} else {
pins->data &= ~bit;
}
}

ret = i2c_reg_write_word_be(drv_data->i2c_master, cfg->i2c_slave_addr,
SX1509B_REG_PULL_DOWN,
pins->pull_down);
if (ret)
goto out;
if (flags & GPIO_POL_INV) {
pins->polarity |= bit;
} else {
pins->polarity &= ~bit;
}

ret = i2c_reg_write_word_be(drv_data->i2c_master, cfg->i2c_slave_addr,
SX1509B_REG_OPEN_DRAIN,
pins->open_drain);
if (flags & GPIO_DS_LO_LOW) {
pins->low_drive |= bit;
} else {
pins->low_drive &= ~bit;
}

outbuf.reg = SX1509B_REG_INPUT_DISABLE;
outbuf.pins.input_disable = sys_cpu_to_be16(pins->input_disable);
outbuf.pins.long_slew = sys_cpu_to_be16(pins->long_slew);
outbuf.pins.low_drive = sys_cpu_to_be16(pins->low_drive);
outbuf.pins.pull_up = sys_cpu_to_be16(pins->pull_up);
outbuf.pins.pull_down = sys_cpu_to_be16(pins->pull_down);
outbuf.pins.open_drain = sys_cpu_to_be16(pins->open_drain);
outbuf.pins.polarity = sys_cpu_to_be16(pins->polarity);
outbuf.pins.dir = sys_cpu_to_be16(pins->dir);
outbuf.pins.data = sys_cpu_to_be16(pins->data);

if (data_first) {
ret = i2c_reg_write_word_be(drv_data->i2c_master,
cfg->i2c_slave_addr,
SX1509B_REG_DATA, pins->data);
if (ret == 0) {
ret = i2c_write(drv_data->i2c_master,
&outbuf.reg,
sizeof(outbuf) - sizeof(pins->data),
cfg->i2c_slave_addr);
}
} else {
ret = i2c_write(drv_data->i2c_master,
&outbuf.reg,
sizeof(outbuf),
cfg->i2c_slave_addr);
}

out:
k_sem_give(&drv_data->lock);
Expand Down Expand Up @@ -323,12 +321,8 @@ static int gpio_sx1509b_init(struct device *dev)

/* Reset state */
drv_data->pin_state = (struct gpio_sx1509b_pin_state) {
.input_disable = 0x0000,
.pull_up = 0x0000,
.pull_down = 0x0000,
.open_drain = 0x0000,
.dir = 0xffff,
.data = 0xffff,
.dir = -1,
.data = -1,
};

ret = i2c_reg_write_byte(drv_data->i2c_master, cfg->i2c_slave_addr,
Expand Down
Loading