Skip to content

[TOPIC-GPIO] boards: nrf52840_pca10090: Convert to use the new GPIO API #20184

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
Nov 4, 2019
Merged

[TOPIC-GPIO] boards: nrf52840_pca10090: Convert to use the new GPIO API #20184

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
22 changes: 9 additions & 13 deletions boards/arm/nrf52840_pca10090/board.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,20 +176,16 @@ static int pins_configure(struct device *port, const struct pin_config cfg[],
{
int err;

/* Write to the pins before configuring them as output,
* to make sure we are driving them to the correct level
* right after they are configured.
*/
for (size_t i = 0; i < pins; i++) {
/* The swiches on the board are active low, so we need
* to negate the IS_ENABLED() value from the tables.
/* A given pin controlling the switch needs to be driven
* to the low state to activate the routing indicated by
* the corresponding IS_ENABLED() macro in the table,
* so configure the pin as output with the proper initial
* state.
*/
err = gpio_pin_set(port, cfg[i].pin, !cfg[i].val);
if (err) {
return cfg[i].pin;
}

err = gpio_pin_configure(port, cfg[i].pin, GPIO_OUTPUT);
u32_t flag = (cfg[i].val ? GPIO_OUTPUT_LOW
: GPIO_OUTPUT_HIGH);
err = gpio_pin_configure(port, cfg[i].pin, flag);
Copy link
Contributor

@lemrey lemrey Oct 28, 2019

Choose a reason for hiding this comment

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

So this new API will configure the pin as output and also set its state to low/high?

Copy link
Contributor

@pabigot pabigot Oct 28, 2019

Choose a reason for hiding this comment

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

Yes, that's what those flags do. Unfortunately there aren't OUTPUT_INIT_ACTIVE and OUTPUT_INIT_INACTIVE.

Since the pins are documented as being active low, and the new API supports operating on logical level, I think it would be more clear to have this be:

unsigned int flags = GPIO_ACTIVE_LOW;

if (cfg[i].pin) {
   flags |= GPIO_OUTPUT_LOW;
} else {
   flags |= GPIO_OUTPUT_HIGH;
}
err = gpio_pin_configure(port, cfg[i].pin, flags);

then...

Copy link
Member Author

@anangl anangl Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Since the pins are documented as being active low

Well, in fact these pins are not documented as being active low. The DK documentation says only about the default destination and the optional destination, activated when a given pin is set as output and driven low or high, respectively. These pins are described as having active-low logic only in the comment before definitions of pins_on_p0 and pins_on_p1 tables (and I left that comment as it was, treating it as to be considered in the context of these tables only; should I change it?). They were also mentioned as being active low in the comment that I reworded, and I intentionally got rid of this "are active low" statement.

Copy link
Contributor

@pabigot pabigot Oct 29, 2019

Choose a reason for hiding this comment

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

OK then.

Copy link
Contributor

@pabigot pabigot Oct 29, 2019

Choose a reason for hiding this comment

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

This comment is still in the code, which is part of why I asked for the change:

 * The switches have active-low logic, so when writing to the port we will
 * need to invert the value to match the IS_ENABLED() logic.

However that's clear enough, so I'm ok with using the negative logic physical value.

if (err) {
return cfg[i].pin;
}
Expand Down Expand Up @@ -218,7 +214,7 @@ static void reset_pin_wait_low(struct device *port, u32_t pin)

/* Wait until the pin is pulled low */
do {
Copy link
Contributor

@pabigot pabigot Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

... here do:

/* Wait until pin becomes active */
while (gpio_pin_get(port, pin) == 0) {
}

Other boards might include pins that are active-high, and using the logic level rather than physical level would make the code more consistent.

Copy link
Contributor

@pabigot pabigot Oct 28, 2019

Choose a reason for hiding this comment

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

I now see the original comment about using _raw to save a few bytes of flash. I prefer clarity of using normal logic, but if I'm alone in that I won't insist.

Copy link
Member Author

@anangl anangl Oct 29, 2019

Choose a reason for hiding this comment

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

Other boards might include pins that are active-high, and using the logic level rather than physical level would make the code more consistent.

This reset pin here is in fact active high. And the above loop waits until it becomes inactive. As the accompanying comment says, this "lets the other side ensure that they are ready".
I'd prefer to left it as it is.

val = gpio_pin_get(port, pin);
val = gpio_pin_get_raw(port, pin);
} while (val > 0);
}

Expand Down