[U-Boot] [PATCH v5 03/12] samsung: common: Add misc file and common function misc_init_r().

[Minkyu Kang]

On 17/01/14 17:36, Przemyslaw Marczak wrote:
> On 01/17/2014 07:26 AM, Minkyu Kang wrote:
>> On 15/01/14 17:18, Przemyslaw Marczak wrote:
>>>>>>>
>>>>>>> In this way we can add other functions in the future even without
>>>>> CONFIG_MISC_INIT_R.
>>>>>>
>>>>>> partly agree.
>>>>>> But, I doubt what is the role of misc.c file.
>>>>>> because of the meaning of miscellaneous is ambiguous, this file have
>>>>>> possibility to be messy.
>>>>>> So, please let me know what is your plan to this file.
>>>>>>
>>>>>
>>>>> I first planned put there only implementation of misc_init_r() and it's
>>>>> subfunctions - as the easy way to display logo and menu for Samsung
>>>>> boards.
>>>>> Piotr has suggested to change the purpose of this file as misc not only
>>>>> for misc_init_r implementation...
>>>> Przemyslaw, I asked you question: what is the misc.c file for?
>>>> If for misc_init_r only then I think the file name "misc.c" is confusing.
>>>> If also other common functions can be put there, then the define MISC_INIT_R
>>>> to compile this file is wrong.
>>>>
>>>
>>> Yes, and next I said that maybe I will change this config dependency, and now I try to do it.
>>>
>>
>> Actually, I feel negative to this changes.
>> Because misc_init_r is a board specific.
>> How you can support if someone want to do something (board specific things) on misc_init_r?
>> I totally understand why you add misc_init_r to common directory. - It means you don't have to explain why you added it :)
>> but it looks little weird.
>> So we will discuss that misc_init_r should go to each boards or stay here? (misc_init_r function only, not including key, menu, logo.. etc)
>>
>> Please let me know your opinions.
>>
>> Thanks,
>> Minkyu Kang.
>>
>>
> 
> The reason why I used misc_init_r for a common purposes is that it is called after all hardware initialization and before u-boot main_loop(), then I don't need to introduce another generic function just to check buttons - this is the only reason.
> 
> Moreover at this time misc_init_r() is implemented only in Trats2, and there are easy to move things.
> 
> You're right - misc_init_r is board specific, but if we make it as a common function, then we also can add board specific code, called here but implemented in board files.

No. it looks wrong architecture.

> 
> If this is wrong, then where is the better place for check keys, display logo and any more vendor common things?

Stay at common directory.

> 
> Or maybe the better solution is just add new function callback to board_init_r() for some vendor specific purposes - and then it can be used for other vendors platforms too.

No.

Here, I've made pseudo code for it.
How you think?

diff --git a/board/samsung/common/misc.c b/board/samsung/common/misc.c
new file mode 100644
index 0000000..cdc48bb
--- /dev/null
+++ b/board/samsung/common/misc.c
@@ -0,0 +1,18 @@
+#ifdef CONFIG_LCD_MENU
+void keys_init(void)
+{
+       /* TODO */
+}
+
+void check_boot_mode(void)
+{
+       /* TODO */
+}
+#endif
+
+#ifdef CONFIG_CMD_BMP
+void draw_logo(void)
+{
+       /* TODO */
+}
+#endif
diff --git a/board/samsung/trats2/trats2.c b/board/samsung/trats2/trats2.c
index be15357..eb4f4dc 100644
--- a/board/samsung/trats2/trats2.c
+++ b/board/samsung/trats2/trats2.c
@@ -623,6 +623,16 @@ int misc_init_r(void)
 
        show_hw_revision();
 
+#ifdef CONFIG_LCD_MENU
+       keys_init();
+       check_boot_mode();
+#endif
+
+#ifdef CONFIG_CMD_BMP
+       if (panel_info.logo_on)
+               draw_logo();
+#endif
+
        return 0;
 }
 #endif

Thanks,
Minkyu Kang.