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

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

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

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.valkeinen@ti.com> wrote:
> > On Fri, 2011-11-11 at 18:39 +0530, mythripk@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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]