Re: [PATCH 3/4] OMAPDSS: HDMI: change the timing match logic
Hi Tomi,
On Fri, Nov 18, 2011 at 12:46 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Wed, 2011-11-16 at 11:01 +0530, K, Mythri P wrote:
On Mon, Nov 14, 2011 at 1:03 PM, Tomi Valkeinen tomi.valkei...@ti.com
wrote:
On Fri, 2011-11-11 at 18:39 +0530, mythr...@ti.com wrote:
-static int get_timings_index(void)
+static bool hdmi_find_code(const struct hdmi_config *timings_arr,
+ int len, struct hdmi_config *timing)
{
- int code;
+ int i;
- if (hdmi.mode == 0)
- code = code_vesa[hdmi.code];
- else
- code = code_cea[hdmi.code];
+ for (i = 0; i len; i++) {
+ if (timings_arr[i].cm.code == hdmi.code) {
+ *timing = timings_arr[i];
+ return true;
+ }
+ }
+
+ return false;
+}
You could return the hdmi_config pointer instead of making a copy and
returning a bool.
In this function i'm passing the timing value and finally there needs
to be one copy whether it is in this function or after the return,
because the timings array is a const and dssdev-paneltimings and
config timings are not, so do you see any benefit of doing that later
or suggest any other method?
Well, I think it's just good design, even if it wouldn't help in this
particular case.
hdmi_find_code is a small utility function, and functions like that
should be designed to be usable in any situation. In this particular
situation you will anyway make a copy, and it doesn't matter if it's the
utility function that does the copy.
But in some other case you could perhaps be interested in only one value
in the hdmi_config that is found. In that case doing a copy is extra,
and it'd be better to return the const struct pointer.
Also, it is a standard design pattern that a find function returns
pointer to the found item, whereas your version returning a bool and
making a copy of the found item is not very standard.
Well i shall update the change to the standard way then :-).
Thanks and regards,
Mythri.
Tomi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] OMAPDSS: HDMI: change the timing match logic
On Wed, 2011-11-16 at 11:01 +0530, K, Mythri P wrote:
On Mon, Nov 14, 2011 at 1:03 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Fri, 2011-11-11 at 18:39 +0530, mythr...@ti.com wrote:
-static int get_timings_index(void)
+static bool hdmi_find_code(const struct hdmi_config *timings_arr,
+ int len, struct hdmi_config *timing)
{
- int code;
+ int i;
- if (hdmi.mode == 0)
- code = code_vesa[hdmi.code];
- else
- code = code_cea[hdmi.code];
+ for (i = 0; i len; i++) {
+ if (timings_arr[i].cm.code == hdmi.code) {
+ *timing = timings_arr[i];
+ return true;
+ }
+ }
+
+ return false;
+}
You could return the hdmi_config pointer instead of making a copy and
returning a bool.
In this function i'm passing the timing value and finally there needs
to be one copy whether it is in this function or after the return,
because the timings array is a const and dssdev-paneltimings and
config timings are not, so do you see any benefit of doing that later
or suggest any other method?
Well, I think it's just good design, even if it wouldn't help in this
particular case.
hdmi_find_code is a small utility function, and functions like that
should be designed to be usable in any situation. In this particular
situation you will anyway make a copy, and it doesn't matter if it's the
utility function that does the copy.
But in some other case you could perhaps be interested in only one value
in the hdmi_config that is found. In that case doing a copy is extra,
and it'd be better to return the const struct pointer.
Also, it is a standard design pattern that a find function returns
pointer to the found item, whereas your version returning a bool and
making a copy of the found item is not very standard.
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 3/4] OMAPDSS: HDMI: change the timing match logic
On Mon, Nov 14, 2011 at 1:03 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Fri, 2011-11-11 at 18:39 +0530, mythr...@ti.com wrote:
From: Mythri P K mythr...@ti.com
Change the timing match logic, Instead of the statically mapped method
to get the corresponding timings for a given code and mode, move to a
simpler array indexed method. It will help to scale up to add more
timings when needed.
Signed-off-by: Mythri P K mythr...@ti.com
---
drivers/video/omap2/dss/hdmi.c | 162
---
1 files changed, 67 insertions(+), 95 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index f76ae47..b859350 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -58,8 +58,6 @@
#define EDID_SIZE_BLOCK0_TIMING_DESCRIPTOR 4
#define EDID_SIZE_BLOCK1_TIMING_DESCRIPTOR 4
-#define OMAP_HDMI_TIMINGS_NB 34
-
#define HDMI_DEFAULT_REGN 16
#define HDMI_DEFAULT_REGM2 1
@@ -88,7 +86,7 @@ static struct {
* map it to corresponding CEA or VESA index.
*/
-static const struct hdmi_config cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
+static const struct hdmi_config cea_timings[] = {
{ {640, 480, 25200, 96, 16, 48, 2, 10, 33, 0, 0, 0}, {1, HDMI_HDMI} },
{ {720, 480, 27027, 62, 16, 60, 6, 9, 30, 0, 0, 0}, {2, HDMI_HDMI} },
{ {1280, 720, 74250, 40, 110, 220, 5, 5, 20, 1, 1, 0}, {4, HDMI_HDMI} },
@@ -104,6 +102,8 @@ static const struct hdmi_config
cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
{ {1920, 1080, 74250, 44, 638, 148, 5, 4, 36, 1, 1, 0}, {32, HDMI_HDMI} },
{ {2880, 480, 108108, 248, 64, 240, 6, 9, 30, 0, 0, 0}, {35, HDMI_HDMI} },
{ {2880, 576, 108000, 256, 48, 272, 5, 5, 39, 0, 0, 0}, {37, HDMI_HDMI} },
+};
+static const struct hdmi_config vesa_timings[] = {
/* VESA From Here */
{ {640, 480, 25175, 96, 16, 48, 2 , 11, 31, 0, 0, 0}, {4, HDMI_DVI} },
{ {800, 600, 4, 128, 40, 88, 4 , 1, 23, 1, 1, 0}, {9, HDMI_DVI} },
@@ -126,39 +126,6 @@ static const struct hdmi_config
cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
{ {1280, 720, 74250, 40, 110, 220, 5, 5, 20, 1, 1, 0}, {0x55, HDMI_DVI} }
};
-/*
- * This is a static mapping array which maps the timing values
- * with corresponding CEA / VESA code
- */
-static const int code_index[OMAP_HDMI_TIMINGS_NB] = {
- 1, 19, 4, 2, 37, 6, 21, 20, 5, 16, 17, 29, 31, 35, 32,
- /* --15 CEA 17-- vesa*/
- 4, 9, 0xE, 0x17, 0x1C, 0x27, 0x20, 0x23, 0x10, 0x2A,
- 0X2F, 0x3A, 0X51, 0X52, 0x16, 0x29, 0x39, 0x1B
-};
-
-/*
- * This is reverse static mapping which maps the CEA / VESA code
- * to the corresponding timing values
- */
-static const int code_cea[39] = {
- -1, 0, 3, 3, 2, 8, 5, 5, -1, -1,
- -1, -1, -1, -1, -1, -1, 9, 10, 10, 1,
- 7, 6, 6, -1, -1, -1, -1, -1, -1, 11,
- 11, 12, 14, -1, -1, 13, 13, 4, 4
-};
-
-static const int code_vesa[85] = {
- -1, -1, -1, -1, 15, -1, -1, -1, -1, 16,
- -1, -1, -1, -1, 17, -1, 23, -1, -1, -1,
- -1, -1, 29, 18, -1, -1, -1, 32, 19, -1,
- -1, -1, 21, -1, -1, 22, -1, -1, -1, 20,
- -1, 30, 24, -1, -1, -1, -1, 25, -1, -1,
- -1, -1, -1, -1, -1, -1, -1, 31, 26, -1,
- -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
- -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
- -1, 27, 28, -1, 33};
-
static int hdmi_runtime_get(void)
{
int r;
@@ -188,82 +155,88 @@ int hdmi_init_display(struct omap_dss_device *dssdev)
return 0;
}
-static int get_timings_index(void)
+static bool hdmi_find_code(const struct hdmi_config *timings_arr,
+ int len, struct hdmi_config *timing)
{
- int code;
+ int i;
- if (hdmi.mode == 0)
- code = code_vesa[hdmi.code];
- else
- code = code_cea[hdmi.code];
+ for (i = 0; i len; i++) {
+ if (timings_arr[i].cm.code == hdmi.code) {
+ *timing = timings_arr[i];
+ return true;
+ }
+ }
+
+ return false;
+}
You could return the hdmi_config pointer instead of making a copy and
returning a bool.
In this function i'm passing the timing value and finally there needs
to be one copy whether it is in this function or after the return,
because the timings array is a const and dssdev-paneltimings and
config timings are not, so do you see any benefit of doing that later
or suggest any other method?
You shouldn't use hdmi.code in this function, but get the code as a
parameter.
- if (code == -1) {
+static void hdmi_get_timings(struct hdmi_config *timing)
+{
+ int r;
+
+ if (hdmi.mode == 0) {
+ r = hdmi_find_code(vesa_timings,
+ ARRAY_SIZE(vesa_timings), timing);
+ } else {
+ r = hdmi_find_code(cea_timings,
+ ARRAY_SIZE(cea_timings), timing);
+ }
+ if (!r) {
/* HDMI code 4
Re: [PATCH 3/4] OMAPDSS: HDMI: change the timing match logic
On Fri, 2011-11-11 at 18:39 +0530, mythr...@ti.com wrote:
From: Mythri P K mythr...@ti.com
Change the timing match logic, Instead of the statically mapped method
to get the corresponding timings for a given code and mode, move to a
simpler array indexed method. It will help to scale up to add more
timings when needed.
Signed-off-by: Mythri P K mythr...@ti.com
---
drivers/video/omap2/dss/hdmi.c | 162 ---
1 files changed, 67 insertions(+), 95 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index f76ae47..b859350 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -58,8 +58,6 @@
#define EDID_SIZE_BLOCK0_TIMING_DESCRIPTOR 4
#define EDID_SIZE_BLOCK1_TIMING_DESCRIPTOR 4
-#define OMAP_HDMI_TIMINGS_NB 34
-
#define HDMI_DEFAULT_REGN 16
#define HDMI_DEFAULT_REGM2 1
@@ -88,7 +86,7 @@ static struct {
* map it to corresponding CEA or VESA index.
*/
-static const struct hdmi_config cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
+static const struct hdmi_config cea_timings[] = {
{ {640, 480, 25200, 96, 16, 48, 2, 10, 33, 0, 0, 0}, {1, HDMI_HDMI} },
{ {720, 480, 27027, 62, 16, 60, 6, 9, 30, 0, 0, 0}, {2, HDMI_HDMI} },
{ {1280, 720, 74250, 40, 110, 220, 5, 5, 20, 1, 1, 0}, {4, HDMI_HDMI} },
@@ -104,6 +102,8 @@ static const struct hdmi_config
cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
{ {1920, 1080, 74250, 44, 638, 148, 5, 4, 36, 1, 1, 0}, {32, HDMI_HDMI} },
{ {2880, 480, 108108, 248, 64, 240, 6, 9, 30, 0, 0, 0}, {35, HDMI_HDMI} },
{ {2880, 576, 108000, 256, 48, 272, 5, 5, 39, 0, 0, 0}, {37, HDMI_HDMI} },
+};
+static const struct hdmi_config vesa_timings[] = {
/* VESA From Here */
{ {640, 480, 25175, 96, 16, 48, 2 , 11, 31, 0, 0, 0}, {4, HDMI_DVI} },
{ {800, 600, 4, 128, 40, 88, 4 , 1, 23, 1, 1, 0}, {9, HDMI_DVI} },
@@ -126,39 +126,6 @@ static const struct hdmi_config
cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
{ {1280, 720, 74250, 40, 110, 220, 5, 5, 20, 1, 1, 0}, {0x55, HDMI_DVI} }
};
-/*
- * This is a static mapping array which maps the timing values
- * with corresponding CEA / VESA code
- */
-static const int code_index[OMAP_HDMI_TIMINGS_NB] = {
- 1, 19, 4, 2, 37, 6, 21, 20, 5, 16, 17, 29, 31, 35, 32,
- /* --15 CEA 17-- vesa*/
- 4, 9, 0xE, 0x17, 0x1C, 0x27, 0x20, 0x23, 0x10, 0x2A,
- 0X2F, 0x3A, 0X51, 0X52, 0x16, 0x29, 0x39, 0x1B
-};
-
-/*
- * This is reverse static mapping which maps the CEA / VESA code
- * to the corresponding timing values
- */
-static const int code_cea[39] = {
- -1, 0, 3, 3, 2, 8, 5, 5, -1, -1,
- -1, -1, -1, -1, -1, -1, 9, 10, 10, 1,
- 7, 6, 6, -1, -1, -1, -1, -1, -1, 11,
- 11, 12, 14, -1, -1, 13, 13, 4, 4
-};
-
-static const int code_vesa[85] = {
- -1, -1, -1, -1, 15, -1, -1, -1, -1, 16,
- -1, -1, -1, -1, 17, -1, 23, -1, -1, -1,
- -1, -1, 29, 18, -1, -1, -1, 32, 19, -1,
- -1, -1, 21, -1, -1, 22, -1, -1, -1, 20,
- -1, 30, 24, -1, -1, -1, -1, 25, -1, -1,
- -1, -1, -1, -1, -1, -1, -1, 31, 26, -1,
- -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
- -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
- -1, 27, 28, -1, 33};
-
static int hdmi_runtime_get(void)
{
int r;
@@ -188,82 +155,88 @@ int hdmi_init_display(struct omap_dss_device *dssdev)
return 0;
}
-static int get_timings_index(void)
+static bool hdmi_find_code(const struct hdmi_config *timings_arr,
+ int len, struct hdmi_config *timing)
{
- int code;
+ int i;
- if (hdmi.mode == 0)
- code = code_vesa[hdmi.code];
- else
- code = code_cea[hdmi.code];
+ for (i = 0; i len; i++) {
+ if (timings_arr[i].cm.code == hdmi.code) {
+ *timing = timings_arr[i];
+ return true;
+ }
+ }
+
+ return false;
+}
You could return the hdmi_config pointer instead of making a copy and
returning a bool.
You shouldn't use hdmi.code in this function, but get the code as a
parameter.
- if (code == -1) {
+static void hdmi_get_timings(struct hdmi_config *timing)
+{
+ int r;
+
+ if (hdmi.mode == 0) {
+ r = hdmi_find_code(vesa_timings,
+ ARRAY_SIZE(vesa_timings), timing);
+ } else {
+ r = hdmi_find_code(cea_timings,
+ ARRAY_SIZE(cea_timings), timing);
+ }
+ if (!r) {
/* HDMI code 4 corresponds to 640 * 480 VGA */
hdmi.code = 4;
/* DVI mode 1 corresponds to HDMI 0 to DVI */
hdmi.mode = HDMI_DVI;
+ *timing = vesa_timings[0];
+ }
+}
Same thing here, you could just return the pointer to hdmi_config.
And also for this function you shouldn't use hdmi.mode and hdmi.code,
but they should
[PATCH 3/4] OMAPDSS: HDMI: change the timing match logic
From: Mythri P K mythr...@ti.com
Change the timing match logic, Instead of the statically mapped method
to get the corresponding timings for a given code and mode, move to a
simpler array indexed method. It will help to scale up to add more
timings when needed.
Signed-off-by: Mythri P K mythr...@ti.com
---
drivers/video/omap2/dss/hdmi.c | 162 ---
1 files changed, 67 insertions(+), 95 deletions(-)
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index f76ae47..b859350 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -58,8 +58,6 @@
#define EDID_SIZE_BLOCK0_TIMING_DESCRIPTOR 4
#define EDID_SIZE_BLOCK1_TIMING_DESCRIPTOR 4
-#define OMAP_HDMI_TIMINGS_NB 34
-
#define HDMI_DEFAULT_REGN 16
#define HDMI_DEFAULT_REGM2 1
@@ -88,7 +86,7 @@ static struct {
* map it to corresponding CEA or VESA index.
*/
-static const struct hdmi_config cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
+static const struct hdmi_config cea_timings[] = {
{ {640, 480, 25200, 96, 16, 48, 2, 10, 33, 0, 0, 0}, {1, HDMI_HDMI} },
{ {720, 480, 27027, 62, 16, 60, 6, 9, 30, 0, 0, 0}, {2, HDMI_HDMI} },
{ {1280, 720, 74250, 40, 110, 220, 5, 5, 20, 1, 1, 0}, {4, HDMI_HDMI} },
@@ -104,6 +102,8 @@ static const struct hdmi_config
cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
{ {1920, 1080, 74250, 44, 638, 148, 5, 4, 36, 1, 1, 0}, {32, HDMI_HDMI} },
{ {2880, 480, 108108, 248, 64, 240, 6, 9, 30, 0, 0, 0}, {35, HDMI_HDMI} },
{ {2880, 576, 108000, 256, 48, 272, 5, 5, 39, 0, 0, 0}, {37, HDMI_HDMI} },
+};
+static const struct hdmi_config vesa_timings[] = {
/* VESA From Here */
{ {640, 480, 25175, 96, 16, 48, 2 , 11, 31, 0, 0, 0}, {4, HDMI_DVI} },
{ {800, 600, 4, 128, 40, 88, 4 , 1, 23, 1, 1, 0}, {9, HDMI_DVI} },
@@ -126,39 +126,6 @@ static const struct hdmi_config
cea_vesa_timings[OMAP_HDMI_TIMINGS_NB] = {
{ {1280, 720, 74250, 40, 110, 220, 5, 5, 20, 1, 1, 0}, {0x55, HDMI_DVI} }
};
-/*
- * This is a static mapping array which maps the timing values
- * with corresponding CEA / VESA code
- */
-static const int code_index[OMAP_HDMI_TIMINGS_NB] = {
- 1, 19, 4, 2, 37, 6, 21, 20, 5, 16, 17, 29, 31, 35, 32,
- /* --15 CEA 17-- vesa*/
- 4, 9, 0xE, 0x17, 0x1C, 0x27, 0x20, 0x23, 0x10, 0x2A,
- 0X2F, 0x3A, 0X51, 0X52, 0x16, 0x29, 0x39, 0x1B
-};
-
-/*
- * This is reverse static mapping which maps the CEA / VESA code
- * to the corresponding timing values
- */
-static const int code_cea[39] = {
- -1, 0, 3, 3, 2, 8, 5, 5, -1, -1,
- -1, -1, -1, -1, -1, -1, 9, 10, 10, 1,
- 7, 6, 6, -1, -1, -1, -1, -1, -1, 11,
- 11, 12, 14, -1, -1, 13, 13, 4, 4
-};
-
-static const int code_vesa[85] = {
- -1, -1, -1, -1, 15, -1, -1, -1, -1, 16,
- -1, -1, -1, -1, 17, -1, 23, -1, -1, -1,
- -1, -1, 29, 18, -1, -1, -1, 32, 19, -1,
- -1, -1, 21, -1, -1, 22, -1, -1, -1, 20,
- -1, 30, 24, -1, -1, -1, -1, 25, -1, -1,
- -1, -1, -1, -1, -1, -1, -1, 31, 26, -1,
- -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
- -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
- -1, 27, 28, -1, 33};
-
static int hdmi_runtime_get(void)
{
int r;
@@ -188,82 +155,88 @@ int hdmi_init_display(struct omap_dss_device *dssdev)
return 0;
}
-static int get_timings_index(void)
+static bool hdmi_find_code(const struct hdmi_config *timings_arr,
+ int len, struct hdmi_config *timing)
{
- int code;
+ int i;
- if (hdmi.mode == 0)
- code = code_vesa[hdmi.code];
- else
- code = code_cea[hdmi.code];
+ for (i = 0; i len; i++) {
+ if (timings_arr[i].cm.code == hdmi.code) {
+ *timing = timings_arr[i];
+ return true;
+ }
+ }
+
+ return false;
+}
- if (code == -1) {
+static void hdmi_get_timings(struct hdmi_config *timing)
+{
+ int r;
+
+ if (hdmi.mode == 0) {
+ r = hdmi_find_code(vesa_timings,
+ ARRAY_SIZE(vesa_timings), timing);
+ } else {
+ r = hdmi_find_code(cea_timings,
+ ARRAY_SIZE(cea_timings), timing);
+ }
+ if (!r) {
/* HDMI code 4 corresponds to 640 * 480 VGA */
hdmi.code = 4;
/* DVI mode 1 corresponds to HDMI 0 to DVI */
hdmi.mode = HDMI_DVI;
+ *timing = vesa_timings[0];
+ }
+}
+
+static bool hdmi_timings_compare(struct omap_video_timings *timing,
+ const struct hdmi_video_timings *temp)
+{
+ int timing_vsync, timing_hsync, temp_vsync, temp_hsync;
+
+ if ((temp-pixel_clock == timing-pixel_clock)
+ (temp-x_res == timing-x_res)
+ (temp-y_res == timing-y_res)) {
- code = code_vesa[hdmi.code];
+
