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

[Chandrabhanu Mahapatra <cmahapatra@ti.com>]

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 <cmahapatra@ti.com>
>> ---
>>  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.