Re: [PATCH] m32r - Upgrade to v2.6.6 kernel

[Andrey Panin]

On 149, 05 28, 2004 at 09:21:49PM +0900, Hirokazu Takata wrote:
> From: Andrey Panin <pazke@xxxxxxxxx>
> > On 149, 05 28, 2004 at 01:16:11PM +0900, Hirokazu Takata wrote:
> > > I would like to send the latest 2.6.6 kernel patch for 
> > > the Renesas M32R processor.
> > > 
> > > Patch information to the stock 2.6.6 kernel is placed as follows:
> > > - m32r architecture dependent portions (arch/m32r, include/asm-m32r)
> > >   http://www.linux-m32r.org/public/linux-2.6.6_m32r_20040528.arch-m32r.patch
> > 
> > Single megapatch (1.5 Mb in size) is not the best way to merge something into Linux kernel.
> 
> Yes, but the m32r port has not been merged into the mainline kernel
> tree yet...  May I send many small patches to LKML?
> 
> 
> > Now quick look at the patch itself:
> 
> Thank you for your quick reply.
> 
> 
> > 1)
> > diff -ruN linux-2.6.6.org/arch/m32r/drivers/8390.c linux-2.6.6/arch/m32r/drivers/8390.c
>  ...
> > diff -ruN linux-2.6.6.org/arch/m32r/drivers/8390.h linux-2.6.6/arch/m32r/drivers/8390.h
>  ...
> > Is this really needed ?
> 
> These files are required for mappi_ne.c.
> The mappi_ne.c is based on ne.c and modified for an eva board "mappi",
> I think it is highly dependent on the target platform.
> 
> There are two reasons that we placed some m32r specific drivers in 
> arch/m32r/drivers/:
> - We think they are highly depends on the target platform, and

So we should merge m32r core first :)

> - We can not merge them to drivers/ because the m32r architecture
>   has not been supported by the mainline linux kernel.

We have many arch specific drivers in drivers/net already.

> > 2) File arch/m32r/drivers/mappi_ne.c contains almost complete copy of drivers/net/ne.c
> > with lots of code probably useless for your systems (old style ISA probing, ISAPnP 
> > support etc.)
> 
> Yes.
> 
> > Also code like this is definetely unacceptable:
> > 
> > +#ifdef CONFIG_PLAT_MAPPI
> > +		outb_p(0x4b, ioaddr + EN0_DCFG);
> > +#elif CONFIG_PLAT_OAKS32R
> > +		outb_p(0x48, ioaddr + EN0_DCFG);
> > +#else
> > +		outb_p(0x49, ioaddr + EN0_DCFG);
> > +#endif
> > 
> > This fragment can be rewritten this way, with all  #ifdef mess hidden in the some header file:
> > 
> > +		outb_p(MY_MAGIC_OFFSET, ioaddr + EN0_DCFG);
> 
> Do you mean I should use drivers/net/ne.c instead of mappi_ne.c?
> I'm not sure how can I merge such target-dependent code fragments to ne.c.
> But I will try to merge them.

I didn't mean merging. My point is that general policy for linux kernel
code is to avoid #ifdef cluttered code as much as possible. It's better
to hide such code in header files.
 
> > 4) Do you really need to reimplement Linux console subsystem in arch/m32r/drivers/video/console.c,
> > arch/m32r/drivers/video/fbmem.c, arch/m32r/drivers/video/fbcon.h ?
> 
> These are obsolete codes, which were used in m32r 2.4 kernel.
> In 2.6 kernel, a framebuffer device and video console driver 
> should be reimplement for m32r targets...
> 
> 
> > 5) Any specific reason to implement read[bwl]/write[bwl] this way:
> > 
> > +unsigned char _readb(unsigned long addr)
> > +{
> > +       return *(volatile unsigned char *)addr;
> > +}
> > 
> > Why not to inline them ?
> 
> They were used for debugging. 
> I will rewrite to inline them, thank you.
> 
> 
> > 6) Lots of ugly debugging #ifdef's in arch/m32r/kernel/ptrace.c
> 
> I agree.
> 
> 
> > 7) arch/m32r/lib/clib.c contains slightly strange abs() function which
> > isn't used anywhere in the patch.
> 
> Hmm..  it is really strange.  I will remove it.

-- 
Andrey Panin		| Linux and UNIX system administrator
pazke@xxxxxxxxx		| PGP key: wwwkeys.pgp.net

Attachment: signature.asc
Description: Digital signature