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@xxxxxx> wrote:
> > On Fri, 2011-11-11 at 18:39 +0530, mythripk@xxxxxx 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:
signature.asc
Description: This is a digitally signed message part
