Skip to content

[TOPIC-GPIO] gpio: Update rv32m1 gpio driver to use new gpio api #19664

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

Merged
merged 3 commits into from
Oct 22, 2019
Merged

[TOPIC-GPIO] gpio: Update rv32m1 gpio driver to use new gpio api #19664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c636636
boards: rv32m1_vega: Configure led pinmuxes as gpios
MaureenHelm Oct 7, 2019
6b03b68
gpio: Update rv32m1 gpio driver to use new gpio api
MaureenHelm Oct 7, 2019
9100d59
tests: gpio: Enable gpio_basic_api test on rv32m1_vega_ri5cy
MaureenHelm Oct 7, 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
5 changes: 5 additions & 0 deletions boards/riscv/rv32m1_vega/pinmux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ static int rv32m1_vega_pinmux_init(struct device *dev)
pinmux_pin_set(porte, 22, PORT_PCR_MUX(kPORT_MuxAsGpio));
pinmux_pin_set(porte, 27, PORT_PCR_MUX(kPORT_MuxAsGpio));

/* RGB LEDs */
pinmux_pin_set(porta, 22, PORT_PCR_MUX(kPORT_MuxAsGpio));
pinmux_pin_set(porta, 23, PORT_PCR_MUX(kPORT_MuxAsGpio));
pinmux_pin_set(porta, 24, PORT_PCR_MUX(kPORT_MuxAsGpio));

return 0;
}

Expand Down
16 changes: 8 additions & 8 deletions boards/riscv/rv32m1_vega/rv32m1_vega.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,19 @@
leds {
compatible = "gpio-leds";
blue_led: led_0 {
gpios = <&gpioa 22 0>;
gpios = <&gpioa 22 GPIO_ACTIVE_HIGH>;
label = "User LD1";
};
green_led: led_1 {
gpios = <&gpioa 23 0>;
gpios = <&gpioa 23 GPIO_ACTIVE_HIGH>;
label = "User LD2";
};
red_led: led_2 {
gpios = <&gpioa 24 0>;
gpios = <&gpioa 24 GPIO_ACTIVE_HIGH>;
label = "User LD3";
};
sts_led: led_3 {
gpios = <&gpioe 0 0>;
gpios = <&gpioe 0 GPIO_ACTIVE_HIGH>;
label = "User LD4";
};
};
Expand All @@ -39,19 +39,19 @@
compatible = "gpio-keys";
user_button_2: button_0 {
label = "User SW2";
gpios = <&gpioa 0 GPIO_INT_ACTIVE_LOW>;
gpios = <&gpioa 0 GPIO_ACTIVE_LOW>;
};
user_button_3: button_1 {
label = "User SW3";
gpios = <&gpioe 8 GPIO_INT_ACTIVE_LOW>;
gpios = <&gpioe 8 GPIO_ACTIVE_LOW>;
};
user_button_4: button_2 {
label = "User SW4";
gpios = <&gpioe 9 GPIO_INT_ACTIVE_LOW>;
gpios = <&gpioe 9 GPIO_ACTIVE_LOW>;
};
user_button_5: button_3 {
label = "User SW5";
gpios = <&gpioe 12 GPIO_INT_ACTIVE_LOW>;
gpios = <&gpioe 12 GPIO_ACTIVE_LOW>;
};
};

Expand Down
210 changes: 168 additions & 42 deletions drivers/gpio/gpio_rv32m1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,72 @@ struct gpio_rv32m1_data {
u32_t pin_callback_enables;
};

static u32_t get_port_pcr_irqc_value_from_flags(struct device *dev,
Copy link
Member

@mnkp mnkp Oct 8, 2019

Choose a reason for hiding this comment

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

This function is not used anymore by gpio_rv32m1_configure. We could move it directly above gpio_rv32m1_pin_interrupt_configure. It would make the code easier to read.

u32_t pin, enum gpio_int_mode mode,
enum gpio_int_trig trig)
{
port_interrupt_t port_interrupt = 0;

if (mode == GPIO_INT_MODE_DISABLED) {
port_interrupt = kPORT_InterruptOrDMADisabled;
} else {
if (mode == GPIO_INT_MODE_LEVEL) {
if (trig == GPIO_INT_TRIG_LOW) {
port_interrupt = kPORT_InterruptLogicZero;
} else {
port_interrupt = kPORT_InterruptLogicOne;
}
} else {
switch (trig) {
case GPIO_INT_TRIG_LOW:
port_interrupt = kPORT_InterruptFallingEdge;
break;
case GPIO_INT_TRIG_HIGH:
port_interrupt = kPORT_InterruptRisingEdge;
break;
case GPIO_INT_TRIG_BOTH:
port_interrupt = kPORT_InterruptEitherEdge;
break;
}
}
}

return PORT_PCR_IRQC(port_interrupt);
}

