Re: [PATCH 1/4] OMAPDSS: Cleanup implementation of LCD channels

On Thursday 28 June 2012 04:20 PM, Tomi Valkeinen wrote:

On Thu, 2012-06-28 at 15:10 +0530, Chandrabhanu Mahapatra wrote:

quoted

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 <redacted>
---
 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.

quoted

+
 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?

quoted

+		.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.

quoted

+}
+
+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.