Add STM32 LPTIM PWM driver #3
Conversation
WalkthroughThis change introduces a new STM32 LPTIM-based PWM driver for Zephyr, extending PWM support with device tree bindings, driver implementation, build configuration, and sample integration for the stm32h573i_dk board. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
tpambor
force-pushed
the
lptim-pwm-stm32
branch
from
3300a7f to
9d390f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
dts/bindings/pwm/st,stm32-lptim-pwm.yaml (1)
22-26: Consider documenting valid channel range.The driver restricts channels to 1–2. Mentioning this in the binding description would help users avoid runtime errors.
description: | Number of items to expect in a PWM - - channel of the timer used for PWM + - channel of the timer used for PWM (valid values: 1 or 2) - period to set in ns - flags : combination of standard flags like PWM_POLARITY_NORMALdrivers/pwm/pwm_stm32_lptim.c (3)
29-35: Moveresetto the config struct.The reset specification is derived from DT and is constant at runtime. Placing it in the data struct is unconventional; Zephyr drivers typically store DT-derived constants in the config struct.
struct pwm_stm32_lptim_data { /** Timer clock (Hz). */ uint32_t tim_clk; - /* Reset controller device configuration */ - const struct reset_dt_spec reset; }; /** PWM configuration. */ struct pwm_stm32_lptim_config { LPTIM_TypeDef *timer; uint32_t prescaler; const struct stm32_pclken *pclken; size_t pclk_len; const struct pinctrl_dev_config *pcfg; + const struct reset_dt_spec reset; };Then update the macro and init function accordingly.
201-202: Handle reset failure.
reset_line_toggle_dt()can fail. Ignoring the return value may leave the peripheral in an undefined state. Consider logging or propagating the error./* Reset timer to default state using RCC */ - (void)reset_line_toggle_dt(&data->reset); + r = reset_line_toggle_dt(&data->reset); + if (r < 0) { + LOG_WRN("Failed to reset timer (%d)", r); + }
119-123: Clarify the 100% duty cycle magic value.Setting
pulse_cycles = 0xfffffor 100% duty relies on compare ≥ ARR yielding constant output. A brief comment clarifying this LPTIM behavior would aid maintainability./* hardware does not support generating 0% duty cycle * workaround by setting to 100% and inverting the polarity */ polarity = (polarity == LL_LPTIM_OUTPUT_POLARITY_REGULAR) ? LL_LPTIM_OUTPUT_POLARITY_INVERSE : LL_LPTIM_OUTPUT_POLARITY_REGULAR; - /* set to max to have 100% duty cycle (of inverse) */ + /* LPTIM outputs constant level when compare >= ARR; use max value */ pulse_cycles = 0xffff; } else if (pulse_cycles == period_cycles) { - /* set to max to have 100% duty cycle */ + /* LPTIM outputs constant level when compare >= ARR; use max value */ pulse_cycles = 0xffff;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
drivers/pwm/CMakeLists.txt(1 hunks)drivers/pwm/Kconfig.stm32(1 hunks)drivers/pwm/pwm_stm32_lptim.c(1 hunks)dts/bindings/pwm/st,stm32-lptim-pwm.yaml(1 hunks)samples/drivers/led/pwm/boards/stm32h573i_dk.overlay(1 hunks)west.yml(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.18.0)
drivers/pwm/pwm_stm32_lptim.c
[error] 54-54: #error "Invalid/unknown toolchain configuration"
(preprocessorErrorDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run compliance checks on patch series (PR)
🔇 Additional comments (6)
drivers/pwm/CMakeLists.txt (1)
69-69: LGTM!The new LPTIM PWM driver source is correctly added within the sorted block and follows the established pattern.
drivers/pwm/Kconfig.stm32 (1)
17-25: LGTM!The Kconfig symbol follows the same pattern as
PWM_STM32with appropriate dependencies and selections.samples/drivers/led/pwm/boards/stm32h573i_dk.overlay (2)
22-32: LGTM!The LPTIM5 configuration correctly applies the new
st,stm32-lptim-pwmcompatible, dual clock sources, and proper pinctrl binding for channel 2.
1-6: The overlay does not require an explicit PWM header include. The PWM macros (PWM_MSECandPWM_POLARITY_INVERTED) used on line 16 are already available through the board's device tree source hierarchy: the overlay is compiled in the context ofstm32h573i_dk.dts, which includesstm32h573i_dk-common.dtsi→stm32h573Xi.dtsi→stm32h573.dtsi→stm32h563.dtsi→stm32h562.dtsi→stm32h5.dtsi. Thestm32h5.dtsifile already includes<zephyr/dt-bindings/pwm/pwm.h>, making these macros available to the overlay without duplication.Likely an incorrect or invalid review comment.
drivers/pwm/pwm_stm32_lptim.c (2)
50-59: LGTM!The channel-to-LL mappings and compare function pointers are cleanly organized for the two supported channels.
Note: The static analysis hint about a preprocessor error is a false positive caused by analyzing the code without the STM32 HAL headers.
160-223: LGTM!The initialization sequence is thorough: clock enable, source configuration, reset, pinctrl, and LPTIM peripheral setup are properly sequenced with appropriate error handling.
| - name: hal_stm32 | ||
| revision: 9d05ebdff47b5071fa092de243a1244e7c27f518 | ||
| revision: pull/330/head | ||
| path: modules/hal/stm32 | ||
| groups: | ||
| - hal |
There was a problem hiding this comment.
Avoid using PR references in manifest revisions.
Using pull/330/head as a revision is unstable—it tracks a mutable PR branch that can be force-pushed, rebased, or deleted. Once the upstream PR is merged, replace this with the resulting stable commit hash.
- name: hal_stm32
- revision: pull/330/head
+ revision: <merged-commit-hash>
path: modules/hal/stm32📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: hal_stm32 | |
| revision: 9d05ebdff47b5071fa092de243a1244e7c27f518 | |
| revision: pull/330/head | |
| path: modules/hal/stm32 | |
| groups: | |
| - hal | |
| - name: hal_stm32 | |
| revision: <merged-commit-hash> | |
| path: modules/hal/stm32 | |
| groups: | |
| - hal |
🤖 Prompt for AI Agents
In west.yml around lines 257-261, the revision field currently uses an unstable
PR ref ("pull/330/head"); change it to a stable commit reference by replacing
the PR ref with the final upstream commit hash (or a tag) that corresponds to
the merged PR. Update the revision value to the specific commit SHA (or a
released tag), verify the path and group entries remain unchanged, and run west
manifest fetch/update to ensure the new revision is resolvable.
9d390f4 to
85c2d2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
drivers/pwm/pwm_stm32_lptim.c (1)
201-202: Check reset line validity before toggling.The reset line toggle return value is explicitly ignored. While the driver might work without reset, silently ignoring errors could hide configuration issues.
Apply this diff to check if reset is configured:
data->tim_clk = tim_clk; - /* Reset timer to default state using RCC */ - (void)reset_line_toggle_dt(&data->reset); + /* Reset timer to default state using RCC if configured */ + if (data->reset.dev != NULL) { + r = reset_line_toggle_dt(&data->reset); + if (r < 0) { + LOG_WRN("Timer reset failed (%d)", r); + } + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
drivers/pwm/CMakeLists.txt(1 hunks)drivers/pwm/Kconfig.stm32(1 hunks)drivers/pwm/pwm_stm32_lptim.c(1 hunks)dts/bindings/pwm/st,stm32-lptim-pwm.yaml(1 hunks)samples/drivers/led/pwm/boards/stm32h573i_dk.overlay(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- dts/bindings/pwm/st,stm32-lptim-pwm.yaml
- drivers/pwm/CMakeLists.txt
- drivers/pwm/Kconfig.stm32
- samples/drivers/led/pwm/boards/stm32h573i_dk.overlay
🧰 Additional context used
🪛 Cppcheck (2.18.0)
drivers/pwm/pwm_stm32_lptim.c
[error] 54-54: #error "Invalid/unknown toolchain configuration"
(preprocessorErrorDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Scan code for licenses
- GitHub Check: Analyze (python)
- GitHub Check: Run compliance checks on patch series (PR)
🔇 Additional comments (6)
drivers/pwm/pwm_stm32_lptim.c (6)
1-44: LGTM!The copyright headers, includes, and data structure definitions are well-organized and appropriate for an STM32 LPTIM PWM driver.
46-59: LGTM!The channel mapping arrays provide a clean abstraction for the two LPTIM channels.
213-224: LGTM!The LPTIM hardware initialization correctly configures the clock source, prescaler (using clever bit manipulation to compute log₂), and operating mode. The prescaler calculation at line 215 reinforces the need for validation that
cfg->prescaleris a power of 2.
227-249: LGTM!The device instantiation macros follow standard Zephyr patterns and correctly extract all necessary configuration from device tree.
80-142: The duty cycle adjustments are correct and align with standard LPTIM hardware behavior. When the compare value is greater than the auto-reload value, the output is held at '1, producing 100% duty cycle. The -1 adjustments follow the standard STM32 pattern (confirmed in official STM32CubeL0 examples where period and pulse values are decremented by 1), accounting for the counter starting from 0. The implementation correctly:
- Sets
pulse_cycles = 0xfffffor 100% duty (line 121-123), which exceeds any 16-bit ARR value- Reduces pulse by 1 for intermediate duty cycles (line 126)
- Reduces period by 1 (line 130)
- Uses polarity inversion as a valid workaround for the hardware's inability to generate 0% duty cycle
No changes needed.
68-78: Polarity inversion is correct for LPTIM hardware.The mapping of
PWM_POLARITY_NORMALtoLL_LPTIM_OUTPUT_POLARITY_INVERSEis confirmed as LPTIM-specific behavior. Comparison with the regular STM32 PWM driver (pwm_stm32.c) shows it mapsPWM_POLARITY_NORMALtoLL_TIM_OCPOLARITY_HIGH(normal mapping), while LPTIM requires the inverted mapping. The existing comment adequately documents this hardware-specific requirement.
| static int pwm_stm32_lptim_get_cycles_per_sec(const struct device *dev, uint32_t channel, | ||
| uint64_t *cycles) | ||
| { | ||
| struct pwm_stm32_lptim_data *data = dev->data; | ||
| const struct pwm_stm32_lptim_config *cfg = dev->config; | ||
|
|
||
| *cycles = (uint64_t)(data->tim_clk / cfg->prescaler); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Add prescaler validation to prevent division by zero.
The function divides by cfg->prescaler without validation. If the prescaler is zero or not a power of 2 (1, 2, 4, 8, 16, 32, 64, 128), this will cause either a division-by-zero error or incorrect frequency calculations.
Add validation in the init function to ensure prescaler is valid:
static int pwm_stm32_lptim_init(const struct device *dev)
{
struct pwm_stm32_lptim_data *data = dev->data;
const struct pwm_stm32_lptim_config *cfg = dev->config;
LPTIM_TypeDef *timer = cfg->timer;
const struct device *clk = DEVICE_DT_GET(STM32_CLOCK_CONTROL_NODE);
uint32_t tim_clk;
int r;
+ /* Validate prescaler is a power of 2 and non-zero */
+ if (cfg->prescaler == 0 || (cfg->prescaler & (cfg->prescaler - 1)) != 0) {
+ LOG_ERR("Invalid prescaler value: %u (must be power of 2)", cfg->prescaler);
+ return -EINVAL;
+ }
+
if (!device_is_ready(clk)) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drivers/pwm/pwm_stm32_lptim.c around lines 144 to 153, the function divides
by cfg->prescaler without validating it; add prescaler validation during driver
init to prevent division-by-zero or wrong calculations by checking
cfg->prescaler is non-zero and one of the allowed LPTIM prescaler values
(1,2,4,8,16,32,64,128), return a negative error code (e.g., -EINVAL) if invalid,
and keep get_cycles_per_sec relying on the validated prescaler (or defensively
check again and return error) so cycles calculation cannot divide by zero or use
unsupported prescaler values.
| r = clock_control_on(clk, (clock_control_subsys_t)&cfg->pclken[0]); | ||
| if (r < 0) { | ||
| LOG_ERR("Could not initialize clock (%d)", r); | ||
| return r; | ||
| } | ||
|
|
||
| if (cfg->pclk_len > 1) { | ||
| /* Enable Timer clock source */ | ||
| r = clock_control_configure(clk, (clock_control_subsys_t)&cfg->pclken[1], NULL); | ||
| if (r != 0) { | ||
| LOG_ERR("Could not configure clock (%d)", r); | ||
| return r; | ||
| } | ||
|
|
||
| r = clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[1], &tim_clk); | ||
| if (r < 0) { | ||
| LOG_ERR("Timer clock rate get error (%d)", r); | ||
| return r; | ||
| } | ||
| } else { | ||
| LOG_ERR("Timer clock source is not specified"); | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
Fix resource leak when clock configuration is insufficient.
If pclk_len <= 1, the function returns -EINVAL (line 195-196) but leaves the peripheral clock enabled from line 175. This wastes power and leaks the resource.
Apply this diff to disable the clock before returning:
if (cfg->pclk_len > 1) {
/* Enable Timer clock source */
r = clock_control_configure(clk, (clock_control_subsys_t)&cfg->pclken[1], NULL);
if (r != 0) {
LOG_ERR("Could not configure clock (%d)", r);
+ (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]);
return r;
}
r = clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[1], &tim_clk);
if (r < 0) {
LOG_ERR("Timer clock rate get error (%d)", r);
+ (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]);
return r;
}
} else {
LOG_ERR("Timer clock source is not specified");
+ (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]);
return -EINVAL;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| r = clock_control_on(clk, (clock_control_subsys_t)&cfg->pclken[0]); | |
| if (r < 0) { | |
| LOG_ERR("Could not initialize clock (%d)", r); | |
| return r; | |
| } | |
| if (cfg->pclk_len > 1) { | |
| /* Enable Timer clock source */ | |
| r = clock_control_configure(clk, (clock_control_subsys_t)&cfg->pclken[1], NULL); | |
| if (r != 0) { | |
| LOG_ERR("Could not configure clock (%d)", r); | |
| return r; | |
| } | |
| r = clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[1], &tim_clk); | |
| if (r < 0) { | |
| LOG_ERR("Timer clock rate get error (%d)", r); | |
| return r; | |
| } | |
| } else { | |
| LOG_ERR("Timer clock source is not specified"); | |
| return -EINVAL; | |
| } | |
| r = clock_control_on(clk, (clock_control_subsys_t)&cfg->pclken[0]); | |
| if (r < 0) { | |
| LOG_ERR("Could not initialize clock (%d)", r); | |
| return r; | |
| } | |
| if (cfg->pclk_len > 1) { | |
| /* Enable Timer clock source */ | |
| r = clock_control_configure(clk, (clock_control_subsys_t)&cfg->pclken[1], NULL); | |
| if (r != 0) { | |
| LOG_ERR("Could not configure clock (%d)", r); | |
| (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]); | |
| return r; | |
| } | |
| r = clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[1], &tim_clk); | |
| if (r < 0) { | |
| LOG_ERR("Timer clock rate get error (%d)", r); | |
| (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]); | |
| return r; | |
| } | |
| } else { | |
| LOG_ERR("Timer clock source is not specified"); | |
| (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]); | |
| return -EINVAL; | |
| } |
🤖 Prompt for AI Agents
In drivers/pwm/pwm_stm32_lptim.c around lines 175 to 197, the function enables
the peripheral clock at line 175 but returns early when cfg->pclk_len <= 1,
leaking the enabled clock; fix by calling clock_control_off(clk,
(clock_control_subsys_t)&cfg->pclken[0]) before each early return in this block
(the -EINVAL return and any other error returns in this branch) so the
peripheral clock is disabled on all error paths, and propagate the original
error code after turning the clock off.
85c2d2a to
382711e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
drivers/pwm/pwm_stm32_lptim.c (2)
144-153: Add prescaler validation to prevent division by zero.The function divides by
cfg->prescalerat line 150 without validation. If the prescaler is zero or not a valid power of 2 (1, 2, 4, 8, 16, 32, 64, 128), this will cause either division-by-zero or incorrect calculations. This validation should be added in the init function as previously suggested.
181-197: Fix resource leak by disabling the peripheral clock on error paths.The peripheral clock enabled at line 175 is not disabled before returning errors at lines 186, 192, and 196. This leaves the clock running unnecessarily and leaks the resource.
🧹 Nitpick comments (1)
drivers/pwm/pwm_stm32_lptim.c (1)
202-202: Consider handling the reset toggle return value.The return value from
reset_line_toggle_dtis explicitly discarded. If the reset operation can fail in a way that affects driver functionality, this error should be handled. If reset is optional or failure is acceptable, a comment explaining why would improve clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
drivers/pwm/CMakeLists.txt(1 hunks)drivers/pwm/Kconfig.stm32(1 hunks)drivers/pwm/pwm_stm32_lptim.c(1 hunks)dts/bindings/pwm/st,stm32-lptim-pwm.yaml(1 hunks)samples/drivers/led/pwm/boards/stm32h573i_dk.overlay(1 hunks)samples/drivers/led/pwm/sample.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- drivers/pwm/Kconfig.stm32
- dts/bindings/pwm/st,stm32-lptim-pwm.yaml
- drivers/pwm/CMakeLists.txt
- samples/drivers/led/pwm/boards/stm32h573i_dk.overlay
🧰 Additional context used
🪛 Cppcheck (2.18.0)
drivers/pwm/pwm_stm32_lptim.c
[error] 54-54: #error "Invalid/unknown toolchain configuration"
(preprocessorErrorDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (python)
- GitHub Check: Run coding guidelines checks on patch series (PR)
- GitHub Check: Scan code for licenses
- GitHub Check: Run compliance checks on patch series (PR)
🔇 Additional comments (9)
samples/drivers/led/pwm/sample.yaml (1)
12-12: LGTM!The addition of
stm32h573i_dkto the integration platforms aligns with the new STM32 LPTIM PWM driver support introduced in this PR.drivers/pwm/pwm_stm32_lptim.c (8)
1-27: LGTM!The copyright headers, license identifier, and includes are appropriate for an STM32 LPTIM PWM driver.
29-44: LGTM!The data and configuration structures follow Zephyr driver patterns and contain the necessary fields for managing LPTIM PWM peripherals.
46-59: LGTM!The channel mapping arrays provide a clean abstraction for the two LPTIM channels and their corresponding LL API functions.
80-142: LGTM!The function correctly handles:
- Channel and period validation
- Hardware limitation workarounds for 0% duty cycle (polarity inversion)
- Period and pulse cycle adjustments for off-by-one compensation
- Dynamic channel configuration and enabling
160-179: LGTM!The clock control device readiness check and peripheral clock enabling are correctly implemented with appropriate error handling.
211-224: LGTM!The timer initialization sequence (clock source, auto-reload, update mode, enable, and start) follows the correct procedure for STM32 LPTIM, assuming the prescaler validation issue is addressed.
227-249: LGTM!The device instantiation macro follows Zephyr conventions correctly, properly initializing per-instance data, configuration structures, and device registration.
68-78: Verify the polarity mapping against STM32 LPTIM documentation.The mapping inverts the expected polarity (PWM_POLARITY_NORMAL maps to LL_LPTIM_OUTPUT_POLARITY_INVERSE). The comment explains this behavior is intentional, but the specific register-level semantics of the WAVEPOL bit and why the inversion is required should be confirmed against the STM32 LPTIM reference manual.
drivers/pwm/pwm_stm32_lptim.c
Outdated
|
|
||
| /* Initialize timer */ | ||
| LL_LPTIM_SetClockSource(timer, LL_LPTIM_CLK_SOURCE_INTERNAL); | ||
| LL_LPTIM_SetPrescaler(timer, (__CLZ(__RBIT(cfg->prescaler)) << LPTIM_CFGR_PRESC_Pos)); |
There was a problem hiding this comment.
Prescaler encoding assumes power-of-2 without validation.
The prescaler register encoding at line 215 uses (__CLZ(__RBIT(cfg->prescaler)) << LPTIM_CFGR_PRESC_Pos), which computes log₂ assuming cfg->prescaler is a power of 2. If the prescaler value from device tree is not a valid power of 2 (1, 2, 4, 8, 16, 32, 64, 128), this will produce incorrect register values. This must be validated earlier in the init function as suggested in the previous review comment for lines 144-153.
🤖 Prompt for AI Agents
In drivers/pwm/pwm_stm32_lptim.c around line 215, the prescaler encoding assumes
cfg->prescaler is a power of two and computes log2 via __CLZ/__RBIT; add an
explicit validation earlier in the init (as referenced near lines 144-153) to
check cfg->prescaler is one of the allowed values (1,2,4,8,16,32,64,128), and if
not return an error (e.g., -EINVAL) and log a clear message; only after that
validation compute the prescaler index (log2) and write the encoded value into
the register so invalid DT values cannot produce incorrect encodings.
Add a PWM driver for the STM32 low-power timer (LPTIM) peripheral on STM32 MCUs. Signed-off-by: Tim Pambor <tim.pambor@codewrights.de>
Add overlay and update sample.yaml to include stm32h573i_dk board. This board can utilize LPTIM5 for PWM generation to drive the red (LD3) LED. Signed-off-by: Tim Pambor <tim.pambor@codewrights.de>
tpambor
force-pushed
the
lptim-pwm-stm32
branch
from
382711e to
39f0c3e
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
added
manifest-hal_silabs
manifest-hal_infineon
manifest-hal_nordic
manifest-hal_renesas
and removed
DNM (manifest)
labels
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
drivers/pwm/pwm_stm32_lptim.c (3)
142-151: Critical: Division by prescaler without validation.Line 148 divides by
cfg->prescalerwithout validation. If the device tree specifies an invalid prescaler (0 or non-power-of-2), this will cause division-by-zero or incorrect frequency calculations.The prescaler must be validated during initialization (see init function review). Valid LPTIM prescaler values are: 1, 2, 4, 8, 16, 32, 64, 128.
172-189: Major: Resource leak on timer clock configuration errors.The peripheral clock is enabled at line 172 but never disabled if subsequent operations fail (lines 180-182, 186-188). This wastes power and leaks the resource.
🔎 Proposed fix to disable clock on error paths
/* Enable Timer clock source */ r = clock_control_configure(clk, (clock_control_subsys_t)&cfg->pclken[1], NULL); if (r != 0) { LOG_ERR("Could not configure clock (%d)", r); + (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]); return r; } r = clock_control_get_rate(clk, (clock_control_subsys_t)&cfg->pclken[1], &data->tim_clk); if (r < 0) { LOG_ERR("Timer clock rate get error (%d)", r); + (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]); return r; }
202-210: Critical: Prescaler encoding assumes power-of-2 without validation.Line 203 computes the prescaler register value using
__CLZ(__RBIT(cfg->prescaler)), which calculates log₂ and only works correctly whencfg->prescaleris a power of 2 (1, 2, 4, 8, 16, 32, 64, 128). If the device tree specifies an invalid prescaler value, this produces incorrect hardware configuration.Add validation at the start of the init function before any resources are allocated.
🔎 Proposed validation at function start
static int pwm_stm32_lptim_init(const struct device *dev) { struct pwm_stm32_lptim_data *data = dev->data; const struct pwm_stm32_lptim_config *cfg = dev->config; LPTIM_TypeDef *timer = cfg->timer; const struct device *clk = DEVICE_DT_GET(STM32_CLOCK_CONTROL_NODE); int r; + /* Validate prescaler is a power of 2 between 1 and 128 */ + if (cfg->prescaler == 0 || cfg->prescaler > 128 || + (cfg->prescaler & (cfg->prescaler - 1)) != 0) { + LOG_ERR("Invalid prescaler: %u (must be power of 2, 1-128)", cfg->prescaler); + return -EINVAL; + } + if (!device_is_ready(clk)) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
drivers/pwm/CMakeLists.txt(1 hunks)drivers/pwm/Kconfig.stm32(1 hunks)drivers/pwm/pwm_stm32_lptim.c(1 hunks)dts/bindings/pwm/st,stm32-lptim-pwm.yaml(1 hunks)samples/drivers/led/pwm/boards/stm32h573i_dk.overlay(1 hunks)samples/drivers/led/pwm/sample.yaml(1 hunks)
🧰 Additional context used
🪛 Cppcheck (2.18.0)
drivers/pwm/pwm_stm32_lptim.c
[error] 54-54: #error "Invalid/unknown toolchain configuration"
(preprocessorErrorDirective)
🔇 Additional comments (12)
drivers/pwm/Kconfig.stm32 (1)
17-25: LGTM! Clean Kconfig integration.The new LPTIM PWM configuration follows the same pattern as the existing PWM_STM32 driver, with appropriate dependencies and selections for RESET and PINCTRL subsystems.
drivers/pwm/pwm_stm32_lptim.c (7)
29-44: Clean data structure design.The separation between runtime data (tim_clk, reset spec) and const configuration (timer registers, prescaler, clocks, pinctrl) follows Zephyr device driver best practices.
46-59: Good use of lookup tables for channel abstraction.The channel-to-LL mappings provide clean abstraction over the hardware channel configuration APIs.
68-76: Polarity mapping correctly documented.The hardware-specific polarity inversion is clearly documented and correctly implemented.
78-140: Well-handled hardware constraints with appropriate workarounds.The function correctly manages LPTIM hardware limitations:
- 0% duty cycle workaround using polarity inversion (lines 110-118) is a clever solution
- 16-bit period enforcement prevents overflow
- Off-by-one adjustments (lines 124, 128) account for timer counting behavior
- On-demand channel enabling avoids unnecessary state changes
158-169: Clock device validation is correct.Proper device readiness check before proceeding with clock operations.
217-238: Good use of BUILD_ASSERT for compile-time validation.The BUILD_ASSERT at line 224 ensures timer clock source is always specified in the device tree, preventing runtime errors from missing configuration. The device instantiation macro follows standard Zephyr patterns.
54-54: Static analysis false positive - safe to ignore.The cppcheck preprocessor error is a false positive. Line 54 is part of a function pointer array initialization, not a preprocessor directive. This occurs because cppcheck lacks full Zephyr build environment context.
drivers/pwm/CMakeLists.txt (1)
69-69: LGTM! Proper build integration.The new driver source is correctly added within the sorted section and follows the established conditional compilation pattern.
samples/drivers/led/pwm/sample.yaml (1)
12-12: Good integration test coverage.Adding stm32h573i_dk as an integration platform ensures the new LPTIM PWM driver is tested with the LED PWM sample.
dts/bindings/pwm/st,stm32-lptim-pwm.yaml (1)
1-31: Well-structured device tree binding.The binding correctly:
- Inherits from base LPTIM binding while blocking inappropriate properties (st,timeout is for input capture)
- Defines 3 PWM cells following standard PWM controller conventions
- Requires pinctrl configuration as expected for hardware PWM output
samples/drivers/led/pwm/boards/stm32h573i_dk.overlay (1)
22-34: Board overlay configuration is correct.The LPTIM5 configuration is appropriate for the STM32H573I-DK board:
- LSE clock source (32.768 kHz) is suitable for low-power PWM
- Prescaler of 2 (valid power-of-2 value) yields 16.384 kHz counter frequency
- 20ms PWM period fits comfortably within the 16-bit timer range
- Channel 2 pinout matches the LD3 red LED on the board
| } | ||
|
|
||
| /* Reset timer to default state using RCC */ | ||
| (void)reset_line_toggle_dt(&data->reset); |
There was a problem hiding this comment.
Minor: Check reset operation return value.
The reset toggle return value is cast to void, discarding potential error information. While reset failures are rare, checking the return value would improve robustness.
🔎 Suggested improvement
- (void)reset_line_toggle_dt(&data->reset);
+ r = reset_line_toggle_dt(&data->reset);
+ if (r < 0) {
+ LOG_WRN("Reset failed (%d), continuing anyway", r);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (void)reset_line_toggle_dt(&data->reset); | |
| r = reset_line_toggle_dt(&data->reset); | |
| if (r < 0) { | |
| LOG_WRN("Reset failed (%d), continuing anyway", r); | |
| } |
🤖 Prompt for AI Agents
In drivers/pwm/pwm_stm32_lptim.c around line 192, the call to
reset_line_toggle_dt(&data->reset) currently casts the result to void and
discards errors; capture the return value, check it for failure, and handle
failures instead of ignoring them — e.g., int rc =
reset_line_toggle_dt(&data->reset); if (rc) { LOG_ERR("reset_line_toggle_dt
failed: %d", rc); return rc; } (or propagate/translate the error per the
surrounding function's error-handling convention).
| r = pinctrl_apply_state(cfg->pcfg, PINCTRL_STATE_DEFAULT); | ||
| if (r < 0) { | ||
| LOG_ERR("PWM pinctrl setup failed (%d)", r); | ||
| return r; | ||
| } |
There was a problem hiding this comment.
Major: Pinctrl error path leaks enabled clocks.
If pinctrl configuration fails, the function returns without disabling the peripheral and timer clocks enabled earlier (lines 172 and 178-179).
🔎 Proposed fix
r = pinctrl_apply_state(cfg->pcfg, PINCTRL_STATE_DEFAULT);
if (r < 0) {
LOG_ERR("PWM pinctrl setup failed (%d)", r);
+ (void)clock_control_off(clk, (clock_control_subsys_t)&cfg->pclken[0]);
return r;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drivers/pwm/pwm_stm32_lptim.c around lines 172, 178-179 and 195-199, the
pinctrl error path returns early and leaks the peripheral and timer clocks
enabled previously; modify the error path so that before returning on
pinctrl_apply_state failure you explicitly disable the timer clock and the
peripheral clock (matching the order used for enable) using the same
clock_control_off calls (and handle/ignore their return values or log failures),
then return the original pinctrl error code; ensure both clocks are always
turned off on any early exit after they were enabled.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.