static int gpio_rv32m1_configure(struct device *dev,
int access_op, u32_t pin, int flags)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
GPIO_Type *gpio_base = config->gpio_base;
PORT_Type *port_base = config->port_base;
port_interrupt_t port_interrupt = 0;
struct gpio_rv32m1_data *data = dev->driver_data;
u32_t mask = 0U;
u32_t pcr = 0U;
u8_t i;

/* Check for an invalid pin number */
if (pin >= ARRAY_SIZE(port_base->PCR)) {
return -EINVAL;
}

/* Check for an invalid pin configuration */
if ((flags & GPIO_INT) && (flags & GPIO_DIR_OUT)) {
if ((flags & GPIO_INT_ENABLE) && ((flags & GPIO_INPUT) == 0)) {
Copy link
Member

@mnkp mnkp Oct 8, 2019

Choose a reason for hiding this comment

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

We're mixing styles. To be consistent / Misra-C compliant we would need to write it as

if (((flags & GPIO_INT_ENABLE) != 0) && ((flags & GPIO_INPUT) == 0)) {

Copy link
Member

@mnkp mnkp Oct 8, 2019

Choose a reason for hiding this comment

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

Actually, I believe we should move this check to gpio_rv32m1_pin_interrupt_configure function (and read input/output configuration from the register).

return -EINVAL;
}

if (((flags & GPIO_INPUT) != 0) && ((flags & GPIO_OUTPUT) != 0)) {
return -ENOTSUP;
}

if ((flags & GPIO_SINGLE_ENDED) != 0) {
return -ENOTSUP;
}

/* Check if GPIO port supports interrupts */
if ((flags & GPIO_INT) && ((config->flags & GPIO_INT) == 0U)) {
return -EINVAL;
if ((flags & GPIO_INT_ENABLE) &&
((config->flags & GPIO_INT_ENABLE) == 0U)) {
Copy link
Member

@mnkp mnkp Oct 8, 2019

Choose a reason for hiding this comment

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

This check is already implemented in gpio_rv32m1_pin_interrupt_configure. We could skip it here.

Copy link
Member Author

@MaureenHelm MaureenHelm Oct 10, 2019

Choose a reason for hiding this comment

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

This is the same as the mcux driver. Can we postpone these changes to a future PR to keep mcux and rv32m1 in sync?

Copy link
Member

@mnkp mnkp Oct 11, 2019

Choose a reason for hiding this comment

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

OK, the code is harmless.

return -ENOTSUP;
}

/* The flags contain options that require touching registers in the
Expand All @@ -63,15 +110,25 @@ static int gpio_rv32m1_configure(struct device *dev,
*/

if (access_op == GPIO_ACCESS_BY_PIN) {
if ((flags & GPIO_DIR_MASK) == GPIO_DIR_IN) {
switch (flags & GPIO_DIR_MASK) {
case GPIO_INPUT:
gpio_base->PDDR &= ~BIT(pin);
} else { /* GPIO_DIR_OUT */
break;
case GPIO_OUTPUT:
if ((flags & GPIO_OUTPUT_INIT_HIGH) != 0) {
gpio_base->PSOR = BIT(pin);
} else if ((flags & GPIO_OUTPUT_INIT_LOW) != 0) {
gpio_base->PCOR = BIT(pin);
}
gpio_base->PDDR |= BIT(pin);
break;
default:
return -ENOTSUP;
}
} else { /* GPIO_ACCESS_BY_PORT */
if ((flags & GPIO_DIR_MASK) == GPIO_DIR_IN) {
if ((flags & GPIO_INPUT) != 0) {
gpio_base->PDDR = 0x0;
} else { /* GPIO_DIR_OUT */
} else { /* GPIO_OUTPUT */
gpio_base->PDDR = 0xFFFFFFFF;
}
}
Expand All @@ -81,11 +138,11 @@ static int gpio_rv32m1_configure(struct device *dev,
*/
mask |= PORT_PCR_PE_MASK | PORT_PCR_PS_MASK;

if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_UP) {
if ((flags & GPIO_PULL_UP) != 0) {
/* Enable the pull and select the pullup resistor. */
pcr |= PORT_PCR_PE_MASK | PORT_PCR_PS_MASK;

} else if ((flags & GPIO_PUD_MASK) == GPIO_PUD_PULL_DOWN) {
} else if ((flags & GPIO_PULL_DOWN) != 0) {
/* Enable the pull and select the pulldown resistor (deselect
* the pullup resistor.
*/
Expand All @@ -97,44 +154,27 @@ static int gpio_rv32m1_configure(struct device *dev,
*/
mask |= PORT_PCR_IRQC_MASK;
Copy link
Member

@mnkp mnkp Oct 8, 2019

Choose a reason for hiding this comment

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

Should be removed?

Copy link
Member Author

@MaureenHelm MaureenHelm Oct 10, 2019

Choose a reason for hiding this comment

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

No, it is still needed

Copy link
Member

@mnkp mnkp Oct 11, 2019

Choose a reason for hiding this comment

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

The code on line 162 will clear IRQ configuration from PCR register but we don't configure interrupts here. If we merge #19553 the gpio_rv32m1_configure would still modify interrupt configuration.


if (flags & GPIO_INT) {
if (flags & GPIO_INT_EDGE) {
if (flags & GPIO_INT_ACTIVE_HIGH) {
port_interrupt = kPORT_InterruptRisingEdge;
} else if (flags & GPIO_INT_DOUBLE_EDGE) {
port_interrupt = kPORT_InterruptEitherEdge;
} else {
port_interrupt = kPORT_InterruptFallingEdge;
}
} else { /* GPIO_INT_LEVEL */
if (flags & GPIO_INT_ACTIVE_HIGH) {
port_interrupt = kPORT_InterruptLogicOne;
} else {
port_interrupt = kPORT_InterruptLogicZero;
}
}
pcr |= PORT_PCR_IRQC(port_interrupt);
}

mask |= PORT_PCR_MUX_MASK;

/* Now we can write the PORT PCR register(s). If accessing by pin, we
* only need to write one PCR register. Otherwise, write all the PCR
* registers in the PORT module (one for each pin).
*/
if (access_op == GPIO_ACCESS_BY_PIN) {
port_base->PCR[pin] = (port_base->PCR[pin] & ~mask) | pcr |
PORT_PCR_MUX(kPORT_MuxAsGpio);
port_base->PCR[pin] = (port_base->PCR[pin] & ~mask) | pcr;
WRITE_BIT(data->pin_callback_enables, pin,
Copy link
Member

@mnkp mnkp Oct 8, 2019

Choose a reason for hiding this comment

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

This is handled by gpio_rv32m1_pin_interrupt_configure function.

flags & GPIO_INT_ENABLE);
} else { /* GPIO_ACCESS_BY_PORT */
for (i = 0U; i < ARRAY_SIZE(port_base->PCR); i++) {
port_base->PCR[i] = (port_base->PCR[pin] & ~mask) | pcr
| PORT_PCR_MUX(kPORT_MuxAsGpio);
port_base->PCR[i] = (port_base->PCR[pin] & ~mask) | pcr;
}
if (flags & GPIO_INT_ENABLE) {
data->pin_callback_enables = 0xFFFFFFFF;
} else {
data->pin_callback_enables = 0x0;
}
}

return 0;
}

static int gpio_rv32m1_write(struct device *dev,
int access_op, u32_t pin, u32_t value)
{
Expand Down Expand Up @@ -180,6 +220,86 @@ static int gpio_rv32m1_read(struct device *dev,
return 0;
}

static int gpio_rv32m1_port_get_raw(struct device *dev, u32_t *value)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
GPIO_Type *gpio_base = config->gpio_base;

*value = gpio_base->PDIR;

return 0;
}

static int gpio_rv32m1_port_set_masked_raw(struct device *dev, u32_t mask,
u32_t value)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
GPIO_Type *gpio_base = config->gpio_base;

gpio_base->PDOR = (gpio_base->PDOR & ~mask) | (mask & value);

return 0;
}

static int gpio_rv32m1_port_set_bits_raw(struct device *dev, u32_t mask)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
GPIO_Type *gpio_base = config->gpio_base;

gpio_base->PSOR = mask;

return 0;
}

static int gpio_rv32m1_port_clear_bits_raw(struct device *dev, u32_t mask)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
GPIO_Type *gpio_base = config->gpio_base;

gpio_base->PCOR = mask;

return 0;
}

static int gpio_rv32m1_port_toggle_bits(struct device *dev, u32_t mask)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
GPIO_Type *gpio_base = config->gpio_base;

gpio_base->PTOR = mask;

return 0;
}

static int gpio_rv32m1_pin_interrupt_configure(struct device *dev,
unsigned int pin, enum gpio_int_mode mode,
enum gpio_int_trig trig)
{
const struct gpio_rv32m1_config *config = dev->config->config_info;
PORT_Type *port_base = config->port_base;
struct gpio_rv32m1_data *data = dev->driver_data;

/* Check for an invalid pin number */
if (pin >= ARRAY_SIZE(port_base->PCR)) {
return -EINVAL;
}

/* Check if GPIO port supports interrupts */
if ((mode != GPIO_INT_MODE_DISABLED) &&
((config->flags & GPIO_INT_ENABLE) == 0U)) {
return -ENOTSUP;
}

u32_t pcr = get_port_pcr_irqc_value_from_flags(dev, pin, mode, trig);

port_base->PCR[pin] = (port_base->PCR[pin] & ~PORT_PCR_IRQC_MASK) | pcr;

WRITE_BIT(data->pin_callback_enables, pin, mode != GPIO_INT_DISABLE);

return 0;
}


static int gpio_rv32m1_manage_callback(struct device *dev,
struct gpio_callback *callback, bool set)
{
Expand Down Expand Up @@ -231,7 +351,7 @@ static void gpio_rv32m1_port_isr(void *arg)
gpio_fire_callbacks(&data->callbacks, dev, enabled_int);

/* Clear the port interrupts */
config->port_base->ISFR = 0xFFFFFFFF;
config->port_base->ISFR = enabled_int;
}

static int gpio_rv32m1_init(struct device *dev)
Expand Down Expand Up @@ -260,6 +380,12 @@ static const struct gpio_driver_api gpio_rv32m1_driver_api = {
.config = gpio_rv32m1_configure,
.write = gpio_rv32m1_write,
.read = gpio_rv32m1_read,
.port_get_raw = gpio_rv32m1_port_get_raw,
.port_set_masked_raw = gpio_rv32m1_port_set_masked_raw,
.port_set_bits_raw = gpio_rv32m1_port_set_bits_raw,
.port_clear_bits_raw = gpio_rv32m1_port_clear_bits_raw,
.port_toggle_bits = gpio_rv32m1_port_toggle_bits,
.pin_interrupt_configure = gpio_rv32m1_pin_interrupt_configure,
.manage_callback = gpio_rv32m1_manage_callback,
.enable_callback = gpio_rv32m1_enable_callback,
.disable_callback = gpio_rv32m1_disable_callback,
Expand All @@ -272,7 +398,7 @@ static const struct gpio_rv32m1_config gpio_rv32m1_porta_config = {
.gpio_base = (GPIO_Type *) DT_OPENISA_RV32M1_GPIO_GPIO_A_BASE_ADDRESS,
.port_base = PORTA,
#ifdef DT_OPENISA_RV32M1_GPIO_GPIO_A_IRQ_0
.flags = GPIO_INT,
.flags = GPIO_INT_ENABLE,
#else
.flags = 0,
#endif
Expand Down Expand Up @@ -317,7 +443,7 @@ static const struct gpio_rv32m1_config gpio_rv32m1_portb_config = {
.gpio_base = (GPIO_Type *) DT_OPENISA_RV32M1_GPIO_GPIO_B_BASE_ADDRESS,
.port_base = PORTB,
#ifdef DT_OPENISA_RV32M1_GPIO_GPIO_B_IRQ_0
.flags = GPIO_INT,
.flags = GPIO_INT_ENABLE,
#else
.flags = 0,
#endif
Expand Down Expand Up @@ -362,7 +488,7 @@ static const struct gpio_rv32m1_config gpio_rv32m1_portc_config = {
.gpio_base = (GPIO_Type *) DT_OPENISA_RV32M1_GPIO_GPIO_C_BASE_ADDRESS,
.port_base = PORTC,
#ifdef DT_OPENISA_RV32M1_GPIO_GPIO_C_IRQ_0
.flags = GPIO_INT,
.flags = GPIO_INT_ENABLE,
#else
.flags = 0,
#endif
Expand Down Expand Up @@ -408,7 +534,7 @@ static const struct gpio_rv32m1_config gpio_rv32m1_portd_config = {
.gpio_base = (GPIO_Type *) DT_OPENISA_RV32M1_GPIO_GPIO_D_BASE_ADDRESS,
.port_base = PORTD,
#ifdef DT_OPENISA_RV32M1_GPIO_GPIO_D_IRQ_0
.flags = GPIO_INT,
.flags = GPIO_INT_ENABLE,
#else
.flags = 0,
#endif
Expand Down Expand Up @@ -453,7 +579,7 @@ static const struct gpio_rv32m1_config gpio_rv32m1_porte_config = {
.gpio_base = (GPIO_Type *) DT_OPENISA_RV32M1_GPIO_GPIO_E_BASE_ADDRESS,
.port_base = PORTE,
#ifdef DT_OPENISA_RV32M1_GPIO_GPIO_E_IRQ_0
.flags = GPIO_INT,
.flags = GPIO_INT_ENABLE,
#else
.flags = 0,
#endif
Expand Down
Loading