drivers: stepper: add stepper driver for allegro a4979 #86620

verenascst · 2025-03-04T12:23:08Z

This PR adds a stepper driver for allegro a4979 microstepping driver.

It has manly been developed analogous to drv8429 with adaptions where they are needed. It was tested with the generic stepper sample.

Adding a stepper driver for allegro a4979 microstepping programmable stepper motor driver. Signed-off-by: Verena Schweinstetter <verena.schweinstetter@zeiss.com>

…legro a4979 Add entry in gpio.dtsi for allegro a4979 stepper in tests/drivers/build_all/stepper Signed-off-by: Verena Schweinstetter <verena.schweinstetter@zeiss.com>

github-actions · 2025-03-04T12:23:47Z

Hello @verenascst, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

faxe1008

Nice work, some things around formatting and minor nits 👍

faxe1008 · 2025-03-04T12:43:43Z

drivers/stepper/allegro/CMakeLists.txt

+zephyr_library()
+zephyr_library_property(ALLOW_EMPTY TRUE)
+
+zephyr_library_sources_ifdef(CONFIG_A4979_STEPPER a4979.c)


Check you editor settings, there should be a new line at the end of file.

faxe1008 · 2025-03-04T12:44:24Z

drivers/stepper/allegro/Kconfig

+
+comment "Allegro Stepper Drivers"
+
+rsource "Kconfig.a4979"


Maybe add a zephyr-keep-sorted directive?

faxe1008 · 2025-03-04T12:45:24Z

drivers/stepper/allegro/a4979.c

+ * 
+ * This structure contains all of the devicetree specifications for the pins
+ * needed by a given A4979 stepper driver.
+ */


Usually we only add doxygen comments for public facing headers. Internal implementation files should not have them.

faxe1008 · 2025-03-04T12:47:55Z

drivers/stepper/allegro/a4979.c

+	data->enabled = enable;
+	if (!enable) {
+		config->common.timing_source->stop(dev);
+		ret = gpio_pin_set_dt(&config->common.step_pin, 0);


Is there a benefit to setting the step pin explicitly?

faxe1008 · 2025-03-04T12:49:00Z

drivers/stepper/allegro/a4979.c

+					    enum stepper_micro_step_resolution *micro_step_res)
+{
+	struct a4979_data *data = dev->data;
+	*micro_step_res = data->micro_step_res;


Compliance check probably wants some new lines here.

faxe1008 · 2025-03-04T12:50:28Z

drivers/stepper/allegro/a4979.c

+	}
+
+	/* Configure microstep pin 0 */
+	ret = gpio_pin_configure_dt(&config->m0_pin, GPIO_OUTPUT_INACTIVE);


I guess you should check the gpio_is_ready_dt there also before trying to configure.

faxe1008 · 2025-03-04T12:51:45Z

drivers/stepper/allegro/a4979.c

+																		\
+	DEVICE_DT_INST_DEFINE(inst, a4979_init, NULL, &a4979_data_##inst, 	\
+					&a4979_config_##inst, POST_KERNEL, 					\
+					CONFIG_STEPPER_INIT_PRIORITY, &a4979_stepper_api);	


Check the formatting of the macro, some lines are too long. clang-format usually gets you 90% of the way there but I guess in some cases it fails for our macrobatics :^)

faxe1008 · 2025-03-04T12:52:19Z

dts/bindings/stepper/allegro/allegro,a4979.yml

+      - counter
+
+properties:
+


Drop the empty line.

faxe1008 · 2025-03-04T12:52:31Z

dts/bindings/stepper/allegro/allegro,a4979.yml

+compatible: "allegro,a4979"
+
+include:
+  #- name: spi-device.yaml


Leftover :^) ?

jilaypandya

Thanks @verenascst for the contribution :-) Try running
python scripts/ci/check_compliance.py after you have committed your changes locally and it should report all the whitespacing & other compliance errors :-)

jilaypandya · 2025-03-04T17:45:39Z

drivers/stepper/allegro/Kconfig

