Stepper tmc5160 #80784

cooked · 2024-11-02T22:58:07Z

Add support for the ADI TMC5160 stepper driver, using the new stepper API

jilaypandya · 2024-11-03T13:43:01Z

Hi @cooked,

Thanks for contributing to the project.

jilaypandya · 2024-11-03T13:43:24Z

Rebase on main, drop the merge commit.
tmc5160 is highly similiar to tmc5130, you can rename the driver to tmc51xx
tmc5041 can support two steppers, this is not the case with tmc51xx, hence there is no need of child nodes. You can singularize the driver instantiation.
Check Commit Message

jilaypandya · 2024-11-03T15:28:40Z

drivers/stepper/adi_tmc/adi_tmc5160_stepper_controller.c

remove extra newline

cooked · 2024-11-03T19:02:59Z

Hi @jilaypandya,
while making the driver generalized for the 51xx in the code, shall I keep the kconfig explicit with both TMC5160 and TMC5130 (while the cmakefile will select the same .c)? or go 51xx in the kconfig as well?

jilaypandya · 2024-11-03T19:14:17Z

Hi @jilaypandya, while making the driver generalized for the 51xx in the code, shall I keep the kconfig explicit with both TMC5160 and TMC5130 (while the cmakefile will select the same .c)? or go 51xx in the kconfig as well?

You can rename it tmc51xx in kconfig as well. tmc51xx as a driver shall serve both tmc5130 and tmc5160.

dipakgmx · 2024-11-03T21:12:41Z

include/zephyr/drivers/stepper/stepper_trinamic.h

The set_ramp_parameters function is better off being within the respective drivers, i.e., tmc5041.c or tmc5160.c and then having a set_ramp_parametersfunction in stepper.h.

We are gathering all trinamic specific functions in this file stepper_trinamic.h. IFDEFs are not really required coz compiler will report an error anyhow :)

zephyrbot · 2024-11-06T07:45:05Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

jilaypandya · 2024-11-06T15:01:09Z

drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c

+	struct spi_dt_spec spi;
+	const uint32_t clock_frequency;
+	const uint16_t default_micro_step_res;
+	// StallGuard


Run python zephyr/scripts/ci/check_compliance.py locally before pushing, it will show you all the compliance errors. https://docs.zephyrproject.org/latest/contribute/guidelines.html

jilaypandya · 2024-11-06T15:05:03Z

dts/bindings/stepper/adi/adi,tmc51xx.yaml

+include:
+  - name: spi-device.yaml
+  - name: adi,trinamic-gconf.yaml
+    property-allowlist:


gconf in tmc51xx has few more registers. You can extend adi, trinamic-gconf.yaml with those extra registers. Initially i just took stuff from tmc5041 and tmc2209, but it can be extended.

jilaypandya · 2024-11-06T15:06:31Z

drivers/stepper/adi_tmc/adi_tmc_spi.c

 	}
 	if ((status_byte & BIT_MASK(1)) != 0) {
-		LOG_WRN("spi dataframe: driver_error(1) detected");
+		LOG_WRN("spi dataframe: driver_error/driver_error(1) detected");


drop driver_error(1)

jilaypandya · 2024-11-06T16:19:39Z

drivers/stepper/adi_tmc/adi_tmc51xx_stepper_controller.c

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


drop the empty line over here. :)

Signed-off-by: Stefano Cottafavi <[email protected]>

faxe1008 · 2024-11-13T20:11:42Z

Since there are many more step-dir based drivers would it make sense to extract them to a device binding that can be used by multiple drivers internally? This would reduce the boilerplate for any other drivers. For inspiration what I mean you can look at the input-kdb-matrix where the same principle is applied.

jilaypandya · 2024-11-13T20:20:17Z

scripts/ci/check_compliance.py


    def run(self):
-        exe = f"clang-format-diff.{'exe' if platform.system() == 'Windows' else 'py'}"
+        exe = f"clang-format-diff{'exe' if platform.system() == 'Windows' else ''}"


This change needs to be reverted

fabiobaltieri · 2024-11-14T12:01:19Z

drivers/stepper/adi_tmc/adi_tmc5160_stepper_controller.c

+	const struct tmc5160_config *config = dev->config;
+	struct tmc5160_data *data = dev->data;


would it make sense to use the child device as argument to tmc5160_write, change these to dev->controller->config and dev->controller->data and just pass dev to this API? Make the code a bit more compact

