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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]

On 2012-11-28 12:41, Chandrabhanu Mahapatra wrote:
> 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 <cmahapatra@ti.com>
> ---
>  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;
>  };
>  
>  #define DISPC_MAX_NR_FIFOS 5
> @@ -1150,7 +1162,8 @@ static void dispc_init_fifos(void)
>  
>  	unit = dispc.feat->buffer_size_unit;
>  
> -	dss_feat_get_reg_field(FEAT_REG_FIFOSIZE, &start, &end);
> +	start = dispc.feat->reg_fields[FEAT_REG_FIFOSIZE].start;
> +	end = dispc.feat->reg_fields[FEAT_REG_FIFOSIZE].end;
>  
>  	for (fifo = 0; fifo < dispc.feat->num_fifos; ++fifo) {
>  		size = REG_GET(DISPC_OVL_FIFO_SIZE_STATUS(fifo), start, end);
> @@ -1214,8 +1227,10 @@ void dispc_ovl_set_fifo_threshold(enum omap_plane plane, u32 low, u32 high)
>  	low /= unit;
>  	high /= unit;
>  
> -	dss_feat_get_reg_field(FEAT_REG_FIFOHIGHTHRESHOLD, &hi_start, &hi_end);
> -	dss_feat_get_reg_field(FEAT_REG_FIFOLOWTHRESHOLD, &lo_start, &lo_end);
> +	hi_start = dispc.feat->reg_fields[FEAT_REG_FIFOHIGHTHRESHOLD].start;
> +	hi_end = dispc.feat->reg_fields[FEAT_REG_FIFOHIGHTHRESHOLD].end;
> +	lo_start = dispc.feat->reg_fields[FEAT_REG_FIFOLOWTHRESHOLD].start;
> +	lo_end = dispc.feat->reg_fields[FEAT_REG_FIFOLOWTHRESHOLD].end;

I think these are quite verbose. Perhaps a helper function which does
the same as dss_feat_get_reg_field() would be better. Or alternatively,
maybe something like:

const struct dss_reg_field *hi_field =
&dispc.feat->reg_fields[FEAT_REG_FIFOHIGHTHRESHOLD];

and then use "hi_field.start" instead of hi_start.

Or something else that makes the above easier to read.

> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 84a7f6a..aa273d8 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -143,6 +143,10 @@ struct reg_field {
>  	u8 low;
>  };
>  
> +struct register_field {
> +	u8 start, end;
> +};
> +

We already have the dss_reg_field struct. I think it's better to move
that to dss.h, and use it, instead of creating an exact duplicate.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]