Use Shared IRQ for stm32 ADC driver #37585

mcscholtz · 2021-08-10T18:37:25Z

I added the ability to use a shared IRQ for the stm32 ADC driver to allow devices with just a single ADC IRQ to use all channels.

Edit: This is my first PR, please let me know if I need to make any changes. Thanks!

Fixes #26874

mcscholtz · 2021-08-10T21:31:30Z

Looks like the failures are due to the configurable prescalar change. I will take a look later when I have time

mcscholtz · 2021-08-11T15:39:40Z

I was reading the contribution guidelines and I missed a few things, such as the commit signoff. I will fix that soon.

mcscholtz · 2021-08-15T19:59:51Z

I fixed the styling issues that caused the compliance checks to fail before. I also veried it with the checkpatch script before pushing it.

mbolivar-nordic

Thanks for your first patch! I have some general coding style comments. Please remember to follow the contribution guidelines by squashing or splitting commits as needed, without piling on a series of "fix this, fix that" commits to address review feedback.

@erwango or another STM32 maintainer will need to do the hardware specific review, but the binding change looks fine to me.

mbolivar-nordic · 2021-08-19T20:39:49Z

drivers/adc/Kconfig.stm32

+	depends on ADC_STM32
+	help
+	  Enable the use of shared interrupts for famalies that only have a single interrupt for all ADC's
+endif


Please add a newline to the end of the file.

The compliance check failed after adding the new line at the end of the file

mbolivar-nordic · 2021-08-19T20:40:30Z

drivers/adc/adc_stm32.c

@@ -227,7 +236,6 @@ struct adc_stm32_data {
 	const struct device *dev;
 	uint16_t *buffer;
 	uint16_t *repeat_buffer;
-


We usually try not to include unrelated whitespace changes in commits which change functionality. I would recommend dropping this change or moving it to a separate commit.

mbolivar-nordic · 2021-08-19T20:41:16Z

drivers/adc/adc_stm32.c

@@ -239,12 +247,17 @@ struct adc_stm32_data {

 struct adc_stm32_cfg {
 	ADC_TypeDef *base;
+	const uint16_t prescalar;


No need for const here as the entire structure is const.

mbolivar-nordic · 2021-08-19T20:41:58Z

drivers/adc/adc_stm32.c

-	LOG_ERR("Conversion time not supportted.");
+	LOG_ERR("Conversion time not supported.");


You might want to split this into its own commit also.

@mbolivar-nordic I can do that, would you recommend to just remove this and the other changes and force update the single comment. Then create new comments for the other changes, like the fix of this typo in this case?

How people split up commits while rebasing is a personal decision, so I can't really offer you a "recommendation" in that sense. Beyond individual preference, tooling varies: for example, I use magit instead of the command line.

However, one simple way to handle this since the PR contains a single commit is to put all its changes back into the working tree:

git checkout feature/stm32f407-adc git reset HEAD~

Then you can split changes in a single file into their own commits using git add -p, e.g. for this particular comment:

git add -p drivers/adc/adc_stm32.c < follow instructions to just stage this hunk> git commit <write a commit comment describing the typo fix>

Then keep adding additional commits for the other changes you want to make.

But again this sort of thing is subject to taste, and I would do it in a different way using my own favorite tools.

mbolivar-nordic · 2021-08-19T20:43:01Z

dts/arm/st/f4/stm32f4.dtsi

-			reg = <0x40012000 0x400>;
+			reg = <0x40012000 0x50>;


Is this really part of the same commit? I think it needs a justification.

@mbolivar-nordic The ADC1 device currently holds the address space for ADC1, ADC2, ADC3.

The address for all ADC's starts at 0x40012000 with a length of 0x400. The following is the addresses of the individual ADC's:
ADC1: 0x40012000 length of 0x50
ADC2: 0x40012100 length of 0x50
ADC3: 0x40012200 length of 0x50
Common: 0x40012300 length of 0xC (need to double check the length of this one)

Maybe someone that knows stm32 well can comment on how to handle it.

OK, then yes, I think this definitely needs a justification and likely a separate commit.

Maybe someone that knows stm32 well can comment on how to handle it.

Let's wait to see what the STM32 maintainers say.

I do agree with @mbolivar-nordic. Please split this in a dedicated commit.

erwango

Thanks for this addition!
Some changes requested otherwise this looks fine.

Then, have you been able to test these changes ?

erwango · 2021-08-24T08:25:10Z

drivers/adc/adc_stm32.c

+static const struct adc_stm32_cfg adc_stm32_cfg_##index = {		\
+	.base = (ADC_TypeDef *)DT_INST_REG_ADDR(index),			\
+	.prescalar = DT_INST_PROP_OR(index, prescalar, 4),		\
+	.irq_cfg_func = adc_stm32_shared_irq_handler,			\


This part of the struct is actually meant to hold the irq init function, not the irq handler.
In current case, it should then be NULL if we don't want to use it.
Alternatively, it could be set to adc_stm32_cfg_func_isr and use the init_irq flag inside this function to make sure irq init is only done once (which would remove need for the adc_stm32_init hack).

erwango · 2021-08-24T08:27:10Z

dts/arm/st/f4/stm32f4.dtsi

-			reg = <0x40012000 0x400>;
+			reg = <0x40012000 0x50>;


I do agree with @mbolivar-nordic. Please split this in a dedicated commit.

erwango · 2021-08-24T08:29:23Z

drivers/adc/adc_stm32.c

+#ifdef CONFIG_ADC_STM32_SHARED_IRQS
+DT_INST_FOREACH_STATUS_OKAY(STM32_ADC_INIT_SHARED)
+#else
 DT_INST_FOREACH_STATUS_OKAY(STM32_ADC_INIT)
+#endif


Alternative otpion is to put the #ifdef CONFIG_ADC_STM32_SHARED_IRQS inside STM32_ADC_INIT definition, to minimize code changes

erwango · 2021-08-24T08:30:36Z

drivers/adc/adc_stm32.c

+#if defined(CONFIG_SOC_SERIES_STM32F2X) ||	\
+	defined(CONFIG_SOC_SERIES_STM32F4X) ||	\
+	defined(CONFIG_SOC_SERIES_STM32F7X)
+static uint32_t adc_stm32_get_prescalar(uint16_t prescalar)


Please put the prescalar changes into a dedicated commit.

erwango · 2021-08-24T08:31:30Z

drivers/adc/Kconfig.stm32

+	default y
+	depends on ADC_STM32
+	help
+	  Enable the use of shared interrupts for famalies that only have a single interrupt for all ADC's


s/famalies/families

mcscholtz · 2021-08-26T17:02:02Z

Then, have you been able to test these changes ?

We have a custom board using the stm32f407 and I have tested using multiple ADC's together and it is working.

erwango

Thanks for the update, few more comments.

erwango · 2021-08-30T08:29:32Z

drivers/adc/adc_stm32.c

+static const struct soc_gpio_pinctrl adc_pins_##index[] =		\
+	ST_STM32_DT_INST_PINCTRL(index, 0);				\
+									\
+	ADC_STM32_CONFIG_ISR(index)					\


Unindent for readability.

erwango · 2021-08-30T08:32:11Z

drivers/adc/adc_stm32.c

+	}
+}
+
+#define ADC_STM32_CONFIG_ISR(index)					\


I see why you used ADC_STM32_CONFIG_ISR, but this is a bit misleading.
Let's use ADC_STM32_CONFIG instead of ADC_STM32_CONFIG_ISR.

erwango · 2021-08-30T08:36:15Z

drivers/adc/adc_stm32.c

+	DT_INST_FOREACH_STATUS_OKAY(HANDLE_IRQS);
+}
+
+static void adc_stm32_cfg_func_isr(void)


This funtion is not technically an isr, what about adc_stm32_irq_init ?

drivers/adc/adc_stm32.c

ycsin · 2021-10-24T11:23:03Z

@mcscholtz You will have to rebase and make sure that you only have one ADC commit on top of the branch if you continue with this branch.

MaureenHelm

@mcscholtz You will have to rebase and make sure that you only have one ADC commit on top of the branch if you continue with this branch.

carlescufi · 2021-12-20T17:34:41Z

@mcscholtz You will have to rebase and make sure that you only have one ADC commit on top of the branch if you continue with this branch.

@MaureenHelm would you pick this PR up? Seems a shame not to merge it if the author is unresponsive.

mcscholtz · 2022-01-10T16:45:49Z

Closing. See #41497

mcscholtz requested review from anangl, cybertale and erwango as code owners August 10, 2021 18:37