fabiobaltieri · 2024-11-14T12:04:20Z

drivers/stepper/adi_tmc/adi_tmc5160_stepper_controller.c

+	}
+	reg_value &= TMC5160_CHOPCONF_MRES_MASK;
+	reg_value >>= TMC5160_CHOPCONF_MRES_SHIFT;
+	*res = (1 << (MICRO_STEP_RES_INDEX(STEPPER_MICRO_STEP_256) - reg_value));


can you do *res = BIT(MICRO_STEP_RES_INDEX(STEPPER_MICRO_STEP_256) - reg_value)?

fabiobaltieri · 2024-11-14T12:06:38Z

drivers/stepper/adi_tmc/adi_tmc5160_stepper_controller.c

+
+	err = tmc5160_write(config->controller, TMC5160_VSTART, ramp_data->vstart);
+	if (err != 0) {
+		return -EIO;


do you have a reason to discard err and return -EIO instead? It's usually a good idea to pass through the error code, than one can log it and it may give some more information of what's wrong in the lower layers

github-actions · 2025-01-14T00:32:22Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

zephyrbot added the area: Stepper label Nov 2, 2024

zephyrbot requested a review from jilaypandya November 2, 2024 22:58

zephyrbot assigned jilaypandya Nov 2, 2024

jilaypandya reviewed Nov 3, 2024

View reviewed changes

drivers/stepper/adi_tmc/adi_tmc5160_stepper_controller.c Outdated

Copy link

Member

jilaypandya Nov 3, 2024

Choose a reason for hiding this comment

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

remove extra newline

jilaypandya requested a review from dipakgmx November 3, 2024 18:26

cooked force-pushed the stepper_tmc5160 branch from 5286eb3 to b7484dc Compare November 3, 2024 18:50

dipakgmx reviewed Nov 3, 2024

View reviewed changes

jilaypandya mentioned this pull request Nov 4, 2024

drivers: stepper: add common helper header for tmc5xxx functions #80795

Merged

cooked requested review from LucasTambor, Vudentz, alwa-nordic, anangl, ankuns, carlescufi, danieldegrasse, dleach02, jhedberg, jimmyzhe, jukkar, mmahadevan108, nandojve, rlubos and tbursztyka as code owners November 6, 2024 07:43

zephyrbot added manifest manifest-babblesim_ext_2G4_libPhyComv1 labels Nov 6, 2024

jilaypandya self-requested a review November 6, 2024 12:41

jilaypandya reviewed Nov 6, 2024

View reviewed changes

review fix WIP, sign off

f75282f

Signed-off-by: Stefano Cottafavi <[email protected]>

zephyrbot added area: Coding Guidelines Coding guidelines and style area: Continuous Integration labels Nov 13, 2024

zephyrbot requested review from carlescufi, fabiobaltieri, jfischer-no, kartben, keith-zephyr, nashif, simhein and stephanosio November 13, 2024 03:44

jilaypandya reviewed Nov 13, 2024

View reviewed changes

jilaypandya requested review from faxe1008 and removed request for carlescufi, jfischer-no, kartben, keith-zephyr, nashif, simhein and stephanosio November 13, 2024 20:28

fabiobaltieri reviewed Nov 14, 2024

View reviewed changes

jilaypandya mentioned this pull request Nov 17, 2024

drivers: stepper: refactor common registers to tmc5xxx #81484

Merged

jilaypandya mentioned this pull request Dec 16, 2024

drivers: stepper: tmc: create rampgen Kconfig template #83063

Merged

github-actions bot added the Stale label Jan 14, 2025

github-actions bot closed this Jan 28, 2025

		const struct tmc5160_config *config = dev->config;
		struct tmc5160_data *data = dev->data;

		#include "adi_tmc_spi.h"

		#include <zephyr/logging/log.h>

Stepper tmc5160 #80784

Stepper tmc5160 #80784

Uh oh!

Conversation

cooked commented Nov 2, 2024

Uh oh!

jilaypandya commented Nov 3, 2024

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cooked commented Nov 3, 2024

Uh oh!

jilaypandya commented Nov 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zephyrbot commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

faxe1008 commented Nov 13, 2024

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!

github-actions bot commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jilaypandya commented Nov 3, 2024 •

edited

Loading

zephyrbot commented Nov 6, 2024 •

edited

Loading