Subject: Re: 16bit aligned NS16550 variant
To: None <takemura@netbsd.org, tech-kern@netbsd.org>
From: None <eeh@netbsd.org>
List: tech-kern
Date: 10/20/2001 20:58:24
| I'd like to change NS16550 serial driver, 'com' to add 16bit register
| access support. And I've made the patch(attached).
|
| Some embedded companion chip and ASIC have UART compatible
| function but you must make 16bit access for thier registers.
|
| With this change, the driver will access it's registers with specified
| access wrappers instead of bus_space_read/write.
| There will be added new entries for compatibility and no ather files
| will be affected. (I found over 50 files calling comprobe1 or
| comcnattach and I  gave up to chage them all.)
|
| Please review the patch. If no one against me, I will commit this 
| change next weekend.

I object.

You are adding a layer of indirection to each register access 
*in addition* to any indirection implemented in the bus_space*
routines.

If this chip you were dealing with had one or two registers that
needed special treatment, you could add some sort of check to the
driver for that specific device.  But what you are doing looks like
it replaces every single device access.

The reason bus_space_* was added in the first place was to abstract
this mess out of the driver.  If this chip has strange requirements
for bus access, you should add an extra bus_space_* layer in the 
attachment glue for this specific device.  

You can take a look at how other ports handle the problem of accessing
com registers when they have a stride of 16-bits by adding some address
twiddling inside the bus_space_{read,write}*() macros.  Or you can
rewrite the bus_space_{read,write}*() macros for your specific port
to vector through function pointers.

But there is no reason to make all architectures pay this price.

Eduardo