Re: [RFC][PATCH 6/13] Equinox SST driver:hardware registers

[Andrew Morton <akpm@osdl.org>]

On Thu, 22 Jun 2006 09:16:35 -0400
"Straub, Michael" <Michael.Straub@avocent.com> wrote:

> Adds Equinox multi-port serial (SST) driver.
> 

hm, I missed this patch series.

> + * Each ICP provides a set of input and output registers per channel.
> + * Input registers for receiving data and output registers for
> trasmitting.
> + * In addition, there are also a set of global registers per board
> which are
> + * used for general configuration and also contain the global attention
> bits.

As Randy says, these patches were exorbitantly, unusable wordwrapped.

> +struct icp_gbl_struct {
> +	u8	filler1[24];
> +	u8	gicp_bus_cntrl;		/* 0x18: bus control */
> +	u8	gicp_rev;		/* 0x19: icp revision */
> +	u8	filler2[2];
> +	u8	gicp_initiate;		/* 0x1c: lmx control & icp
> enable */
> +	u8	gicp_scan_spd;		/* 0x1d: lmx scan speeds */
> +	u8	gicp_tmr_size;		/* 0x1e: interval timer scale
> preset */
> +	u8	gicp_tmr_count;		/* 0x1f: interval timer count
> preset */
> +	u8	filler3[24];
> +	u8	gicp_watchdog;		/* 0x38: watchdog timer */
> +	u8	filler4[3];
> +	u8	gicp_attn;		/* 0x3c: global status */
> +	u8	gicp_chan;		/* 0x3d: current channel number
> */
> +	u16	gicp_frame_ctr;		/* 0x3e: frame counter */
> +	u8	filler5[24];
> +	u8	gicp_rcv_attn[8];	/* 0x58: receive attention bits
> */
> +	u8	filler6[24];
> +	u8	gicp_xmit_attn[8];	/* 0x78: transmit attention bits
> */
> +};
>
> ...
>
> +struct ssp4_gbl_struct {
> +	u8	bus_cntrl;		/* 0x0: global control register
> */
> +	u8	rev;			/* 0x1: revision Level number */
> +	u8	on_line;		/* 0x2: on-line */
> +	u8	filler1;
> +	u8	chan_ctr;		/* 0x4: active channel number */
> +	u8	filler2;
> +	u8	chan_attn;		/* 0x6: channel attention bits
> */
> +	u8	tmr_evnt;		/* 0x7: timer event bits */
> +	u8	filler3[17];
> +	u8	type;			/* 0x19: board type */
> +	u8	filler4[34];
> +	u8	attn_pend;		/* 0x3c: channel attention */
> +};
> +
>
> ...
>
> +struct cin_bnk_struct {
> +	u16	bank_nxt_dma;		/* 0x0: offset to next in dma */
> +	u8	bank_fifo_lvl;		/* 0x2: char count held in fifo
> */
> +	u8	bank_tags_l;		/* 0x3: input tags, low byte */
> +	u16	bank_signals;		/* 0x4: channel input signals */
> +	u8	filler0;
> +	u8	bank_tags_h;		/* 0x7: input tags, high byte */
> +	u8	bank_fifo[8];		/* 0x8: input 8 char fifo */
> +	u16	bank_num_chars;		/* 0x10: char count received */
> +	u16	bank_events;		/* 0x12: input events detected
> */
> +};
>
> [ etc ]
>

What are these?  Do they describe chip register sets?

If so, there's a tight compiler dependency here.  I don't know whether any
version of the compiler for any architecture will muck around adding
padding in here.  There's a risk, I guess.

> +
> +/* return active input queue tail pointer */
> +#define GET_TAIL() \
> +{ \
> +	if (icpi->cin_q_cntrl & TAIL_PTR_B) \
> +		return (SSTRD16(icpi->cin_tail_ptr_b)); \
> +	else \
> +		return (SSTRD16(icpi->cin_tail_ptr_a)); \
> +}

Please, no.

- It's a macro qhich requires that the caller have a local variable named
  `icpi'.  At a minimum, the name of the pointer should be an arg to the
  macro.

- The macro hides a `return' statement.  That's a show-stopper.

