Re: [PATCH 2/7] OMAPDSS: DISPC: Move DISPC specific dss_reg_fields to dispc_features

On 2012-12-03 08:29, Chandrabhanu Mahapatra wrote:

On Thursday 29 November 2012 05:48 PM, Tomi Valkeinen wrote:

quoted

On 2012-11-28 12:41, Chandrabhanu Mahapatra wrote:

quoted

The register fields in dss_reg_fields specific to DISPC are moved from struct
omap_dss_features to corresponding dispc_reg_fields, initialized in struct
dispc_features, thereby enabling local access.

Signed-off-by: Chandrabhanu Mahapatra <redacted>
---
 drivers/video/omap2/dss/dispc.c        |   87 ++++++++++++++++++++++++++++----
 drivers/video/omap2/dss/dss.h          |    4 ++
 drivers/video/omap2/dss/dss_features.c |   28 ----------
 drivers/video/omap2/dss/dss_features.h |    7 ---
 4 files changed, 80 insertions(+), 46 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 9f259ba..21fc522 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c

@@ -80,6 +80,16 @@ struct dispc_irq_stats {
 	unsigned irqs[32];
 };
 
+enum dispc_feat_reg_field {
+	FEAT_REG_FIRHINC,
+	FEAT_REG_FIRVINC,
+	FEAT_REG_FIFOLOWTHRESHOLD,
+	FEAT_REG_FIFOHIGHTHRESHOLD,
+	FEAT_REG_FIFOSIZE,
+	FEAT_REG_HORIZONTALACCU,
+	FEAT_REG_VERTICALACCU,
+};
+
 struct dispc_features {
 	u8 sw_start;
 	u8 fp_start;

@@ -107,6 +117,8 @@ struct dispc_features {
 
 	u32 buffer_size_unit;
 	u32 burst_size_unit;
+
+	struct register_field *reg_fields;
 };

Hmm, would it be simpler to have an explicit struct for the reg fields.
I mean something like:

struct dispc_reg_fields {
	struct register_field firhinc;
	struct register_field firvinc;
	struct register_field fifo_low_threshold;
	...
};

Then accessing it would be

dispc.feat->reg_fields.firhinc.start;

instead of

dispc.feat->reg_fields[FEAT_REG_FIFOSIZE].start;

Not a big difference, but I don't see any benefit in having an array of
reg fields here.

 Tomi

I was thinking to move reg_fields into the dispc_feats structure as

	.burst_size_unit	=	8,
	.reg_fields		=	{
		.firhinc			= { 12,  0 },
		.firvinc			= { 28, 16 },
		.fifo_low_thresh		= { 11,  0 },
		.fifo_high_thresh		= { 27, 16 },
		.fifosize			= { 10,  0 },
		.hori_accu			= {  9,  0 },
		.vert_accu			= { 25, 16 },
	},

This would give us dispc.feat->reg_fields.firhinc.start;
but at the same time would create duplicate information for
omap34xx_rev1_0_dispc_feats and omap34xx_rev3_0_dispc_feats. However,
this duplication never occurs anywhere else in dss.c or dsi.c.

If we still go with the older approach of having dispc_reg_fields
outside dispc_feats the only way  it works is

.reg_fields		=	&omap2_dispc_reg_fields

which changes as dispc.feat->reg_fields->firhinc.start;

but avoids duplicate information. Both approaches seem good enough to me.

I would keep the pointer approach. We may add support for new DSS
versions, for AM3xxx or AM4xxx SoCs, which possibly may have the same
register fields as some other DSS versions.

 Tomi