+# SPDX-FileCopyrightText: Copyright (c) 2025 Carl Zeiss Meditec AG
+# SPDX-License-Identifier: Apache-2.0
+
+config STEPPER_ALLEGRO


Suggested change

config STEPPER_ALLEGRO

menuconfig STEPPER_ALLEGRO

jilaypandya · 2025-03-04T17:47:08Z

drivers/stepper/allegro/Kconfig.a4979

+# SPDX-License-Identifier: Apache-2.0
+
+config A4979_STEPPER
+	bool "Activate driver for A4979 stepper motor driver"


Suggested change

bool "Activate driver for A4979 stepper motor driver"

bool "Activate allegro A4979 stepper driver"

jilaypandya · 2025-03-04T17:48:46Z

drivers/stepper/allegro/Kconfig.a4979

+	default y
+	depends on DT_HAS_ALLEGRO_A4979_ENABLED
+	select STEP_DIR_STEPPER
+	select STEPPER_STEP_DIR_GENERATE_ISR_SAFE_EVENTS


User should be able to select this :)

Suggested change

select STEPPER_STEP_DIR_GENERATE_ISR_SAFE_EVENTS

jilaypandya · 2025-03-04T17:49:03Z

drivers/stepper/allegro/a4979.c

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


Suggested change

jilaypandya · 2025-03-04T17:50:36Z

drivers/stepper/allegro/a4979.c

+ * 
+ * This structure contains mutable data used by a A4979 stepper driver.
+ */
+struct a4979_data {


Same as what @faxe1008 mentioned above :)

jilaypandya · 2025-03-04T19:40:11Z

dts/bindings/stepper/allegro/allegro,a4979.yml

@@ -0,0 +1,46 @@
+description: |


The spacing needs to be adjusted over here a bit :)

tmc2209 is a good example.
https://docs.zephyrproject.org/latest/build/dts/api/bindings/stepper/adi/adi%2Ctmc2209.html

verenascst added 2 commits March 4, 2025 11:13

drivers: stepper: Add stepper driver for allegro a4979

fb0a8e5

Adding a stepper driver for allegro a4979 microstepping programmable stepper motor driver. Signed-off-by: Verena Schweinstetter <verena.schweinstetter@zeiss.com>

drivers: stepper: Add entry to tests/drivers/build_all/stepper for al…

f1f5dd9

…legro a4979 Add entry in gpio.dtsi for allegro a4979 stepper in tests/drivers/build_all/stepper Signed-off-by: Verena Schweinstetter <verena.schweinstetter@zeiss.com>

zephyrbot added the area: Stepper label Mar 4, 2025

zephyrbot requested review from bjarki-andreasen, dipakgmx, fabiobaltieri, faxe1008 and jilaypandya March 4, 2025 12:23

zephyrbot assigned jilaypandya Mar 4, 2025

faxe1008 reviewed Mar 4, 2025

View reviewed changes

jilaypandya reviewed Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: stepper: add stepper driver for allegro a4979 #86620

drivers: stepper: add stepper driver for allegro a4979 #86620

verenascst commented Mar 4, 2025

github-actions bot commented Mar 4, 2025

faxe1008 left a comment

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

faxe1008 Mar 4, 2025

jilaypandya left a comment

jilaypandya Mar 4, 2025

jilaypandya Mar 4, 2025

jilaypandya Mar 4, 2025

jilaypandya Mar 4, 2025

jilaypandya Mar 4, 2025

jilaypandya Mar 4, 2025

	bool "Activate driver for A4979 stepper motor driver"
	bool "Activate allegro A4979 stepper driver"

		#include "../step_dir/step_dir_stepper_common.h"

		#include <zephyr/logging/log.h>

		- counter

		properties:

drivers: stepper: add stepper driver for allegro a4979 #86620

Are you sure you want to change the base?

drivers: stepper: add stepper driver for allegro a4979 #86620

Conversation

verenascst commented Mar 4, 2025

github-actions bot commented Mar 4, 2025

faxe1008 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jilaypandya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment