Hi Tomi,
On Fri, Nov 18, 2011 at 12:46 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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.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.
>
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
