raiden00pl commented on code in PR #16904:
URL: https://github.com/apache/nuttx/pull/16904#discussion_r2303157398
##########
arch/arm/src/stm32h5/stm32_adc.h:
##########
@@ -463,10 +463,88 @@
# endif
#endif
+/* IOCTL Support */
+
+#define ANIOC_STM32H5_WDOG_CFG _ANIOC(AN_STM32H5_FIRST + 0)
+#define ANIOC_STM32H5_WDOG2_CFG _ANIOC(AN_STM32H5_FIRST + 1)
+#define ANIOC_STM32H5_WDOG3_CFG _ANIOC(AN_STM32H5_FIRST + 2)
+#define ANIOC_STM32H5_WDOG_GET_EVENT _ANIOC(AN_STM32H5_FIRST + 3) /* OUT:
struct stm32_adc_wdg_event_s* */
+#define ANIOC_STM32H5_WDOG_SIGCFG _ANIOC(AN_STM32H5_FIRST + 4) /* IN:
struct stm32_adc_sigcfg_s* */
+
+/* Watchdog Event Queue Support */
+
+#define WQ_LEN 128
+#define WQ_MASK (WQ_LEN - 1)
+
/****************************************************************************
* Public Types
****************************************************************************/
+/* STM32H5 ADC channel configuration */
+
+struct stm32_adc_channel_s
+{
+ uint8_t chan; /* Channel Number */
+ uint32_t p_gpio; /* P GPIO */
+ uint32_t n_gpio; /* N GPIO */
+ uint8_t tsamp:3; /* Sampling time */
+ uint8_t mode:1; /* Single-ended 0 or differential mode 1 */
+ uint8_t injected:1; /* Regular 0 or Injected 1 */
+ uint8_t _res:3; /* Reserved */
+};
Review Comment:
> Lastly, there is precedent for this. Look at nrf53_adc.h.
`nrf53_adc.h` is not ST chip. Consistency matters between chips of the same
vendor, especially in the case of ST where peripheral IP cores are shared
between different families.
Keep in mind that someone has to maintain this code later. Chances are high
that you won't be working on this project in a year or two. Who will take over
maintenance this driver then? Most likely others that are interested in ST
chips like me. The more the code differs from the rest of ST code, the more
difficult it will be to maintain.
> However, I've seen no implementations in the stm32 families that implement
differential mode.
The ADC diff mode configuration can be done as well using `#define`'s placed
in `board.h` together with the ADC pin definitions or with Kconfig options. Is
this the correct solution? I don't know, it must be thought out and discussed.
> The stm32f7 is the only implementation I've seen that allows you to set a
non-default sample time. It doesn't make a lot of sense for the H5.
You can change sample time for `stm32`, `stm32f7`, `stm32f0l0g0` from board
initialization level, but you have to enable `CONFIG_STM32xxx_ADC_LL_OPS`.
> I'm not going to do things simply because we did them that way before.
Look at the adc_interrupt routines for stm32 architectures regarding the
watchdogs. It became clear to me, as I tested, that these interrupt service
routines make no sense. Reading DR doesn't always give you what caused the
interrupt. Turning off conversions on a watchdog interrupt also does not make
sense for all applications. Maybe scrutinize that?
and that's exactly the problem. Someone added this code long time ago and
then disappeared. Now it's unclear what the author had in mind. Most likely it
met his/her requirements and was sufficient for a given application. We don't
know.
That's why we need standardized ADC interface so we know what to expect from
a given ADC feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
