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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux