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

[Minkyu Kang]

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.