Re: [PATCH] SC26XX: New serial driver for SC2681 uarts

[tsbogend@alpha.franken.de (Thomas Bogendoerfer)]

On Tue, Dec 04, 2007 at 07:27:38PM -0800, Andrew Morton wrote:
> grumble.
> 
> These:
> 
> > +#define READ_SC(p, r)        readb((p)->membase + RD_##r)
> > +#define WRITE_SC(p, r, v)    writeb((v), (p)->membase + WR_##r)
> 
> and these:
> 
> > +#define READ_SC_PORT(p, r)     read_sc_port(p, RD_PORT_##r)
> > +#define WRITE_SC_PORT(p, r, v) write_sc_port(p, WR_PORT_##r, v)
> 
> really don't need to exist.  All they do is make the code harder to read.

but they make the code safer. The chip has common register and port
registers, which are randomly splattered over the address range. And
some of them are read only, some write only. Read only and Write
only register live at the same register offset and their function
usually doesn't have anything in common. By using these macros I'll
get compile errors when doing a READ_SC from a write only register
and vice versa. I will also get compile errors, if I try to access a
common register via READ_SC_PORT/WRITE_SC_PORT. 

If there is a better way to get more readable code for you and
the safety I'd like, just tell me.

> Think of the poor reader who sees this:
> 
> 		status = READ_SC_PORT(port, SR);
> 
> and then goes madly searching for "SR".

which he then finds by this name in the data sheet. All the register
names are named as close to the datasheet as possible. And I consider
consulting datasheets when looking at drivers a pretty good idea.

> Code is written once and is read a thousand times.  Please optimise for
> reading.

it's no big deal to change that, but is getting bitten by stupid chips
worth it ? I'd prefer to get a compile error than debugging a runtime
error.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/