Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions drivers/comparator/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ zephyr_library_sources_ifdef(CONFIG_COMPARATOR_NRF_COMP comparator_nrf_comp.c)
zephyr_library_sources_ifdef(CONFIG_COMPARATOR_NRF_LPCOMP comparator_nrf_lpcomp.c)
zephyr_library_sources_ifdef(CONFIG_COMPARATOR_SHELL comparator_shell.c)
zephyr_library_sources_ifdef(CONFIG_COMPARATOR_RENESAS_RA comparator_renesas_ra.c)
zephyr_library_sources_ifdef(CONFIG_COMPARATOR_STM32_COMP comparator_stm32_comp.c)
1 change: 1 addition & 0 deletions drivers/comparator/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ rsource "Kconfig.nrf_comp"
rsource "Kconfig.nrf_lpcomp"
rsource "Kconfig.shell"
rsource "Kconfig.renesas_ra"
rsource "Kconfig.stm32_comp"

endif # COMPARATOR
9 changes: 9 additions & 0 deletions drivers/comparator/Kconfig.stm32_comp
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Copyright (c) 2025 Alexander Kozhinov <[email protected]>
# SPDX-License-Identifier: Apache-2.0

config COMPARATOR_STM32_COMP
bool "ST STM32 comparator driver"
default y
depends on DT_HAS_ST_STM32_COMP_ENABLED
select PINCTRL
select EXTI_STM32
345 changes: 345 additions & 0 deletions drivers/comparator/comparator_stm32_comp.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,345 @@
/*
* Copyright (c) 2025 Alexander Kozhinov <[email protected]>
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/irq.h>
#include <zephyr/kernel.h>
#include <zephyr/pm/device.h>
#include <zephyr/logging/log.h>
#include <zephyr/sys/util_macro.h>
#include <zephyr/drivers/pinctrl.h>
#include <zephyr/drivers/comparator.h>
#include <zephyr/drivers/clock_control.h>
#include <zephyr/drivers/clock_control/stm32_clock_control.h>
#include <zephyr/drivers/interrupt_controller/intc_exti_stm32.h>

#include <stm32_ll_comp.h>
#include <stm32_ll_system.h>

LOG_MODULE_REGISTER(stm32_comp, CONFIG_COMPARATOR_LOG_LEVEL);

#define DT_DRV_COMPAT st_stm32_comp

#define LL_COMP_INPUT_PLUS_IN0 LL_COMP_INPUT_PLUS_IO1
#define LL_COMP_INPUT_PLUS_IN1 LL_COMP_INPUT_PLUS_IO2

#define LL_COMP_INPUT_MINUS_IN0 LL_COMP_INPUT_MINUS_IO1
#define LL_COMP_INPUT_MINUS_IN1 LL_COMP_INPUT_MINUS_IO2

#define STM32_COMP_DT_INST_P_IN(inst) \
CONCAT(LL_COMP_INPUT_PLUS_, DT_INST_STRING_TOKEN(inst, positive_input))

#define STM32_COMP_DT_INST_N_IN(inst) \
CONCAT(LL_COMP_INPUT_MINUS_, DT_INST_STRING_TOKEN(inst, negative_input))

#define STM32_COMP_DT_INST_HYST_MODE(inst) \
CONCAT(LL_COMP_HYSTERESIS_, DT_INST_STRING_TOKEN(inst, hysteresis))

#define STM32_COMP_DT_INST_INV_OUT(inst) \
CONCAT(LL_COMP_OUTPUTPOL_, DT_INST_STRING_TOKEN(inst, invert_output))

#define STM32_COMP_DT_INST_BLANK_SEL(inst) \
CONCAT(LL_COMP_BLANKINGSRC_, DT_INST_STRING_TOKEN(inst, st_blank_sel))

#define STM32_COMP_DT_INST_LOCK(inst) DT_INST_PROP(inst, st_lock_enable)

#define STM32_COMP_DT_MILLER_EFFECT_HOLD_DISABLE(inst) \
DT_INST_PROP_OR(inst, st_miller_effect_hold_disable, false)

#define STM32_COMP_DT_EXTI_LINE_NUMBER(inst) DT_INST_PROP(inst, st_exti_line)

/* Value 0 always relates to the default value of COMP PWRMODE bit field */
#define STM32_COMP_DT_POWER_MODE(inst) \
COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, st_power_mode), \
(CONCAT(LL_COMP_POWERMODE_, \
DT_INST_STRING_TOKEN(inst, st_power_mode))), \
(0))
Comment on lines +54 to +57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation suggestion

	COND_CODE_1(DT_INST_NODE_HAS_PROP(inst, st_power_mode),			\
		    (CONCAT(LL_COMP_POWERMODE_,					\
			    DT_INST_STRING_TOKEN(inst, st_power_mode))),	\
		    (0))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we leave it as it is? There are already style checks successfully running CI. In my EXTI driver PR there were already a long description, why and how other interpretations than in CI are pretty subjective and could have different outcomes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is not a blocking issue. Yet, it would make the code more readable for others and from a maintenance perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty common reason for such request - readability of the code :) But. There is a big interpretation difference. GitHub is know for showing the code with own indentations and thus the code may look different than checked one in the editor. The root folder of zephyr project contains.editorconfig - this file defines also for vscode how to display tabs and spaces. Also any other ide shall show you same code style. Therefore please do not rely just on that, GitHub shows you. I can assure you - you will see different visual representation as soon as you open it in vim or vscode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look here: #85508 (comment)
So far I remember, I used to use .editorconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwango Nice. Thnx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore please do not rely just on that, GitHub shows you. I can assure you - you will see different visual representation as soon as you open it in vim or vscode.

I fully agree :-) Personally use vim over various SW projects each with its own style preferences.
I don't know if Github can be tuned to adapt the tab size to the project being sourced.

There are already style checks successfully running CI.

Despite CI Compliance Checks processing reports many issues, it does not address all of them.
In the end, I think what's best is what makes the implementation easier to understand.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etienne-lms He he. Here we come to the next problem: How shall I adapt the visual appearance of the code, if that, what you see is different than, what I see? I think this could be major point of disagreement. Therefore I prefer to leave this job to proper CI configuration. No CI complaints - the code is good with allowed freedom interpretation range of the creator.

Copy link
Contributor Author

@KozhinovAlexander KozhinovAlexander Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etienne-lms BTW: I could highly recommend https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig if you use vscode with zephyr and 8 in #96325 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, leave as is if you prefer. That said, as-is tend to show CONCAT() operates on 3 arguments, while it's rather COND_CODE_1() that does.
Not a blocking issue.


struct stm32_comp_config {
COMP_TypeDef *comp;
struct stm32_pclken *pclken;
const struct pinctrl_dev_config *pincfg;
void (*irq_init)(void);
uint32_t irq_nr;
uint32_t exti_line_number;
bool lock_enable;
bool miller_effect_hold_disable;
uint32_t power_mode;
uint32_t input_plus;
uint32_t input_minus;
uint32_t hysteresis;
uint32_t invert_output;
uint32_t blank_sel;
};

struct stm32_comp_data {
comparator_callback_t callback;
void *user_data;
};

static bool stm32_comp_is_resumed(const struct device *dev)
{
#ifdef CONFIG_PM_DEVICE
enum pm_device_state state;

(void)pm_device_state_get(dev, &state);
return state == PM_DEVICE_STATE_ACTIVE;
#else
return true;
#endif /* CONFIG_PM_DEVICE */
}

static int stm32_comp_get_output(const struct device *dev)
{
const struct stm32_comp_config *cfg = dev->config;
COMP_TypeDef *comp = cfg->comp;

return LL_COMP_ReadOutputLevel(comp);
}

static int stm32_comp_set_trigger(const struct device *dev, enum comparator_trigger trigger)
{
const struct stm32_comp_config *cfg = dev->config;
struct stm32_comp_data *data = dev->data;
stm32_exti_trigger_type exti_trigger = 0U;
COMP_TypeDef *comp = cfg->comp;
int ret = 0;

switch (trigger) {
case COMPARATOR_TRIGGER_NONE:
exti_trigger = STM32_EXTI_TRIG_NONE;
break;
case COMPARATOR_TRIGGER_RISING_EDGE:
exti_trigger = STM32_EXTI_TRIG_RISING;
break;
case COMPARATOR_TRIGGER_FALLING_EDGE:
exti_trigger = STM32_EXTI_TRIG_FALLING;
break;
case COMPARATOR_TRIGGER_BOTH_EDGES:
exti_trigger = STM32_EXTI_TRIG_BOTH;
break;
default:
LOG_ERR("%s: Unsupported trigger mode %d", dev->name, trigger);
return -ENOTSUP;
}

irq_disable(cfg->irq_nr);
LL_COMP_Disable(comp);

ret = stm32_exti_enable(cfg->exti_line_number, exti_trigger,
STM32_EXTI_MODE_IT);
if (ret != 0) {
LOG_ERR("%s: EXTI init failed (%d)", dev->name, ret);
return ret;
}

if (stm32_comp_is_resumed(dev)) {
LL_COMP_Enable(comp);
}

if (data->callback != NULL) {
irq_enable(cfg->irq_nr);
}

return ret;
}

