Re: [PATCH v4 1/2] r8169: Dereference MMIO address immediately before use

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, 2018-03-01 at 21:01 +0100, Heiner Kallweit wrote:
> Am 01.03.2018 um 12:27 schrieb Andy Shevchenko:

> > Next step might be a conversion of RTL_Wxx() / RTL_Rxx() macros
> > to inline functions for sake of type checking.

^^^^^

> >  /* write/read MMIO register */
> > -#define RTL_W8(reg, val8)	writeb ((val8), ioaddr + (reg))
> > -#define RTL_W16(reg, val16)	writew ((val16), ioaddr + (reg))
> > -#define RTL_W32(reg, val32)	writel ((val32), ioaddr + (reg))
> > -#define RTL_R8(reg)		readb (ioaddr + (reg))
> > -#define RTL_R16(reg)		readw (ioaddr + (reg))
> > -#define RTL_R32(reg)		readl (ioaddr + (reg))
> > +#define RTL_W8(tp, reg, val8)	writeb((val8), tp->mmio_addr +
> > (reg))
> > +#define RTL_W16(tp, reg, val16)	writew((val16), tp-
> > >mmio_addr + (reg))
> > +#define RTL_W32(tp, reg, val32)	writel((val32), tp-
> > >mmio_addr + (reg))
> > +#define RTL_R8(tp, reg)		readb(tp->mmio_addr + (reg))
> > +#define RTL_R16(tp, reg)		readw(tp->mmio_addr +
> > (reg))
> > +#define RTL_R32(tp, reg)		readl(tp->mmio_addr +
> > (reg))
 
> I like te idea because I also found the frequent definition of
> variable ioaddr
> w/o explicit use quite ugly. Just instead of using macro's we could
> define
> RTL_R32 et al as inline functions.

I left a last paragraph in my commit message. I wouldn't really to do a
functional change right now. It may be easily done as simple followup,
plain to test, etc.

> In parallel to the readl API there's the ioread32() API. Even though I
> read
> somewhere that the readl API is a legacy API, I don't have an idea
> which one
> to prefer and why.

You don't need ioread*().
Or do you have an example of hardware that uses I/O instead of MMIO?

Rather some _relaxed variants might be useful.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy