linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: tsbogend@alpha.franken.de (Thomas Bogendoerfer)
To: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org, linux-mips@linux-mips.org,
	Andy Whitcroft <apw@shadowen.org>,
	Alan Cox <alan@lxorguk.ukuu.org.uk>
Subject: Re: [PATCH] SC26XX: New serial driver for SC2681 uarts
Date: Wed, 5 Dec 2007 10:25:06 +0100	[thread overview]
Message-ID: <20071205092506.GA6691@alpha.franken.de> (raw)
In-Reply-To: <20071204192738.54e79a97.akpm@linux-foundation.org>

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 ]

  reply	other threads:[~2007-12-05  9:32 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-12-02 19:43 [PATCH] SC26XX: New serial driver for SC2681 uarts Thomas Bogendoerfer
2007-12-03 23:53 ` Andrew Morton
2007-12-03 23:57   ` Arjan van de Ven
2007-12-04  8:25     ` Thomas Bogendoerfer
2007-12-04 11:01       ` Geert Uytterhoeven
2007-12-04 12:45       ` Alan Cox
2007-12-04 23:57         ` Thomas Bogendoerfer
2007-12-04 23:41   ` Thomas Bogendoerfer
2007-12-05  3:27     ` Andrew Morton
2007-12-05  9:25       ` Thomas Bogendoerfer [this message]
2007-12-22  9:47         ` Pekka Enberg
2007-12-21 17:36   ` Andy Whitcroft

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20071205092506.GA6691@alpha.franken.de \
    --to=tsbogend@alpha.franken.de \
    --cc=akpm@linux-foundation.org \
    --cc=alan@lxorguk.ukuu.org.uk \
    --cc=apw@shadowen.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).