lkml.org 
[lkml]   [2008]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2008-05-13 14:45    [W:0.256 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site