[U-Boot] [PATCH 1/3] arm: add CONFIG_MACH_TYPE option and documentation

[Igor Grinberg]

Hi Albert,

On 07/08/11 00:06, Igor Grinberg wrote:
> On 07/07/11 20:46, Albert ARIBAUD wrote:
>> Le 07/07/2011 18:51, Igor Grinberg a écrit :
>>
>>>>> If we have this option and it is documented, then any new board can use it
>>>>> instead of thinking (although it is simple) where and how to dereference
>>>>> the bi_arch_number.
>>>> Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it?
>>> For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.),
>>> If you need Christopher's patch, then there are cases when the machid is not set, right?
>>> When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then
>>> goes to fix the code, but again he thinks, where is the best place to put it?
>>> For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial.
>>> With this patch, you get the explanation and also a place to put the machid definition.
>>> With this patch, you just define the configuration "variable" and the whole thing will be done for you.
>>> Another example would be the board/nvidia/*, the code is shared as much as possible,
>>> and the mach_type is set in the common code. That is something I would expect to be done
>>> for all ARM boards, not just for nvidia...
>> I see your point.
>>
>> Now the issue I foresee is that this commonalization has benefits only for boards which currently set their bi_arch_number in board_init_f(),
> Vast majority of boards set their bi_arch_number in board_init().
> I went through all the boards and there is only one that set it in board_early_init_f()
> and several that do this in checkboard().
>
> This makes me think of v2 of this patch which will set the bi_arch_number in board_init_f()
> just before the init_sequence[] array is run.
>
>> but has no incentive -- that's a code that will be used only in a few places and could stay that way for quite long, because boards that will not adhere to it will still build unchanged.
> Well, I don't like to force people do something by breaking their builds...
> In general, I think that any change should not break any existing code (at least not on purpose).
> At least, this is how it works in Linux.
>
>> IOW, there is no benefit for e.g. ED Mini V2, to use CONFIG_MACH_TYPE, so why would it? Thus instead of simplifying and commonalizing, this feature will *add* to the code base complexity.
>>
>> Unless the goal is to add this macro *and* change all related board codes in the same patchset? I don't see it as feasible either.
> Well, I can do the change board/* wide, but it will take some time to accomplish.
> Also, we still don't have an exact list of boards for removal, so I'd like to wait until
> the removal takes place, so there will be less boards to consider.
>
>> Any suggestion for ensuring adoption of the feature wherever it can be used?
> Currently, I can think of:
> 1) Changing all relevant boards to use it - will eliminate "bad" examples.
> 2) Pointing to the use of CONFIG_MACH_TYPE during code review.
> 3) Introduce one more config option, like CONFIG_DYNAMIC_MACH_TYPE
> and change the patch to something like:
> #ifndef CONFIG_DYNAMIC_MACH_TYPE
> bi_arch_number = CONFIG_MACH_TYPE;
> #endif
>
> If we decide to go for 3), it would integrate nicely with Christopher's patch.

So, what will it be?
If it will be 1 and 2, then I'd like to get this patch in, so I can start working on 1.

If it will be 3, then I want to make the change and resubmit,
hoping for current merge window...



-- 
Regards,
Igor.