[U-Boot-Users] [PATCH] cfi_flash.c bug fix

[Grant Likely]

On 7/4/07, eran liberty <eran.liberty at gmail.com> wrote:
> On 7/3/07, Grant Likely <grant.likely at secretlab.ca> wrote:
> > On 7/3/07, eran.liberty at gmail.com <eran.liberty at gmail.com> wrote:
> > > --- drivers/cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000)   (revision 69)
> > > +++ drivers/cfi_flash.c (.../branches/exsw6000) (revision 69)
> > > @@ -1281,7 +1281,9 @@
> > >                         erase_region_count = (tmp & 0xffff) + 1;
> > >                         debug ("erase_region_count = %d erase_region_size = %d\n",
> > >                                 erase_region_count, erase_region_size);
> > > -                       for (j = 0; j < erase_region_count; j++) {
> > > +                       for (j = 0;
> > > +                            j < erase_region_count
> > > +                            && sect_cnt < CFG_MAX_FLASH_SECT; j++) {
> > >                                 info->start[sect_cnt] = sector;
> > >                                 sector += (erase_region_size * size_ratio);
> > >
> > Nit: rather than adding a test in the for loop, can you modify
> > erase_region_count before the loop to reflect the maximum size?  I
> > think it will result in cleaner code.
>
> If I had not missed anything,
> your suggestion tie together two variables which are not necessarily
> connected. True, your suggestion will prevent the j variable from
> letting the sect_cnt overflow in the , should be last, iteration.
> But i feel it is circumventive and less straight forward.
> The variable causing the spill over is sect_cnt, there for it should
> be the one that being tested against its own boundaries.

My 'nitpick' is about the for loop becoming longer than 80chars, and
hence being wrapped to 3 lines which makes it harder to read.  I'd
prefer code that keeps the for(;;) statement short for readability.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195