Re: [U-Boot] [PATCH 052/080] cfi_flash: Reduce the scope of some variables

Hi Masahiro,

On Fri, Sep 29, 2017 at 7:10 PM, Masahiro Yamada yamada.masahiro@socionext.com wrote:

2017-09-29 21:52 GMT+09:00 Mario Six mario.six@gdsys.cc:

...

Signed-off-by: Mario Six mario.six@gdsys.cc

drivers/mtd/cfi_flash.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index c47ba416b9..c597c621ca 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -179,10 +179,10 @@ __maybe_weak u64 flash_read64(void *addr) static flash_info_t *flash_get_info(ulong base) { int i;

•   flash_info_t *info;
  
    for (i = 0; i < CONFIG_SYS_MAX_FLASH_BANKS; i++) {
  

•           info = &flash_info[i];
  

•           flash_info_t *info = &flash_info[i];
  

•           if (info->size && info->start[0] <= base &&
                base <= info->start[0] + info->size - 1)
                    return info;
  

@@ -222,8 +222,6 @@ static inline void flash_unmap(flash_info_t *info, flash_sect_t sect, static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf) { int i;

•   int cword_offset;
  

•   int cp_offset;
  

#if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA) u32 cmd_le = cpu_to_le32(cmd); #endif @@ -231,9 +229,10 @@ static void flash_make_cmd(flash_info_t *info, u32 cmd, void *cmdbuf) uchar *cp = (uchar *) cmdbuf;

    for (i = info->portwidth; i > 0; i--) {

•           cword_offset = (info->portwidth - i) % info->chipwidth;
  

•           int cword_offset = (info->portwidth - i) % info->chipwidth;
  

•           int cp_offset = info->portwidth - i;
  

#if defined(__LITTLE_ENDIAN) || defined(CONFIG_SYS_WRITE_SWAPPED_DATA)

•           cp_offset = info->portwidth - i;
  

•           val = *((uchar *)&cmd_le + cword_offset);
  

#else cp_offset = i - 1; @@ -2053,16 +2052,10 @@ static void flash_fixup_num(flash_info_t *info, struct cfi_qry *qry) ulong flash_get_size(phys_addr_t base, int banknum) { flash_info_t *info = &flash_info[banknum];

•   int i, j;
    flash_sect_t sect_cnt;
    phys_addr_t sector;
  

•   unsigned long tmp;
  

•   int size_ratio;
    uchar num_erase_regions;
  

•   int erase_region_size;
  

•   int erase_region_count;
    struct cfi_qry qry;
  

•   unsigned long max_size;
  
    memset(&qry, 0, sizeof(qry));
  
  

@@ -2075,6 +2068,11 @@ ulong flash_get_size(phys_addr_t base, int banknum) info->start[0] = (ulong)map_physmem(base, info->portwidth, MAP_NOCACHE);

    if (flash_detect_cfi(info, &qry)) {

•           int i;
  

•           int size_ratio;
  

•           unsigned long max_size;
  

•           unsigned long tmp;
  

•           info->vendor = le16_to_cpu(get_unaligned(&qry.p_id));
            info->ext_addr = le16_to_cpu(get_unaligned(&qry.p_adr));
            num_erase_regions = qry.num_erase_regions;
  

@@ -2159,6 +2157,10 @@ ulong flash_get_size(phys_addr_t base, int banknum) sect_cnt = 0; sector = base; for (i = 0; i < num_erase_regions; i++) {

•                   int j;
  

•                   int erase_region_size;
  

•                   int erase_region_count;
  

•                   if (i > NUM_ERASE_REGIONS) {
                            printf("%d erase regions found, only %d used\n",
                                    num_erase_regions, NUM_ERASE_REGIONS);
  

--

Are these changes necessary?

Looks like your personal preference to me.

Well, scope reduction of variables is a relatively common refactoring method (see, for example, chapter 10.4 of Code Complete, Second Edition). Also, the relatively commonly used CppCheck linter automatically finds these instances very often, which is how I found them.

But no, it is not strictly necessary; I can drop these patches for v2.

Best regards,

Mario