drivers: stepper: ADI TMC2130 Stepper Driver #90677

jbehrensnx · 2025-05-27T13:52:18Z

This PR adds a driver for the ADI TMC2130 stepper driver. The driver only supports the step-dir-spi interface (simply called step-dir in official documentation) of the TMC2130. The pure gpio (called standalone, also a step-dir interface) and pure spi interfaces are not supported.

I moved some devicetree bindings from adi,trinamic-ramp-generator.yaml to adi,trinamic-common.yaml as they are unrelated to ramp generation and used by the TMC2130 driver.

The driver tested using the shell as well as the drv84xx/api test suite.

Moved some dt properties for tmc drivers from ramp-generator to common as they are also used by non-ramp drivers. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

faxe1008 · 2025-05-27T14:31:12Z

drivers/stepper/adi_tmc/adi_tmc_reg.h

-#define TMC5XXX_WRITE_BIT        0x80U
-#define TMC5XXX_ADDRESS_MASK     0x7FU
+#define TMC5XXX_WRITE_BIT    0x80U
+#define TMC5XXX_ADDRESS_MASK 0x7FU


Don't know about @jilaypandya but I'd avoid pure formatting changes.

That file is a mess. Every time you touch it, you have clang formatting changes

okay, let's rename this file to adi_tmc_reg_50xx.h. That solves the issue for this PR. And yeah i also agree that pure formatting changes should be in a separate commit.

I think we need to also split this file into internal/external register definitions.

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

faxe1008 · 2025-05-27T14:44:29Z

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

+	}
+
+	/* Read GSTAT register to clear any errors. */
+	tmc_spi_read_register(&config->spi, TMC2130_ADDRESS_MASK, TMC2130_GSTAT, &gstat_data);


Maybe add a check of whether or not spi is ready.

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

tests/drivers/stepper/drv84xx/api/prj.conf

jilaypandya · 2025-05-27T15:28:14Z

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#define DT_DRV_COMPAT adi_tmc2130_spi_step_dir


why is the suffix spi_step_dir used here? ain't it similiar to tmc2209 which has both uart and step-dir interface.

Looking from dts, this driver is a spi-device, hence it has to be on a spi bus.

The tmc2130 has 3 separate interfaces

Pure gpio inteface: step-dir with configuration via gpio pins, also called "Standalone" in documentation

Step-Dir with SPI based configuration - the one used by this driver, also called "Step-Dir" in documentation

Pure spi interface, similar to the tmc2209s uart interface

This driver is only intended to ever cover the second interface, with separate drivers needed for the other two. I choose this name to clearly differentiate the first and second interfaces or second and third, as neither _step_dir nor _spi would have been distinct/unique, allowing confusion between 2 interfaces. Incidentally, I would recommend adding the _step_dir suffix or something similar to the tmc22xx driver to make clear which interface it uses.

Concerning the second thing you said, I am assuming, that you are referring to the example in the dt binding yaml? I am going to correct that in the next update, making it similar to the 51xx example.

Yep just went through the datasheet. I'd say that we align the naming as how it's mentioned in the datasheet. So if somebody really wanted to write a gpio interface driver they could write a tmc2130-standalone, probably doesn't sound so fancy but the mapping with the datasheet would be easier imo.

3 drivers that can be are:

tmc2130-sd -> spi-device

tmc2130-spi -> spi-device

tmc2130-standalone -> gpio-based

Also i think now is a good time to refactor these trinamic drivers in their respective folders.i think @dipakgmx is already doing it for tmc51xx. You could also add a adi_tmc2130_reg.h in that folder.

I think I already mentioned, why I dont like to names fro the datasheet and dont think they are good. The TMC2130 has 2 step-dir modes (spi config and gpio config) and 2 spi modes (step-dir and only spi), which is why I choose the name I did to make it very clear which mode this driver supports.

Hi @jbehrensnx,

