Sorry for a bit late response, but I'd appreciate if I could have
more time for review before merge. Thanks,

garrett_damore@tadpole.com wrote:

> > - Why don't you put map[] member to struct com_softc?
> Because lots of com routines don't actually have/use the softc. 
> Particularly stuff that might be used with console/kgdb support.  This
> common structure lives in the softc, and can also live outside the softc
> for use in code that doesn't have the softc.

IMHO, it looks a bit inconsistent to have both functions,
ones take com_softc and the others do com_regs.
(of course current implementation which passes only iot, ioh
 and iobase is also ugly though)

Actually you have changed such functions (which don't take com_softc)
into wrapper ones to prepare struct com_regs and create other real
functions which take struct com_regs.
Why don't you make them wrapper functions which prepare struct softc?
How about structures like this?

---
struct com_regs {
	bus_space_tag_t		cr_iot;
	bus_space_handle_t	cr_ioh;
	bus_addr_t		cr_iobase;
	bus_size_t		cr_nports;
	bus_size_t		cr_regmap[16];
};

struct com_softc {
	struct device sc_dev;
	void *sc_si;
	struct tty *sc_tty;

	struct callout sc_diag_callout;

	int sc_frequency;

	struct com_regs sc_regs;
#define sc_iot		sc_regs.cr_iot
#define sc_ioh		sc_regs.cr_ioh
#define sc_iobase	sc_regs.cr_iobase
#define sc_nports	sc_regs.cr_nports
#define sc_regmap	sc_regs.cr_regmap

	bus_space_handle_t sc_hayespioh;
 :

---
 :
#define	CSR_WRITE_1(sc, o, v)	\
	bus_space_write_1((sc)->sc_iot, (sc)->sc_ioh, (sc)->sc_regmap[o], v)
#define	CSR_READ_1(sc, o)	\
	bus_space_read_1((sc)->sc_iot, (sc)->cr_ioh, (sc)->cr_map[o])
 :
int
cominit(struct com_softc *sc, int rate, int frequency, int type,
    tcflag_t cflag)
{

	if (bus_space_map(sc->sc_iot, sc->sc_iobase, sc->sc_nports, 0,
		&sc->sc_ioh))
		return (ENOMEM); /* ??? */

	rate = comspeed(rate, frequency, type);
	if (type != COM_TYPE_AU1x00) {
		/* no EFR on alchemy */
		CSR_WRITE_1(sc, COM_REG_LCR, LCR_EERS);
 :
}
 :
int
comcnattach1(struct com_softc *sc, int rate, int frequency, int type,
    tcflag_t cflag)
{
	int res;

	comconsregs = sc->sc_reg;

	res = cominit(sc, rate, frequency, type, cflag);
 :
}

int
comcnattach(bus_space_tag_t iot, bus_addr_t iobase, int rate, int frequency,
    int type, tcflag_t cflag)
{
	struct com_softc sc_store;
	struct com_softc *sc = &sc_store;

	sc->sc_iot = iot;
	sc->sc_iobase = iobase;
	sc->sc_nports = COM_NPORTS;
#ifdef	COM_REGMAP
	memcpy(sc->sc_regmap, com_std_map, sizeof(sc->sc_regmap));
#endif

	return comcnattach1(sc, rate, frequency, type, cflag);
}

---

In this implementation, you have to copy struct com_regs
from comconsregs to com_softc for com_common_{put,get}c().
If you don't like the copy, we could have some hacked
struct com_regs which can be passed as struct softc:

---
struct com_regs {
	/* these members should be sync'ed with ones in struct com_softc */
	struct device		cr_dev;	/* dummy */

	bus_space_tag_t		cr_iot;
	bus_space_handle_t	cr_ioh;
	bus_addr_t		cr_iobase;
	bus_size_t		cr_nports;
	bus_size_t		cr_regmap[16];
};

struct com_softc {
	struct device sc_dev;

	bus_space_tag_t sc_iot;
	bus_space_handle_t sc_ioh;
	bus_addr_t sc_iobase;
	bus_size_t sc_nports;
	bus_size_t sc_regmap[16];

	bus_space_handle_t sc_hayespioh;

	void *sc_si;
	struct tty *sc_tty;

	struct callout sc_diag_callout;

	int sc_frequency;
 :
---

I'm not sure if these implementations are good idea
(because I'm not a programmer but just a dumb reader),
but I think it could reduce changes from -current common style.
(note I don't test these changes at all though)

> > - Where is "COM_REGSMAP" defined in your code?
> 
> In conf/files (or rather, opt_com.h)
> 
> >   Is it possible to define it only if a particular MD
> >   attachment which requires special layout is configured?
>
> Yes.  Stick it in MD config file (e.g. I have it in files.alchemy and
> files.atheros, but not in i386.)

Hmm. What happens if we have to support another com variant which
is attached to MI bus but also has odd regster mapping?

> > - Do all MD attachments have to initialize map[] array
> >   if COM_REGSMAP is defined?
> 
> No, there is a default "COM_INIT_REGS" that initializes this for
> standard 16550 layout.

Well, you are adding COM_INIT_REGS() macro in _all_ MD attachments.
That was my concern. Some people may complain about it if they have
their own private drivers, or if you miss some attachments on the merge.

I wonder if we can initialize the regmap array in com_attach_subr()
according to some proper quirk flag in softc:
---
	if ((sc->sc_flags & COM_F_HAVE_OWN_REGMAP) == 0)
		COM_INIT_REGMAP(sc->sc_regmap);
---

Then I guess we could use most current MD attachments without
extra changes.
---
Izumi Tsutsui