Skip to content

drivers: stepper: add tmc51xx support#88350

Merged
kartben merged 2 commits into
zephyrproject-rtos:mainfrom
apni2:apni2/add_tmc51xx_support
Apr 22, 2025
Merged

drivers: stepper: add tmc51xx support#88350
kartben merged 2 commits into
zephyrproject-rtos:mainfrom
apni2:apni2/add_tmc51xx_support

Conversation

@apni2
Copy link
Copy Markdown
Contributor

@apni2 apni2 commented Apr 9, 2025

Add tmc51xx (tmc5160) support. Most of the code can be shared with the current tmc50xx (tmc5041) implementation.

Comment thread drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx-gconf.yaml Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx-gconf.yaml Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx-gconf.yaml Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx-ramp-generator.yaml Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx.yaml Outdated
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch from 6840750 to 0463fc0 Compare April 9, 2025 10:32
Comment thread include/zephyr/drivers/stepper/stepper_trinamic.h Outdated
Comment thread drivers/stepper/adi_tmc/adi_tmc_reg.h Outdated
@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 9, 2025

This is first attempt at supporting the tmc5160. I have tried to do it as simple as possible while still reusing the tmc50xx code, but there may be a better way of doing this.

For instance the following config changes would simplify the code, but offer less flexibility in configuring the drivers.

CONFIG_STEPPER_ADI_TMC50XX_RAMPSTAT_POLL -> CONFIG_STEPPER_ADI_TMC5XXX_RAMPSTAT_POLL
CONFIG_STEPPER_ADI_TMC50XX_RAMPSTAT_POLL_INTERVAL_IN_MSEC -> CONFIG_STEPPER_ADI_TMC5XXX_RAMPSTAT_POLL_INTERVAL_IN_MSEC
CONFIG_STEPPER_ADI_TMC50XX_RAMP_GEN -> CONFIG_STEPPER_ADI_TMC5XXX_RAMP_GEN

@jilaypandya
Copy link
Copy Markdown
Member

This is first attempt at supporting the tmc5160. I have tried to do it as simple as possible while still reusing the tmc50xx code, but there may be a better way of doing this.

For instance the following config changes would simplify the code, but offer less flexibility in configuring the drivers.

CONFIG_STEPPER_ADI_TMC50XX_RAMPSTAT_POLL -> CONFIG_STEPPER_ADI_TMC5XXX_RAMPSTAT_POLL CONFIG_STEPPER_ADI_TMC50XX_RAMPSTAT_POLL_INTERVAL_IN_MSEC -> CONFIG_STEPPER_ADI_TMC5XXX_RAMPSTAT_POLL_INTERVAL_IN_MSEC CONFIG_STEPPER_ADI_TMC50XX_RAMP_GEN -> CONFIG_STEPPER_ADI_TMC5XXX_RAMP_GEN

These configs are per driver basis, if you have both the drivers on your board, you should be able to select these features individually for both the drivers.

Suggestion: In the first attempt, we can just have tmc51xx driver, without refactoring common code out and then refactoring common code out could be done as a subsequent PR?

@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 9, 2025

These configs are per driver basis, if you have both the drivers on your board, you should be able to select these features individually for both the drivers.

That was also my thoughts when doing this, but I wonder if there will ever be a setup using both drivers. If one of the drivers uses the feature most of the code will be pulled in so not at lot is saved by having two defines. A common define combined with runtime check would be almost as good.

Suggestion: In the first attempt, we can just have tmc51xx driver, without refactoring common code out and then refactoring common code out could be done as a subsequent PR?

I actually started with that, but almost all the code ended up being shared. Therefore I think it would be better to start with a PR splitting tmc50xx into common and specific parts and adding tmc51xx in a separate PR.

Comment thread dts/bindings/stepper/adi/adi,tmc51xx.yaml Outdated
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch from 0463fc0 to 8395d14 Compare April 10, 2025 06:45
@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 10, 2025

The PR has been refactored making it simpler to follow the code changes in each commit.

@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch from 8395d14 to bb4bd9c Compare April 10, 2025 07:18
@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 10, 2025

For SPI status logging see: apni2@c691701

Comment thread dts/bindings/stepper/adi/adi,tmc50xx.yaml
Comment thread dts/bindings/stepper/adi/adi,tmc51xx.yaml Outdated
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch from bb4bd9c to 5d7943b Compare April 10, 2025 14:17
@jilaypandya
Copy link
Copy Markdown
Member

Hi @apni2 under the reviews list, there is an option called convert to draft, you can click on that if this PR is actually supported to be converted to draft :)

@jilaypandya jilaypandya changed the title Draft: Add tmc51xx support drivers: stepper: add tmc51xx support Apr 11, 2025
Copy link
Copy Markdown
Member

@jilaypandya jilaypandya left a comment

Choose a reason for hiding this comment

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

IMO, Would be easier to have the tmc51xx driver merged without refactoring commonalities as of now, we followed the same approach with step/dir drivers.

Comment thread dts/bindings/stepper/adi/adi,tmc51xx.yaml Outdated
Comment thread drivers/stepper/adi_tmc/adi_tmc5xxx_stepper_controller.h Outdated
Comment thread drivers/stepper/adi_tmc/adi_tmc_reg.h
Copy link
Copy Markdown
Member

@jilaypandya jilaypandya left a comment

Choose a reason for hiding this comment

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

We are really close, just couple of minor Nits and you need to have a build-all entry

Ping @dipakgmx can you have a look at this as well :)

Following todos are left and then we can get this merged

Comment thread include/zephyr/drivers/stepper/stepper_trinamic.h Outdated
Comment thread include/zephyr/drivers/stepper/stepper_trinamic.h
Comment thread include/zephyr/drivers/stepper/stepper_trinamic.h
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch 2 times, most recently from 66603fa to c123cab Compare April 15, 2025 13:01
jilaypandya
jilaypandya previously approved these changes Apr 15, 2025
Comment thread tests/drivers/build_all/stepper/spi.dtsi
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch from c123cab to 9f26581 Compare April 15, 2025 13:28
@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 15, 2025

We are really close, just couple of minor Nits and you need to have a build-all entry

Ping @dipakgmx can you have a look at this as well :)

Following todos are left and then we can get this merged

* [ ]  build all entry https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/build_all/stepper/spi.dtsi.

* [ ]  squash all the tmc51xx specific commits would have to be squashed into one. The changes to tmc50xx can be a separate commit.

Hi @dipakgmx
Will you have a look.

@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch 2 times, most recently from acf5f77 to 6468dd3 Compare April 16, 2025 08:41
@dipakgmx
Copy link
Copy Markdown
Member

Hi @dipakgmx Will you have a look.

Hi, sorry. Missed out the comment. I will have a look at it.

@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 16, 2025

Hi @dipakgmx Will you have a look.

Hi, sorry. Missed out the comment. I will have a look at it.

No problem, thank you :-)

Copy link
Copy Markdown
Member

@dipakgmx dipakgmx left a comment

Choose a reason for hiding this comment

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

Some minor comments, else looks good to me.

Comment thread drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c
Comment thread drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c Outdated
Comment thread drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx.yaml Outdated
@jilaypandya
Copy link
Copy Markdown
Member

jilaypandya commented Apr 16, 2025

Just out of curiosity, did you use the tmc50xx sample or tests while developing the driver?

Asking for feedback so that these tools(samples,tests) or documentation can be made improved :)

Comment thread drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c Outdated
Comment thread include/zephyr/drivers/stepper/stepper_trinamic.h Outdated
Comment thread dts/bindings/stepper/adi/adi,tmc51xx.yaml
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch 2 times, most recently from e84b642 to c06a4f6 Compare April 22, 2025 07:38
Comment thread drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c Outdated
Comment thread drivers/stepper/adi_tmc/adi_tmc_reg.h
@apni2
Copy link
Copy Markdown
Contributor Author

apni2 commented Apr 22, 2025

Just out of curiosity, did you use the tmc50xx sample or tests while developing the driver?

Asking for feedback so that these tools(samples,tests) or documentation can be made improved :)

I used the shell functions for testing the driver.

dipakgmx
dipakgmx previously approved these changes Apr 22, 2025
apni2 added 2 commits April 22, 2025 10:37
Add Kconfig option. Find common regs. Update ramp generator data.

Signed-off-by: Anders Nielsen <anders.nielsen@prevas.dk>
Add tmc51xx support based on tmc50xx implementation.

Signed-off-by: Anders Nielsen <anders.nielsen@prevas.dk>
@apni2 apni2 force-pushed the apni2/add_tmc51xx_support branch from 5e2ff8f to 854eb27 Compare April 22, 2025 08:37
@jilaypandya jilaypandya added this to the v4.2.0 milestone Apr 22, 2025
@kartben kartben merged commit b98bd7c into zephyrproject-rtos:main Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants