I am not thrilled with this with this patch, as I think it has some
significant issues.

1) the whole fakesc thing looks, well, ugly.

2) you "free" a softc.  this code depends on behavior of how autoconf
manages softcs, and I don't think a target device driver has any
business depending on that.

3) you've created another "more or less arbitrary" set of wrapper
routines so that you can pass e.g. whether the pins are primary or
secondary in as an argument.  There is no need for this.

4) all this, and you're still violating the bus_space abstraction by
hard coding GPIO_BASE, etc.

What I would rather see would be a much simpler abstraction:

Just hard code the addresses GPIO_BASE and GPIO2_BASE in the code, and
just use those addresses (adjusted with MIPS_PHYS_TO_KSEG1()).  Remove
the use of the softc argument from the augpio_read/augpio2_read
routines, and instead ignore the first arg.  (The arg has to be passed
to make the GPIO framework happy.)  You can then pass NULL into them
from wherever.  If you don't like passing NULL, hide it behind a macro
that does the "right" thing, e.g.

#define   AUGPIO_READ(pin)   augpio_read(NULL, pin)
#define   AUGPIO2_READ(pin)   augpio2_read(NULL pin)

and so forth.

I can make this change for you if you like, it would only take a few
minutes.  :-)  Probably less time than I've spent reviewing and
responding to these changes.  :-)

    -- Garrett

