Messages in this thread | | | Date | Tue, 13 May 2008 08:42:08 -0400 | From | "Mike Frysinger" <> | Subject | 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 |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |