Re: [igt-dev] [PATCH i-g-t v11 2/3] lib/intel_mmio: add funtions for read/write register funtions

Chris Wilson
2019-08-29T08:06:41Z

Quoting Kempczynski, Zbigniew (2019-08-29 06:03:37)
> On Wed, 2019-08-28 at 20:12 +0200, Daniel Mrzyglod wrote:
> > This patch is first move to extend functionality of intel_mmio library.
> > There was limitation for 1 device, adding pointer for IO functions to
> > mmaped area gives us possibility to use those IO functions for other
> > mmaped
> > devices.
> >
> <cut>
> > -uint16_t INREG16(uint32_t reg);
> > -uint8_t INREG8(uint32_t reg);
> > -void OUTREG(uint32_t reg, uint32_t val);
> > -void OUTREG16(uint32_t reg, uint16_t val);
> > -void OUTREG8(uint32_t reg, uint8_t val);
> > +
> > +uint32_t INREG_DEV(void *mmio, uint32_t reg);
> > +uint16_t INREG16_DEV(void *mmio, uint32_t reg);
> > +uint8_t INREG8_DEV(void *mmio, uint32_t reg);
> > +void OUTREG_DEV(void *mmio, uint32_t reg, uint32_t val);
> > +void OUTREG16_DEV(void *mmio, uint32_t reg, uint16_t val);
> > +void OUTREG8_DEV(void *mmio, uint32_t reg, uint8_t val);
> > +
> > +#define INREG_PTR(n,...) INREG##n##_DEV(__VA_ARGS__)
> > +#define INREG_NOPTR(n,...) INREG##n##_DEV(igt_global_mmio, __VA_ARGS__)
> > +#define INREG_GET_MACRO(_1,_2,NAME,...) NAME
> > +#define INREGX(n,...) INREG_GET_MACRO(__VA_ARGS__, INREG_PTR,
> > INREG_NOPTR)(n, __VA_ARGS__)
> > +#define INREG16(...) INREGX(16, __VA_ARGS__)
> > +#define INREG8(...) INREGX(8, __VA_ARGS__)
> > +#define INREG(...) INREGX(,__VA_ARGS__)
> > +
> > +#define OUTREG_PTR(n,...) OUTREG##n##_DEV(__VA_ARGS__)
> > +#define OUTREG_NOPTR(n,...) OUTREG##n##_DEV(igt_global_mmio, __VA_ARGS__)
> > +#define OUTREG_GET_MACRO(_1,_2,_3,NAME,...) NAME
> > +#define OUTREGX(n, ...) OUTREG_GET_MACRO(__VA_ARGS__, OUTREG_PTR,
> > OUTREG_NOPTR)(n, __VA_ARGS__)
> > +#define OUTREG16(...) OUTREGX(16, __VA_ARGS__)
> > +#define OUTREG8(...) OUTREGX(8, __VA_ARGS__)
> > +#define OUTREG(...) OUTREGX(,__VA_ARGS__)
>
> You'll likely became my local macro God, but this is overkill IMO.
>
> For sake of readability I would:
> 1. make OUTREGX_DEV, INREGX_DEV (X = {,8,16}) inline functions
> 2. make OUTREGX, INREG (X = {,8,16}) macros with predefined igt_global_mmio
> as first argument to *_DEV() function.
>
> If someone needs to use different mmio than igt_global_mmio he
> should use *_DEV().
>
> Chris, could you speak up about above?

I would have finished the job by building the inlines with the macros...
-Chris