samd21/adc: initial implementation#6929
samd21/adc: initial implementation#6929thomaseichinger merged 1 commit intoRIOT-OS:masterfrom travisgriggs:samd21_adc
Conversation
| /* ADC 0 device configuration */ | ||
| #define ADC_0_DEV ADC | ||
| #define ADC_0_IRQ ADC_IRQn | ||
| #define ADC_0_CHANNELS (3U) |
There was a problem hiding this comment.
fixed. tabs. old habits die hard.
| #define ADC_0_PRESCALER ADC_CTRLB_PRESCALER_DIV512 | ||
| static const adc_channel_t adc_channels[] = { | ||
| /* port, pin, muxpos */ | ||
| {GPIO_PIN(PA, 10), ADC_INPUTCTRL_MUXPOS_PIN18}, // EXT2, pin 3 |
There was a problem hiding this comment.
Here and all following, please use /* C style comments */
There was a problem hiding this comment.
also fixed. old habits, again.
cpu/samd21/periph/adc.c
Outdated
|
|
||
| static mutex_t lock = MUTEX_INIT; | ||
|
|
||
| static inline void prep(void) |
There was a problem hiding this comment.
For your consideration, usually static function names are prefixed by a underscore RIOT e.g. _function_name. This is not a coding convention but might improve consistency.
There was a problem hiding this comment.
static variables as well? Or just the functions?
There was a problem hiding this comment.
I changed lock to _lock as well. I can back out if that's going to far.
|
Yes, we had seen #4162, and wanted to avoid the demise of that attempt. So we just stuck with the simple/straightforward approach that satisfied the basic riot driver. Said API is good enough for our current project anyway. |
thomaseichinger
added
CI: needs squashing
|
@travisgriggs Thanks for your changes. Can't test but code looks good to me. @PeterKietzmann do you agree? |
|
looking at the jenkins CI, other samd21 based boards should be adapted as well (arduino-zero, sodaq-autonomo and samr21-xpro) |
|
squashed |
| #include "periph/adc.h" | ||
| #include "periph_conf.h" | ||
| #include "mutex.h" | ||
|
|
There was a problem hiding this comment.
Please guard the implementation by #if ADC_NUMOF. This way we work around adding the configurations for other boards.
There was a problem hiding this comment.
fixed (I think I did what you wanted, found something similar in nrf51 variant). resquashed as well
thomaseichinger
added
CI: ready for build
|
@aabadie Is the ADC configuration in arduino-zero's |
Indeed, I don't remember why I added this configuration. Normally, it should match the actual ADC configuration of the board.
Sure |
|
@thomaseichinger, I opened #6943 regarding the issue with arduino-zero board. |
| typedef struct { | ||
| gpio_t pin; | ||
| uint32_t muxpos; | ||
| } adc_channel_t; |
There was a problem hiding this comment.
Please add doxygen documentation here. Something like
/**
* @brief ADC Channel Configuration
*/
typedef struct {
gpio_t pin; /**< ADC channel pin */
uint32_t muxpos; /**< ADC channel pin multiplexer value */
} adc_channel_t;
|
Also, please rebase since #6943 got merged. |
|
I'm out for the rest of the week at a wedding; I'll make the change first thing Monday morning. Thanks for the continuing guidance. |
|
doxygen updates and squashed |
|
rebased again to keep it on top of master |
thomaseichinger
added
CI: ready for build
|
@travisgriggs All set, ACK & Go |
4 participants
This is an implementation of the simple RIOT ADC driver for the samd21 chip. The samd21-xpro board definition is modified to include this peripheral.
I have a simple "examples" app I wrote to test it, but was uncertain whether that should be included or not for just this. Seemed like it should be a different PR if there was interest at all.