Hi @attie-argentum ,
I was wondering if you have few time to check this proposal.

I've checked over the handling of ADC_REFCTRL_REFSEL_VDD_1_2 does appear to hold true for L21, D20, D21, D5x and E5x.

However, we loose access to the ADC_REF_VDD_1 reference, which is indeed valid in some parts, and is used in the design I'm currently working on... importantly, this is not expected to be 1v (or variable internal reference on SAML21), but is expected to be VDDANA.
The SAML21 uses INTVCC2 / 0x5, and SAMD5x / E5x use INTVCC1 / 0x3.

I think for absolute clarity and flexibility, my preferred solution would be to place definitions in the relevant soc.h, and then use #ifdefs in the switch for to enable the modes that are supported.
It seems that all parts I've checked support AREFA and AREFB - them mapping onto ADC_REF_EXTERNAL0 and ADC_REF_EXTERNAL1 is fine, and no changes are required there.

Example for SAML21 soc.h:

#define ADC_REFCTRL_REFSEL_INTERNAL ADC_REFCTRL_REFSEL_INTREF
#define ADC_REFCTRL_REFSEL_VDD_1_2  ADC_REFCTRL_REFSEL_INTVCC1
#define ADC_REFCTRL_REFSEL_VDD_1    ADC_REFCTRL_REFSEL_INTVCC2

PS: in the SAML21 PR, I pre-define the ADC_REFCTRL_REFSEL_VDD_1 and ADC_REFCTRL_REFSEL_VDD_1_2 values for the SAML21, in a fairly nasty way... I think shifting these mappings / definitions to be in soc.h as standard would be nice.

However, we loose access to the ADC_REF_VDD_1 reference, which is indeed valid in some parts, and is used in the design I'm currently working on... importantly, this is not expected to be 1v (or variable internal reference on SAML21), but is expected to be VDDANA.

