On Thursday 28 June 2012 04:20 PM, Tomi Valkeinen wrote: > On Thu, 2012-06-28 at 15:10 +0530, Chandrabhanu Mahapatra wrote: >> The current implementation of LCD channels and managers consists of a number >> of >> if-else construct which has been replaced by a simpler interface. A constant >> structure mgr_desc has been created in Display Controller (DISPC) module. The >> mgr_desc contains for each channel its name, irqs and is initialized one >> time >> with all registers and their corresponding fields to be written to enable >> various features of Display Subsystem. This structure is later used by >> various >> functions of DISPC which simplifies the further implementation of LCD >> channels >> and its corresponding managers. >> >> Signed-off-by: Chandrabhanu Mahapatra <[email protected]> >> --- >> drivers/video/omap2/dss/dispc.c | 233 >> +++++++++++++++++-------------------- >> drivers/video/omap2/dss/dsi.c | 6 +- >> drivers/video/omap2/dss/dss.h | 6 + >> drivers/video/omap2/dss/manager.c | 12 +-- >> 4 files changed, 121 insertions(+), 136 deletions(-) >> >> diff --git a/drivers/video/omap2/dss/dispc.c >> b/drivers/video/omap2/dss/dispc.c >> index 4749ac3..6c16b81 100644 >> --- a/drivers/video/omap2/dss/dispc.c >> +++ b/drivers/video/omap2/dss/dispc.c >> @@ -57,6 +57,8 @@ >> >> #define DISPC_MAX_NR_ISRS 8 >> >> +#define DISPC_MGR_FLD_MAX 9 > You could have this in the enum mgr_ref_fields, as a last entry. Then > it'll automatically have the value 9, and will adjust automatically when > we add new fields. And actually "MAX" is not quite right. MAX would be > 8, as that's the maximum value for the vields. "NUM" is perhaps more > correct. Its really a clever idea to have it as the last field of enum mgr_ref_fields. To make it distinct from other fields I can add a comment on top of it saying its for count of above fields or I am fine with names as DISPC_MGR_FLD_COUNT / NUM. > >> + >> struct omap_dispc_isr_data { >> omap_dispc_isr_t isr; >> void *arg; >> @@ -119,6 +121,78 @@ enum omap_color_component { >> DISPC_COLOR_COMPONENT_UV = 1 << 1, >> }; >> >> +enum mgr_reg_fields { >> + DISPC_MGR_FLD_ENABLE, >> + DISPC_MGR_FLD_STNTFT, >> + DISPC_MGR_FLD_GO, >> + DISPC_MGR_FLD_TFTDATALINES, >> + DISPC_MGR_FLD_STALLMODE, >> + DISPC_MGR_FLD_TCKENABLE, >> + DISPC_MGR_FLD_TCKSELECTION, >> + DISPC_MGR_FLD_CPR, >> + DISPC_MGR_FLD_FIFOHANDCHECK, >> +}; >> + >> +static const struct { >> + const char *name; >> + u32 vsync_irq; >> + u32 framedone_irq; >> + u32 sync_lost_irq; >> + struct reg_field reg_desc[DISPC_MGR_FLD_MAX]; >> +} mgr_desc[] = { >> + [OMAP_DSS_CHANNEL_LCD] = { >> + .name = "LCD", >> + .vsync_irq = DISPC_IRQ_VSYNC, >> + .framedone_irq = DISPC_IRQ_FRAMEDONE, >> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST, >> + .reg_desc = { >> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 0, >> 0 }, >> + [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL, 3, >> 3 }, >> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 5, >> 5 }, >> + [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL, 9, >> 8 }, >> + [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL, 11, >> 11 }, >> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 10, >> 10 }, >> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 11, >> 11 }, >> + [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG, 15, >> 15 }, >> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, >> 16 }, >> + }, >> + }, >> + [OMAP_DSS_CHANNEL_DIGIT] = { >> + .name = "DIGIT", >> + .vsync_irq = DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN, >> + .framedone_irq = DISPC_IRQ_FRAMEDONETV, > There's a problem with this one. FRAMEDONETV is a new thing, appeared in > omap4. So for this we need a system to select the data depending on the > DSS version. > > I suggest you remove the framedone_irq entry for now, and keep the old > code to handle the framedone irq. Let's add it later when we can handle > the differences between omap versions. > > Or actually, looking at the code, perhaps you can keep the framedone_irq > field, but set it to 0 for DIGIT mgr. This would keep the functionality > the same as it is now, because there's only one place in dispc.c where > we use FRAMEDONETV, and your patch doesn't touch it. In other places we > presume that TV out does not have FRAMEDONE interrupt (i.e. the irq > number is 0). The purpose of FRAMEDONETV IRQ as seen from dispc_mgr_enable_digit_out() looks as to interrupt when active frame related to HDMI is done and so DISPC is disabled. I think I misinterpreted and used it here. Can please explain the exact purpose of DISPC_IRQ_FRAMEDONETV? >> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST_DIGIT, >> + .reg_desc = { >> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 1, >> 1 }, >> + [DISPC_MGR_FLD_STNTFT] = { }, >> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 6, >> 6 }, >> + [DISPC_MGR_FLD_TFTDATALINES] = { }, >> + [DISPC_MGR_FLD_STALLMODE] = { }, >> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 12, >> 12 }, >> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 13, >> 13 }, >> + [DISPC_MGR_FLD_CPR] = { }, >> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, >> 16 }, >> + }, >> + }, >> + [OMAP_DSS_CHANNEL_LCD2] = { >> + .name = "LCD2", >> + .vsync_irq = DISPC_IRQ_VSYNC2, >> + .framedone_irq = DISPC_IRQ_FRAMEDONE2, >> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST2, >> + .reg_desc = { >> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL2, 0, >> 0 }, >> + [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL2, 3, >> 3 }, >> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL2, 5, >> 5 }, >> + [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL2, 9, >> 8 }, >> + [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL2, 11, >> 11 }, >> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG2, 10, >> 10 }, >> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG2, 11, >> 11 }, >> + [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG2, 15, >> 15 }, >> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG2, 16, >> 16 }, >> + }, >> + }, >> +}; >> + >> static void _omap_dispc_set_irqs(void); >> >> static inline void dispc_write_reg(const u16 idx, u32 val) >> @@ -131,6 +205,19 @@ static inline u32 dispc_read_reg(const u16 idx) >> return __raw_readl(dispc.base + idx); >> } >> >> +static u32 mgr_fld_read(enum omap_channel channel, enum mgr_reg_fields >> regfld) >> +{ >> + const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld]; >> + return FLD_GET(dispc_read_reg(rfld.reg), rfld.high, rfld.low); > This could use REG_GET(), instead of FLD_GET(dispc_read_reg(...)) ok. > >> +} >> + >> +static void mgr_fld_write(enum omap_channel channel, >> + enum mgr_reg_fields regfld, int val) { >> + const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld]; >> + dispc_write_reg(rfld.reg, FLD_MOD(dispc_read_reg(rfld.reg), val, >> + rfld.high, rfld.low)); >> +} > And this one could use REG_FLD_MOD(). > > Otherwise, looks good. > > Tomi > ok.
-- Chandrabhanu Mahapatra Texas Instruments India Pvt. Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
