Re: [PATCH 1/1] [MTD/MAPS] Blackfin Async Flash Maps: Handle the case where flash memory and ethernet mac/phy are mapped onto the same async bank

[Mike Frysinger]

On Tue, May 13, 2008 at 4:07 AM, Jörn Engel <joern@xxxxxxxxx> wrote:
> On Tue, 13 May 2008 12:38:45 +0800, Bryan Wu wrote:
>  > +static void bfin_copy_from(struct map_info *map, void *to, unsigned long from, ssize_t len)
>  > +{
>  > +     size_t i;
>  > +     map_word test;
>  > +
>  > +     if ((unsigned long)to & 0x1) {
>  > +             for (i = 0; i < len; i += 2) {
>  > +                     u16 *dst = (u16 *)(to + i);
>  > +                     test = bfin_read(map, from + i);
>  > +                     put_unaligned(test.x[0], dst);
>  > +             }
>  > +     } else {
>  > +             for (i = 0; i < len; i += 2) {
>  > +                     u16 *dst = (u16 *)(to + i);
>  > +                     test = bfin_read(map, from + i);
>  > +                     *dst = test.x[0];
>  > +             }
>  > +     }
>  > +
>  > +     if (len & 0x1) {
>  > +             u8 *last_to_byte = (u8 *)(to + i);
>  > +             test = bfin_read(map, from + i);
>  > +             *last_to_byte = (u8)test.x[0];
>  > +     }
>  > +}
>
>  The pointer casts are superfluous.  Linus prefers variable declarations
>  up front (sorry for my bad example).  The "+ i" in the last conditional
>  is a bit dangerous as any changes to the loop can break it.  Linux code
>  has lots of churn and not everyone is careful enough to spot such
>  subtleties.
>  And I believe you can improve performance by killing the put_unaligned
>  in the loop.  So if we put it all together the end result should be
>  something like this:
>
>
>  static void bfin_copy_from(struct map_info *map, void *to, unsigned long from, ssize_t len)
>  {
>         size_t i;
>         map_word test;
>         u8 *byte;
>         u16 *dst;
>
>
>         if ((unsigned long)to & 0x1) {
>                 byte = to;
>                 test = bfin_read(map, from);
>                 *byte = test.x[0] >> 8;
>                 to++;
>                 from++;
>                 len--;
>
>         }
>
>         for (i = 0; i < len; i += 2) {
>                 dst = to + i;
>
>                 test = bfin_read(map, from + i);
>                 *dst = test.x[0];
>         }
>
>         if ((len & 0x1) {
>                 byte = to + len - 1;
>                 test = bfin_read(map, from + len - 1);
>                 *byte = test.x[0] & 0xff;
>         }
>  }
>
>  What do you think?

actually, i think it'll be simpler to just scrap the whole function:
static void bfin_copy_from(struct map_info *map, void *to, unsigned
long from, ssize_t len)
{
    struct async_state *state = (struct async_state *)map->map_priv_1;

    switch_to_flash(state);

    memcpy(to, map->virt + from, len);

    switch_back(state);
}
-mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/