Messages in this thread | | | Date | Thu, 21 Aug 2008 09:36:54 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 3/13 v2] viafb: accel.c, accel.h |
| |
On Thu, 21 Aug 2008 22:30:57 +0800 <JosephChan@via.com.tw> wrote:
> Hi Andrew, > > It's our carelessness to check with x86_64 version. And thanks for reminding this. > The > > > >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. > > >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? > > > >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.
| |