Re: IBM405GP/GPr OPB bus_space endian (powerpc/ibm4xx/dev/opb.c)

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

In article <44143C93.2090007%netbsd.org@localhost>
shige%NetBSD.org@localhost wrote:

> I think, I'm going to commit its fix.
> I'm sorry to trouble tou, but please confirm again now whether is it ok?

IMHO it's still better to post the latest patch to be committed,
but I'd like some cosmetics in your previous patch:
(sorry maybe I'm a paranoid again ;-)

> RCS file: /cvsroot/src/sys/arch/evbppc/conf/files.obs405,v
> retrieving revision 1.12
> diff -u -u -r1.12 files.obs405
> --- sys/arch/evbppc/conf/files.obs405 10 Sep 2005 04:34:39 -0000      1.12
> +++ sys/arch/evbppc/conf/files.obs405 1 Oct 2005 16:12:52 -0000
> @@ -14,9 +14,9 @@
>  file arch/evbppc/obs405/obs405_autoconf.c
>  file arch/evbppc/obs405/obs405_machdep.c
>  
> -device obsled
> -attach  obsled at gpio
> -file arch/evbppc/obs405/dev/obsled.c         obsled
> +#device obsled
> +#attach  obsled at ibmgpio
> +#file        arch/evbppc/obs405/dev/obsled.c         obsled

Why is obsled commented out?
Will it be rewritten to use MI gpio later?

> RCS file: /cvsroot/src/sys/arch/evbppc/obs405/obs266_machdep.c,v
> retrieving revision 1.1
> diff -u -u -r1.1 obs266_machdep.c
> --- sys/arch/evbppc/obs405/obs266_machdep.c   18 Mar 2005 14:12:34 -0000      
> 1.1
> +++ sys/arch/evbppc/obs405/obs266_machdep.c   1 Oct 2005 16:12:52 -0000
> @@ -214,7 +214,7 @@
>       static char str[256];
>       char *ap = str, *ap1 = ap;
>  
> -     obs266_led_set(OBS266_LED_ON);
> +     /* obs266_led_set(OBS266_LED_ON); */

I'm not sure how the leds should be handled by MI gpio,
but please use #if 0/#endif pair to disable code.

> RCS file: /cvsroot/src/sys/arch/powerpc/conf/files.ibm4xx,v
> retrieving revision 1.7
> diff -u -u -r1.7 files.ibm4xx
> --- sys/arch/powerpc/conf/files.ibm4xx        23 Jan 2005 19:24:31 -0000      
> 1.7
> +++ sys/arch/powerpc/conf/files.ibm4xx        1 Oct 2005 16:12:52 -0000
> @@ -30,9 +30,9 @@
>  file arch/powerpc/ibm4xx/dev/com_opb.c       com_opb
>  
>  # On-chip GPIO controller
> -device       gpio { addr, [size = -1] }
> -attach       gpio at opb
> -file arch/powerpc/ibm4xx/dev/gpio_opb.c      gpio
> +device       gpio_opb: gpiobus
> +attach       gpio_opb at opb
> +file arch/powerpc/ibm4xx/dev/gpio_opb.c      gpio_opb

Underscore in a device name (i.e. "giop_opb0 at opb?" in config files)
looks weird a bit to me, and I'm afraid it causes some confusion.
Isn't "opbgpio" like epgpio for arm/ep93xx better?

> RCS file: /cvsroot/src/sys/arch/powerpc/ibm4xx/dev/opb.c,v
> retrieving revision 1.20
> diff -u -u -r1.20 opb.c
> --- sys/arch/powerpc/ibm4xx/dev/opb.c 26 Aug 2005 13:19:37 -0000      1.20
> +++ sys/arch/powerpc/ibm4xx/dev/opb.c 1 Oct 2005 16:12:52 -0000
 :
>  static struct powerpc_bus_space opb_tag = {
> -     _BUS_SPACE_LITTLE_ENDIAN|_BUS_SPACE_MEM_TYPE,
> +     _BUS_SPACE_BIG_ENDIAN|_BUS_SPACE_MEM_TYPE,
>       0x0, IBM405GP_UART0_BASE, 0x1000

if_emac.c uses bus_space_{read,write}_stream_4() to access
registers, but I wonder why these stream ops are needed...
(powerpc/bus_space.c doesn't handle stream ops correctly anyway)

Lastly, random KNF stuff:
- uint32_t rather than u_int32_t
- no parenthesis around return values
- "Insert an empty line if the function has no local variables."
etc.

---
Izumi Tsutsui