Refactor tmc5xxx drivers & modules to reuse code #91988

dipakgmx · 2025-06-22T16:46:17Z

This pull request introduces refactoring of the TMC5xxx & TMC51xx series to reuse
the common code between the modules.

Now, a common handler is used for handling bus transactions for both TMC5xxx
stepper motor controllers. This consolidates the SPI and UART communication
code in a shared implementation that can be used by both TMC50xx and TMC51xx
controllers.

The changes now introduces a new adi_tmc5xxx_core files provide a unified API for basic
controller operations like register read/write, motor control and diagnostics.

This refactoring also moves the register definitions to the tmc5xxx subdirectory
and removes the no longer needed adi_tmc5xxx_common.h file.

jilaypandya

At first glance, it looks good. Some minor refactoring comments. Do you want this to be in 4.2?

jilaypandya · 2025-06-22T16:53:54Z

drivers/stepper/adi_tmc/Kconfig.tmc50xx

I think it's best to move the Kconfigs of tmc5xxx drivers directly in the tmc5xxx folder

jilaypandya · 2025-06-22T16:56:43Z

drivers/stepper/adi_tmc/tmc5xxx/adi_tmc_reg.h

can this file be renamed to adi_tmc5xxx_reg.h. Could you look at the unwanted formatting changes?

jilaypandya · 2025-06-22T16:57:42Z

drivers/stepper/adi_tmc/tmc5xxx/tmc50xx.c

@@ -0,0 +1,335 @@
+/*
+ * SPDX-FileCopyrightText: Copyright (c) 2024 Carl Zeiss Meditec AG
+ * SPDX-FileCopyrightText: Copyright (c) 2025 Jilay Sandeep Pandya


you can add your name as well :)

jilaypandya · 2025-06-22T16:58:28Z

drivers/stepper/adi_tmc/tmc5xxx/tmc50xx.c

+#include "adi_tmc5xxx_core.h"
+
+#include <zephyr/logging/log.h>
+LOG_MODULE_REGISTER(tmc5xxx, CONFIG_STEPPER_LOG_LEVEL);


Suggested change

LOG_MODULE_REGISTER(tmc5xxx, CONFIG_STEPPER_LOG_LEVEL);

LOG_MODULE_REGISTER(tmc50xx, CONFIG_STEPPER_LOG_LEVEL);

The problem here is, we have bunch of core functions, which are shared between the 2 models. Now, when we log, with tmc50xx, and we have logs coming out of core module, we cannot distinguish. Hence I went for the tmc5xxx for all the files, but with the device name logging, we can figure out what device is generating this message

The reasoning is plausible :) @bjarki-andreasen what would one do in such a case? I would have still kept it as tmc50xx, considering that the log is coming out of the tmc50xx.c module.

jilaypandya · 2025-06-22T17:00:06Z

drivers/stepper/adi_tmc/tmc5xxx/tmc50xx.c

+		k_work_init_delayable(&data->stallguard_dwork, tmc5xxx_stallguard_work_handler);
+
+		err = tmc5xxx_write_reg(ctx, TMC5XXX_SWMODE(ctx->motor_index), BIT(10));
+


Suggested change

jilaypandya · 2025-06-22T17:01:01Z

drivers/stepper/adi_tmc/tmc5xxx/tmc51xx.c

+
+#include "tmc51xx.h"
+#include <zephyr/logging/log.h>
+


trivial

Suggested change

jilaypandya · 2025-06-22T17:01:20Z

drivers/stepper/adi_tmc/tmc5xxx/tmc51xx.c

+#include "tmc51xx.h"
+#include <zephyr/logging/log.h>
+
+LOG_MODULE_REGISTER(tmc5xxx, CONFIG_STEPPER_LOG_LEVEL);


Suggested change

LOG_MODULE_REGISTER(tmc5xxx, CONFIG_STEPPER_LOG_LEVEL);

LOG_MODULE_REGISTER(tmc51xx, CONFIG_STEPPER_LOG_LEVEL);

jilaypandya · 2025-06-22T17:02:38Z

drivers/stepper/adi_tmc/tmc5xxx/tmc51xx.c

+
+	/* Configure DIAG0 GPIO interrupt pin */
+	IF_ENABLED(TMC51XX_BUS_SPI, ({
+	if (config->comm_type == TMC_COMM_SPI && config->diag0_gpio.port) {


could you refactor this routine out into a separate function to enhance readability

jilaypandya

I think the separate commits are to ease the review process. I am just dropping a Request Change, so that we don't forget to squash them before merge.

This follows the same pattern as the tmc50xx series (wherein the controller) had 2 motors. Now the tmx51xx also considers the motor as a child binding, enabling resuing the code between the 2 models. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

Create a common handler for handling bus transactions for both TMC5xxx stepper motor controllers. This consolidates the SPI and UART communication code in a shared implementation that can be used by both TMC50xx and TMC51xx controllers. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

The implementation moves the common bus transaction code from individual driver files into reusable core modules. The new adi_tmc5xxx_core files provide a unified API for basic controller operations like register read/write, motor control, and diagnostics. This refactoring also moves the register definitions to the tmc5xxx subdirectory and removes the no longer needed adi_tmc5xxx_common.h file. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

The drivers are now refactored to use the core module for handling the common functions. Rampstat polling, ramp setting and diagnostics are left to the individual drivers since they are very chip specific. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

Since the motor is now made a child of the controller, the macro to pull the ramp parameters are now refactored to pull it out of the node. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

Update tmc50xx driver to conditionally select SPI support Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

reorganize CMakeLists for TMC5xxx support subsequent to the changes. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

The dts layout is updated for tmc51xx nodes, since the motor is now made a child of the parent controller. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

sonarqubecloud · 2025-06-22T20:37:39Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

dipakgmx · 2025-06-22T20:53:29Z

At first glance, it looks good. Some minor refactoring comments. Do you want this to be in 4.2?

Yes, that would be great. Would you mind testing on the tmc50xx unit? I have tested it now on spi & uart configuration of tmc51xx and it works.

jilaypandya · 2025-06-22T21:01:22Z

Sure I'll do it tomorrow.

faxe1008 · 2025-06-25T09:08:43Z

drivers/stepper/adi_tmc/tmc5xxx/adi_tmc5xxx_core.c

+		}
+
+		/* For UART or SPI without DIAG0, schedule RAMPSTAT polling */
+#if defined(CONFIG_STEPPER_ADI_TMC50XX_RAMPSTAT_POLL_INTERVAL_IN_MSEC)


If you have a TMC50XX and TMC51XX on the same board this will produce odd behaviour. Only thing that I see is adding this into config?

faxe1008 · 2025-06-25T09:12:04Z

drivers/stepper/adi_tmc/tmc5xxx/adi_tmc5xxx_core.c

+	struct tmc5xxx_stepper_data const *data = dev->data;
+	const struct tmc5xxx_core_context *ctx = &data->core;
+
+	if (!VALID_MICRO_STEP_RES(res)) {


iirc this was moved to the API itself, right @jilaypandya . So not needed here

faxe1008 · 2025-06-25T09:14:55Z

drivers/stepper/adi_tmc/tmc5xxx/adi_tmc5xxx_core.c

+	}
+
+	const uint8_t sg_result = FIELD_GET(TMC5XXX_DRV_STATUS_SG_RESULT_MASK, drv_status);
+	const bool sg_status = FIELD_GET(TMC5XXX_DRV_STATUS_SG_STATUS_MASK, drv_status);


I mean it does not hurt, but why const these specifically?

faxe1008 · 2025-06-25T09:15:40Z

drivers/stepper/adi_tmc/tmc5xxx/adi_tmc5xxx_core.h

+struct tmc5xxx_controller_config {
+	union tmc_bus bus;                    /* Bus connection (SPI/UART) */
+	const struct tmc_bus_io *bus_io;      /* Bus I/O operations */
+	uint8_t comm_type;                    /* Communication type */


move the comm_type at the end of the struct, wastes 3 bytes less of padding on 32 bit

smaherdev · 2025-06-27T13:49:05Z

drivers/stepper/adi_tmc/tmc5xxx/tmc50xx.c

+
+#endif
+
+static int tmc51xx_controller_init(const struct device *dev)


Suggested change

static int tmc51xx_controller_init(const struct device *dev)

static int tmc50xx_controller_init(const struct device *dev)

smaherdev · 2025-06-27T13:49:26Z

drivers/stepper/adi_tmc/tmc5xxx/tmc50xx.c

+	DT_INST_FOREACH_CHILD_STATUS_OKAY(inst, TMC50XX_STEPPER_DEFINE);                           \
+                                                                                                   \
+	/* Define the controller device */                                                         \
+	DEVICE_DT_INST_DEFINE(inst, tmc51xx_controller_init, NULL,                                 \


Suggested change

DEVICE_DT_INST_DEFINE(inst, tmc51xx_controller_init, NULL, \

DEVICE_DT_INST_DEFINE(inst, tmc50xx_controller_init, NULL, \

github-actions bot added platform: ADI Analog Devices, Inc. area: Stepper labels Jun 22, 2025

github-actions bot requested review from bjarki-andreasen, fabiobaltieri, faxe1008, galak, jbehrensnx, jilaypandya, MaureenHelm, microbuilder, ozersa, ttmut and yasinustunerg June 22, 2025 16:47

github-actions bot assigned jilaypandya Jun 22, 2025

jilaypandya reviewed Jun 22, 2025

View reviewed changes

jilaypandya requested changes Jun 22, 2025

View reviewed changes

jilaypandya changed the title ~~Refactor tmc5xxx modules to reuse code~~ Refactor tmc5xxx drivers & modules to reuse code Jun 22, 2025

dipakgmx added 8 commits June 22, 2025 22:12

include: zephyr: drivers: stepper: refactor trinamic ramp get macro

c8ffe14

Since the motor is now made a child of the controller, the macro to pull the ramp parameters are now refactored to pull it out of the node. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

drivers: stepper: adi_tmc: spi selection logic

cd7fa66

Update tmc50xx driver to conditionally select SPI support Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

drivers: stepper: adi_tmc: cmake reorg

7817c72

reorganize CMakeLists for TMC5xxx support subsequent to the changes. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

tests: drivers: build_all: stepper: update tmc51xx nodes

0c11185

The dts layout is updated for tmc51xx nodes, since the motor is now made a child of the parent controller. Signed-off-by: Dipak Shetty <shetty.dipak@gmx.com>

dipakgmx force-pushed the feat/tmc5xxx-refactor branch from 9de529c to 0c11185 Compare June 22, 2025 20:13

faxe1008 reviewed Jun 25, 2025

View reviewed changes

smaherdev reviewed Jun 27, 2025

View reviewed changes

	LOG_MODULE_REGISTER(tmc5xxx, CONFIG_STEPPER_LOG_LEVEL);
	LOG_MODULE_REGISTER(tmc50xx, CONFIG_STEPPER_LOG_LEVEL);

		k_work_init_delayable(&data->stallguard_dwork, tmc5xxx_stallguard_work_handler);

		err = tmc5xxx_write_reg(ctx, TMC5XXX_SWMODE(ctx->motor_index), BIT(10));

	LOG_MODULE_REGISTER(tmc5xxx, CONFIG_STEPPER_LOG_LEVEL);
	LOG_MODULE_REGISTER(tmc51xx, CONFIG_STEPPER_LOG_LEVEL);


		#endif

		static int tmc51xx_controller_init(const struct device *dev)

	static int tmc51xx_controller_init(const struct device *dev)
	static int tmc50xx_controller_init(const struct device *dev)

	DEVICE_DT_INST_DEFINE(inst, tmc51xx_controller_init, NULL, \
	DEVICE_DT_INST_DEFINE(inst, tmc50xx_controller_init, NULL, \


		#include "tmc51xx.h"
		#include <zephyr/logging/log.h>

Refactor tmc5xxx drivers & modules to reuse code #91988

Are you sure you want to change the base?

Refactor tmc5xxx drivers & modules to reuse code #91988

Conversation

dipakgmx commented Jun 22, 2025

Uh oh!

jilaypandya left a comment

Choose a reason for hiding this comment

Uh oh!

jilaypandya Jun 22, 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!

jilaypandya Jun 22, 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!

jilaypandya Jun 22, 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!

jilaypandya Jun 22, 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 22, 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!

jilaypandya Jun 22, 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!

jilaypandya Jun 22, 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!

jilaypandya left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 22, 2025

Quality Gate passed

Uh oh!

dipakgmx commented Jun 22, 2025

Uh oh!

jilaypandya commented Jun 22, 2025

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!

jilaypandya Jun 22, 2025 •

edited

Loading

jilaypandya Jun 22, 2025 •

edited

Loading

jilaypandya Jun 22, 2025 •

edited

Loading

jilaypandya Jun 22, 2025 •

edited

Loading

jilaypandya Jun 22, 2025 •

edited

Loading

jilaypandya Jun 22, 2025 •

edited

Loading

jilaypandya Jun 22, 2025 •

edited

Loading