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

[Hirokazu Takata <takata@linux-m32r.org>]

From: Andrey Panin <pazke@donpac.ru>
> 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
- We can not merge them to drivers/ because the m32r architecture
  has not been supported by the mainline linux kernel.


> 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.


> 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@donpac.ru		| PGP key: wwwkeys.pgp.net
> [2 Digital signature <application/pgp-signature (7bit)>]
> 

Thank you for your kind comments, again.
--
Hirokazu Takata
Linux M32R Project:  http://www.linux-m32r.org