Skip to content

Conversation

@jilaypandya
Copy link
Member

@jilaypandya jilaypandya commented Nov 3, 2024

This commit adds a common helper header for tmc5xxx driver in #80784

@jilaypandya jilaypandya force-pushed the feature/tmc5041-common-functions branch 2 times, most recently from 5e583c8 to e2e6ae9 Compare November 3, 2024 16:34
@jilaypandya jilaypandya marked this pull request as draft November 3, 2024 16:38
@jilaypandya jilaypandya force-pushed the feature/tmc5041-common-functions branch from e2e6ae9 to 4966f00 Compare November 3, 2024 16:53
@jilaypandya jilaypandya marked this pull request as ready for review November 3, 2024 16:54
@jilaypandya jilaypandya force-pushed the feature/tmc5041-common-functions branch from 4966f00 to 42ebdff Compare November 3, 2024 17:29
Comment on lines 35 to 37
inline void tmc5xxx_calculate_velocity_from_hz_to_fclk(const uint32_t velocity_hz,
uint32_t *velocity_fclk,
const uint32_t clock_frequency)
Copy link
Contributor

@bjarki-andreasen bjarki-andreasen Nov 3, 2024

Choose a reason for hiding this comment

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

I would update the signature to:

static inline uint32_t tmc5xxx_calculate_velocity_from_hz_to_fclk(uint64_t velocity_hz,
						                  uint32_t clock_frequency)
{
        __ASSERT_NO_MSG(clock_frequency);
        return (velocity_hz << TMC5XXX_CLOCK_FREQ_SHIFT) / clock_frequency;
}
  • static inline to allow the compiler to either inline it or make it a static function, whichever is most efficient.
  • since the function "can't fail", just return the result. Otherwise we would return a status, and copy out the result.
  • add an assert to prevent divide by zero error
  • make velocity_hz 64 bit removing the need to promote it and make it mutable (no const) to allow the compiler to work directly on the variable which is already on the stack
  • since we are copying the arguments entirely, const really does not help us :) const is useful when referencing a structure outside of the function, so we don't modify something we don't own, inside the function its not really helpful :)

@jilaypandya jilaypandya force-pushed the feature/tmc5041-common-functions branch 2 times, most recently from e43b127 to e42bb56 Compare November 3, 2024 20:24
@jilaypandya jilaypandya force-pushed the feature/tmc5041-common-functions branch from e42bb56 to 82eb9fb Compare November 3, 2024 20:31
Comment on lines 481 to 482
Copy link
Contributor

@bjarki-andreasen bjarki-andreasen Nov 3, 2024

Choose a reason for hiding this comment

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

the indentation here can be helped by keeping the definition of uint32_t velocity_fclk in the header (where it should be according to MISRA), and even defining tmc5041_config->clock_frequency:

uint32_t clock_frequency = tmc5041_config->clock_frequency;
uint32_t velocity_fclk;
...
velocity_fclk = tmc5xxx_calculate_velocity_from_hz_to_fclk(velocity, clock_frequency);

the compiler will optimize the clock_frequency away :)

This commit adds a common helper header for tmc5xxx driver

Signed-off-by: Jilay Pandya <[email protected]>
Copy link
Contributor

@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.

Nice :)

#endif

/** Common Registers for TMC5041 and TMC51XX */
#if defined(CONFIG_STEPPER_ADI_TMC5041)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this rely on the correct include order? The autoconf is not pulled in otherwise.

@mmahadevan108 mmahadevan108 added this to the v4.1.0 milestone Nov 7, 2024
@nashif nashif merged commit 5032d8e into zephyrproject-rtos:main Nov 16, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants