Re: [PATCH v2 5/5] tty/serial: Add earlycon support for Spreadtrum serial driver

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

On Mon, 20 Oct 2014 18:23:38 +0800
Orson Zhai <orsonzhai@gmail.com> wrote:

> On Sun, Oct 19, 2014 at 5:06 AM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
> > On Fri, 17 Oct 2014 17:54:25 +0800
> > Chunyan Zhang <chunyan.zhang@spreadtrum.com> wrote:
> >
> >> Add serial driver for spreadtrum sharkl platform with earlycon
> >> support at first.
> >
> >> +#define UART_TXD     0x0000
> >> +#define UART_RXD     0x0004
> >> +#define UART_STS0    0x0008
> >> +#define UART_STS1    0x000C
> >> +#define UART_IEN     0x0010
> >> +#define UART_ICLR    0x0014
> >> +#define UART_CTL0    0x0018
> >> +#define UART_CTL1    0x001C
> >> +#define UART_CTL2    0x0020
> >> +#define UART_CLKD0   0x0024
> >> +#define UART_CLKD1   0x0028
> >> +#define UART_STS2    0x002C
> >> +
> >> +/*line status */
> >> +#define UART_LSR_TX_OVER     (0x1<<15)
> >
> > Given we use UART_ for all the 8250 defines it might be better to use
> > something else - SHARK_LSR_TX_OVER etc to avoid future confusion
> >
> 
> Does it matter if those macro are only used in the specific c file?
> >From my point of  view, private register macro could be treated as
> static definitions like static variable or functions in C file.
> I also noted that many people use a prefix for the other
> manufacturers' macro definition in source file .
> I have no intention to break the habit of kernel but just for discussion.

Your problem is this. In the kernel headers are definitions like

include/uapi/linux/serial_reg.h:#define UART_TX 0	/* Out: Transmit
buffer */

you are adding identical (but conflicting in places) defines. If some
random change in the include files causes serial_reg.h to be included by
a header you include then it will break.

In addition people habitually use tools like LXR to figure out where
defines and functions are. Having these conflicting definitions will
cause confusion. Better they have prefixes.

Alan