Re: [PATCH 3/4] OMAPDSS: HDMI: change the timing match logic

["K, Mythri P" <mythripk@ti.com>]

On Mon, Nov 14, 2011 at 1:03 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Fri, 2011-11-11 at 18:39 +0530, mythripk@ti.com wrote:
>> From: Mythri P K <mythripk@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 <mythripk@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, 40000, 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 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 be parameters to this function.
>
In the next patch i remove the hdmi.code and mode and use the config instead.

> And the code in the error path shouldn't modify hdmi.code or hdmi.mode,
> it should just return NULL, and the caller would then do what it wants,
> in this case use VGA as a fallback.
ok.
>
>> +
>> +static bool hdmi_timings_compare(struct omap_video_timings *timing,
>> +                             const struct hdmi_video_timings *temp)
>
> Don't call the second argument "temp". It's not a temporary variable.
> Naming the parameters timing1 and timing2 would make much more sense.
>
ok.

Thanks and regards,
Mythri.
>  Tomi
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html