Skip to content

SAMD21/SAMR21: Add more ADC lines#7009

Merged
aabadie merged 3 commits intoRIOT-OS:masterfrom
kbumsik:io1-fix
May 7, 2017
Merged

SAMD21/SAMR21: Add more ADC lines#7009
aabadie merged 3 commits intoRIOT-OS:masterfrom
kbumsik:io1-fix

Conversation

@kbumsik
Copy link
Copy Markdown
Contributor

@kbumsik kbumsik commented May 5, 2017 •
edited
Loading

This is a followup to #6929 .
This PR completes ADC line definitions of EXT headers for SAMD21/SAMR21 XPlained Pro.
I tested all lines using test/periph_adc and the conversion works well.

This change is Reviewable

@kbumsik kbumsik changed the title SAMD21/SAMR21: Added more ADC lines SAMD21/SAMR21: Add more ADC lines May 5, 2017
aabadie
aabadie requested changes May 5, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Changes look good out there. Just one minor comment regarding the adc channel struct definition that could factorized.
Samr21-xpro is finally about to have ADC support !

boards/samr21-xpro/include/periph_conf.h Outdated
/**
* @brief ADC Channel Configuration
*/
typedef struct {
Copy link
Copy Markdown
Contributor

@aabadie aabadie May 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This was taken from samd21-xpro but I think that this struct definition should be factorized in cpu/samd21/include/periph_cpu.h.

Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen May 5, 2017

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Contributor Author

@kbumsik kbumsik May 5, 2017

Choose a reason for hiding this comment

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

You are absolutely right. I will move it.
Regarding type name, I think I need to change the type name to adc_conf_chan_t to match other peripheral struct names?

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented May 5, 2017

Much better.
Can you split your commit in 3:

  • one related to the move of adc_conf_chan_t to cpu with comment: "cpu/samd21: move adc channel struct definition to periph_cpu.h"
  • one related to addition of channels for samd21-xpro: "boards/samd21-xpro: add more adc lines"
  • one related to addition of ADC for samr21-xpro: "boards/samr21-xpro: add adc support"

Then it will be fine !

@kbumsik
Copy link
Copy Markdown
Contributor Author

kbumsik commented May 5, 2017

@aabadie Sure. Please check it.

@aabadie aabadie added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 5, 2017
@aabadie aabadie added this to the Release 2017.07 milestone May 5, 2017
aabadie
aabadie approved these changes May 5, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Untested ACK

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 5, 2017
@kbumsik kbumsik mentioned this pull request May 6, 2017
Merged
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented May 7, 2017

Looking at the datasheets, the changes are good for samr21-xpro and Murdock is happy, merging :)

@aabadie aabadie merged commit e18acd2 into RIOT-OS:master May 7, 2017
@thomaseichinger thomaseichinger mentioned this pull request Jun 29, 2017
Closed
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen left review comments

@aabadie aabadie aabadie approved these changes

Assignees

No one assigned

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2017.07

Development

Successfully merging this pull request may close these issues.

3 participants

@kbumsik @aabadie @haukepetersen