2013/10/24 Paul Burton <paul.bur...@imgtec.com>:
>
>
> On 24/10/13 11:43, Gabor Juhos wrote:
>>
>> 2013.10.24. 12:10 keltezéssel, Paul Burton írta:
>>>
>>> On 24/10/13 11:04, Gabor Juhos wrote:
>>>>
>>>> 2013.10.24. 11:31 keltezéssel, Paul Burton írta:
>>>>>
>>>>> This is in preparation for adapting this board to function correctly on
>>>>> a physical MIPS Malta board. The board is moved into an "imgtec" vendor
>>>>> directory at the same time in order to ready us for any other boards
>>>>> supported by Imagination in the future.
>>>>>
>>>>> Signed-off-by: Paul Burton <paul.bur...@imgtec.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - move to an imgtec vendor directory to reduce churn should other
>>>>> Imagination boards be added later
>>>>> - enable rename detection for the patch
>>>>> ---
>>>>> arch/mips/lib/bootm.c | 12
>>>>> ++++++------
>>>>> board/{qemu-malta => imgtec/malta}/Makefile | 0
>>>>> board/{qemu-malta => imgtec/malta}/lowlevel_init.S | 0
>>>>> board/{qemu-malta/qemu-malta.c => imgtec/malta/malta.c} | 0
>>>>> boards.cfg | 4 ++--
>>>>> include/configs/{qemu-malta.h => malta.h} | 2 +-
>>>>> 6 files changed, 9 insertions(+), 9 deletions(-)
>>>>> rename board/{qemu-malta => imgtec/malta}/Makefile (100%)
>>>>> rename board/{qemu-malta => imgtec/malta}/lowlevel_init.S (100%)
>>>>> rename board/{qemu-malta/qemu-malta.c => imgtec/malta/malta.c}
>>>>> (100%)
>>>>> rename include/configs/{qemu-malta.h => malta.h} (98%)
>>>>>
>>>>> diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
>>>>> index 66340ea..1febf29 100644
>>>>> --- a/arch/mips/lib/bootm.c
>>>>> +++ b/arch/mips/lib/bootm.c
>>>>> @@ -17,10 +17,10 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>> #define LINUX_MAX_ENVS 256
>>>>> #define LINUX_MAX_ARGS 256
>>>>>
>>>>> -#if defined(CONFIG_QEMU_MALTA)
>>>>> -#define mips_boot_qemu_malta 1
>>>>> +#if defined(CONFIG_MALTA)
>>>>> +#define mips_boot_malta 1
>>>>> #else
>>>>> -#define mips_boot_qemu_malta 0
>>>>> +#define mips_boot_malta 0
>>>>> #endif
>>>>>
>>>>> static int linux_argc;
>>>>> @@ -139,7 +139,7 @@ static void linux_env_set(const char *env_name,
>>>>> const
>>>>> char *env_val)
>>>>> strcpy(linux_env_p, env_name);
>>>>> linux_env_p += strlen(env_name);
>>>>>
>>>>> - if (mips_boot_qemu_malta) {
>>>>> + if (mips_boot_malta) {
>>>>> linux_env_p++;
>>>>> linux_env[++linux_env_idx] = linux_env_p;
>>>>> } else {
>>>>> @@ -196,7 +196,7 @@ static void boot_prep_linux(bootm_headers_t
>>>>> *images)
>>>>> if (cp)
>>>>> linux_env_set("eth1addr", cp);
>>>>>
>>>>> - if (mips_boot_qemu_malta)
>>>>> + if (mips_boot_malta)
>>>>> linux_env_set("modetty0", "38400n8r");
>>>>> }
>>>>>
>>>>> @@ -210,7 +210,7 @@ static void boot_jump_linux(bootm_headers_t
>>>>> *images)
>>>>>
>>>>> bootstage_mark(BOOTSTAGE_ID_RUN_OS);
>>>>>
>>>>> - if (mips_boot_qemu_malta)
>>>>> + if (mips_boot_malta)
>>>>> linux_extra = gd->ram_size;
>>>>>
>>>>> /* we assume that the kernel is in place */
>>>>
>>>>
>>>> The changes in bootm.c are not strictly related to the rename. It would
>>>> be
>>>> better to do these in a separate patch.
>>>
>>>
>>> I disagree. The board was renamed from qemu-malta to malta, so it makes
>>> perfect
>>> sense to change CONFIG_QEMU_MALTA to CONFIG_MALTA and
>>> mips_boot_qemu_malta to
>>> mips_boot_malta at the same time. These changes are purely a part of the
>>> rename,
>>> made for consistency.
>>
>>
>> I agree that it is related to the rename. However renaming the
>> mips_boot_qemu_malta constant is a simple cleanup which can be done in a
>> preceding patch. The same is true for the CONFIG_QEMU_MALTA->CONFIG_MALTA
>> conversion.
>>
>> In general, renaming a file and altering that within the same patch makes
>> the
>> review harder.
>>
>> -Gabor
>>
>>
>
> And for more complex changes I might agree, but only a single file is both
> renamed and modified by this patch: include/configs/qemu-malta.h =>
> include/configs/malta.h. Only a single line is modified within that file,
> changing CONFIG_QEMU_MALTA to CONFIG_MALTA. I don't think it's very
> difficult to understand and I think splitting it would be overkill. So if
> Daniel tells me (v3 of) this patch isn't getting in as-is then I'll split
> it, but short of that I'm not changing it.
>
> Thanks,
> Paul
>
Actually I don't care. It is one logical change which is also trivial
and bisectable. If you want, you could move the renaming of
CONFIG_QEMU_MALTA and mips_boot_qemu_malta to a separate patch.
--
Best regards,
Daniel
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
