Skip to content

Commit d18f491

Browse files
jilaypandyakartben
jilaypandya
authored and
kartben
committed
drivers: stepper: refactor enable(dev,flag) to enable & disable
refactoring enable function into enable and disable increasing readability and increasing coherence with other stepper apis in terms of nomenclature Signed-off-by: Jilay Pandya <[email protected]>
1 parent 3b4383b commit d18f491

File tree

15 files changed

+287
-125
lines changed

15 files changed

+287
-125
lines changed

‎drivers/stepper/adi_tmc/adi_tmc22xx_stepper_controller.c

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,20 @@ struct tmc22xx_data {
2525

2626
STEP_DIR_STEPPER_STRUCT_CHECK(struct tmc22xx_config, struct tmc22xx_data);
2727

28-
static int tmc22xx_stepper_enable(const struct device *dev, const bool enable)
28+
static int tmc22xx_stepper_enable(const struct device *dev)
2929
{
3030
const struct tmc22xx_config *config = dev->config;
3131

32-
LOG_DBG("Stepper motor controller %s %s", dev->name, enable ? "enabled" : "disabled");
33-
if (enable) {
34-
return gpio_pin_set_dt(&config->enable_pin, 1);
35-
} else {
36-
return gpio_pin_set_dt(&config->enable_pin, 0);
37-
}
32+
LOG_DBG("Enabling Stepper motor controller %s", dev->name);
33+
return gpio_pin_set_dt(&config->enable_pin, 1);
34+
}
35+
36+
static int tmc22xx_stepper_disable(const struct device *dev)
37+
{
38+
const struct tmc22xx_config *config = dev->config;
39+
40+
LOG_DBG("Disabling Stepper motor controller %s", dev->name);
41+
return gpio_pin_set_dt(&config->enable_pin, 0);
3842
}
3943

4044
static int tmc22xx_stepper_set_micro_step_res(const struct device *dev,
@@ -145,6 +149,7 @@ static int tmc22xx_stepper_init(const struct device *dev)
145149

146150
static DEVICE_API(stepper, tmc22xx_stepper_api) = {
147151
.enable = tmc22xx_stepper_enable,
152+
.disable = tmc22xx_stepper_disable,
148153
.move_by = step_dir_stepper_common_move_by,
149154
.is_moving = step_dir_stepper_common_is_moving,
150155
.set_reference_position = step_dir_stepper_common_set_reference_position,

‎drivers/stepper/adi_tmc/adi_tmc50xx_stepper_controller.c

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -258,9 +258,9 @@ static void rampstat_work_handler(struct k_work *work)
258258

259259
#endif
260260

261-
static int tmc50xx_stepper_enable(const struct device *dev, const bool enable)
261+
static int tmc50xx_stepper_enable(const struct device *dev)
262262
{
263-
LOG_DBG("Stepper motor controller %s %s", dev->name, enable ? "enabled" : "disabled");
263+
LOG_DBG("Enabling Stepper motor controller %s", dev->name);
264264
const struct tmc50xx_stepper_config *config = dev->config;
265265
uint32_t reg_value;
266266
int err;
@@ -270,17 +270,26 @@ static int tmc50xx_stepper_enable(const struct device *dev, const bool enable)
270270
return -EIO;
271271
}
272272

273-
if (enable) {
274-
reg_value |= TMC5XXX_CHOPCONF_DRV_ENABLE_MASK;
275-
} else {
276-
reg_value &= ~TMC5XXX_CHOPCONF_DRV_ENABLE_MASK;
277-
}
273+
reg_value |= TMC5XXX_CHOPCONF_DRV_ENABLE_MASK;
278274

279-
err = tmc50xx_write(config->controller, TMC50XX_CHOPCONF(config->index), reg_value);
275+
return tmc50xx_write(config->controller, TMC50XX_CHOPCONF(config->index), reg_value);
276+
}
277+
278+
static int tmc50xx_stepper_disable(const struct device *dev)
279+
{
280+
LOG_DBG("Disabling Stepper motor controller %s", dev->name);
281+
const struct tmc50xx_stepper_config *config = dev->config;
282+
uint32_t reg_value;
283+
int err;
284+
285+
err = tmc50xx_read(config->controller, TMC50XX_CHOPCONF(config->index), &reg_value);
280286
if (err != 0) {
281287
return -EIO;
282288
}
283-
return 0;
289+
290+
reg_value &= ~TMC5XXX_CHOPCONF_DRV_ENABLE_MASK;
291+
292+
return tmc50xx_write(config->controller, TMC50XX_CHOPCONF(config->index), reg_value);
284293
}
285294

286295
static int tmc50xx_stepper_is_moving(const struct device *dev, bool *is_moving)
@@ -681,6 +690,20 @@ static int tmc50xx_stepper_init(const struct device *dev)
681690
return 0;
682691
}
683692

693+
static DEVICE_API(stepper, tmc50xx_stepper_api) = {
694+
.enable = tmc50xx_stepper_enable,
695+
.disable = tmc50xx_stepper_disable,
696+
.is_moving = tmc50xx_stepper_is_moving,
697+
.move_by = tmc50xx_stepper_move_by,
698+
.set_micro_step_res = tmc50xx_stepper_set_micro_step_res,
699+
.get_micro_step_res = tmc50xx_stepper_get_micro_step_res,
700+
.set_reference_position = tmc50xx_stepper_set_reference_position,
701+
.get_actual_position = tmc50xx_stepper_get_actual_position,
702+
.move_to = tmc50xx_stepper_move_to,
703+
.run = tmc50xx_stepper_run,
704+
.set_event_callback = tmc50xx_stepper_set_event_callback,
705+
};
706+
684707
#define TMC50XX_SHAFT_CONFIG(child) \
685708
(DT_PROP(child, invert_direction) << TMC50XX_GCONF_SHAFT_SHIFT(DT_REG_ADDR(child))) |
686709

@@ -705,23 +728,10 @@ static int tmc50xx_stepper_init(const struct device *dev)
705728
static struct tmc50xx_stepper_data tmc50xx_stepper_data_##child = { \
706729
.stepper = DEVICE_DT_GET(child),};
707730

708-
#define TMC50XX_STEPPER_API_DEFINE(child) \
709-
static DEVICE_API(stepper, tmc50xx_stepper_api_##child) = { \
710-
.enable = tmc50xx_stepper_enable, \
711-
.is_moving = tmc50xx_stepper_is_moving, \
712-
.move_by = tmc50xx_stepper_move_by, \
713-
.set_micro_step_res = tmc50xx_stepper_set_micro_step_res, \
714-
.get_micro_step_res = tmc50xx_stepper_get_micro_step_res, \
715-
.set_reference_position = tmc50xx_stepper_set_reference_position, \
716-
.get_actual_position = tmc50xx_stepper_get_actual_position, \
717-
.move_to = tmc50xx_stepper_move_to, \
718-
.run = tmc50xx_stepper_run, \
719-
.set_event_callback = tmc50xx_stepper_set_event_callback, };
720-
721731
#define TMC50XX_STEPPER_DEFINE(child) \
722732
DEVICE_DT_DEFINE(child, tmc50xx_stepper_init, NULL, &tmc50xx_stepper_data_##child, \
723733
&tmc50xx_stepper_config_##child, POST_KERNEL, \
724-
CONFIG_STEPPER_INIT_PRIORITY, &tmc50xx_stepper_api_##child);
734+
CONFIG_STEPPER_INIT_PRIORITY, &tmc50xx_stepper_api);
725735

