Skip to content

drivers: stepper: unify msx-gpios #88798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+87 −88
Open

drivers: stepper: unify msx-gpios #88798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions drivers/stepper/adi_tmc/adi_tmc22xx_stepper_controller.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ LOG_MODULE_REGISTER(tmc22xx, CONFIG_STEPPER_LOG_LEVEL);
struct tmc22xx_config {
struct step_dir_stepper_common_config common;
const struct gpio_dt_spec enable_pin;
const struct gpio_dt_spec *msx_pins;
enum stepper_micro_step_resolution *msx_resolutions;
};

Expand Down Expand Up @@ -48,7 +47,7 @@ static int tmc22xx_stepper_set_micro_step_res(const struct device *dev,
const struct tmc22xx_config *config = dev->config;
int ret;

if (!config->msx_pins) {
if (!config->common.msx_pins) {
LOG_ERR("Microstep resolution pins are not configured");
return -ENODEV;
}
Expand All @@ -58,13 +57,13 @@ static int tmc22xx_stepper_set_micro_step_res(const struct device *dev,
continue;
}

ret = gpio_pin_set_dt(&config->msx_pins[0], i & 0x01);
ret = gpio_pin_set_dt(&config->common.msx_pins[0], i & 0x01);
if (ret < 0) {
LOG_ERR("Failed to set MS1 pin: %d", ret);
return ret;
}

ret = gpio_pin_set_dt(&config->msx_pins[1], (i & 0x02) >> 1);
ret = gpio_pin_set_dt(&config->common.msx_pins[1], (i & 0x02) >> 1);
if (ret < 0) {
LOG_ERR("Failed to set MS2 pin: %d", ret);
return ret;
Expand Down Expand Up @@ -93,12 +92,12 @@ static int tmc22xx_stepper_configure_msx_pins(const struct device *dev)
int ret;

for (uint8_t i = 0; i < MSX_PIN_COUNT; i++) {
if (!gpio_is_ready_dt(&config->msx_pins[i])) {
if (!gpio_is_ready_dt(&config->common.msx_pins[i])) {
LOG_ERR("MSX pin %u are not ready", i);
return -ENODEV;
}

ret = gpio_pin_configure_dt(&config->msx_pins[i], GPIO_OUTPUT);
ret = gpio_pin_configure_dt(&config->common.msx_pins[i], GPIO_OUTPUT);
if (ret < 0) {
LOG_ERR("Failed to configure msx pin %u", i);
return ret;
Expand All @@ -124,7 +123,7 @@ static int tmc22xx_stepper_init(const struct device *dev)
return ret;
}

if (config->msx_pins) {
if (config->common.msx_pins) {
ret = tmc22xx_stepper_configure_msx_pins(dev);
if (ret < 0) {
LOG_ERR("Failed to configure MSX pins: %d", ret);
Expand Down Expand Up @@ -176,11 +175,10 @@ static DEVICE_API(stepper, tmc22xx_stepper_api) = {
)) \
\
static const struct tmc22xx_config tmc22xx_config_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT(inst), \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT( \
inst, tmc22xx_stepper_msx_pins_##inst), \
.enable_pin = GPIO_DT_SPEC_INST_GET(inst, en_gpios), \
.msx_resolutions = msx_table, \
IF_ENABLED(DT_INST_NODE_HAS_PROP(inst, msx_gpios), \
(.msx_pins = tmc22xx_stepper_msx_pins_##inst)) \
}; \
static struct tmc22xx_data tmc22xx_data_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_DATA_INIT(inst), \
Expand Down
52 changes: 29 additions & 23 deletions drivers/stepper/allegro/a4979.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ struct a4979_config {
const struct step_dir_stepper_common_config common;
const struct gpio_dt_spec en_pin;
const struct gpio_dt_spec reset_pin;
const struct gpio_dt_spec m0_pin;
const struct gpio_dt_spec m1_pin;
};

struct a4979_data {
Expand Down Expand Up @@ -130,11 +128,11 @@ static int a4979_stepper_set_micro_step_res(const struct device *dev,
return -ENOTSUP;
}

ret = a4979_set_microstep_pin(dev, &config->m0_pin, m0_value);
ret = a4979_set_microstep_pin(dev, &config->common.msx_pins[0], m0_value);
if (ret != 0) {
return ret;
}
ret = a4979_set_microstep_pin(dev, &config->m1_pin, m1_value);
ret = a4979_set_microstep_pin(dev, &config->common.msx_pins[1], m1_value);
if (ret != 0) {
return ret;
}
Expand Down Expand Up @@ -228,22 +226,22 @@ static int a4979_init(const struct device *dev)
}

/* Configure microstep pin 0 */
if (!gpio_is_ready_dt(&config->m0_pin)) {
if (!gpio_is_ready_dt(&config->common.msx_pins[0])) {
LOG_ERR("m0 Pin is not ready");
return -ENODEV;
}
ret = gpio_pin_configure_dt(&config->m0_pin, GPIO_OUTPUT_INACTIVE);
ret = gpio_pin_configure_dt(&config->common.msx_pins[0], GPIO_OUTPUT_INACTIVE);
if (ret != 0) {
LOG_ERR("%s: Failed to configure m0_pin (error: %d)", dev->name, ret);
return ret;
}

/* Configure microstep pin 1 */
if (!gpio_is_ready_dt(&config->m1_pin)) {
if (!gpio_is_ready_dt(&config->common.msx_pins[1])) {
LOG_ERR("m1 Pin is not ready");
return -ENODEV;
}
ret = gpio_pin_configure_dt(&config->m1_pin, GPIO_OUTPUT_INACTIVE);
ret = gpio_pin_configure_dt(&config->common.msx_pins[1], GPIO_OUTPUT_INACTIVE);
if (ret != 0) {
LOG_ERR("%s: Failed to configure m1_pin (error: %d)", dev->name, ret);
return ret;
Expand Down Expand Up @@ -282,21 +280,29 @@ static DEVICE_API(stepper, a4979_stepper_api) = {
};

#define A4979_DEVICE(inst) \
\
static const struct a4979_config a4979_config_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT(inst), \
.en_pin = GPIO_DT_SPEC_INST_GET_OR(inst, en_gpios, {0}), \
.reset_pin = GPIO_DT_SPEC_INST_GET_OR(inst, reset_gpios, {0}), \
.m0_pin = GPIO_DT_SPEC_INST_GET(inst, m0_gpios), \
.m1_pin = GPIO_DT_SPEC_INST_GET(inst, m1_gpios), \
}; \
\
static struct a4979_data a4979_data_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_DATA_INIT(inst), \
.micro_step_res = DT_INST_PROP(inst, micro_step_res), \
}; \
\
DEVICE_DT_INST_DEFINE(inst, a4979_init, NULL, &a4979_data_##inst, &a4979_config_##inst, \
IF_ENABLED(DT_INST_NODE_HAS_PROP(inst, msx_gpios), ( \
static const struct gpio_dt_spec a4979_stepper_msx_pins_##inst[] = { \
DT_INST_FOREACH_PROP_ELEM_SEP( \
inst, msx_gpios, GPIO_DT_SPEC_GET_BY_IDX, (,) \
), \
}; \
BUILD_ASSERT( \
ARRAY_SIZE(a4979_stepper_msx_pins_##inst) == 2, \
"Two microstep config pins needed"); \
)) \
static const struct a4979_config a4979_config_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT( \
inst, a4979_stepper_msx_pins_##inst), \
.en_pin = GPIO_DT_SPEC_INST_GET_OR(inst, en_gpios, {0}), \
.reset_pin = GPIO_DT_SPEC_INST_GET_OR(inst, reset_gpios, {0}), \
}; \
\
static struct a4979_data a4979_data_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_DATA_INIT(inst), \
.micro_step_res = DT_INST_PROP(inst, micro_step_res), \
}; \
\
DEVICE_DT_INST_DEFINE(inst, a4979_init, NULL, &a4979_data_##inst, &a4979_config_##inst, \
POST_KERNEL, CONFIG_STEPPER_INIT_PRIORITY, &a4979_stepper_api);

DT_INST_FOREACH_STATUS_OKAY(A4979_DEVICE)
14 changes: 8 additions & 6 deletions drivers/stepper/step_dir/step_dir_stepper_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
struct step_dir_stepper_common_config {
const struct gpio_dt_spec step_pin;
const struct gpio_dt_spec dir_pin;
const struct gpio_dt_spec *msx_pins;
bool dual_edge;
const struct stepper_timing_source_api *timing_source;
const struct device *counter;
Expand All @@ -42,24 +43,25 @@
*
* @param node_id The devicetree node identifier.
*/
#define STEP_DIR_STEPPER_DT_COMMON_CONFIG_INIT(node_id) \
{ \
#define STEP_DIR_STEPPER_DT_COMMON_CONFIG_INIT(node_id, msx_gpio_array) \
{ \
IF_ENABLED( DT_NODE_HAS_PROP(node_id, msx_gpios), (.msx_pins = msx_gpio_array,)) \

Check failure on line 48 in drivers/stepper/step_dir/step_dir_stepper_common.h

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

SPACING 

drivers/stepper/step_dir/step_dir_stepper_common.h:48  space prohibited after that open parenthesis '('
.step_pin = GPIO_DT_SPEC_GET(node_id, step_gpios), \
.dir_pin = GPIO_DT_SPEC_GET(node_id, dir_gpios), \
.dual_edge = DT_PROP_OR(node_id, dual_edge_step, false), \
.counter = DEVICE_DT_GET_OR_NULL(DT_PHANDLE(node_id, counter)), \
.invert_direction = DT_PROP(node_id, invert_direction), \
.timing_source = COND_CODE_1(DT_NODE_HAS_PROP(node_id, counter), \
(&step_counter_timing_source_api), \
(&step_work_timing_source_api)), \
(&step_work_timing_source_api)), \
}

/**
* @brief Initialize common step direction stepper config from devicetree instance.
* @param inst Instance.
*/
#define STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT(inst) \
STEP_DIR_STEPPER_DT_COMMON_CONFIG_INIT(DT_DRV_INST(inst))
#define STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT(inst, msx_gpio_array) \
STEP_DIR_STEPPER_DT_COMMON_CONFIG_INIT(DT_DRV_INST(inst), msx_gpio_array)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not read the property in here? Currently there is again potential for giving the property different names since the driver does the property checking.

Copy link
Member Author

Choose a reason for hiding this comment

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

the *msx-pins cannot be scalar initialized here, or? The pointer has to be initialized to an already existing array.


/**
* @brief Common step direction stepper data.
Expand Down Expand Up @@ -150,7 +152,7 @@
* @return 0 on success, or a negative error code on failure.
*/
int step_dir_stepper_common_set_microstep_interval(const struct device *dev,
const uint64_t microstep_interval_ns);
const uint64_t microstep_interval_ns);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated formatting :^)


/**
* @brief Set the reference position of the stepper motor.
Expand Down
40 changes: 23 additions & 17 deletions drivers/stepper/ti/drv84xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
struct step_dir_stepper_common_config common;
struct gpio_dt_spec sleep_pin;
struct gpio_dt_spec en_pin;
struct gpio_dt_spec m0_pin;
struct gpio_dt_spec m1_pin;
struct gpio_dt_spec fault_pin;
};

Expand Down Expand Up @@ -106,14 +104,14 @@
uint8_t m0_value = data->pin_states.m0;
uint8_t m1_value = data->pin_states.m1;

ret = drv84xx_set_microstep_pin(dev, &config->m0_pin, m0_value);
ret = drv84xx_set_microstep_pin(dev, &config->common.msx_pins[0], m0_value);
if (ret != 0) {
LOG_ERR("%s: Failed to restore microstep configuration (error: %d)", dev->name,
ret);
return ret;
}

ret = drv84xx_set_microstep_pin(dev, &config->m1_pin, m1_value);
ret = drv84xx_set_microstep_pin(dev, &config->common.msx_pins[1], m1_value);
if (ret != 0) {
LOG_ERR("%s: Failed to restore microstep configuration (error: %d)", dev->name,
ret);
Expand Down Expand Up @@ -272,7 +270,7 @@
uint8_t m0_value = 0;
uint8_t m1_value = 0;

if ((config->m0_pin.port == NULL) || (config->m1_pin.port == NULL)) {
if ((config->common.msx_pins[0].port == NULL) || (config->common.msx_pins[1].port == NULL)) {

Check warning on line 273 in drivers/stepper/ti/drv84xx.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

LONG_LINE 

drivers/stepper/ti/drv84xx.c:273  line length of 101 exceeds 100 columns

LOG_ERR("%s: Failed to set microstep resolution: microstep pins are not defined "
"(error: %d)",
Expand Down Expand Up @@ -326,12 +324,12 @@
return -ENOTSUP;
};

ret = drv84xx_set_microstep_pin(dev, &config->m0_pin, m0_value);
ret = drv84xx_set_microstep_pin(dev, &config->common.msx_pins[0], m0_value);
if (ret != 0) {
return ret;
}

ret = drv84xx_set_microstep_pin(dev, &config->m1_pin, m1_value);
ret = drv84xx_set_microstep_pin(dev, &config->common.msx_pins[1], m1_value);
if (ret != 0) {
return ret;
}
Expand Down Expand Up @@ -421,8 +419,8 @@
}

/* Configure microstep pin 0 if it is available */
if (config->m0_pin.port != NULL) {
ret = gpio_pin_configure_dt(&config->m0_pin, GPIO_OUTPUT_INACTIVE);
if (config->common.msx_pins[0].port != NULL) {
ret = gpio_pin_configure_dt(&config->common.msx_pins[0], GPIO_OUTPUT_INACTIVE);
if (ret != 0) {
LOG_ERR("%s: Failed to configure m0_pin (error: %d)", dev->name, ret);
return ret;
Expand All @@ -431,16 +429,16 @@
}

/* Configure microstep pin 1 if it is available */
if (config->m1_pin.port != NULL) {
ret = gpio_pin_configure_dt(&config->m1_pin, GPIO_OUTPUT_INACTIVE);
if (config->common.msx_pins[1].port != NULL) {
ret = gpio_pin_configure_dt(&config->common.msx_pins[1], GPIO_OUTPUT_INACTIVE);
if (ret != 0) {
LOG_ERR("%s: Failed to configure m1_pin (error: %d)", dev->name, ret);
return ret;
}
data->pin_states.m1 = 0U;
}

if ((config->m0_pin.port != NULL) && (config->m1_pin.port != NULL)) {
if ((config->common.msx_pins[0].port != NULL) && (config->common.msx_pins[1].port != NULL)) {

Check warning on line 441 in drivers/stepper/ti/drv84xx.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

LONG_LINE 

drivers/stepper/ti/drv84xx.c:441  line length of 101 exceeds 100 columns
ret = drv84xx_set_micro_step_res(dev, data->ustep_res);
if (ret != 0) {
return ret;
Expand Down Expand Up @@ -492,13 +490,21 @@
};

#define DRV84XX_DEVICE(inst) \
\
static const struct drv84xx_config drv84xx_config_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT(inst), \
IF_ENABLED(DT_INST_NODE_HAS_PROP(inst, msx_gpios), ( \
static const struct gpio_dt_spec drv84xx_stepper_msx_pins_##inst[] = { \
DT_INST_FOREACH_PROP_ELEM_SEP( \
inst, msx_gpios, GPIO_DT_SPEC_GET_BY_IDX, (,) \
), \
}; \
BUILD_ASSERT( \
ARRAY_SIZE(drv84xx_stepper_msx_pins_##inst) == 2, \
"Two microstep config pins needed"); \
)) \
static const struct drv84xx_config drv84xx_config_##inst = { \
.common = STEP_DIR_STEPPER_DT_INST_COMMON_CONFIG_INIT( \
inst, drv84xx_stepper_msx_pins_##inst), \
.sleep_pin = GPIO_DT_SPEC_INST_GET_OR(inst, sleep_gpios, {0}), \
.en_pin = GPIO_DT_SPEC_INST_GET_OR(inst, en_gpios, {0}), \
.m0_pin = GPIO_DT_SPEC_INST_GET_OR(inst, m0_gpios, {0}), \
.m1_pin = GPIO_DT_SPEC_INST_GET_OR(inst, m1_gpios, {0}), \
.fault_pin = GPIO_DT_SPEC_INST_GET_OR(inst, fault_gpios, {0}), \
}; \
\
Expand Down
8 changes: 0 additions & 8 deletions dts/bindings/stepper/adi/adi,tmc2209.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,6 @@ include:
- name: stepper-controller.yaml

properties:
msx-gpios:
type: phandle-array
description: |
An array of GPIO pins for configuring the microstep resolution of the driver.
The pins should be listed in the following order:
- MS1
- MS2

dual-edge-step:
type: boolean
description: |
Expand Down
14 changes: 2 additions & 12 deletions dts/bindings/stepper/allegro/allegro,a4979.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ description: |
dir-gpios = <&gpiod 14 GPIO_ACTIVE_HIGH>;
step-gpios = <&gpiod 15 GPIO_ACTIVE_HIGH>;
en-gpios = <&gpiod 11 GPIO_ACTIVE_HIGH>;
m0-gpios = <&gpiod 13 0>;
m1-gpios = <&gpiod 12 0>;
msx-gpios = <&gpiod 13 0>,
<&gpiod 12 0>;
counter = <&counter5>;
};

Expand All @@ -27,16 +27,6 @@ include:
- name: stepper-controller.yaml

properties:
m0-gpios:
required: true
type: phandle-array
description: Microstep configuration pin 0.

m1-gpios:
required: true
type: phandle-array
description: Microstep configuration pin 1.

reset-gpios:
type: phandle-array
required: true
Expand Down
9 changes: 9 additions & 0 deletions dts/bindings/stepper/stepper-controller.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ properties:
The GPIO pins used to send direction signals to the stepper motor.
Pin will be driven high for forward direction and low for reverse direction.

msx-gpios:
type: phandle-array
description: |
An array of GPIO pins for configuring the microstep resolution of the driver.
The pins should be listed in the following order:
- MS1
- MS2
- MS3

counter:
type: phandle
description: Counter used for generating step-accurate pulse signals.
Loading
Loading