drivers: pwm: sf32lb: add pwm driver support #99362
Conversation
zephyrbot
added
platform: SF32LB
area: Boards/SoCs
area: Devicetree Bindings
area: PWM
zephyrbot
requested review from
anangl,
cameled,
gmarull and
henrikbrixandersen
ck-telecom
force-pushed
the
sf32lb_pwm
branch
2 times, most recently
from
abaa219 to
4e3c499
Compare
ck-telecom
force-pushed
the
sf32lb_pwm
branch
from
4e3c499 to
864b69b
Compare
ck-telecom
force-pushed
the
sf32lb_pwm
branch
3 times, most recently
from
33d03b8 to
0d2a95f
Compare
ck-telecom
force-pushed
the
sf32lb_pwm
branch
2 times, most recently
from
325be13 to
8a8ae46
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds PWM driver support for the SF32LB platform, implementing a GPTIM2-based PWM controller with support for up to 4 channels.
Key Changes
- New PWM driver implementation using the General Purpose Timer (GPTIM2) peripheral
- Device tree bindings for both the GPT timer and PWM controller
- Clock control support for GPTIM2 with a fixed 24 MHz clock rate
- Integration with Zephyr's PWM subsystem including Kconfig and build system updates
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/pwm/pwm_sf32lb_gpt.c | Core PWM driver implementation with set_cycles and get_cycles_per_sec operations |
| drivers/pwm/Kconfig.sf32lb | Configuration option for SF32LB GPT-based PWM driver |
| drivers/pwm/Kconfig | Integration of SF32LB PWM Kconfig into main PWM menu |
| drivers/pwm/CMakeLists.txt | Build system integration for the new PWM driver |
| dts/bindings/pwm/sifli,sf32lb-gpt-pwm.yaml | Device tree binding for PWM controller functionality |
| dts/bindings/counter/sifli,sf32lb-gpt-timer.yaml | Device tree binding for GPT timer peripheral |
| dts/arm/sifli/sf32lb52x.dtsi | Device tree node definition for GPTIM2 with PWM child node |
| include/zephyr/drivers/clock_control/sf32lb.h | New macro for accessing clock specs from parent DT nodes |
| drivers/clock_control/clock_control_sf32lb_rcc.c | Clock rate implementation for GPTIM2 peripheral |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ck-telecom
force-pushed
the
sf32lb_pwm
branch
2 times, most recently
from
0854a15 to
b34add3
Compare
ck-telecom
force-pushed
the
sf32lb_pwm
branch
from
b34add3 to
da3de36
Compare
|
|
||
| prescaler: | ||
| type: int | ||
| default: 0 |
There was a problem hiding this comment.
Defaults need justification per guidelines
There was a problem hiding this comment.
Why? This is not a generic binding, but rather a vendor-specific one.
There was a problem hiding this comment.
Others eg ST do this for the same property, what is the rule here?
There was a problem hiding this comment.
See also another case with prefix on a vendor specific binding: https://elixir.bootlin.com/linux/v6.17.8/source/Documentation/devicetree/bindings/mfd/st,stm32-timers.yaml#L92
There was a problem hiding this comment.
Defaults need justification per guidelines
register value is 0 by default, it means 1:1
This needs to be added to description then (see RCC PR as a reference)
There was a problem hiding this comment.
The devicetree specification v0.4 says:
As I read that, non-standard properties on standard nodes need to use a prefix. Using a property prefix on a non-standard node does not make much sense to me.
So doesn't this qualify as a non-standard property? I'd keep consistency with existing bindings here, ie, use sifli,prescaler.
There was a problem hiding this comment.
see dts/bingdings/counter/*, always using prescaler, even st
There was a problem hiding this comment.
see dts/bingdings/counter/*, always using
prescaler, even st
See dts/bindings/timer/st,stm32-timers.yaml (and binding should be under timer, not counter, see e.g. https://elixir.bootlin.com/linux/v6.16.6/source/Documentation/devicetree/bindings/timer/st,stm32-timer.yaml)
There was a problem hiding this comment.
drivers/timer/* for os ticks, drivers/counter for generic timer, so is it okay under dts/bindings/counter?
ck-telecom
force-pushed
the
sf32lb_pwm
branch
from
da3de36 to
1c4d051
Compare
ck-telecom
force-pushed
the
sf32lb_pwm
branch
4 times, most recently
from
ebb2c2c to
8b283c0
Compare
Add GPT timer bindings for sf32lb platform Signed-off-by: Qingsong Gou <[email protected]>
Add gpt based pwm device bindings for sf32lb platform Signed-off-by: Qingsong Gou <[email protected]>
Add gpt2 controller for sf32lb platform Signed-off-by: Qingsong Gou <[email protected]>
Add SF32LB_CLOCK_DT_INST_PARENT_SPEC_GET clock macro Signed-off-by: Qingsong Gou <[email protected]>
ck-telecom
force-pushed
the
sf32lb_pwm
branch
from
8b283c0 to
56a44b0
Compare
Add gpt based pwm driver for sf32lb platform Signed-off-by: Qingsong Gou <[email protected]>
ck-telecom
force-pushed
the
sf32lb_pwm
branch
from
56a44b0 to
7b4bb66
Compare
|
4 participants
GPTIM2 based pwm