I understood your point about ADC_REF_VDD_1. I`ll review later.

I think for absolute clarity and flexibility, my preferred solution would be to place definitions in the relevant soc.h, and then use #ifdefs in the switch for to enable the modes that are supported.

We have two paths in place today:

• Fix at hal_atmel. This is what have been doing for SAM SoCs, mostly because peripheral are same and only reg names are different.
• For SAM0 only, fixups were introduced.

I prefer not add a third option.

Fix at hal_atmel. This is what have been doing for SAM SoCs, mostly because peripheral are same and only reg names are different.

I'm not sure if this is what you're meaning, but putting these #defines (or equivalent) in hal_atmel for each SoC instead works for me...

I agree with @attie-argentum regarding the ADC_REF_VDD_1 macro, the reference voltage selection varies greatly for each model.

Each microcontroller of this series has its own set of selectable references (as you can see below). @nandojve I think it might be hard to make adc_reference represent all possible selections without a huge sequence of #if CONFIG_SOC_SERIES_SAMXXXs in the fixup file and adding more options in enum adc_reference. I don't know what would be the best solution in this case.

SAMD21:

SAML21:

If the only solution is to implement each one in the fixup file, I volunteer to do it (if needed).

@lucasssvaz - watch out for REFCTRL.REFSEL (which in the case of SAML21 can pick a variable reference) vs VREF.SEL (which is the configuration of that reference / what you've shown)

@lucasssvaz - watch out for REFCTRL.REFSEL (which in the case of SAML21 can pick a variable reference) vs VREF.SEL (which is the configuration of that reference / what you've shown)

Thanks, fixed it

@attie-argentum , @lucasssvaz ,

Since there is no good solution, I propose to keep internal voltage reference as is. For all devices the internal reference tracks VDDANA somehow and the default value is 1.0V. If that reference is good or not it is out of scope because code should select appropriated parameter and user should calibrate device on all temperature range.

In the case of ADC_REF_VDD_1 I will not object to track VDDANA. I think have some sense because ADC_REF_VDD_1_2 is VDDANA / 2 for all devices.

Since there is no good solution, I propose to keep internal voltage reference as is.

Agreed

In the case of ADC_REF_VDD_1 I will not object to track VDDANA. I think have some sense because ADC_REF_VDD_1_2 is VDDANA / 2 for all devices.

I believe this is the documented / expected behaviour (VDD_1 -> 1/1, VDD_1_2 -> 1/2, etc...)
https://docs.zephyrproject.org/latest/hardware/peripherals/adc.html#c.adc_reference.ADC_REF_VDD_1
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/adc.h#L75-L78

/** @brief ADC references. */
enum adc_reference {
	ADC_REF_VDD_1,     /**< VDD. */
	ADC_REF_VDD_1_2,   /**< VDD/2. */
	ADC_REF_VDD_1_3,   /**< VDD/3. */
	ADC_REF_VDD_1_4,   /**< VDD/4. */
	ADC_REF_INTERNAL,  /**< Internal. */
	ADC_REF_EXTERNAL0, /**< External, input 0. */
	ADC_REF_EXTERNAL1, /**< External, input 1. */
};

Since there is no good solution, I propose to keep internal voltage reference as is.

Agreed

In the case of ADC_REF_VDD_1 I will not object to track VDDANA. I think have some sense because ADC_REF_VDD_1_2 is VDDANA / 2 for all devices.

I believe this is the documented / expected behaviour (VDD_1 -> 1/1, VDD_1_2 -> 1/2, etc...) https://docs.zephyrproject.org/latest/hardware/peripherals/adc.html#c.adc_reference.ADC_REF_VDD_1 https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/adc.h#L75-L78

/** @brief ADC references. */
enum adc_reference {
	ADC_REF_VDD_1,     /**< VDD. */
	ADC_REF_VDD_1_2,   /**< VDD/2. */
	ADC_REF_VDD_1_3,   /**< VDD/3. */
	ADC_REF_VDD_1_4,   /**< VDD/4. */
	ADC_REF_INTERNAL,  /**< Internal. */
	ADC_REF_EXTERNAL0, /**< External, input 0. */
	ADC_REF_EXTERNAL1, /**< External, input 1. */
};

Sure, so current version fits well, right? Or do you still see any change for improvement?

@nandojve Looks good. The only improvement I can see is to somehow support those unusual reference values (I think this is unfeasible for now but would be nice to have in a future update).
Is there any already implemented way to manually define these values inside the ADC API (without using adc_enum) ?

Also, ADC_REF_VDD_1 is still valid for the D21 (It is unsupported in this MCU, although it selects the 1V internal reference with the current proposed solution). IMO it should be either unsupported (returning the "Selected reference is not valid" error) or have this behavior properly documented somewhere (maybe atmel,sam0-adc?)

Sure, so current version fits well, right?

Gah, sorry - I missed the update... Yes, LGTM, though I'm not 100% convinced that switching on (ADC0_BANDGAP / ADC_REFCTRL_REFSEL_INTVCC2) will hold true for all / future SoCs.

Also, ADC_REF_VDD_1 is still valid for the D21 [...] IMO it should be either unsupported (giving the "Selected reference is not valid" error) or have this behavior properly documented somewhere (maybe atmel,sam0-adc?)

Agreed - "ADC_REF_VDD_1 picks VDDANA, but falls back to 1.0v" isn't ideal.

Sure, so current version fits well, right?

Gah, sorry - I missed the update... Yes, LGTM, though I'm not 100% convinced that switching on (ADC0_BANDGAP / ADC_REFCTRL_REFSEL_INTVCC2) will hold true for all / future SoCs.

Also, ADC_REF_VDD_1 is still valid for the D21 [...] IMO it should be either unsupported (giving the "Selected reference is not valid" error) or have this behavior properly documented somewhere (maybe atmel,sam0-adc?)

Agreed - "ADC_REF_VDD_1 picks VDDANA, but falls back to 1.0v" isn't ideal.

The other thing that can be made is:

#ifndef ADC_REFCTRL_REFSEL_VDD_1
#  if defined(ADC0_BANDGAP)
#    define ADC_REFCTRL_REFSEL_VDD_1 ADC_REFCTRL_REFSEL_INTVCC1
#  elif defined(ADC_REFCTRL_REFSEL_INTVCC2)
#    define ADC_REFCTRL_REFSEL_VDD_1 ADC_REFCTRL_REFSEL_INTVCC2
#  else
- #    define ADC_REFCTRL_REFSEL_VDD_1 ADC_REFCTRL_REFSEL_INT1V
+ #    define ADC_REFCTRL_REFSEL_VDD_1 ADC_REFCTRL_REFSEL_INTVCC0
#  endif
#endif

So, people that uses 2.96V or 3.2V (1/1.48 or 1/1.6) can have something to work too.

@nandojve Seems like an 'ok' solution, as long as this behavior is documented somewhere easy to find. This way new Zephyr devs using these processors won't have to delve deep into the source code to find out how it actually works.

The other thing that can be made is:

Personally, I'd prefer to not define ADC_REFCTRL_REFSEL_VDD_1 here, and then use the original ifdefs in the switch to mark it as "not supported" / EINVAL... This does indeed mean that the other references can't be used, but it's also predictable behaviour by just reading the reference manuals (both SoC and Zephyr), and not the sources.

... but I'm not going to block / be difficult about it.

@attie-argentum Agreed. If I had to choose I would prefer to have a predictable behavior over the use of unusual references (although the ideal solution would be to rework the ADC API to have both)

Hi, where are we at with this PR? If you've worked out a solution, can someone please approve?

LGTM, don't think I can approve though.

@mbolivar-nordic I also can't approve it but it seems OK to me. Thanks for the help guys!