drivers: adc: sam0: Fix driver and sample #41319

attie-argentum · 2021-12-19T00:07:03Z

The adc_sam0 driver previously expected sequence->channels == 1, which is not compatible with the ADC sample, and might be unexpected. In an attempt to avoid breaking changes, I have revised this to require a single bit set (with no confirmation as to whether it is the correct bit). The input channel for this driver needs to be set using the "configurable inputs" API instead - differential or not.

I have also revised adc_sam0 so that it supports devices with MCLK, but only one ADC. This will be important for SAML21 support (see #41308).

Finally, I have revised the drivers/adc sample so that it will operate the "configurable inputs" API correctly.

This PR is split out from #41308.

nandojve

Thank you for this PR @attie-argentum ,

I particulary don't like #if .. #endif in code but sometimes we need to use. Did you considered add a fixup for adc? It could be something similar to those at soc/arm/atmel_sam0/common.

zephyr/soc/arm/atmel_sam0/common/sercom_fixup_samd5x.h

Line 7 in f076540

#ifdef MCLK_APBAMASK_SERCOM0

drivers/adc/adc_sam0.c

codecov-commenter · 2021-12-20T19:20:17Z

Codecov Report

Merging #41319 (f076540) into main (5fda6a3) will increase coverage by 0.03%.
The diff coverage is 59.13%.

❗ Current head f076540 differs from pull request most recent head 05a0acd. Consider uploading reports for the commit 05a0acd to get more accurate results

@@            Coverage Diff             @@
##             main   #41319      +/-   ##
==========================================
+ Coverage   51.24%   51.27%   +0.03%     
==========================================
  Files         605      606       +1     
  Lines       70878    71042     +164     
  Branches    16333    16364      +31     
==========================================
+ Hits        36318    36425     +107     
- Misses      28572    28607      +35     
- Partials     5988     6010      +22

Impacted Files	Coverage Δ
include/device.h	`84.61% <ø> (ø)`
include/net/ethernet.h	`51.28% <ø> (ø)`
subsys/net/ip/ipv6.c	`56.75% <0.00%> (ø)`
subsys/net/ip/net_shell.c	`6.67% <0.00%> (-0.04%)`	⬇️
subsys/net/lib/sockets/sockets_packet.c	`36.76% <0.00%> (-0.28%)`	⬇️
subsys/net/ip/ipv6_nbr.c	`33.49% <41.17%> (+0.42%)`	⬆️
subsys/net/ip/route.c	`56.10% <64.70%> (+5.13%)`	⬆️
kernel/userspace.c	`87.50% <75.00%> (-0.27%)`	⬇️
lib/os/heap_listener.c	`88.23% <88.23%> (ø)`
lib/libc/newlib/libc-hooks.c	`52.41% <100.00%> (+0.38%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fda6a3...05a0acd. Read the comment docs.

attie-argentum · 2021-12-21T02:43:45Z

@nandojve thanks for your comments.

I didn't consider a fixup.h, because I didn't know they existed... I'll look into it.

Regarding your 4x remaining comments, I hope my reasoning stands. If not, I'll do what I think you're after...

attie-argentum · 2021-12-21T03:35:03Z

@nandojve I believe I have now implemented an adc_fixup.h as requested. I've left this as a separate patch, as it's quite far-reaching...

I've confirmed that this runs on a SAML21 XPro, and that it builds for a SAMD21 XPro, and a SAME54 XPro - I believe this should cover the bases on variants.

nandojve

Hi @attie-argentum ,

I think fixups makes code far more clean to read. Thank you for doing it!

@nandojve I believe I have now implemented an adc_fixup.h as requested. I've left this as a separate patch, as it's quite far-reaching...

I will suggest that if you are willing to do, you could mode fixups before you introduce SAML21 support. This will clean the path to add the new SoC support.

drivers/adc/adc_sam0.c

soc/arm/atmel_sam0/same51/soc.h

attie-argentum · 2022-01-05T00:46:24Z

@nandojve I've now re-worked the fixup header.

The SAML21 support is added transparently, as I felt this was acceptable and actually makes the most sense... it won't be usable until #41308 is merged (unless another existing part has the same shape ADC).

I've also rebased on main.

nandojve

Hi @attie-argentum ,

Fixup file helped to read code. Thank you for introduce it.

drivers/adc/adc_sam0.c

soc/arm/atmel_sam0/same51/soc.h

drivers/adc/adc_sam0.c

attie-argentum · 2022-01-20T01:58:13Z

I have some more fixes for the ADC driver coming - are is it okay for me to roll them into this PR, or would a new PR be preferred?

REFCTRL is enable-protected on the SAML21, meaning that the ADC needs to be disabled before changing it.
- Docs indicate that this is probably true for SAMD21 too (along with the "discard next sample after reference change")
- SAME54 seems to not have this restriction
On SAML21, the reference mapping is different
- Zephyr's ADC_REF_VDD_1_2 -> ASF's ADC_REFCTRL_REFSEL_INTVCC1
- Zephyr's ADC_REF_VDD_1 -> ASF's ADC_REFCTRL_REFSEL_INTVCC2
- I'd propose putting this into a new per-soc fixup header.

On second thought, this should mostly live in #41308

I have however made some minor cleanup and rebased on main.

nandojve · 2022-01-30T12:37:30Z

Hi @anangl , @stephanosio , @mnkp,

Should this fix be at v3.0?

Previously this was expected to be equal to 1 at all times. This doesn't play well with the sample or other users (e.g: adc_shell). Instead, we should count the number of active channels in the bitfield, and ensure that only one is identified. Signed-off-by: Attie Grande <[email protected]>

All drivers that require input_positive to be set should be given it. Some parts may require an offset, so abstract this away too. Signed-off-by: Attie Grande <[email protected]>

Add a basic device tree overlay to support the use of the ATSAMD21 XPro dev board with the ADC sample application. Signed-off-by: Attie Grande <[email protected]>

The ADC driver now supports three different implementations. To maintain readability, this patch implements an adc_fixup.h that permits more generic access to relevant registers. This patch also introduces support for a new third shape ADC - as found in the SAML21 for example. Signed-off-by: Attie Grande <[email protected]>

Local variables should not be in capitals. Signed-off-by: Attie Grande <[email protected]>

attie-argentum · 2022-02-02T03:38:01Z

Rebase

attie-argentum requested review from anangl and nashif as code owners December 19, 2021 00:07

github-actions bot added area: ADC Analog-to-Digital Converter (ADC) area: Samples Samples labels Dec 19, 2021

attie-argentum changed the title ~~Fixup SAM0 ADC Driver, and ADC Sample~~ Fixup SAM0 ADC driver, and ADC sample Dec 19, 2021

attie-argentum mentioned this pull request Dec 19, 2021

Add support for Atmel SAML21, SAMR34, SAMR35 parts #41308

Merged

nandojve added the platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) label Dec 19, 2021

nandojve requested review from mnkp, nandojve and stephanosio December 19, 2021 00:54

attie-argentum force-pushed the adc-fixup branch from bdeedd4 to 05a0acd Compare December 19, 2021 01:12

nandojve changed the title ~~Fixup SAM0 ADC driver, and ADC sample~~ drivers: adc: sam0: Fix driver and sample Dec 20, 2021

nandojve requested changes Dec 20, 2021

View reviewed changes

attie-argentum force-pushed the adc-fixup branch from 05a0acd to 5537ac4 Compare December 21, 2021 02:28

attie-argentum requested review from MaureenHelm, galak and ioannisg as code owners December 21, 2021 03:34

zephyrbot assigned anangl Dec 21, 2021

nandojve requested changes Dec 21, 2021

View reviewed changes

attie-argentum force-pushed the adc-fixup branch 2 times, most recently from d079c40 to a527b91 Compare January 5, 2022 00:43

attie-argentum force-pushed the adc-fixup branch from a527b91 to 745997c Compare January 5, 2022 22:30

attie-argentum force-pushed the adc-fixup branch from 745997c to f2216fb Compare January 14, 2022 17:05

nandojve requested changes Jan 15, 2022

View reviewed changes

drivers/adc/adc_sam0.c Outdated Show resolved Hide resolved

drivers/adc/adc_sam0.c Outdated Show resolved Hide resolved

soc/arm/atmel_sam0/same51/soc.h Outdated Show resolved Hide resolved

attie-argentum force-pushed the adc-fixup branch 2 times, most recently from c88fe84 to 97e3c68 Compare January 15, 2022 22:32

attie-argentum force-pushed the adc-fixup branch 2 times, most recently from 36fec91 to 932378a Compare January 17, 2022 23:52

nandojve approved these changes Jan 18, 2022

View reviewed changes

nandojve mentioned this pull request Jan 18, 2022

Sam0 adc fix #41853

Closed

KimBP reviewed Jan 18, 2022

View reviewed changes

drivers/adc/adc_sam0.c Outdated Show resolved Hide resolved

attie-argentum force-pushed the adc-fixup branch 2 times, most recently from 064c4b3 to a6e18a7 Compare January 19, 2022 01:33

attie-argentum force-pushed the adc-fixup branch 2 times, most recently from 1f301eb to c792ab3 Compare January 20, 2022 02:46

nandojve added this to the v3.0.0 milestone Jan 20, 2022

attie-argentum force-pushed the adc-fixup branch 2 times, most recently from 2e5a9df to eb421ef Compare January 28, 2022 01:02

attie-argentum added 5 commits February 2, 2022 03:20

samples: drivers: adc: Revise handling of configurable inputs

0b5a7f6

All drivers that require input_positive to be set should be given it. Some parts may require an offset, so abstract this away too. Signed-off-by: Attie Grande <[email protected]>

samples: drivers: adc: Add a device tree overlay for atsamd21_xpro

d5a15de

Add a basic device tree overlay to support the use of the ATSAMD21 XPro dev board with the ADC sample application. Signed-off-by: Attie Grande <[email protected]>

drivers: adc: adc_sam0: Change local variables to lower case

838a56e

Local variables should not be in capitals. Signed-off-by: Attie Grande <[email protected]>

attie-argentum force-pushed the adc-fixup branch from eb421ef to 838a56e Compare February 2, 2022 03:22

attie-argentum mentioned this pull request Feb 2, 2022

Import Atmel SAML21, SAMR34 and SAMR35 header files from ASF library zephyrproject-rtos/hal_atmel#17

Merged

carlescufi approved these changes Feb 4, 2022

View reviewed changes

carlescufi merged commit 24478c8 into zephyrproject-rtos:main Feb 4, 2022

attie-argentum deleted the adc-fixup branch February 4, 2022 18:23

drivers: adc: sam0: Fix driver and sample #41319

drivers: adc: sam0: Fix driver and sample #41319

Uh oh!

Conversation

attie-argentum commented Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nandojve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Dec 20, 2021

Codecov Report

Uh oh!

attie-argentum commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

attie-argentum commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nandojve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

attie-argentum commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nandojve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

attie-argentum commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nandojve commented Jan 30, 2022

Uh oh!

attie-argentum commented Feb 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

attie-argentum commented Dec 19, 2021 •

edited

Loading

attie-argentum commented Dec 21, 2021 •

edited

Loading

attie-argentum commented Dec 21, 2021 •

edited

Loading

nandojve left a comment •

edited

Loading

attie-argentum commented Jan 5, 2022 •

edited

Loading

attie-argentum commented Jan 20, 2022 •

edited

Loading