726736
#define TMC50XX_DEFINE(inst) \
727737
BUILD_ASSERT(DT_INST_CHILD_NUM(inst) <= 2, "tmc50xx can drive two steppers at max"); \
@@ -739,7 +749,6 @@ static int tmc50xx_stepper_init(const struct device *dev)
739749
.clock_frequency = DT_INST_PROP(inst, clock_frequency),}; \
740750
DT_INST_FOREACH_CHILD(inst, TMC50XX_STEPPER_CONFIG_DEFINE); \
741751
DT_INST_FOREACH_CHILD(inst, TMC50XX_STEPPER_DATA_DEFINE); \
742-
DT_INST_FOREACH_CHILD(inst, TMC50XX_STEPPER_API_DEFINE); \
743752
DT_INST_FOREACH_CHILD(inst, TMC50XX_STEPPER_DEFINE); \
744753
DEVICE_DT_INST_DEFINE(inst, tmc50xx_init, NULL, &tmc50xx_data_##inst, \
745754
&tmc50xx_config_##inst, POST_KERNEL, CONFIG_STEPPER_INIT_PRIORITY,\

‎drivers/stepper/allegro/a4979.c

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ static int a4979_set_microstep_pin(const struct device *dev, const struct gpio_d
5050
return 0;
5151
}
5252

53-
static int a4979_stepper_enable(const struct device *dev, bool enable)
53+
static int a4979_stepper_enable(const struct device *dev)
5454
{
5555
int ret;
5656
const struct a4979_config *config = dev->config;
@@ -63,17 +63,40 @@ static int a4979_stepper_enable(const struct device *dev, bool enable)
6363
return -ENOTSUP;
6464
}
6565

66-
ret = gpio_pin_set_dt(&config->en_pin, enable);
66+
ret = gpio_pin_set_dt(&config->en_pin, 1);
6767
if (ret != 0) {
6868
LOG_ERR("%s: Failed to set en_pin (error: %d)", dev->name, ret);
6969
return ret;
7070
}
7171

72-
data->enabled = enable;
73-
if (!enable) {
74-
config->common.timing_source->stop(dev);
72+
data->enabled = true;
73+
74+
return 0;
75+
}
76+
77+
static int a4979_stepper_disable(const struct device *dev)
78+
{
79+
80+
int ret;
81+
const struct a4979_config *config = dev->config;
82+
struct a4979_data *data = dev->data;
83+
bool has_enable_pin = config->en_pin.port != NULL;
84+
85+
/* Check availability of enable pin, as it might be hardwired. */
86+
if (!has_enable_pin) {
87+
LOG_ERR("%s: Enable pin undefined.", dev->name);
88+
return -ENOTSUP;
89+
}
90+
91+
ret = gpio_pin_set_dt(&config->en_pin, 0);
92+
if (ret != 0) {
93+
LOG_ERR("%s: Failed to set en_pin (error: %d)", dev->name, ret);
94+
return ret;
7595
}
7696

97+
config->common.timing_source->stop(dev);
98+
data->enabled = false;
99+
77100
return 0;
78101
}
79102

@@ -245,6 +268,7 @@ static int a4979_init(const struct device *dev)
245268

246269
static DEVICE_API(stepper, a4979_stepper_api) = {
247270
.enable = a4979_stepper_enable,
271+
.disable = a4979_stepper_disable,
248272
.move_by = a4979_stepper_move_by,
249273
.move_to = a4979_move_to,
250274
.is_moving = step_dir_stepper_common_is_moving,

‎drivers/stepper/fake_stepper_controller.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@ struct fake_stepper_data {
1919
int32_t actual_position;
2020
};
2121

22-
DEFINE_FAKE_VALUE_FUNC(int, fake_stepper_enable, const struct device *, bool);
22+
DEFINE_FAKE_VALUE_FUNC(int, fake_stepper_enable, const struct device *);
23+
24+
DEFINE_FAKE_VALUE_FUNC(int, fake_stepper_disable, const struct device *);
2325

2426
DEFINE_FAKE_VALUE_FUNC(int, fake_stepper_is_moving, const struct device *, bool *);
2527

@@ -91,6 +93,7 @@ static void fake_stepper_reset_rule_before(const struct ztest_unit_test *test, v
9193
ARG_UNUSED(fixture);
9294

9395
RESET_FAKE(fake_stepper_enable);
96+
RESET_FAKE(fake_stepper_disable);
9497
RESET_FAKE(fake_stepper_move_by);
9598
RESET_FAKE(fake_stepper_is_moving);
9699
RESET_FAKE(fake_stepper_set_microstep_interval);
@@ -128,6 +131,7 @@ static int fake_stepper_init(const struct device *dev)
128131

129132
static DEVICE_API(stepper, fake_stepper_driver_api) = {
130133
.enable = fake_stepper_enable,
134+
.disable = fake_stepper_disable,
131135
.move_by = fake_stepper_move_by,
132136
.is_moving = fake_stepper_is_moving,
133137
.set_microstep_interval = fake_stepper_set_microstep_interval,

‎drivers/stepper/gpio_stepper_controller.c

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -325,23 +325,35 @@ static int gpio_stepper_set_event_callback(const struct device *dev,
325325
return 0;
326326
}
327327

328-
static int gpio_stepper_enable(const struct device *dev, bool enable)
328+
static int gpio_stepper_enable(const struct device *dev)
329329
{
330330
struct gpio_stepper_data *data = dev->data;
331331
int err;
332332

333-
if (data->is_enabled == enable) {
333+
if (data->is_enabled) {
334+
LOG_WRN("Stepper motor is already enabled");
334335
return 0;
335336
}
336337

337338
K_SPINLOCK(&data->lock) {
338-
data->is_enabled = enable;
339+
err = energize_coils(dev, true);
340+
if (err == 0) {
341+
data->is_enabled = true;
342+
}
343+
}
344+
return err;
345+
}
339346

340-
if (enable) {
341-
err = energize_coils(dev, true);
342-
} else {
343-
(void)k_work_cancel_delayable(&data->stepper_dwork);
344-
err = energize_coils(dev, false);
347+
static int gpio_stepper_disable(const struct device *dev)
348+
{
349+
struct gpio_stepper_data *data = dev->data;
350+
int err;
351+
352+
K_SPINLOCK(&data->lock) {
353+
(void)k_work_cancel_delayable(&data->stepper_dwork);
354+
err = energize_coils(dev, false);
355+
if (err == 0) {
356+
data->is_enabled = false;
345357
}
346358
}
347359
return err;
@@ -379,6 +391,7 @@ static int gpio_stepper_init(const struct device *dev)
379391

380392
static DEVICE_API(stepper, gpio_stepper_api) = {
381393
.enable = gpio_stepper_enable,
394+
.disable = gpio_stepper_disable,
382395
.set_micro_step_res = gpio_stepper_set_micro_step_res,
383396
.get_micro_step_res = gpio_stepper_get_micro_step_res,
384397
.set_reference_position = gpio_stepper_set_reference_position,

‎drivers/stepper/stepper_shell.c

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -178,19 +178,32 @@ static int parse_device_arg(const struct shell *sh, char **argv, const struct de
178178
static int cmd_stepper_enable(const struct shell *sh, size_t argc, char **argv)
179179
{
180180
const struct device *dev;
181-
int err = 0;
182-
bool enable = shell_strtobool(argv[ARG_IDX_PARAM], 10, &err);
181+
int err;
183182

183+
err = parse_device_arg(sh, argv, &dev);
184184
if (err < 0) {
185185
return err;
186186
}
187187

188+
err = stepper_enable(dev);
189+
if (err) {
190+
shell_error(sh, "Error: %d", err);
191+
}
192+
193+
return err;
194+
}
195+
196+
static int cmd_stepper_disable(const struct shell *sh, size_t argc, char **argv)
197+
{
198+
const struct device *dev;
199+
int err;
200+
188201
err = parse_device_arg(sh, argv, &dev);
189202
if (err < 0) {
190203
return err;
191204
}
192205

193-
err = stepper_enable(dev, enable);
206+
err = stepper_disable(dev);
194207
if (err) {
195208
shell_error(sh, "Error: %d", err);
196209
}
@@ -477,8 +490,8 @@ static int cmd_stepper_info(const struct shell *sh, size_t argc, char **argv)
477490

478491
SHELL_STATIC_SUBCMD_SET_CREATE(
479492
stepper_cmds,
480-
SHELL_CMD_ARG(enable, &dsub_pos_stepper_motor_name, "<device> <on/off>", cmd_stepper_enable,
481-
3, 0),
493+
SHELL_CMD_ARG(enable, &dsub_pos_stepper_motor_name, "<device>", cmd_stepper_enable, 2, 0),
494+
SHELL_CMD_ARG(disable, &dsub_pos_stepper_motor_name, "<device>", cmd_stepper_disable, 2, 0),
482495
SHELL_CMD_ARG(set_micro_step_res, &dsub_pos_stepper_motor_name_microstep,
483496
"<device> <resolution>", cmd_stepper_set_micro_step_res, 3, 0),
484497
SHELL_CMD_ARG(get_micro_step_res, &dsub_pos_stepper_motor_name, "<device>",

0 commit comments

Comments
 (0)