drivers: video: stm32: HAL return value handling #97284
Conversation
zephyrbot
requested review from
avolmat-st,
djiatsaf-st,
erwango,
FRASTM,
gautierg-st,
GeorgeCGV,
josuah,
loicpoulain,
marwaiehm-st,
mathieuchopstm and
ngphibang
drivers/video/video_stm32_dcmi.c
Outdated
| HAL_StatusTypeDef hal_ret; | ||
|
|
||
| HAL_DCMI_Suspend(hdcmi); | ||
| hal _ret = HAL_DCMI_Suspend(hdcmi); |
There was a problem hiding this comment.
Variable name to be corrected. Seems there is space between hal and _ret.
drivers/video/video_stm32_dcmipp.c
Outdated
| ret = HAL_DCMIPP_PIPE_DisableISPRawBayer2RGB(&dcmipp->hdcmipp, | ||
| DCMIPP_PIPE1); | ||
| if (ret != HAL_OK) { | ||
| hal_et = HAL_DCMIPP_PIPE_DisableISPRawBayer2RGB(&dcmipp->hdcmipp, |
drivers/video/video_stm32_dcmipp.c
Outdated
| /* Stop the external ISP handling */ | ||
| ret = stm32_dcmipp_isp_stop(); | ||
| if (ret < 0) { | ||
| if (ret != 0) { |
There was a problem hiding this comment.
In the other place in the driver, we use ret < 0 (except for HAL return values) so is there a specific reason to use != 0 instead here ?
There was a problem hiding this comment.
+1. We should stick to < 0 unless there's a good reason not to.
There was a problem hiding this comment.
Well, I changed this line for consistency with the one you also commented below.
(edited) actually a stupid idea of mine since the above sets ret only when STM32_DCMIPP_HAS_PIXEL_PIPES is enabled.
But I agree they can be consistent testing both against < 0.
I'll update.
drivers/video/video_stm32_dcmipp.c
Outdated
| goto out; | ||
| } | ||
| if (ret != HAL_OK) { | ||
| if (ret != 0) { |
etienne-lms
force-pushed
the
stm32-hal-ret-video
branch
from
0cf27b3 to
6231c71
Compare
drivers/video/video_stm32_dcmipp.c
Outdated
| struct stm32_dcmipp_data *dcmipp = pipe->dcmipp; | ||
| const DCMIPP_ColorConversionConfTypeDef *cfg = NULL; | ||
| int ret; | ||
| HAL_StatusTypeDef ret; |
There was a problem hiding this comment.
Would it be sensible to always variables involving the hal be named hal_ret even when there is no ret?
Otherwise, code that is moved from a function or another quickly faces the same issue.
Thank you for this PR.
There was a problem hiding this comment.
I agree it would be more consistent and less error prone on future changes.
I did not want to be too much intrusive but as you raise the question I guess it's worth it.
I'll update accordingly.
etienne-lms
force-pushed
the
stm32-hal-ret-video
branch
from
6231c71 to
afc7a56
Compare
Clarify HAL return value is of type HAL_StatusTypeDef and not an int in STM32 DCMI and DCMIPP drivers. For consistency, rename the variable holding HAL return value from ret to hal_ret. Signed-off-by: Etienne Carriere <[email protected]>
Add missing test of some HAL functions return value. Signed-off-by: Etienne Carriere <[email protected]>
etienne-lms
force-pushed
the
stm32-hal-ret-video
branch
from
afc7a56 to
495e37b
Compare
|
|
Review comments addressed. |
Add missing test of some HAL functions return value in STM32 DCMI and DCMIPP drivers.
Clarify HAL return value is of type
HAL_StatusTypeDefand not anintin STM32 DCMI and DCMIPP drivers.