static int stm32_comp_trigger_is_pending(const struct device *dev)
{
const struct stm32_comp_config *cfg = dev->config;

if (stm32_exti_is_pending(cfg->exti_line_number)) {
stm32_exti_clear_pending(cfg->exti_line_number);
return 1;
}
return 0;
}

static int stm32_comp_set_trigger_callback(const struct device *dev, comparator_callback_t callback,
void *user_data)
{
const struct stm32_comp_config *cfg = dev->config;
struct stm32_comp_data *data = dev->data;

irq_disable(cfg->irq_nr);

data->callback = callback;
data->user_data = user_data;

irq_enable(cfg->irq_nr);

if (data->callback != NULL && stm32_comp_trigger_is_pending(dev)) {
callback(dev, user_data);
}

return 0;
}

static int stm32_comp_pm_callback(const struct device *dev, enum pm_device_action action)
{
const struct stm32_comp_config *cfg = dev->config;
COMP_TypeDef *comp = cfg->comp;

if (LL_COMP_IsLocked(comp)) {
LOG_ERR("%s is locked", dev->name);
return -EACCES;
}

if (action == PM_DEVICE_ACTION_RESUME) {
LL_COMP_Enable(comp);
if (cfg->lock_enable) {
LL_COMP_Lock(comp);
}
}

if (IS_ENABLED(CONFIG_PM_DEVICE) && action == PM_DEVICE_ACTION_SUSPEND) {
LL_COMP_Disable(comp);
}

return 0;
}

static void stm32_comp_irq_handler(const struct device *dev)
{
const struct stm32_comp_config *cfg = dev->config;
struct stm32_comp_data *data = dev->data;

if (stm32_exti_is_pending(cfg->exti_line_number)) {
stm32_exti_clear_pending(cfg->exti_line_number);
}

if (data->callback == NULL) {
return;
}

data->callback(dev, data->user_data);
}

static int stm32_comp_init(const struct device *dev)
{
const struct device *const clk = DEVICE_DT_GET(STM32_CLOCK_CONTROL_NODE);
const struct stm32_comp_config *cfg = dev->config;
COMP_TypeDef *comp = cfg->comp;
int ret = 0;

if (!device_is_ready(clk)) {
LOG_ERR("%s clock control device not ready", dev->name);
return -ENODEV;
}

/* Enable COMP bus clock */
ret = clock_control_on(clk, &cfg->pclken[0]);
if (ret != 0) {
LOG_ERR("%s clock op failed (%d)", dev->name, ret);
return ret;
}

/* Enable COMP clock source */
#ifndef CONFIG_SOC_SERIES_STM32G4X
/* stm32g4 clock configuration is not necessary,
* since it is enough to turn APB2 clock on (see: RM0440 Rev 8 767/2138)
*/
ret = clock_control_configure(clk, &cfg->pclken[1], NULL);
if (ret != 0) {
LOG_ERR("%s clock configure failed (%d)", dev->name, ret);
return ret;
}
#endif /* !CONFIG_SOC_SERIES_STM32G4X */

/* Configure COMP inputs as specified in Device Tree, if any */
ret = pinctrl_apply_state(cfg->pincfg, PINCTRL_STATE_DEFAULT);
if (ret < 0 && ret != -ENOENT) {
/*
* If the COMP is used only with internal channels, then no pinctrl is
* provided in Device Tree, and pinctrl_apply_state returns -ENOENT,
* but this should not be treated as an error.
*/
LOG_ERR("%s pinctrl setup failed (%d)", dev->name, ret);
return ret;
}

if (LL_COMP_IsLocked(comp)) {
/* COMP instance shall not be locked */
LOG_ERR("%s COMP instance is locked", dev->name);
return -EACCES;
}

#ifndef CONFIG_SOC_SERIES_STM32G4X
LL_COMP_SetPowerMode(comp, cfg->power_mode);
#endif /* !CONFIG_SOC_SERIES_STM32G4X */

LL_COMP_SetInputMinus(comp, cfg->input_minus);
LL_COMP_SetInputPlus(comp, cfg->input_plus);
LL_COMP_SetInputHysteresis(comp, cfg->hysteresis);
LL_COMP_SetOutputPolarity(comp, cfg->invert_output);
LL_COMP_SetOutputBlankingSource(comp, cfg->blank_sel);

#if defined(CONFIG_SOC_SERIES_STM32G4X)
/* Undocumented bit, refer to st,miller-effect-hold-disable DT binding */
WRITE_BIT(comp->CSR, 1, cfg->miller_effect_hold_disable ? 0 : 1);
#endif /* CONFIG_SOC_SERIES_STM32G4X */

cfg->irq_init();

return pm_device_driver_init(dev, stm32_comp_pm_callback);
}

static DEVICE_API(comparator, stm32_comp_comp_api) = {
.get_output = stm32_comp_get_output,
.set_trigger = stm32_comp_set_trigger,
.set_trigger_callback = stm32_comp_set_trigger_callback,
.trigger_is_pending = stm32_comp_trigger_is_pending,
};

#define STM32_COMP_IRQ_HANDLER_SYM(inst) stm32_comp_irq_init_##inst

#define STM32_COMP_IRQ_HANDLER_DEFINE(inst) \
\
static void STM32_COMP_IRQ_HANDLER_SYM(inst)(void) \
{ \
IRQ_CONNECT(DT_INST_IRQN(inst), DT_INST_IRQ(inst, priority), \
stm32_comp_irq_handler, DEVICE_DT_INST_GET(inst), 0); \
irq_enable(DT_INST_IRQN(inst)); \
}

#define STM32_COMP_DEVICE(inst) \
\
static struct stm32_pclken comp_##inst##_clk[] = STM32_DT_INST_CLOCKS(inst); \
\
PINCTRL_DT_INST_DEFINE(inst); \
\
static struct stm32_comp_data stm32_comp_data_##inst; \
\
STM32_COMP_IRQ_HANDLER_DEFINE(inst) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an empty line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it after line 309 (see above). Maybe code suggestion would be better next time.

static const struct stm32_comp_config stm32_comp_config_##inst = { \
.comp = (COMP_TypeDef *)DT_INST_REG_ADDR(inst), \
.pclken = comp_##inst##_clk, \
.pincfg = PINCTRL_DT_INST_DEV_CONFIG_GET(inst), \
.irq_init = STM32_COMP_IRQ_HANDLER_SYM(inst), \
.irq_nr = DT_INST_IRQN(inst), \
.exti_line_number = STM32_COMP_DT_EXTI_LINE_NUMBER(inst), \
.lock_enable = STM32_COMP_DT_INST_LOCK(inst), \
.miller_effect_hold_disable = \
STM32_COMP_DT_MILLER_EFFECT_HOLD_DISABLE(inst), \
.power_mode = STM32_COMP_DT_POWER_MODE(inst), \
.input_plus = STM32_COMP_DT_INST_P_IN(inst), \
.input_minus = STM32_COMP_DT_INST_N_IN(inst), \
.hysteresis = STM32_COMP_DT_INST_HYST_MODE(inst), \
.invert_output = STM32_COMP_DT_INST_INV_OUT(inst), \
.blank_sel = STM32_COMP_DT_INST_BLANK_SEL(inst) \
}; \
\
PM_DEVICE_DT_INST_DEFINE(inst, stm32_comp_pm_callback); \
\
DEVICE_DT_INST_DEFINE(inst, \
stm32_comp_init, \
PM_DEVICE_DT_INST_GET(inst), \
&stm32_comp_data_##inst, \
&stm32_comp_config_##inst, \
POST_KERNEL, \
CONFIG_COMPARATOR_INIT_PRIORITY, \
&stm32_comp_comp_api \
);

DT_INST_FOREACH_STATUS_OKAY(STM32_COMP_DEVICE)
Loading