KIYOHARA Takashi wrote:
> Hi! Garrett,
>
>
> The reply apologizes ..slowly...  ;-<
>
>
> From: "Garrett D'Amore" <garrett_damore@tadpole.com>
> Date: Tue, 21 Mar 2006 21:53:22 -0800
>
>   
>> The reason for this is that there are "ordering constraints" otherwise
>> -- how do you augpio has attached before aupcmcia?   This is something
>> that I think autoconf lacks support for expressing (at least where the
>> dependency is not implied as a result of child-parent relationship.)
>>     
>
> Is it a problem in the following methods?  For instance, it can fake attach
> it earlier more temporarily than augpio.  Please call augpio_attach_fake()
> if you operate augpio earlier than attach of augpio. 
> The prototype of the function is assumed to put it in a suitable header
> file. 
>
> Thanks,
> --
> kiyohara
>   
> ------------------------------------------------------------------------
>
> Index: dev/augpio.c
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/mips/alchemy/dev/augpio.c,v
> retrieving revision 1.3
> diff -u -r1.3 augpio.c
> --- dev/augpio.c	18 Feb 2006 23:21:06 -0000	1.3
> +++ dev/augpio.c	24 Mar 2006 00:30:56 -0000
> @@ -41,6 +41,7 @@
>  #include <sys/device.h>
>  #include <sys/gpio.h>
>  #include <sys/kernel.h>
> +#include <sys/malloc.h>
>  
>  #include <dev/gpio/gpiovar.h>
>  
> @@ -62,6 +63,11 @@
>  	const char 			*sc_name;
>  	int				(*sc_getctl)(void *, int);
>  };
> +enum {
> +	primary = 0,
> +	secondary = 1,
> +	ngpio = 2
> +};
>  
>  static int augpio_read(void *, int);
>  static void augpio_write(void *, int, int);
> @@ -73,6 +79,12 @@
>  static void augpio2_ctl(void *, int, int);
>  static int augpio2_getctl(void *, int);
>  
> +int augpio_attach_fake(int);
> +int augpioread(int, int);
> +void augpiowrite(int, int, int);
> +int augpiogetctl(int, int);
> +void augpioctl(int, int, int);
> +
>  static int augpio_match(struct device *, struct cfdata *, void *);
>  static void augpio_attach(struct device *, struct device *, void *);
>  
> @@ -85,6 +97,7 @@
>  	((*(volatile uint32_t *)MIPS_PHYS_TO_KSEG1(x)) = (v))
>  
>  static int augpio_found = 0;
> +static struct augpio_softc *softces[ngpio];
>  
>  int
>  augpio_match(struct device *parent, struct cfdata *match, void *aux)
> @@ -105,7 +118,7 @@
>  {
>  	int	pin;
>  
> -	struct augpio_softc *sc = (struct augpio_softc *)self;
> +	struct augpio_softc *sc = (struct augpio_softc *)self, *fakesc;
>  	struct aubus_attach_args *aa = aux;
>  	struct gpiobus_attach_args gba;
>  
> @@ -114,6 +127,12 @@
>  	sc->sc_gc.gp_cookie = sc;
>  
>  	if (aa->aa_addrs[0] == GPIO_BASE) {
> +		if (softces[primary] != NULL) {
> +			fakesc = softces[primary];
> +			bus_space_unmap(fakesc->sc_bst,
> +			    fakesc->sc_bsh, AUGPIO_SIZE);
> +			free(fakesc, M_TEMP);
> +		}
>  
>  		if (bus_space_map(sc->sc_bst, aa->aa_addrs[0],
>  			AUGPIO_SIZE, 0, &sc->sc_bsh) != 0) {
> @@ -128,6 +147,7 @@
>  		sc->sc_getctl = augpio_getctl;
>  		sc->sc_name = "primary block";
>  
> +		softces[primary] = sc;
>  	} else if (aa->aa_addrs[0] == GPIO2_BASE) {
>  		/*
>  		 * We rely on firmware (or platform init code) to initialize
> @@ -135,6 +155,12 @@
>  		 * resetting the GPIO2 block can have nasty effects (e.g.
>  		 * reset PCI bus...)
>  		 */
> +		if (softces[secondary] != NULL) {
> +			fakesc = softces[secondary];
> +			bus_space_unmap(fakesc->sc_bst,
> +			    fakesc->sc_bsh, AUGPIO2_SIZE);
> +			free(fakesc, M_TEMP);
> +		}
>  		if (bus_space_map(sc->sc_bst, aa->aa_addrs[0],
>  			AUGPIO2_SIZE, 0, &sc->sc_bsh) != 0) {
>  			printf(": cannot map registers!\n");
> @@ -147,6 +173,7 @@
>  		sc->sc_getctl = augpio2_getctl;
>  		sc->sc_name = "secondary block";
>  
> +		softces[secondary] = sc;
>  	} else {
>  		printf(": unidentified block\n");
>  		return;
> @@ -287,3 +314,71 @@
>  	}
>  }
>  
> +int
> +augpio_attach_fake(int gpio)
> +{
> +	struct augpio_softc *fakesc;
> +	bus_addr_t address;
> +	bus_size_t size;
> +
> +	if (softces[gpio] == NULL)
> +		return 0;
> +
> +	fakesc = malloc(sizeof (struct augpio_softc), M_TEMP, M_NOWAIT);
> +	fakesc->sc_bst = aubus_st;
> +	if (gpio == primary) {
> +		fakesc->sc_gc.gp_pin_read = augpio_read;
> +		fakesc->sc_gc.gp_pin_write = augpio_write;
> +		fakesc->sc_gc.gp_pin_ctl = augpio_ctl;
> +		fakesc->sc_getctl = augpio_getctl;
> +		address = GPIO_BASE;
> +		size = AUGPIO_SIZE;
> +	} else if (gpio == secondary) {
> +		fakesc->sc_gc.gp_pin_read = augpio2_read;
> +		fakesc->sc_gc.gp_pin_write = augpio2_write;
> +		fakesc->sc_gc.gp_pin_ctl = augpio2_ctl;
> +		fakesc->sc_getctl = augpio2_getctl;
> +		address = GPIO2_BASE;
> +		size = AUGPIO2_SIZE;
> +	} else
> +		return ~0;
> +
> +	if (bus_space_map(fakesc->sc_bst,
> +	    address, size, 0, &fakesc->sc_bsh) != 0)
> +		return ~0;
> +	softces[gpio] = fakesc;
> +
> +	return 0;
> +}
> +
> +int 
> +augpioread(int gpio, int pin)
> +{
> +
> +	KASSERT(softces[gpio] != NULL);
> +	return softces[gpio]->sc_gc.gp_pin_read(softces[gpio], pin);
> +}
> +
> +void 
> +augpiowrite(int gpio, int pin, int value)
> +{
> +
> +	KASSERT(softces[gpio] != NULL);
> +	return softces[gpio]->sc_gc.gp_pin_write(softces[gpio], pin, value);
> +}
> +
> +void 
> +augpioctl(int gpio, int pin, int flags)
> +{
> +
> +	KASSERT(softces[gpio] != NULL);
> +	return softces[gpio]->sc_gc.gp_pin_ctl(softces[gpio], pin, flags);
> +}
> +
> +int 
> +augpiogetctl(int gpio, int pin)
> +{
> +
> +	KASSERT(softces[gpio] != NULL);
> +	return softces[gpio]->sc_getctl(softces[gpio], pin);
> +}
>   


-- 
Garrett D'Amore, Principal Software Engineer
Tadpole Computer / Computing Technologies Division,
General Dynamics C4 Systems
http://www.tadpolecomputer.com/
Phone: 951 325-2134  Fax: 951 325-2191