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

[Przemyslaw Marczak]

On 01/17/2014 07:26 AM, Minkyu Kang wrote:
> On 15/01/14 17:18, Przemyslaw Marczak wrote:
>> Hello Piotr,
>>
>> On 01/15/2014 09:08 AM, Piotr Wilczek wrote:
>>> Dear Przemyslaw,
>>>
>>>> -----Original Message-----
>>>> From: Przemyslaw Marczak [mailto:p.marczak at samsung.com]
>>>> Sent: Wednesday, January 15, 2014 8:51 AM
>>>> To: Minkyu Kang
>>>> Cc: u-boot at lists.denx.de; jh80.chung at samsung.com;
>>>> human.hwang at samsung.com; dh09.lee at samsung.com; ideal.song at samsung.com;
>>>> Piotr Wilczek; Lukasz Majewski
>>>> Subject: Re: [PATCH v5 03/12] samsung: common: Add misc file and common
>>>> function misc_init_r().
>>>>
>>>> Hello,
>>>>
>>>> On 01/15/2014 08:35 AM, Minkyu Kang wrote:
>>>>> On 14/01/14 22:55, Przemyslaw Marczak wrote:
>>>>>> Hello,
>>>>>> In case of discussion with Piotr Wilczek maybe it is better to make
>>>> some changes in this patch.
>>>>>>
>>>>>> On 01/10/2014 03:31 PM, Przemyslaw Marczak wrote:
>>>>>>> Config: CONFIG_MISC_INIT_R enables implementation of misc_init_r()
>>>>>>> in common file::
>>>>>>> - board/samsung/common/misc.c
>>>>>>>
>>>>>>> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
>>>>>>> Acked-by: Jaehoon Chung <jh80.chung at samsung.com>
>>>>>>> ---
>>>>>>> Changes v2:
>>>>>>> - change CONFIG_SAMSUNG to CONFIG_MISC_INIT_R
>>>>>>>
>>>>>>> Changes v3:
>>>>>>> - fix merge conflict in board/samsung/common/Makefile
>>>>>>>
>>>>>>> Changes v4:
>>>>>>> - none
>>>>>>>
>>>>>>> Changes v5:
>>>>>>> - add acked-by
>>>>>>>
>>>>>>>      board/samsung/common/Makefile |    1 +
>>>>>>>      board/samsung/common/misc.c   |   14 ++++++++++++++
>>>>>>>      2 files changed, 15 insertions(+)
>>>>>>>      create mode 100644 board/samsung/common/misc.c
>>>>>>>
>>>>>>> diff --git a/board/samsung/common/Makefile
>>>>>>> b/board/samsung/common/Makefile index 22bd6b1..79547a3 100644
>>>>>>> --- a/board/samsung/common/Makefile
>>>>>>> +++ b/board/samsung/common/Makefile
>>>>>>> @@ -8,6 +8,7 @@
>>>>>>>      obj-$(CONFIG_SOFT_I2C_MULTI_BUS) += multi_i2c.o
>>>>>>>      obj-$(CONFIG_THOR_FUNCTION) += thor.o
>>>>>>>      obj-$(CONFIG_CMD_USB_MASS_STORAGE) += ums.o
>>>>>>> +obj-$(CONFIG_MISC_INIT_R) += misc.o
>>>>>> here change to:
>>>>>> obj-y += misc.o
>>>>>>
>>>>>>>
>>>>>>>      ifndef CONFIG_SPL_BUILD
>>>>>>>      obj-$(CONFIG_BOARD_COMMON)    += board.o
>>>>>>> diff --git a/board/samsung/common/misc.c
>>>>>>> b/board/samsung/common/misc.c new file mode 100644 index
>>>>>>> 0000000..3764d12
>>>>>>> --- /dev/null
>>>>>>> +++ b/board/samsung/common/misc.c
>>>>>>> @@ -0,0 +1,14 @@
>>>>>>> +/*
>>>>>>> + * Copyright (C) 2013 Samsung Electronics
>>>>>>> + * Przemyslaw Marczak <p.marczak at samsung.com>
>>>>>>> + *
>>>>>>> + * SPDX-License-Identifier:    GPL-2.0+
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <common.h>
>>>>>>> +
>>>>>>
>>>>>> and here:
>>>>>> #ifdef CONFIG_MISC_INIT_R
>>>>>>
>>>>>>> +/* Common for Samsung boards */
>>>>>>> +int misc_init_r(void)
>>>>>>> +{
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>>
>>>>>> #endif
>>>>>>
>>>>>> 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.

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

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.

Thank you,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com