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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot