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 wrote:
> On Tue, 13 May 2008 12:38:45 +0800, Bryan Wu wrote:
>  >
>  > +#define pr_devinit(fmt, args...) ({ static const __devinitdata char __fmt[] = fmt; printk(__fmt, ## args); })
>
>  Is this still needed?

yes

>  > +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.

left over due to "from" being unsigned long ... forgot that "to" is a
void *.  dropped

>  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?

seems reasonable ... i'll have to grab the board again and test it out.

>  > +static void bfin_write(struct map_info *map, map_word d1, unsigned long ofs)
>  > +{
>  > +     struct async_state *state = (struct async_state *)map->map_priv_1;
>  > +     u16 d;
>  > +
>  > +     d = (u16)d1.x[0];
>
>  You didn't need the cast above.  Why here?

*shrug* it's always been there.  i'll drop it.

thanks!
-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/