github-actions bot added area: ADC Analog-to-Digital Converter (ADC) area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding platform: STM32 ST Micro STM32 labels Aug 10, 2021

mcscholtz mentioned this pull request Aug 10, 2021

Enhance the ADC functionality on the STM32 devices to all available ADC channels #32288

Closed

mcscholtz force-pushed the feature/stm32f407-adc branch 2 times, most recently from ba886e5 to 124bdda Compare August 12, 2021 18:59

zephyrbot requested review from ABOSTM, FRASTM, galak and mbolivar-nordic August 19, 2021 12:42

zephyrbot assigned erwango Aug 19, 2021

mbolivar-nordic suggested changes Aug 19, 2021

View reviewed changes

mcscholtz force-pushed the feature/stm32f407-adc branch from 124bdda to 3eb045c Compare August 23, 2021 16:00

erwango requested changes Aug 24, 2021

View reviewed changes

mcscholtz force-pushed the feature/stm32f407-adc branch 4 times, most recently from 6ef9dc8 to b00eee0 Compare August 26, 2021 23:05

mcscholtz requested review from erwango and mbolivar-nordic August 27, 2021 00:53

erwango requested changes Aug 30, 2021

View reviewed changes

mcscholtz force-pushed the feature/stm32f407-adc branch from b00eee0 to 06973db Compare August 30, 2021 22:08

mcscholtz force-pushed the feature/stm32f407-adc branch from 4459091 to 24e1f3a Compare October 22, 2021 19:20

Merge branch 'main' into feature/stm32f407-adc

4c1bceb

ycsin reviewed Oct 24, 2021

View reviewed changes

drivers/adc/adc_stm32.c Show resolved Hide resolved

erwango mentioned this pull request Nov 2, 2021

STM32: Multiple ADCs using same interrupt not possible #26874

Closed

mbolivar-nordic approved these changes Nov 4, 2021

View reviewed changes

tejlmand approved these changes Nov 8, 2021

View reviewed changes

MaureenHelm requested changes Nov 8, 2021

View reviewed changes

ycsin mentioned this pull request Dec 29, 2021

drivers: arm: stm32: adc: Added Shared IRQ #41497

Merged

mcscholtz closed this Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Shared IRQ for stm32 ADC driver #37585

Use Shared IRQ for stm32 ADC driver #37585

mcscholtz commented Aug 10, 2021 •

edited by erwango

Loading

mcscholtz commented Aug 10, 2021

mcscholtz commented Aug 11, 2021

mcscholtz commented Aug 15, 2021

mbolivar-nordic left a comment

mbolivar-nordic Aug 19, 2021

mcscholtz Aug 31, 2021

mbolivar-nordic Aug 19, 2021

mbolivar-nordic Aug 19, 2021

mbolivar-nordic Aug 19, 2021

mcscholtz Aug 19, 2021

mbolivar-nordic Aug 20, 2021

mbolivar-nordic Aug 19, 2021

mcscholtz Aug 19, 2021

mbolivar-nordic Aug 20, 2021

erwango Aug 24, 2021

erwango left a comment

erwango Aug 24, 2021

erwango Aug 24, 2021

erwango Aug 24, 2021

erwango Aug 24, 2021

mcscholtz Aug 26, 2021

erwango Aug 24, 2021

mcscholtz commented Aug 26, 2021

erwango left a comment

erwango Aug 30, 2021

erwango Aug 30, 2021

erwango Aug 30, 2021

ycsin commented Oct 24, 2021

MaureenHelm left a comment

carlescufi commented Dec 20, 2021

mcscholtz commented Jan 10, 2022

		LOG_ERR("Conversion time not supportted.");
		LOG_ERR("Conversion time not supported.");

Use Shared IRQ for stm32 ADC driver #37585

Use Shared IRQ for stm32 ADC driver #37585

Conversation

mcscholtz commented Aug 10, 2021 • edited by erwango Loading

mcscholtz commented Aug 10, 2021

mcscholtz commented Aug 11, 2021

mcscholtz commented Aug 15, 2021

mbolivar-nordic 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

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

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

mcscholtz commented Aug 26, 2021

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

ycsin commented Oct 24, 2021

MaureenHelm left a comment

Choose a reason for hiding this comment

carlescufi commented Dec 20, 2021

mcscholtz commented Jan 10, 2022

mcscholtz commented Aug 10, 2021 •

edited by erwango

Loading