Skip to content

drivers: stepper: tmc_spi: remove print_status_byte #88795

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

Merged
merged 1 commit into from
Apr 29, 2025

Conversation

jilaypandya
Copy link
Member

@jilaypandya jilaypandya commented Apr 18, 2025

print_status_byte is specific to tmc50xx and hence shoud not be placeed in common tmc_spi.c which is supposed to be reused by a variety of drivers

This print of status byte should rather be part of driver code

dipakgmx
dipakgmx previously approved these changes Apr 18, 2025
Copilot

This comment was marked as resolved.

@jilaypandya jilaypandya force-pushed the fix/tmc-spi branch 2 times, most recently from dab87ce to 847e8b0 Compare April 18, 2025 16:06
@@ -76,7 +60,7 @@ int tmc_spi_read_register(const struct spi_dt_spec *bus, const uint8_t read_addr
((uint32_t)rx_buffer[3] << 8) + (uint32_t)rx_buffer[4];

print_tx_rx_buffer(tx_buffer, rx_buffer);
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to even drop this? I assume, on the spi layer this would be logged?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah let's drop it or make it a Kconfig symbol?

Copy link
Member

Choose a reason for hiding this comment

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

I guess drop it, but i remember it correctly, the SPI logs would only show the data buffer address (not sure).
But I would say, we can drop this.

#include "adi_tmc_spi.h"

#define BUFFER_SIZE 5U

#include <zephyr/logging/log.h>

LOG_MODULE_REGISTER(tmc_spi, CONFIG_SPI_LOG_LEVEL);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LOG_MODULE_REGISTER(tmc_spi, CONFIG_SPI_LOG_LEVEL);
LOG_MODULE_REGISTER(tmc_spi, CONFIG_STEPPER_LOG_LEVEL);

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 is required for print_tx_rx_buffer , should we drop that function as well?

Copy link
Member

Choose a reason for hiding this comment

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

That was also my question here.

Copy link
Contributor

@apni2 apni2 Apr 22, 2025

Choose a reason for hiding this comment

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

I think decoding the tmc specific SPI status is a nice debug feature. So consider combining with: #88102

dipakgmx
dipakgmx previously approved these changes Apr 19, 2025
faxe1008
faxe1008 previously approved these changes Apr 22, 2025
print_status_byte is specific to tmc50xx and hence shoud not
be placeed in common tmc_spi.c which is supposed to be reused
by a variety of drivers

Signed-off-by: Jilay Pandya <[email protected]>
@fabiobaltieri fabiobaltieri merged commit a371f0c into zephyrproject-rtos:main Apr 29, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Stepper platform: ADI Analog Devices, Inc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants