drivers: adc: SAM0: Enable automatic result devision when oversampling #76963
Conversation
|
Hello @guenzel-kinexon, and thank you very much for your first pull request to the Zephyr project! |
zephyrbot
added
platform: Microchip SAM
zephyrbot
requested review from
anangl,
attie-argentum,
decsny,
mnkp,
nandojve,
pdgendt and
stephanosio
|
please address the compliance failures |
|
I'll try to get it done as soon as possible |
guenzel-kinexon
force-pushed
the
adjust-resolution-sam0-adc
branch
from
46a8e93 to
4015a33
Compare
done |
There was a problem hiding this comment.
Hi @guenzel-kinexon,
You are doing far more than applying the fix. On Atmel we use 80 chars.
Split you PR doing first the coding formatting that you want.
Do no use clang!
Then add your change.
drivers/adc/adc_sam0.c
Outdated
There was a problem hiding this comment.
You shoud create an enum to store the oversampling value and do not add a default on the switch. Compiler will ensure that all valid entries defined on the enum will be on the switch case.
There was a problem hiding this comment.
The oversampling value is a numeric value from the DT which could ofc. be converted to an enum but I feel it would be an overkill to cast it just for the switch statement.
The default is modeled after how the peripheral works. All oversampling values above 4 have a constant adjustment value. An argument could be made that maybe a
if (oversampling < 4) {} else {} would be more suited here.
There was a problem hiding this comment.
Hi @guenzel-kinexon ,
Let's forget for a moment the implementation details and focus on the fix.
I was looking on the datasheet and it is not clear to me what you want to achieve. Could you explain in details what you want to achieve?
I see main sections (below) to explain how to use those bits. They should be combined with AVGCTRL.SAMPLENUM[3:0] but you are not changing then.
45.6.2.9 Accumulation
45.6.2.10 Averaging
45.6.2.11 Oversampling and Decimation
There was a problem hiding this comment.
Ultimately I want to use the peripheral to do hardware-averaging. To make this work the registers for accumulation as well as averaging have to be set.
The accumulation is already done in the driver (line 319, right above the switch statement). What is missing, is the averaging configuration, which I added in this PR.
As described in Accumulation section of the datasheet (chapter 33.6.6 for the SAMD21 datasheet), up to 16 samples the hardware does no automatic division. For 32 samples it already adds a factor but that is expected that the user already set the correct factor in the ADJRES field (as described in the Averaging chapter).
The datasheet is not super clear about this imho. but with trial&error on real hardware I figured it out to be a manual+automatic scheme requiring the driver to set the adjustment factor from 0..4 for averaging up to 16 samples and then let the hardware handle additional division for averaging factors beyond that.
There was a problem hiding this comment.
Ok, I see now. Thank you for the fix.
Yes, you can useif (oversampling < 4) {} else {}
Doesn't the contribution guidelines enforce a Clang formatting of the code? I am a bit lost which formatting rules to follow here |
No clang format is informative, not blocking. |
|
@guenzel-kinexon thanks for your first PR to the project :) Will you be able to come back to it or do you still have questions regarding next steps? |
|
I will try to fix it up asap. |
guenzel-kinexon
force-pushed
the
adjust-resolution-sam0-adc
branch
from
4015a33 to
37e8e3a
Compare
The SAM0 ADC has an internal register to divide the accumulated results of oversampling the ADC by the amount of samples collected. This value has to be set by the driver and was missing. Signed-off-by: Patrick Günzel <[email protected]>
guenzel-kinexon
force-pushed
the
adjust-resolution-sam0-adc
branch
from
37e8e3a to
4939f44
Compare
6 participants
The SAM0 ADC has an internal register to divide the accumulated results of oversampling the ADC by the amount of samples collected. This value has to be set by the driver and was missing.
Fixes #74607