Re: [PATCH 3/13 v2] viafb: accel.c, accel.h

On Thu, 21 Aug 2008 22:30:57 +0800 [off-list ref] wrote:

Hi Andrew,
 
It's our carelessness to check with x86_64 version. And thanks for reminding this.
The 
 
 

quoted

due to things like this:

       MMIO_OUT32(VIA_REG_DSTBASE,
                  ((u32) (info->screen_base) - (u32) viafb_FB_MM) >> 3);

now, we could just shut the warnings up by casting to (long) instead.

quoted

But I wonder what's going on in there.  ->screen_base came from
ioremap_nocache() and AFAIK there's no guarantee that this virtual
address will be less than 4G on 64-bit machines?

quoted

If the !VIA_MMIO code has no valid use then please just remove it.

The VIA_MMIO!=0 macros inplicitly reference a local variable which is
the kind of dopey programming trick which should not be performed in
new code.  It would be much better to do

static inline void via_writel(struct viafb_par *viaparinfo, int reg, u32 val)
{
       writel(val, viaparinfo->io_virt + reg);
}

and use that everywhere.

It would also be quite acceptable to simply open-code the

       writel(val, viaparinfo->io_virt + reg);

at all callsites.

Those macros may have made it easier to _write_ the code, but they make
it harder to _read_ it.  That's a wrong tradeoff.

Yes, the code seems to be cleaner after removing the macro. 
We define this macro for convenient debugging originally 
(MMIO couldn't work due to some wired reason, we can 
transfer to PORT easily by changing VIA_MMIO=0). 
It is a real tradeoff.

ok.

If necessary, we could modify this patch by following your
suggestions. 
Should we do that? or they're already done by you?

An incremental patch against the previous patches would be best, please.

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/