Add driver support for ADC1 of stm32 #13447

cybertale · 2019-02-16T12:01:13Z

This PR contains pinmux, dts and driver support for ADC1 for all series of stm32 that zephyr supports.

zephyrbot · 2019-02-16T12:09:34Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

cybertale · 2019-02-16T12:19:42Z

To test this code, we should enable adc driver in menuconfig and add pinmux config in board specific file.
For f4 board, my configuration is

#ifdef CONFIG_ADC0_CHAN0
	{STM32_PIN_PA0, STM32F4_PINMUX_FUNC_PA0_ADC123_IN0},
#endif

Test application code is

 * Copyright (c) 2012-2014 Wind River Systems, Inc.
 *
 * SPDX-License-Identifier: Apache-2.0
 */

#include <adc.h>
#include <zephyr.h>
#include <misc/printk.h>

uint16_t temp_data[8];

static const struct adc_channel_cfg channel_cfg_0 = {
        .channel_id       = 0,
};

const struct adc_sequence sequence = {
        .channels    = BIT(0),
        .buffer      = temp_data,
        .buffer_size = sizeof(temp_data),
        .resolution  = 12,
};

void main(void)
{
        int ret;
        struct device *adc_dev = device_get_binding(DT_ADC_0_NAME);
        adc_channel_setup(adc_dev, &channel_cfg_0);

        while (1) {
                ret = adc_read(adc_dev, &sequence);
                if (ret != 0) {
                        printk("adc_read() failed with code %d", ret);
                } else {
                        printk("adc_read() value %d\n", temp_data[0]);
                }
                k_sleep(1000);
        }
}

cybertale · 2019-02-16T12:47:37Z

Hi @erwango , I'v only tested this code on f4 board, could you test it on other boards that available for you when you got some time?

cybertale · 2019-02-17T05:49:24Z

Generic DMA driver of stm32 is under development in #13364 , without DMA support many stm32s won't be able to catch up EOC interrupts generated by channels conversion, this will cause overrun error and lost data. So Only one channel can be put in one adc sequence in this driver.

KwonTae-young · 2019-02-18T04:34:19Z

I was interested in the ADC driver for STM32.
I tested it on ST STM32F4DISCOVERY as shown below.

west init zephyrproject -m https://github.com/KwonTae-young/zephyr/ --mr adc_stm32-new_stm32f4-disco
cd zephyrproject/zephyr
source zephyr-env.sh
mkdir samples/basic/adc/build
cd samples/basic/adc/build
cmake -DBOARD=stm32f4_disco ..
make -j8 flash

ADC works well.

********** Booting Zephyr OS zephyr-v1.13.0-4846-g2eee90a41b *****
adc_read() value 1128
adc_read() value 952
adc_read() value 927
adc_read() value 1083
adc_read() value 916
adc_read() value 904

cybertale · 2019-02-18T09:37:04Z

@KwonTae-young Thanks for your interest. 😁

drivers/adc/adc_stm32.c

drivers/adc/Kconfig.stm32

erwango

Initial feedback

dts/arm/st/f0/stm32f0.dtsi

drivers/adc/Kconfig.stm32

erwango · 2019-02-18T13:25:04Z

drivers/adc/adc_stm32_v2.c

+
+void adc_stm32_init_soc(ADC_TypeDef *adc)
+{
+	adc->CR2 |= ADC_CR2_ADON;


Why not using LL_ADC_Enable and factorize this part?

LL_ADC_Enable can be applied here, but I was thinking if I used LL lib somewhere, it's better to use LL lib everywhere. We talked about why I do not think LL lib is good for this generic driver, and except this function, some functions like getting EOC flags from registers uses different functions between different series of stm32 in LL lib.
So should I use LL and direct register access in the same driver?
I personally prefer to use direct register access for all operations.

cybertale · 2019-02-20T02:58:24Z

This PR should be only merged after #13501 is merged.

anangl · 2019-02-25T08:47:02Z

drivers/adc/adc_stm32.c

+
+	adc_context_start_read(&data->ctx, sequence);
+	err = adc_context_wait_for_completion(&data->ctx);
+	adc_context_release(&data->ctx, err);


Remove this line. The lock is now released in adc_stm32_read[_sync].

cybertale · 2019-02-27T07:15:00Z

Updated to use LL lib.

erwango · 2019-02-27T08:01:10Z

CODEOWNERS

@@ -131,6 +131,7 @@
 /dts/arm/st/                              @erwango
 /dts/arm/nordic/                          @ioannisg @carlescufi
 /dts/bindings/can                         @alexanderwachter
+/dts/bindings/iio/adc/st,stm32-adc.yaml   @cybertale


Please also add drivers/adc/adc_stm32.c

erwango · 2019-03-07T12:30:16Z

@cybertale , since your work is close to mature and complete, could you update the description of the commit with a description of the work and the way it was tested? (this could edited later on with further addition)

cybertale · 2019-03-07T12:38:36Z

@erwango Sure!

anangl · 2019-03-07T12:41:14Z

tests/drivers/adc/adc_api/src/test_adc.c

+#define ADC_GAIN		ADC_GAIN_1
+#define ADC_REFERENCE		ADC_REF_INTERNAL
+#define ADC_ACQUISITION_TIME	ADC_ACQ_TIME_DEFAULT
+#define ADC_1ST_CHANNEL_ID	0


These parameters are exactly the same as the ones for NUCLEO_F091RC. Wouldn't it be better to use:

#elif defined(CONFIG_BOARD_NUCLEO_F091RC) || \ defined(CONFIG_BOARD_NUCLEO_F103RB)

Same applies to a couple of Nucleo boards using 10-bit resolution.

Yes, that's better

anangl · 2019-03-07T12:42:37Z

tests/drivers/adc/adc_api/src/test_adc.c

+#define ADC_ACQUISITION_TIME	ADC_ACQ_TIME_DEFAULT
+#define ADC_1ST_CHANNEL_ID	0
+
+#elif defined(CONFIG_BOARD_NUCLEO_F103RB)


Duplicate entry.

dbkinder

+1 for doc changes

erwango

This is promising, some more comments

erwango · 2019-03-08T07:56:24Z

drivers/adc/Kconfig.stm32

+	bool "STM32 ADC driver"
+	depends on SOC_FAMILY_STM32
+	help
+		Enable the driver implementation for the stm32xx ADC


Use two spaces alignment difference between "help" and help paragrah.

erwango · 2019-03-08T07:58:48Z

drivers/adc/adc_stm32.c

+
+	adc_context_on_sampling_done(&data->ctx, dev);
+
+	LOG_INF("ISR triggered.");


Change to LOG_DBG

erwango · 2019-03-08T07:59:34Z

drivers/adc/adc_stm32.c

+	adc_stm32_setup_speed(dev, channel_cfg->channel_id,
+				  acq_time_index);
+
+	LOG_INF("Channel setup succeeded!");


erwango · 2019-03-08T08:01:41Z

drivers/adc/Kconfig.stm32

+	default y
+	help
+		Enable ADC1
+


Miss ADC_2 and _3, since available in the driver

I'm actually thinking about removing them, because there are so many different product lines, and different amount of ADCs they have, and also some ADCs share interrupt vector, so there should be some code for identifying which instance calls the interrupt. I would like another PR to do these things because this PR can be merged so users of STM32 can have basic ADC support.
But I'm worrying what Geo said about the merge window, but I see zephyr's doc says

As a general rule, if you miss the merge window for a given feature, the best thing to do is to wait for the next development cycle. (An occasional exception is made for drivers for previously unsupported hardware; if they do not touch any other in-tree code, they cannot cause regressions and should be safe to add at any time).

So if this passes all tests and clean, we should be able to merge this PR soon, right?

This is up to @galak to settle on this. Though, I'm not quite in favor of it. Even it could not be a source of regression, it could be a source of bugs, and branch maturity is tightly controlled.
Besides, there are several PR that could pretend be merged as this PR (L1/G0/WB). There is no reason this one is privileged.
So I think the paragraph you mention should actually be corrected.

Understand, so I'll just wait. ;)

cybertale · 2019-03-10T12:09:24Z

It seems like the Shippable tests failed because another work is under going, #14242 should be able to solve the problem.

anangl · 2019-03-11T07:48:02Z

boards/arm/nucleo_f401re/pinmux.c

@@ -41,6 +41,8 @@ static const struct pin_config pinconf[] = {
 	{STM32_PIN_PB14, STM32F4_PINMUX_FUNC_PB14_SPI2_MISO},
 	{STM32_PIN_PB15, STM32F4_PINMUX_FUNC_PB15_SPI2_MOSI},
 #endif	/* CONFIG_SPI_1 */
+#ifdef CONFIG_ADC_1
+#endif	/* CONFIG_ADC_1 */


Is this empty #ifdef block intentional?

Sorry, my mistake

GeorgeCGV · 2019-03-13T18:07:56Z

@cybertale, tested on L452 SoC, works great.

shphilippe · 2019-04-04T23:40:57Z

drivers/adc/adc_stm32.c

+	LL_ADC_EnableInternalRegulator(adc);
+	u32_t wait_loop_index = ((LL_ADC_DELAY_INTERNAL_REGUL_STAB_US / 10UL) *
+				(SystemCoreClock / (100000UL * 2UL)));
+	while (wait_loop_index != 0UL) {


k_busy_wait(LL_ADC_DELAY_INTERNAL_REGUL_STAB_US);

That's better.

shphilippe · 2019-04-05T00:23:31Z

btw, I've added support for the STM32G0 in this branch: https://github.com/shphilippe/zephyr/tree/stm32g070-adc

cybertale · 2019-04-05T12:09:43Z

btw, I've added support for the STM32G0 in this branch: https://github.com/shphilippe/zephyr/tree/stm32g070-adc

I see G0 support is not in plan of milestone v1.15, right? So you can feel free to push support for ADC of G0 after this. Or what's your plan? :)

shphilippe · 2019-04-05T16:07:14Z

I don't have much plans. Just trying to contribute back code which can be open sourced :)

erwango

Mostly nitpicks

erwango · 2019-04-30T13:37:42Z

drivers/adc/adc_stm32.c

+		return -EIO;
+	}
+
+#if defined(CONFIG_SOC_SERIES_STM32L4X)


I feel some more comments in this init function would be welcome

erwango · 2019-04-30T13:38:09Z

drivers/adc/adc_stm32.c

+#if defined(CONFIG_SOC_SERIES_STM32F0X) || \
+	defined(CONFIG_SOC_SERIES_STM32L0X)
+	LL_ADC_SetClock(adc, LL_ADC_CLOCK_SYNC_PCLK_DIV4);
+#endif


Why not using elif there?

erwango · 2019-04-30T13:40:07Z

drivers/adc/adc_stm32.c

+#endif
+	LL_ADC_Enable(adc);
+#ifdef CONFIG_SOC_SERIES_STM32L4X
+	/* Refer to the description of ADRDY in reference manual. */


"Refer to reference manual" applies to the while code I guess.
A plain comment would be nice.

erwango · 2019-04-30T13:41:07Z

drivers/adc/adc_stm32.c

+
+	LL_ADC_SetCommonPathInternalCh(ADC, LL_ADC_PATH_INTERNAL_VREFINT);
+#endif
+	LL_ADC_Enable(adc);


Break lines around this.

erwango · 2019-04-30T15:02:19Z

drivers/adc/adc_stm32.c

+
+	if (sequence->options) {
+		needed_buffer_size *= (1 + sequence->options->extra_samplings);
+	}


Break a line here

erwango · 2019-04-30T15:02:27Z

drivers/adc/adc_stm32.c

+		LOG_ERR("Provided buffer is too small (%u/%u)",
+				sequence->buffer_size, needed_buffer_size);
+		return -ENOMEM;
+	}


erwango · 2019-04-30T15:06:50Z

drivers/adc/adc_stm32.c

+	LL_ADC_EnableIT_EOCS(adc);
+#endif
+
+	adc_context_start_read(&data->ctx, sequence);


Break a line here.

cybertale · 2019-05-05T02:06:22Z

Hi @erwango , I added some comments and fixed style issues, how about this time?

All series of stm32 have at least one ADC instance and this commit adds one ADC node to the root dts file of each soc, and also adds fixing up mappings to them. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

This commit adds pinmux defines for all the external ADC lines supported by stm32. All defines are named after the datasheet of the corresponding product lines. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

This commit adds driver support for ADC1 on all 8 supported series of stm32 with resolution and conversion time selection and calibration. Currently DMA is not supported for all series, and without it, zephyr won't be able to catch up ADC's end of conversion interrupt, so this version of the driver supports one channel conversion only. Users want multi-channel conversion should use multiple sequences in their app code. This driver uses LL lib rather than HAL because the current HAL lib for ADC will call HAL_DMA_* functions rather than using zephyr's common DMA interface, so that way the driver will break the consistency of the code. This driver has been tested on multiple nucleo boards including NUCLEO_F091RC/F103RB/F207ZG/F302R8/F401RE/F746ZG/L073RZ/L476RG and all passed the test cases in tests/drivers/adc/adc_api. If the external ADC line is floating, it may fail the tests since ADC may get 0V and the test cases think 0 is failing. Connect it to any voltage source between 0-3.3V will help passing the test cases. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

Some configuration for the boards have to be added into test_adc.c file so user can test driver with the test cases. Several nucleo boards are added including F091RC/F103RB/F207ZG/F302R8/F401RE/F746ZG/L073RZ/L476RG. And also ADC dts and pinmux configuration are added into boards own pinmux.c and dts file. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

The ADC driver in this PR has been tested working on these nucleo boards, so ADC support is added to the boards doc. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

cybertale requested review from anangl and erwango as code owners February 16, 2019 12:01

cybertale mentioned this pull request Feb 17, 2019

[WIP] drivers: adc: add adc driver support for STM32 #12460

Closed

erwango added area: ADC Analog-to-Digital Converter (ADC) platform: STM32 ST Micro STM32 labels Feb 18, 2019

anangl requested changes Feb 18, 2019

View reviewed changes

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

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

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

drivers/adc/Kconfig.stm32 Outdated Show resolved Hide resolved

erwango requested changes Feb 18, 2019

View reviewed changes

anangl reviewed Feb 25, 2019

View reviewed changes

erwango reviewed Feb 27, 2019

View reviewed changes

cybertale requested a review from dbkinder as a code owner March 7, 2019 11:47

anangl reviewed Mar 7, 2019

View reviewed changes

dbkinder approved these changes Mar 7, 2019

View reviewed changes

erwango requested changes Mar 8, 2019

View reviewed changes

This was referenced Mar 10, 2019

drivers: gpio: stm32: add ASCR configuration for L47x+ #14131

Merged

sensor: fxos8700: Convert to new DT_<COMPAT>_<INSTANCE> defines #14242

Merged

anangl approved these changes Mar 11, 2019

View reviewed changes

erwango added this to the v1.15.0 milestone Mar 18, 2019

cybertale requested a review from galak as a code owner March 30, 2019 04:31

shphilippe reviewed Apr 4, 2019

View reviewed changes

erwango reviewed Apr 30, 2019

View reviewed changes

cybertale requested a review from nashif as a code owner May 4, 2019 14:02

erwango approved these changes May 6, 2019

View reviewed changes

zephyrproject-rtos deleted a comment from codecov-io May 7, 2019

cybertale added 5 commits May 7, 2019 08:48

dts: arm: stm32: add dts support for ADC1 of stm32

7e8f8f6

All series of stm32 have at least one ADC instance and this commit adds one ADC node to the root dts file of each soc, and also adds fixing up mappings to them. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

drivers: pinmux: stm32: add pinmux support for ADC of stm32

2ae1f39

This commit adds pinmux defines for all the external ADC lines supported by stm32. All defines are named after the datasheet of the corresponding product lines. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

doc: boards: arm: nucleo: add ADC support to the boards doc

9bb471d

The ADC driver in this PR has been tested working on these nucleo boards, so ADC support is added to the boards doc. Signed-off-by: Song Qiang <songqiang1304521@gmail.com>

nashif merged commit d96ffc1 into zephyrproject-rtos:master May 8, 2019

erwango mentioned this pull request Jul 23, 2019

Add STM32 ADC driver #10896

Closed

disposedtrolley mentioned this pull request Oct 18, 2020

ADC1 doesn't read correctly on STM32F7 #29309

Closed


		adc_context_on_sampling_done(&data->ctx, dev);

		LOG_INF("ISR triggered.");

Add driver support for ADC1 of stm32 #13447

Add driver support for ADC1 of stm32 #13447

Conversation

cybertale commented Feb 16, 2019

zephyrbot commented Feb 16, 2019 • edited

cybertale commented Feb 16, 2019

cybertale commented Feb 16, 2019 • edited

cybertale commented Feb 17, 2019

KwonTae-young commented Feb 18, 2019 • edited

cybertale commented Feb 18, 2019

erwango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybertale commented Feb 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybertale commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Mar 7, 2019

cybertale commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkinder left a comment

Choose a reason for hiding this comment

erwango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybertale commented Mar 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeCGV commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shphilippe commented Apr 5, 2019

cybertale commented Apr 5, 2019

shphilippe commented Apr 5, 2019

erwango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybertale commented May 5, 2019

zephyrbot commented Feb 16, 2019 •

edited

cybertale commented Feb 16, 2019 •

edited

KwonTae-young commented Feb 18, 2019 •

edited

cybertale commented Feb 20, 2019 •

edited