-
Notifications
You must be signed in to change notification settings - Fork 7.6k
drivers: sdhc: sdhc_spi: release bus on error, and fix CMD12 failue logic #72754
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: sdhc: sdhc_spi: release bus on error, and fix CMD12 failue logic #72754
Conversation
Properly release SPI bus on transmit error within the SDHC SPI driver. In these cases return code is not checked, as we wish to return the error code from the failed transfer to the SD stack. Fixes zephyrproject-rtos#72364 Signed-off-by: Daniel DeGrasse <[email protected]>
e80ffa1
to
8383861
Compare
drivers/sdhc/sdhc_spi.c
Outdated
(struct sdhc_command *)&stop_cmd, | ||
false); | ||
while ((stop_ret != 0) && (retries < 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it retries < 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this should be retries > 0
.
drivers/sdhc/sdhc_spi.c
Outdated
ret = stop_ret = sdhc_spi_send_cmd(dev, | ||
(struct sdhc_command *)&stop_cmd, | ||
false); | ||
retries--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the same variable retries in so many places is making this code confusing to read for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way you'd rather structure this? As far as I know, only two locations in this function decrement the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it's the changing and the checking, it feels like this code is doing 2 different things with one variable, there is retries for sending the stop command and retries for the request as a whole, should these be two different counts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see the concern- given how we document the retries
field, having a separate retry counter for this makes sense- I'll add one.
b54afd8
to
6459fa3
Compare
Rework CMD12 failure logic for SDHC SPI driver. Previously, the error code of CMD12 was not checked, so even if CMD12 failed to send the initial command would be retried. Change this behavior to retry CMD12 until it succeeds. If CMD12 fails, its error code will be propagated to the caller. Otherwise, the return code from the command being sent by the caller will be propagated. Fixes zephyrproject-rtos#72365 Signed-off-by: Daniel DeGrasse <[email protected]>
6459fa3
to
4ce25b8
Compare
Properly release SPI bus on transmit error within the SDHC SPI driver. In these cases return code is not checked, as we wish to return the error code from the failed transfer to the SD stack.
Fixes #72364
drivers: sdhc: sdhc_spi: rework CMD12 failure logic
Rework CMD12 failure logic for SDHC SPI driver. Previously, the error
code of CMD12 was not checked, so even if CMD12 failed to send the
initial command would be retried. Change this behavior to retry CMD12
until it succeeds. If CMD12 fails, its error code will be propagated to
the caller. Otherwise, the return code from the command being sent by
the caller will be propagated.
Fixes #72365