in the case of adi,tmc51xx, there are also different configurations possible, albeit till now its either spi/uart device. However, if i were to add a tmc516 step/dir only configuration i would just create a new dt-bindings which would not be a bus-device with appropriate nam and the driver would still have a DT_DRV_COMPAT of adi_tmc51xx.

We should not use prefixes like spi, step-dir with drivers, since the supported configuration is implicitly known through the available bindings.

Relying on implicity through devicetree bindings is a really bad idea. If a physical stepper driver supports multiple interfaces, the software driver needs to explicitly state which interface(s) it supports. Stating it in the .yaml is the minimum, but if different software drivers are used for different interfaces (like it makes sense for the TMC2130), they need different names and compatibles anyway, so stating it there as well is just sensible.
Note that for hardware drivers with multiple interfaces where one software driver is sufficient - like the tmc51xx series, this is obviously not needed.
The thing to remember is that for the tmc5160 the differences between spi and uart are (from a cursory glance) relatively small, just changing the communication protocol. But for the tmc2130 hey are far more significant, making multiple software drivers a sensible solution.

What do you mean by they need different compatibles anyway? You can check e.g. bme280 sensor -- both SPI and I2C binding have the same compatible, the driver simply behaves differently based on which bus the device node is attached to.

Relying on implicity through devicetree bindings is a really bad idea.

What and why? Figure out the configuration at compile time from the bindings, and pick the driver configuration. Whats the need to make this so complex?

I think I wasn't clear what I meant. Implicitly selecting the correct bus (i2c vs spi for example) depending on dt binding is fine.
The thing I have issues with are tings like, in a hypothetical example for this driver, selecting whether to use pure spi or step-dir-spi interface depending on whether the step-pin is defined or not. In that case I would prefer an explicit selection via a dt property (e.g. a boolean step-dir). All of this comes from my experience with timing-sources where I dislike the implicit selection depending on whether the counter property exist).
There is also the fact that for the TMC2130 there are significant differences between between spi and gpio interfaces to the point that I feel that they should be different software drivers, as they would share next to no code.

With all of that said, I don't plan to develop drivers for the other interfaces anytime soon, and if everybody else prefers no suffix, I can remove it.

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

tests/drivers/build_all/stepper/spi.dtsi

jilaypandya · 2025-05-27T15:34:04Z

drivers/stepper/adi_tmc/adi_tmc_reg.h

+ * @}
+ */
+
+#ifdef CONFIG_STEPPER_ADI_TMC2130_SPI_STEP_DIR


Let's Split this in a new file adi_tmc_reg_2130.h

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

jilaypandya · 2025-05-27T15:42:39Z

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

+	struct tmc2130_spi_sd_data *data = dev->data;
+	int ret;
+
+	K_SPINLOCK((struct k_spinlock *)&data->common.lock) {


you can also enable/disable via spi, or? Not a must for this PR, though, would be nice if you could give a look into it.

Yeah, I am aware of it, but there is a little bit more to it and it is not a priority for me during this PR.

jilaypandya · 2025-05-27T15:46:29Z

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

+			K_SPINLOCK_BREAK;
+		}
+		if (!config->common.dual_edge) {
+			ret = gpio_pin_set_dt(&config->common.step_pin, 0);


is this step required? Disabling the driver should turn of all the coils, or?

From what I remember this came up in other driver discussions. At the moment this is not needed, but as soon as the step signal has 50% duty cycle, the driver might stop with step signal high, this is to fix that, through there is an argument to be made, that the step-dir implementation should ensure that it does not stop on a step signal high with dual_edge disabled.

jilaypandya · 2025-05-27T15:47:12Z

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

+	struct tmc2130_spi_sd_data *data = dev->data;
+
+	if (!data->enabled) {
+		LOG_ERR("Failed to move by delta, device is not enabled");


Suggested change

LOG_ERR("Failed to move by delta, device is not enabled");

LOG_ERR("Failed to move by %d, device is not enabled", steps);

drv84xx and a4979 also use this message + the step amount is irrelevant as it is an enable/disable question. Also see the message for move_to.

boards/shields/mikroe_silent_step_2_click/mikroe_silent_step_2_click.overlay

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

boards/shields/mikroe_silent_step_2_click/doc/index.rst

kartben · 2025-06-03T09:45:18Z

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#define DT_DRV_COMPAT adi_tmc2130_spi_step_dir


What do you mean by they need different compatibles anyway? You can check e.g. bme280 sensor -- both SPI and I2C binding have the same compatible, the driver simply behaves differently based on which bus the device node is attached to.

Added a driver for the tmc2130 stepper driver. The driver is only for the tmc2130s step-dir interface in combination with spi-based configuration. The other control interfaces of the tmc2130 are not supported. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

Adds the tmc2130 stepper driver to the build_all test. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

The Silent Step 2 Click shield offers a ADI TMC3130 stepper driver. Some pins are accessed via a TI TCA9538A gpio expander. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

jbehrensnx · 2025-06-03T14:20:25Z

That should be the majority of things outside of large scale renaming and splitting the reg.h file. That + plus some other changes to file locations will require a rebase if I am not mistaken.

sonarqubecloud · 2025-06-03T14:37:26Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

drivers: stepper: Restructured TMC Devictree Bindings

083cb1a

Moved some dt properties for tmc drivers from ramp-generator to common as they are also used by non-ramp drivers. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

github-actions bot added platform: ADI area: Stepper area: Shields labels May 27, 2025

github-actions bot requested review from avisconti, bjarki-andreasen, dipakgmx, erwango, fabiobaltieri, faxe1008, galak, jfischer-no, jilaypandya, kartben, MaureenHelm, microbuilder, ozersa, ttmut and yasinustunerg May 27, 2025 13:53

github-actions bot assigned jilaypandya May 27, 2025

faxe1008 reviewed May 27, 2025

View reviewed changes

dipakgmx requested changes May 27, 2025

View reviewed changes

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c Show resolved Hide resolved

tests/drivers/stepper/drv84xx/api/prj.conf Outdated Show resolved Hide resolved

jilaypandya reviewed May 27, 2025

View reviewed changes

dipakgmx reviewed May 27, 2025

View reviewed changes

boards/shields/mikroe_silent_step_2_click/mikroe_silent_step_2_click.overlay Outdated Show resolved Hide resolved

dipakgmx reviewed May 27, 2025

View reviewed changes

drivers/stepper/adi_tmc/tmc2130_spi_step_dir.c Show resolved Hide resolved

kartben reviewed Jun 3, 2025

View reviewed changes

jbehrensnx added 3 commits June 3, 2025 16:13

drivers: stepper: test: Adds tmc2130 to build_all test

49d5108

Adds the tmc2130 stepper driver to the build_all test. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

boards: shields: Add Mikroe Silent Step 2 Click Shield

64e0536

The Silent Step 2 Click shield offers a ADI TMC3130 stepper driver. Some pins are accessed via a TI TCA9538A gpio expander. Signed-off-by: Jan Behrens <jan.behrens@navimatix.de>

jbehrensnx force-pushed the tmc2130-integration branch from 736deba to 64e0536 Compare June 3, 2025 14:18

	LOG_ERR("Failed to move by delta, device is not enabled");
	LOG_ERR("Failed to move by %d, device is not enabled", steps);

drivers: stepper: ADI TMC2130 Stepper Driver #90677

Are you sure you want to change the base?

drivers: stepper: ADI TMC2130 Stepper Driver #90677

Conversation

jbehrensnx commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jilaypandya May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jilaypandya May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jilaypandya Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dipakgmx Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jilaypandya May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbehrensnx commented Jun 3, 2025

Uh oh!

sonarqubecloud bot commented Jun 3, 2025

Quality Gate passed

Uh oh!

Uh oh!

jilaypandya May 27, 2025 •

edited

Loading

jilaypandya May 28, 2025 •

edited

Loading

jilaypandya Jun 3, 2025 •

edited

Loading

dipakgmx Jun 3, 2025 •

edited

Loading

jilaypandya May 27, 2025 •

edited

Loading