Skip to content

[TOPIC-GPIO] drivers: ieee802154_rf2xx: convert to the new GPIO API #22214

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 1 commit into from
Jan 28, 2020
Merged

[TOPIC-GPIO] drivers: ieee802154_rf2xx: convert to the new GPIO API #22214

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
8 changes: 4 additions & 4 deletions boards/arm/atsamr21_xpro/atsamr21_xpro.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,10 @@
reg = <0x0>;
label = "RF2XX_0";
spi-max-frequency = <8000000>;
irq-gpios = <&portb 0 0>;
reset-gpios = <&portb 15 0>;
slptr-gpios = <&porta 20 0>;
dig2-gpios = <&portb 17 0>;
irq-gpios = <&portb 0 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>;
Copy link
Member Author

@mnkp mnkp Jan 26, 2020

Choose a reason for hiding this comment

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

The RF2XX data sheet doesn't mention anything about not driving output pins, like IRQ, in sleep state. It seems like there is no need to configure pin pull downs. Nevertheless, I preserved the existing configuration since I don't have a hardware to test.

Copy link
Member

@nandojve nandojve Jan 27, 2020

Choose a reason for hiding this comment

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

Yes, is not clear for sure. The section 1.3 Digital Pins we got about DIGx pins. For IRQ pin, since entering on the TRX_OFF state from P_ON, SLEEP, DEEP_SLEEP or RESET state is indicated by interrupt IRQ_4 (AWAKE_END), if enabled, we can assume this pin have some level at sleep. The major problem with IRQ is user configurable and can be level High or level Low. Today I'm setting the default (level High). The MCLK is low if disabled.

The most important are RESET and SLPTR. This pins MUST NOT float any time otherwise radio will be at unstable state.

Today radio don't have any Power Manage functions implemented and all this can be revised in future. For the moment, I would keep same values.

reset-gpios = <&portb 15 GPIO_ACTIVE_LOW>;
slptr-gpios = <&porta 20 GPIO_ACTIVE_HIGH>;
dig2-gpios = <&portb 17 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>;
status = "okay";
};
};
Expand Down
29 changes: 16 additions & 13 deletions drivers/ieee802154/ieee802154_rf2xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ static int rf2xx_start(struct device *dev)
struct rf2xx_context *ctx = dev->driver_data;

k_mutex_lock(&ctx->phy_mutex, K_FOREVER);
gpio_pin_enable_callback(ctx->irq_gpio, conf->irq.pin);
gpio_pin_interrupt_configure(ctx->irq_gpio, conf->irq.pin,
GPIO_INT_EDGE_TO_ACTIVE);
rf2xx_trx_set_rx_state(dev);
k_mutex_unlock(&ctx->phy_mutex);

Expand All @@ -468,7 +469,8 @@ static int rf2xx_stop(struct device *dev)
struct rf2xx_context *ctx = dev->driver_data;

k_mutex_lock(&ctx->phy_mutex, K_FOREVER);
gpio_pin_disable_callback(ctx->irq_gpio, conf->irq.pin);
gpio_pin_interrupt_configure(ctx->irq_gpio, conf->irq.pin,
GPIO_INT_DISABLE);
rf2xx_trx_set_state(dev, RF2XX_TRX_PHY_STATE_CMD_TRX_OFF);
k_mutex_unlock(&ctx->phy_mutex);

Expand Down Expand Up @@ -569,9 +571,10 @@ static inline int configure_gpios(struct device *dev)
conf->irq.devname);
return -EINVAL;
}
gpio_pin_configure(ctx->irq_gpio, conf->irq.pin,
GPIO_DIR_IN | GPIO_INT | GPIO_INT_EDGE |
GPIO_PUD_PULL_DOWN | GPIO_INT_ACTIVE_HIGH);
gpio_pin_configure(ctx->irq_gpio, conf->irq.pin, conf->irq.flags |
GPIO_INPUT);
gpio_pin_interrupt_configure(ctx->irq_gpio, conf->irq.pin,
GPIO_INT_EDGE_TO_ACTIVE);

/* Chip RESET line */
ctx->reset_gpio = device_get_binding(conf->reset.devname);
Expand All @@ -580,8 +583,8 @@ static inline int configure_gpios(struct device *dev)
conf->reset.devname);
return -EINVAL;
}
gpio_pin_configure(ctx->reset_gpio, conf->reset.pin,
GPIO_DIR_OUT | GPIO_PUD_NORMAL | GPIO_POL_NORMAL);
gpio_pin_configure(ctx->reset_gpio, conf->reset.pin, conf->reset.flags |
GPIO_OUTPUT_INACTIVE);

/* Chip SLPTR line */
ctx->slptr_gpio = device_get_binding(conf->slptr.devname);
Expand All @@ -590,18 +593,18 @@ static inline int configure_gpios(struct device *dev)
conf->slptr.devname);
return -EINVAL;
}
gpio_pin_configure(ctx->slptr_gpio, conf->slptr.pin,
GPIO_DIR_OUT | GPIO_PUD_NORMAL | GPIO_POL_NORMAL);
gpio_pin_configure(ctx->slptr_gpio, conf->slptr.pin, conf->slptr.flags |
GPIO_OUTPUT_INACTIVE);

/* Chip DIG2 line (Optional feature) */
ctx->dig2_gpio = device_get_binding(conf->dig2.devname);
if (ctx->dig2_gpio != NULL) {
LOG_INF("Optional instance of %s device activated",
conf->dig2.devname);
gpio_pin_configure(ctx->dig2_gpio, conf->dig2.pin,
GPIO_DIR_IN |
GPIO_PUD_PULL_DOWN |
GPIO_INT_ACTIVE_HIGH);
conf->dig2.flags | GPIO_INPUT);
gpio_pin_interrupt_configure(ctx->dig2_gpio, conf->dig2.pin,
GPIO_INT_EDGE_TO_ACTIVE);
}

/* Chip CLKM line (Optional feature) */
Expand All @@ -610,7 +613,7 @@ static inline int configure_gpios(struct device *dev)
LOG_INF("Optional instance of %s device activated",
conf->clkm.devname);
gpio_pin_configure(ctx->clkm_gpio, conf->clkm.pin,
GPIO_DIR_IN | GPIO_PUD_NORMAL);
conf->clkm.flags | GPIO_INPUT);
}

return 0;
Expand Down
12 changes: 6 additions & 6 deletions drivers/ieee802154/ieee802154_rf2xx_iface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,27 @@ void rf2xx_iface_phy_rst(struct device *dev)
const struct rf2xx_context *ctx = dev->driver_data;

/* Ensure control lines have correct levels. */
gpio_pin_write(ctx->reset_gpio, conf->reset.pin, 1);
gpio_pin_write(ctx->slptr_gpio, conf->slptr.pin, 0);
gpio_pin_set(ctx->reset_gpio, conf->reset.pin, 0);
gpio_pin_set(ctx->slptr_gpio, conf->slptr.pin, 0);

/* Wait typical time of timer TR1. */
k_busy_wait(330);

gpio_pin_write(ctx->reset_gpio, conf->reset.pin, 0);
gpio_pin_set(ctx->reset_gpio, conf->reset.pin, 1);
k_busy_wait(10);
gpio_pin_write(ctx->reset_gpio, conf->reset.pin, 1);
gpio_pin_set(ctx->reset_gpio, conf->reset.pin, 0);
Copy link
Member

@nandojve nandojve Jan 27, 2020

Choose a reason for hiding this comment

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

Please, make sure to keep original sequence otherwise radio will be at RESET state.

Copy link
Contributor

@pabigot pabigot Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@nandojve I believe this is correct. Because the reset pin is marked active low and gpio_pin_set() operates on the active level, the command before the wait enables reset, and the one after disables it.

Copy link
Member

@nandojve nandojve Jan 27, 2020

Choose a reason for hiding this comment

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

Understood, but reset is active low see 1.1 Pin Descriptions on the datasheet. If this is on code need to be fixed anyway. I will have access to radios again next week and I can go in deep. How urgent is this change?

Copy link
Contributor

@pabigot pabigot Jan 27, 2020

Choose a reason for hiding this comment

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

Yes, I mis-wrote: reset is active low in its devicetree definition, so passing 1 to gpio_pin_set() makes it low. This is different from gpio_pin_set_raw().

Urgency: This will be merged today or tomorrow to enable a major refactoring required to get this branch into 2.2.

Copy link
Member

@nandojve nandojve Jan 27, 2020

Choose a reason for hiding this comment

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

Ok, please, move forward!

}
void rf2xx_iface_phy_tx_start(struct device *dev)
{
const struct rf2xx_config *conf = dev->config->config_info;
const struct rf2xx_context *ctx = dev->driver_data;

/* Start TX transmission at rise edge */
gpio_pin_write(ctx->slptr_gpio, conf->slptr.pin, 1);
gpio_pin_set(ctx->slptr_gpio, conf->slptr.pin, 1);
/* 16.125[μs] delay to detect signal */
k_busy_wait(20);
/* restore initial pin state */
gpio_pin_write(ctx->slptr_gpio, conf->slptr.pin, 0);
gpio_pin_set(ctx->slptr_gpio, conf->slptr.pin, 0);
}

u8_t rf2xx_iface_reg_read(struct device *dev,
Expand Down