zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.52k stars 6.45k forks source link

STM32 ADC and Device Tree #67634

Closed attie-argentum closed 8 months ago

attie-argentum commented 8 months ago

I often design boards that use a potential divider to permit measuring voltages above the maximum for the part. In this case, I set the channel's zephyr,vref-mv property to be the "top" of the divider, not the true reference voltage - this allows a direct conversion from the raw ADC reading to the measured voltage at the top of the potential divider (using adc_raw_to_millivolts_dt()), rather than requiring further conversion from the divider's tap to the top.

It appears that I've just stumbled onto a quirk(?) with STM32's ADC driver, and possibly the ADC device tree utility macros. Apologies for bringing the two under one roof, but it seems they are interconnected to some degree...

cc: @carlescufi, @nashif, @gmarull, @stephanosio, @FRASTM, @cybertale

STM32 Uses ADC_REF_INTERNAL Incorrectly(?)

https://github.com/zephyrproject-rtos/zephyr/blob/92af172159d5b0e0c32e31b0f1aac3bf03341314/drivers/adc/adc_stm32.c#L1234-L1237

Many MCUs are able to measure against an internal reference voltage, but that's not how STM32s are wired up... instead they require VREF+ to be supplied externally, and provide an internal reference on a dedicated ADC channel, which permits measuring the voltage at VREF+ "backwards" ... at least that's true for the STM32L4 I'm using and STM32F103 I just sanity checked against.

... is the use of ADC_REF_INTERNAL correct here? In my case VREF+ == VDDA, but this doesn't need to hold true across all designs. That said, it's an assumption that is already made in a comment:

https://github.com/zephyrproject-rtos/zephyr/blob/92af172159d5b0e0c32e31b0f1aac3bf03341314/drivers/adc/adc_stm32.c#L1479

No other references are supported by the STM32, so perhaps a fairly harmless patch would be to also accept ADC_REF_EXTERNAL0 here? Dropping support for ADC_REF_INTERNAL will likely be an unpopular move...

adc_raw_to_millivolts_dt() ignores zephyr,vref-mv

https://github.com/zephyrproject-rtos/zephyr/blob/5c06d60cceee12bf46ecd3e26c05cd1c6f3cf3fb/include/zephyr/drivers/adc.h#L814-L818

Paired with the above, I found that adc_raw_to_millivolts_dt() will ignore the "technically incorrect, but holistically correct" value I've given for zephyr,vref-mv when the ADC_REF_INTERNAL reference is used for the channel. I'm unsure if that's bad behavior, but it's certainly problematic for STM32s, where this reference is probably used incorrectly. Even if the internal reference was used, this "top of the divider" technique could still be useful for situations like this.

Do we think it would be acceptable to rework this check like so?

if ((spec->vref_mv == 0) && (spec->channel_cfg.reference == ADC_REF_INTERNAL)) { 
    vref_mv = (int32_t)adc_ref_internal(spec->dev); 
} else { 
    vref_mv = spec->vref_mv; 
} 

spec->vref_mv is supposedly zero if the property isn't present:

https://github.com/zephyrproject-rtos/zephyr/blob/5c06d60cceee12bf46ecd3e26c05cd1c6f3cf3fb/include/zephyr/drivers/adc.h#L271-L277

attie-argentum commented 8 months ago

67635 alone resolves the issue satisfactorily for me, so I'm going to close this... If anyone want's to discuss the STM32 driver's use of ADC_REF_INTERNAL further, please feel free to reopen. Apologies for the wide-ish CC.

PeeJay commented 1 week ago

So ADC_REF_INTERNAL is the only implemented reference option? The device tree bindings page lists all the options the hardware supports. I was just trying to use ADC_REF_EXTERNAL0 and was surprised when I got an Invalid argument error.