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.

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

Jump to bottom

drivers: stepper: adi: trinamic tmc5041 #79163

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

drivers: stepper: adi: trinamic tmc5041 #79163

wants to merge 2 commits into from

Conversation

jilaypandya
Copy link
Contributor

@jilaypandya jilaypandya commented Sep 29, 2024 •
edited
Loading

This PR does the following:

  1. Introduce initial structure for trinamic stepper drivers.
  2. TMC5041 driver.

Todos:

  • Add this driver to build once the file structutre and API changes have been agreed upon
  • Add invert direction to stepper-controller.yaml

@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from 92e4488 to f6d112b Compare September 29, 2024 09:03
@jilaypandya jilaypandya marked this pull request as draft September 29, 2024 09:03
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch 3 times, most recently from 5d3070e to c1c828a Compare September 29, 2024 09:25
@jilaypandya jilaypandya marked this pull request as ready for review September 29, 2024 09:30
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from c1c828a to 7d84fc3 Compare September 29, 2024 10:17
bjarki-andreasen
bjarki-andreasen previously requested changes Sep 29, 2024
View reviewed changes
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Device driver specific APIs go to include/zephyr/drivers/stepper/ and are not exposed using an API structure, they are just "normal" functions :)

@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch 2 times, most recently from 7d16948 to 466ad9a Compare September 29, 2024 11:12
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch 5 times, most recently from d5cd2cb to 563a319 Compare September 29, 2024 15:46
bjarki-andreasen
bjarki-andreasen reviewed Sep 29, 2024
View reviewed changes
include/zephyr/drivers/stepper/stepper_trinamic.h Show resolved Hide resolved
include/zephyr/drivers/stepper/stepper_trinamic.h Outdated
extern "C" {
#endif

#ifdef CONFIG_TRINAMIC_RAMP_GENERATOR
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to ifdef these, the structs and function declarations can be visible, only the driver needs to be "ifdefed" out AKA not included in the build :)

drivers/stepper/adi/Kconfig Outdated Show resolved Hide resolved
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from 563a319 to 295ca4a Compare September 29, 2024 19:36
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch 2 times, most recently from 3af7d3f to cf74a7c Compare September 30, 2024 09:07
bjarki-andreasen
bjarki-andreasen reviewed Sep 30, 2024
View reviewed changes
include/zephyr/drivers/stepper/stepper_trinamic.h Outdated
Comment on lines 41 to 49
.tzerowait = DT_PROP_OR(node, tzerowait, 0), \
.vstart = DT_PROP_OR(node, vstart, 0), .a1 = DT_PROP_OR(node, a1, 0), \
.amax = DT_PROP_OR(node, amax, 0), .d1 = DT_PROP_OR(node, d1, 0), \
.dmax = DT_PROP_OR(node, dmax, 0), .v1 = DT_PROP_OR(node, v1, 0), \
.vmax = DT_PROP_OR(node, vmax, 0), .vstop = DT_PROP_OR(node, vstop, 0), \
.vhigh = DT_PROP_OR(node, vhigh, 0), .vcoolthrs = DT_PROP_OR(node, vcoolthrs, 0), \
.iholdrun = (TMC5041_IRUN(DT_PROP_OR(node, irun, 0)) | \
TMC5041_IHOLD(DT_PROP_OR(node, ihold, 0)) | \
TMC5041_IHOLDDELAY(DT_PROP_OR(node, iholddelay, 0))), \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you format this so it matches the layout of struct tmc_ramp_generator_data ?

	{                 
		.vstart = DT_PROP_OR(node, vstart, 0),
                .a1 = DT_PROP_OR(node, a1, 0),
                ... 
        }

that makes it easy to read/review since you can go one by one without searching :)

@jilaypandya
Copy link
Contributor Author

Update:
I have removed the interrupt detection for now. Will introduce that feature in an upcoming PR most probably. However there is a provision to poll the RAMP_STAT register and check for pos_reached or stall event

@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch 2 times, most recently from 2d1293f to e13026e Compare October 1, 2024 18:55
dipakgmx
dipakgmx reviewed Oct 1, 2024
View reviewed changes
drivers/stepper/adi_tmc/ad_tmc5041_stepper_controller.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

File name seems incorrect. Needs to be adi_tmc5041_stepper_controller.c.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i took the reference from sensors actually. over there they have names such adltc2990.c and so on, in the folder adi they use prefix ad for analog devices. Probably let's keep it that way for now?, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a second thought, adi_tmc sounds better

Copy link
Contributor

Choose a reason for hiding this comment

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

is the ad_ prefix needed in the first place?

Copy link
Contributor Author

@jilaypandya jilaypandya Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think adi_tmc prefix looks good, it's done at some places in the project, in sensors its done widely
#79163 (comment)

dipakgmx
dipakgmx reviewed Oct 1, 2024
View reviewed changes
drivers/stepper/adi_tmc/ad_tmc_reg.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

File needs to to have tmc5041 in its name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of putting all the registers in a single file like this, I think a lot of these registers are repetitive and could be used across drivers.

#ifdef CONFIG_STEPPER_ADI_TMC5041

#define TMC5041_MOTOR_ADDR(m)     (0x20 << (m))
#define TMC5041_MOTOR_ADDR_DRV(m) ((m) << 4)
#define TMC5041_MOTOR_ADDR_PWM(m) ((m) << 3)

@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from e13026e to a8b150b Compare October 1, 2024 21:22
@bjarki-andreasen bjarki-andreasen dismissed their stale review October 2, 2024 11:31

The reason for nack has been resolved

@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from a8b150b to 071d397 Compare October 2, 2024 22:03
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from 071d397 to 677d88c Compare October 2, 2024 23:26
faxe1008
faxe1008 reviewed Oct 3, 2024
View reviewed changes
drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c Outdated
return -EIO;
}

*is_moving = (FIELD_GET(TMC5041_DRV_STATUS_STST_SHIFT_BIT, reg_value) == 1U) ? true : false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ternary not needed here the result is already bool

drivers/stepper/adi_tmc/adi_tmc5041_stepper_controller.c Outdated
const struct tmc5041_config *config = dev->config;
int err;

k_sem_init(&data->sem, 0, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could add a static declaration of the semaphore to the device definition macro and only store a pointer to it in the data, this would remove the need for this initialisation. Or just combine the two lines by setting the initial count to 1 straight away.

@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch 2 times, most recently from 9bfcdfd to 335e777 Compare October 3, 2024 06:53
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from 335e777 to f10c767 Compare October 7, 2024 11:00
@jilaypandya
drivers: stepper: adi: trinamic tmc5041
493f7dc 
This commit introduces initial structure for trinamic drivers
TMC5041 is implemented with following features:
- StallGuard
- RAMPSTAT_POLL
- RAMP_GEN

Signed-off-by: Dipak Shetty <dipak.shetty@zeiss.com>
Signed-off-by: Jilay Pandya <jilay.pandya@zeiss.com>
@jilaypandya jilaypandya force-pushed the feature/introduce-trinamic-driver branch from f10c767 to 493f7dc Compare October 7, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarull gmarull Awaiting requested review from gmarull

@fabiobaltieri fabiobaltieri Awaiting requested review from fabiobaltieri

@trinamic-ed trinamic-ed Awaiting requested review from trinamic-ed

@bjarki-andreasen bjarki-andreasen Awaiting requested review from bjarki-andreasen

@faxe1008 faxe1008 Awaiting requested review from faxe1008

@dipakgmx dipakgmx Awaiting requested review from dipakgmx

Requested changes must be addressed to merge this pull request.

Assignees

@jilaypandya jilaypandya

Labels
area: Stepper
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jilaypandya @fabiobaltieri @trinamic-ed @faxe1008 @gmarull @dipakgmx @bjarki-andreasen @zephyrbot