Hi,
> >
> > This patch adds the configuration registers found in MCDE.
>
> > +
> > +#define MCDE_VAL2REG(__reg, __fld, __val) \
> > + (((__val) << __reg##_##__fld##_SHIFT) &
> __reg##_##__fld##_MASK)
> > +#define MCDE_REG2VAL(__reg, __fld, __val) \
> > + (((__val) & __reg##_##__fld##_MASK) >>
> __reg##_##__fld##_SHIFT)
> > +
> > +#define MCDE_CR 0x00000000
> > +#define MCDE_CR_DSICMD2_EN_V1_SHIFT 0
> > +#define MCDE_CR_DSICMD2_EN_V1_MASK 0x00000001
> > +#define MCDE_CR_DSICMD2_EN_V1(__x) \
> > + MCDE_VAL2REG(MCDE_CR, DSICMD2_EN_V1, __x)
> > +#define MCDE_CR_DSICMD1_EN_V1_SHIFT 1
> > +#define MCDE_CR_DSICMD1_EN_V1_MASK 0x00000002
> > +#define MCDE_CR_DSICMD1_EN_V1(__x) \
> > + MCDE_VAL2REG(MCDE_CR, DSICMD1_EN_V1, __x)
> > +#define MCDE_CR_DSI0_EN_V3_SHIFT 0
> > +#define MCDE_CR_DSI0_EN_V3_MASK 0x00000001
> > +#define MCDE_CR_DSI0_EN_V3(__x) \
> > + MCDE_VAL2REG(MCDE_CR, DSI0_EN_V3, __x)
>
> This looks all rather unreadable. The easiest way is usually to just
> define the bit mask, i.e. the second line of each register definition,
> which you can use to mask the bits. It's also useful to indent the
> lines
> so you can easily tell the register offsets apart from the contents:
>
> #define MCDE_CR 0x00000000
> #define MCDE_CR_DSICMD2_EN_V1 0x00000001
> #define MCDE_CR_DSICMD1_EN_V1 0x00000002
>
> Some people prefer to express all this in C instead of macros:
>
> struct mcde_registers {
> enum {
> mcde_cr_dsicmd2_en = 0x00000001,
> mcde_cr_dsicmd1_en = 0x00000002,
> ...
> } cr;
> enum {
> mcde_conf0_syncmux0 = 0x00000001,
> ...
> } conf0;
> ...
> };
>
> This gives you better type safety, but which one you choose is your
> decision.
>
> Arnd
All these header files,
Configuration, pixel processing, formatter, dsi link registers are auto generated from an xml file.
This is made because there are many registers which a prone to change for new hardware revisions and there is a possibility to exclude registers that are not used.
The chance of manual errors are less this way.
I also believe that these helper macros makes the source code easier to read.
For example.
#define MCDE_CR_DSICMD2_EN_V1_SHIFT 0
#define MCDE_CR_DSICMD2_EN_V1_MASK 0x00000001
#define MCDE_CR_DSICMD2_EN_V1(__x) \
MCDE_VAL2REG(MCDE_CR, DSICMD2_EN_V1, __x)
Writing a single field in the register MCDE_CR can e.g. be done like this:
mcde_wfld(MCDE_CR, DSICMD1_EN_V1, true);
and for a whole register (a totally other register but just to show):
mcde_wreg(MCDE_ROTACONF + chnl_id * MCDE_ROTACONF_GROUPOFFSET,
MCDE_ROTACONF_ROTBURSTSIZE_ENUM(8W) |
MCDE_ROTACONF_ROTBURSTSIZE_HW(1) |
MCDE_ROTACONF_ROTDIR(regs->rotdir) |
MCDE_ROTACONF_STRIP_WIDTH_ENUM(16PIX) |
MCDE_ROTACONF_RD_MAXOUT_ENUM(4_REQ) |
MCDE_ROTACONF_WR_MAXOUT_ENUM(8_REQ));
I agree that the header files looks a bit unreadable I will try indent as you suggested I am just worried about the file size. (Patch limit 100kbyte)
/Jimmy
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
