Skip to content
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

Jump to bottom

cpu, cc2538: add gpio alternative functions #7373

Merged
merged 2 commits into from
Aug 24, 2017
Merged

cpu, cc2538: add gpio alternative functions #7373

merged 2 commits into from
Aug 24, 2017

Conversation

smlng
Copy link
Member

@smlng smlng commented Jul 17, 2017

based on discussion in #7316 this add alternative gpio functions to be used in periph drivers to hide internal gpio configuration.

@smlng smlng added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 17, 2017
@smlng smlng self-assigned this Jul 17, 2017
@smlng smlng requested a review from haukepetersen July 17, 2017 12:12
This was referenced Jul 17, 2017
Closed
Merged
Closed
Merged
@smlng smlng requested review from PeterKietzmann, A-Paul and a user July 17, 2017 13:27
@smlng
Copy link
Member Author

smlng commented Jul 17, 2017

quite urgent, as need by lots of other periph drivers, see reference PRs above.

@smlng smlng force-pushed the cpu/cc2538/gpio_af branch 4 times, most recently from b5c2e03 to 5396d4a Compare July 20, 2017 12:28
@smlng
Copy link
Member Author

smlng commented Jul 27, 2017

several PR are waiting for this one, could someone find time to review and ACK?!

@smlng
Copy link
Member Author

smlng commented Aug 15, 2017

ping, anyone care to review, please!

A-Paul
A-Paul reviewed Aug 16, 2017
View reviewed changes
boards/remote-pa/board.c Outdated
@@ -60,7 +60,7 @@ static void rf_switch_init(void)
RF_SWITCH_PORT->DIR |= (1 << RF_SWITCH_PIN);

/* configure io-mux for used pins */
IOC->OVER[RF_SWITCH_PIN] = IOC_OVERRIDE_OE;
IOC_PXX_OVER[RF_SWITCH_PIN] = IOC_OVERRIDE_OE;
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't this whole block be handled by a simple gpio_init?

Copy link
Member Author

Choose a reason for hiding this comment

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

you might be right, will look into it shortly.

cpu/cc2538/include/periph_cpu.h Outdated
*
* @param[in] pin gpio pin
* @return 0 on success, otherwise error
Copy link
Member

Choose a reason for hiding this comment

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

This description suggests that any kind of error checking is done in the function. Looking in the function body, there isn't any. Maybe put something more concrete here, like:
0 if pin != GPIO_UNDEF, -1 otherwise

cpu/cc2538/periph/gpio.c Outdated
return -1;
}

if (over >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why dont't you just use an unsigned type for this parameter and omit the condition check?

Copy link
Member

Choose a reason for hiding this comment

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

I already answered myself by reading (and understanding) the function description. ;)

@A-Paul
Copy link
Member

A-Paul commented Aug 16, 2017

ping, anyone care to review, please!
@smlng, sorry I wrote comments some time ago, but failed to hit the "subit review" button.

haukepetersen
haukepetersen requested changes Aug 17, 2017
View reviewed changes
cpu/cc2538/include/periph_cpu.h Outdated
@@ -127,24 +127,15 @@ static inline uint8_t gpio_pp_num(gpio_t pin)
}

/**
* @brief Helper function to enable gpio hardware control
* @brief Alternative GPIO init function
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little arbitrary to me. I think this function initializes a pin in alternate function mode (or peripheral mode) or whatever TI is calling this. So not the function is an alternative to the default init function, but the resulting pin mode is an alternative to the default input or output mode...

boards/remote-pa/board.c Outdated
@@ -60,7 +60,7 @@ static void rf_switch_init(void)
RF_SWITCH_PORT->DIR |= (1 << RF_SWITCH_PIN);

/* configure io-mux for used pins */
IOC->OVER[RF_SWITCH_PIN] = IOC_OVERRIDE_OE;
IOC_PXX_OVER[RF_SWITCH_PIN] = IOC_OVERRIDE_OE;
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't this whole block be handled by a simple gpio_init?

cpu/cc2538/periph/gpio.c Outdated
{
if (pin == GPIO_UNDEF) {
return -1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be done using an assert()

Copy link
Member Author

Choose a reason for hiding this comment

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

true that.

@smlng
Copy link
Member Author

smlng commented Aug 21, 2017

@haukepetersen comments addressed

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 21, 2017
haukepetersen
haukepetersen requested changes Aug 21, 2017
View reviewed changes
boards/remote-pa/board.c Outdated
RF_SWITCH_INTERNAL;
/* Set RF 2.4GHz as default */
gpio_init(RF_SWITCH_GPIO, GPIO_OUT);
RF_SWITCH_2_4_GHZ;
Copy link
Contributor

Choose a reason for hiding this comment

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

indention off by 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I somehow messed up my editor config and tab size is 2 instead of 4 - need to fix that

boards/remote-pa/include/board.h Outdated
#define RF_SWITCH_GPIO GPIO_PD4
#define RF_SWITCH_SUB_GHZ gpio_set(RF_SWITCH_GPIO)
#define RF_SWITCH_2_4_GHZ gpio_clear(RF_SWITCH_GPIO)
#define RF_SWITCH_TOGGLE gpio_toggle(RF_SWITCH_GPIO)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO in this context, a toggle operation does not really make sense, does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

well that depends on the use-case, actually these boards have 2 radios (sub GHz and 2.4GHz). They (can) share the same antenna, hence you may want to use only one at a time which makes this short-hand macro quite useful, or not?

boards/remote-pa/include/board.h Outdated
#define RF_SWITCH_EXTERNAL (RF_SWITCH_PORT->DATA |= (1 << RF_SWITCH_PIN))
#define RF_SWITCH_INTERNAL (RF_SWITCH_PORT->DATA &= ~(1 << RF_SWITCH_PIN))
#define RF_SWITCH_TOGGLE (RF_SWITCH_PORT->DATA ^= (1 << RF_SWITCH_PIN))
#define RF_SWITCH_GPIO GPIO_PD4
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, but shouldn't this be GPIO_PIN(3, 4)?!

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but there are lots of these macro usages throughout the cc2538 code base and I remove them in #7320, all at once.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I'll that here right away.

@smlng
Copy link
Member Author

smlng commented Aug 22, 2017

comments addressed

haukepetersen
haukepetersen approved these changes Aug 24, 2017
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

ACK

@smlng
Copy link
Member Author

smlng commented Aug 24, 2017

squashed

@haukepetersen
Copy link
Contributor

and go.

@haukepetersen haukepetersen merged commit 138092f into RIOT-OS:master Aug 24, 2017
@smlng smlng deleted the cpu/cc2538/gpio_af branch August 25, 2017 08:11
@smlng smlng mentioned this pull request Aug 25, 2017
Merged
@smlng smlng mentioned this pull request Aug 30, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Paul A-Paul A-Paul left review comments

@haukepetersen haukepetersen haukepetersen approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

@smlng smlng

Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smlng @A-Paul @haukepetersen