I suggest this be reimplemented as a function.  Possibly inlined.

> +/* set input queue tail pointer */
> +#define SET_TAIL(val) \
> +{ \
> +        if (icpi->cin_q_cntrl & TAIL_PTR_B) \
> +		SSTWR16(icpi->cin_tail_ptr_a, (val)); \
> +	else \
> +		SSTWR16(icpi->cin_tail_ptr_b, (val)); \
> +	icpi->cin_q_cntrl ^= TAIL_PTR_B; \
> +}

This also can be a regular C function.

> +/* freeze active input register bank */
> +#define FREEZ_BANK(mpc) \
> +{ \
> +	u16 cie = CHAN_ATTN_SET | SSTRD16(icpi->cin_attn_ena); \
> +	int frztimeo = 0; \
> +	u8 lcks = 0; \
> +	SSTWR16(icpi->cin_attn_ena, 0); \
> +	if ((icpi->cin_locks & DIS_BANK_A) == DIS_BANK_A) { \
> +		/* Bank A is active (locked) */ \
> +		icpb = &icpi->cin_bank_b; \
> +		lcks = BANK_B_ACT; \
> +	} else  \
> +		/* Bank B is active (locked) */ \
> +		icpb = &icpi->cin_bank_a; \
> +	if (!(SSTRD16(icpb->bank_events) & EV_REG_UPDT)) { \
> +		while ((icpi->cin_intern_flgs & 0x80) != lcks) \
> +			if (++frztimeo > 8000) break; \
> +	}  \
> +	mpc->mpc_icpb = icpb; \
> +	icpi->cin_locks ^= (DIS_BANK_A | DIS_BANK_B); /* flip banks */ \
> +	eqnx_chnl_sync(mpc); \
> +	mpc->mpc_cin_events |= SSTRD16(icpb->bank_events); \
> +	SSTWR16(icpb->bank_events, 0); \
> +	SSTWR16(icpi->cin_attn_ena, cie); \
> +}

No way, sorry.  Implement as a function.

> +/* get and return output events for the channel */
> +#define TX_EVENTS(x, mpc) \
> +{ \
> +	volatile u16 csr = SSTRD16(icpo->cout_status); \
> +	volatile u16 oie = SSTRD16(icpo->cout_attn_enbl); \
> +	SSTWR16(icpo->cout_attn_enbl, 0); \
> +	if (csr & TXSR_EV_B_ACT) { \
> +		icpo->cout_lck_cntrl ^= (LCK_EVT_A | LCK_EVT_B); \
> +		eqnx_chnl_sync(mpc); \
> +		(x) |= SSTRD16(icpo->cout_events_b); \
> +		SSTWR16(icpo->cout_events_b, 0); \
> +	} else { \
> +		icpo->cout_lck_cntrl ^= (LCK_EVT_A | LCK_EVT_B); \
> +		eqnx_chnl_sync(mpc); \
> +		(x) |= SSTRD16(icpo->cout_events_a); \
> +		SSTWR16(icpo->cout_events_a, 0); \
> +	} \
> +	if ((x) & EV_TX_EMPTY_Q0) \
> +		oie &= ~ENA_TX_EMPTY_Q0;\
> +	if ((x) & EV_TX_LOW_Q0) \
> +		oie &= ~ENA_TX_LOW_Q0;\
> +	SSTWR16(icpo->cout_attn_enbl, oie); \
> +}

Ditto.


> +/* returns outbound control signals for channel */
> +#define GET_CTRL_SIGS(mpc,val) val =
> SSTRD16(mpc->mpc_icpo->cout_cntrl_sig);
>
> +/* sets outbound control signals for channel */
> +#define SET_CTRL_SIGS(mpc, val)
> SSTWR16((mpc->mpc_icpo)->cout_cntrl_sig, val);

That's

> +#define        SSTRD16(x)      (cpu_to_le16(x))

Please remove this macro - just open-code the cpu_to_le16() everywhere
(edit the diffs..)

And please review all patches for excess parenthesisation and fix that up.