| Message ID | 1354066303-29762-5-git-send-email-troy.kisky@boundarydevices.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Dear Troy Kisky, In message <1354066303-29762-5-git-send-email-troy.kisky@boundarydevices.com> you wrote: > Need to move accesses to the static variables to > a function where struct data_src is used. Could you please elucidate why exactly this is _needed_? > + /* Be able to detect if the cfg file has no BOOT_FROM tag */ > + g_flash_offset = FLASH_OFFSET_UNDEFINED; > + memset(&ds, 0, sizeof(struct data_src)); Is this initialization really needed? Best regards, Wolfgang Denk
On 11/28/2012 2:38 AM, Wolfgang Denk wrote: > Dear Troy Kisky, > > In message <1354066303-29762-5-git-send-email-troy.kisky@boundarydevices.com> you wrote: >> Need to move accesses to the static variables to >> a function where struct data_src is used. > Could you please elucidate why exactly this is _needed_? My goal was to reduce the number of static variables, but strictly speaking it has little benefit other than giving me a warm fuzzy feeling. I'm not that only one that dislikes static though. > >> + /* Be able to detect if the cfg file has no BOOT_FROM tag */ >> + g_flash_offset = FLASH_OFFSET_UNDEFINED; >> + memset(&ds, 0, sizeof(struct data_src)); > Is this initialization really needed? > > Best regards, > > Wolfgang Denk > ds is on the stack, and even if not needed now, I like to avoid future random bugs. Troy
Dear Troy Kisky, In message <50B659AD.9090704@boundarydevices.com> you wrote: > > > Could you please elucidate why exactly this is _needed_? > > My goal was to reduce the number of static variables, but strictly speaking > it has little benefit other than giving me a warm fuzzy feeling. > > I'm not that only one that dislikes static though. ... > ds is on the stack, and even if not needed now, I like to avoid future > random bugs. Did you check the impact of your changes on the memory footprint? Changing code that uses a static variable initialized (implicitly or explicitly) to zero [which results in allocation of the BSS segment, i. e. zero space in the code or in the image file] into real code is something that is a bit of expensive just for satisfying random "dislikes". I think you should better leave that as is. The code is pretty efficent that way, and you increase it for little or no benefit. Best regards, Wolfgang Denk
diff --git a/tools/imximage.c b/tools/imximage.c index 97e5c4b..3a010a6 100644 --- a/tools/imximage.c +++ b/tools/imximage.c @@ -407,8 +407,11 @@ static void parse_cfg_fld(struct imx_header *imxhdr, int32_t *cmd, break; } } -static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name) + +static int parse_cfg_file(struct imx_header *imxhdr, char *name, + uint32_t entry_point) { + struct data_src ds; FILE *fd = NULL; char *line = NULL; char *token, *saveptr1, *saveptr2; @@ -418,6 +421,10 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name) int dcd_len = 0; int32_t cmd; + /* Be able to detect if the cfg file has no BOOT_FROM tag */ + g_flash_offset = FLASH_OFFSET_UNDEFINED; + memset(&ds, 0, sizeof(struct data_src)); + ds.imxhdr = imxhdr; /* * In order to not change the old imx cfg file * by adding VERSION command into it, here need @@ -465,10 +472,10 @@ static uint32_t parse_cfg_file(struct imx_header *imxhdr, char *name) fprintf(stderr, "Error: No BOOT_FROM tag in %s\n", name); exit(EXIT_FAILURE); } - return dcd_len; + /* Set the imx header */ + return (*set_imx_hdr)(imxhdr, dcd_len, entry_point, g_flash_offset); } - static int imximage_check_image_types(uint8_t type) { if (type == IH_TYPE_IMXIMAGE) @@ -510,21 +517,16 @@ int imximage_vrec_header(struct mkimage_params *params, struct image_type_params *tparams) { struct imx_header *imxhdr; - uint32_t dcd_len; imxhdr = calloc(1, MAX_HEADER_SIZE); if (!imxhdr) { fprintf(stderr, "Error: out of memory\n"); exit(EXIT_FAILURE); } - /* Be able to detect if the cfg file has no BOOT_FROM tag */ - g_flash_offset = FLASH_OFFSET_UNDEFINED; - /* Parse dcd configuration file */ - dcd_len = parse_cfg_file(imxhdr, params->imagename); - /* Set the imx header */ - imximage_params.header_size = (*set_imx_hdr)(imxhdr, dcd_len, - params->ep, g_flash_offset); + /* Parse dcd configuration file */ + imximage_params.header_size = parse_cfg_file(imxhdr, params->imagename, + params->ep); imximage_params.hdr = imxhdr; return 0; } diff --git a/tools/imximage.h b/tools/imximage.h index 0f39447..2895378 100644 --- a/tools/imximage.h +++ b/tools/imximage.h @@ -171,4 +171,7 @@ typedef void (*set_dcd_rst_t)(struct imx_header *imxhdr, typedef int (*set_imx_hdr_t)(struct imx_header *imxhdr, uint32_t dcd_len, uint32_t entry_point, uint32_t flash_offset); +struct data_src { + struct imx_header *imxhdr; +}; #endif /* _IMXIMAGE_H_ */
Need to move accesses to the static variables to a function where struct data_src is used. Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com> --- v3: new patch --- tools/imximage.c | 24 +++++++++++++----------- tools/imximage.h | 3 +++ 2 files changed, 16 insertions(+), 11 deletions(-)