| Message ID | 20170929125238.26226-52-mario.six@gdsys.cc |
|---|---|
| State | Changes Requested |
| Delegated to: | Wolfgang Denk |
| Headers | show |
| Series | [U-Boot,001/080] mpc8308rdb: Fix style violation | expand |
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.
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
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);
Signed-off-by: Mario Six <mario.six@gdsys.cc> --- drivers/mtd/cfi_flash.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)