drivers: remove on-stack copying of dma cfg on stm32 drivers #100223
Conversation
mariopaja
force-pushed
the
st_remove_on_stack_dma_cfg
branch
3 times, most recently
from
ae6322a to
2ca6e51
Compare
mariopaja
force-pushed
the
st_remove_on_stack_dma_cfg
branch
from
2ca6e51 to
007c79b
Compare
zephyrbot
added
area: Video
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
TomasBarakNXP,
anangl,
avolmat-st,
de-nordic,
djiatsaf-st,
erwango,
etienne-lms,
gautierg-st,
josuah,
juickar,
loicpoulain,
mathieuchopstm and
rghaddab
| struct k_fifo fifo_in; | ||
| struct k_fifo fifo_out; | ||
| struct video_buffer *vbuf; | ||
| struct stream dma; |
There was a problem hiding this comment.
This change (moving struct stream dma from config to data) should at least be explained in the commit message (maybe even be its own commit since it's more than changing . to ->).
Otherwise LGTM.
mathieuchopstm
left a comment
mathieuchopstm
left a comment
There was a problem hiding this comment.
LGTM except for the remark of @gautierg-st
This change removes the on-stack copying and instead uses a pointer to the configuration to avoid unnecessary stack usage Signed-off-by: Mario Paja <mariopaja@hotmail.com>
mariopaja
force-pushed
the
st_remove_on_stack_dma_cfg
branch
from
007c79b to
5fca018
Compare
avolmat-st
left a comment
avolmat-st
left a comment
There was a problem hiding this comment.
Very minor point so I am also ok to approve it as is. Since dma pointer in introduced in the stm32_dma_init function, one of dma_cfg pointer can directly be retrieved via the dma pointer as done in all other places of that function.
drivers/video/video_stm32_dcmi.c
Outdated
| * how to route callbacks. | ||
| */ | ||
| struct dma_config dma_cfg = config->dma.cfg; | ||
| struct dma_config *dma_cfg = &data->dma.cfg; |
There was a problem hiding this comment.
| struct dma_config *dma_cfg = &data->dma.cfg; | |
| struct dma_config *dma_cfg = &dma->cfg; |
There was a problem hiding this comment.
This kind of change should be examined carefully, not saying it is wrong.
It's not just a matter of stack-usage, it's also a question of: Do we want the subsequent values assigned to the members of the stack-allocated struct to be setted globally, or just be passed to the functions called locally with the struct as an arg and then be forgotten?
Since the drivers in qst seem to be working fine without those values being setted globally (unless there are undiscovered bugs, or the values are setted elsewhere), I kind of question the need to set them in the global config/data struct.
This change moves the dma stream from _config to _data, allowing direct reference the DMA stream rather than duplicating it on the stack. Additionally, it aligns the declaration to the other STM32 drivers. Signed-off-by: Mario Paja <mariopaja@hotmail.com>
mariopaja
force-pushed
the
st_remove_on_stack_dma_cfg
branch
from
5fca018 to
8607c6b
Compare
|
As far as the DCMI is concerned, anyway this structure stream is already part of the global structures and was finally only partially used. With that done, the globally allocated area is used instead of the stack, releasing pressure on the stack. Moreover, I do not see to have those values (which were stored in the stack before) in the global structure stream pointer so I am fine with this modification. |
8 participants
This change removes the on-stack copying and instead uses a pointer to the configuration to avoid unnecessary stack usage