Skip to content
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.

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

drivers: stepper: refactor enable(dev,flag) to enable & disable #87491

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jilaypandya
Copy link
Member

@jilaypandya jilaypandya commented Mar 21, 2025

refactoring enable function into enable and disable increasing readability and increasing coherence with other stepper apis in terms of nomenclature

#87229

@jilaypandya jilaypandya requested a review from dipakgmx March 21, 2025 18:14
@jilaypandya jilaypandya force-pushed the fix/stepper-87229 branch 4 times, most recently from 5621e31 to 683fd66 Compare March 22, 2025 18:44
@jilaypandya jilaypandya marked this pull request as ready for review March 23, 2025 08:23
@zephyrbot zephyrbot added area: Samples Samples Release Notes To be mentioned in the release notes area: Stepper labels Mar 23, 2025
@jilaypandya jilaypandya removed the Release Notes To be mentioned in the release notes label Mar 23, 2025
@jilaypandya jilaypandya added the Release Notes To be mentioned in the release notes label Mar 23, 2025
@@ -708,6 +726,7 @@ static int tmc50xx_stepper_init(const struct device *dev)
#define TMC50XX_STEPPER_API_DEFINE(child) \
static DEVICE_API(stepper, tmc50xx_stepper_api_##child) = { \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed this - why is there an instance of the api per stepper? Shouldn't there only be one?

Copy link
Member Author

Choose a reason for hiding this comment

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

This particular driver can support two steppers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah but the api is the same for both. If you'd have two bme sensors on your board you end up with only one api instance in your binary - I feel like the same applies here, unlese I miss something

Copy link
Member Author

Choose a reason for hiding this comment

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

That's 100% correct, thanks :) Have rectified it :)

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]>
- Add migration guide entry for enable->enable/disable function
- Add stepper_disable to stepper.rst

Signed-off-by: Jilay Pandya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Samples Samples area: Stepper Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants