Re: [loongson-PATCH-v3 00/25] loongson-based machines support

[Wu Zhangjin <wuzhangjin@gmail.com>]

Hi,

On Tue, 2009-06-09 at 00:38 +0800, Zhang Le wrote:
> First of all, sorry for late comment and thanks to Zhangjin for the great work.
> 
> However, I have some suggestions.
> 
> On 20:58 Thu 04 Jun     , wuzhangjin@gmail.com wrote:
> > Wu Zhangjin (25):
> >   add vmlinux.32 in .gitignore
> >   fix-warning: incompatible argument type of pci_fixup_irqs
> >   fix-warning: incompatible argument type of virt_to_phys
> 
> I think these 3 patch could be submitted separately, since they are not quite
> related to Loongson.
> 

Yeap, I will split all of the non-loongson-specific patches out in the
coming -v4 patch series and also split the immature ones out too.

> >   change the naming methods
> 
> In this patch, I found function get_system_type() still returns wrong name,
> "lemote-fulong". In later patches, I found this string was changed to a macro,
> MACH_NAME. Then, the function becomes more complicated and/or sophisticated,
> because of the addition of machname array.

the MACH_NAME macro method is originally picked from lm2e-fixes branch
of Philippe's git://git.linux-cisco.org/linux-mips.git, this method is
used to share the get_system_type() function between different machines.

and the machtype with machname array is only used to share the same
kernel image file between yeeloong-7inch(menglong?) and yeeloong-8.9inch
source code.

I think it will be "bad" to add a new kernel option named MENGLONG or
something else, and add a new yeeloong-7inch directory in
arch/mips/loongson/ for it, because the difference between
yeeloong-7inch and yeeloong-8.0 inch is very small(the shutdown logic
and screen size). and also, simply add two new kernel options like
YEELOONG-7INCH and YEELOONG-89INCH with #ifdef..#else...#endif is also
not that good, is that?

so, i just added a machtype kernel command line(perhaps we can use the
systype or machtype variable in pmon instead as Arnaud Patard
suggested). and perhaps this machtype can also be used to share the the
kernel image file among the future machines.

what about your suggestion here? is there another good solution? or just
keep it simple: just define the get_system_type() function for every
machine and add a new kernel option for yeeloong-7inch?

> I don't know if this is an established or widely accepted policy, but
> intuitively, at the very least IMHO, a series of patches should only provide
> one correct implementation of a particular function, not provide one wrong
> function then override it with a correct one.
> 
> If I were you, I would do a 'git reset' first.
> Then 'git add' and/or 'git rm' some files which contain similar changes.
> Then 'git commit'.
> Repeat the last two steps, until all the changes have been committed.
> 

I will try to rebase and merge some of the similar patches, if not work,
then did as you suggested, thanks a lot!

thanks!
Wu Zhangjin