RE: [RFC][PATCH 9/13] Equinox SST driver: tty interface

[Straub, Michael]

Thanks for the comments.
I'll apply the suggested changes, not just here but to all of the source
files.

mike

-----Original Message-----
From: Randy.Dunlap [mailto:rdunlap@xxxxxxxxxxxx] 
Sent: Monday, July 03, 2006 7:53 PM
To: Straub, Michael
Cc: linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [RFC][PATCH 9/13] Equinox SST driver: tty interface

On Thu, 22 Jun 2006 09:20:28 -0400 Straub, Michael wrote:

> Adds Equinox multi-port serial (SST) driver.
> 
> Part 9: new source file: drivers/char/eqnx/sst_tty.c.  Provides the 
> standard TTY interface routines for the SST driver.


---
 sst_tty.c | 1961
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

 1 files changed, 1961 insertions(+)

Use "diffstat -p1 -w70" please.

diff -Naurp -X dontdiff linux-2.6.17/drivers/char/eqnx/sst_tty.c
linux-2.6.17.eqnx/drivers/char/eqnx/sst_tty.c
--- linux-2.6.17/drivers/char/eqnx/sst_tty.c	1969-12-31
19:00:00.000000000 -0500
+++ linux-2.6.17.eqnx/drivers/char/eqnx/sst_tty.c	2006-06-20
09:50:09.000000000 -0400
@@ -0,0 +1,1961 @@
+/*
+
+#include <linux/config.h>

don't need config.h (autoconf.h is automatically #included)

+#include <linux/version.h>
+
+#ifdef CONFIG_MODVERSIONS
+#define MODVERSIONS	1
+#endif

not used

+/* defer call to write_wakeup */
+int eqnx_write_wakeup_deferred = 0;

static?

+/* external variable and routines */
+/**********************************************************************
*****/

These should be in a header file:

+extern int eqnx_nssps;
+extern struct mpdev eqnx_dev[MAXSSP];
+extern struct mpchan *eqnx_chan;
+extern struct datascope eqnx_dscope[2];
+extern char *eqnx_tmpwritebuf;
+extern struct semaphore eqnx_tmpwritesem;
+
+extern void eqnx_megaparam(int);
+extern int eqnx_modem(int, int);
+extern void eqnx_frame_wait(struct mpchan *, int);
+extern void eqnx_chnl_sync(struct mpchan *);
+extern int SSTMINOR(unsigned int, unsigned int);
+extern u32 eqnx_get_modem_info(struct mpchan *);
+extern void eqnx_set_modem_info(struct mpchan *, unsigned int, unsigned
int,
+				struct tty_struct *);
+
+/*
+ * eqnx_open(tty, filp)
+ *
+ * Open of tty device associated with SST channel.
+ *
+ * tty	= pointer to tty structure being opened.
+ * filp	= file structure.
+ */

Might as well use kernel-doc format since it is almost there.
See Documentation/kernel-doc-nano-HOWTO.txt.
(multiple places)

+int eqnx_open(struct tty_struct *tty, struct file *filp)
+{

static?

+	/* validate channel number */
+	if ((d > (eqnx_nssps * MAXCHNL_BRD)) || (eqnx_nssps == 0) || (d
< 0))
+		return (-ENODEV);

no parens (multiple places)

+	if (tty->termios == NULL) {
+		*tty->termios = *mpc->normaltermios;
+	}

Don't use braces for 1-statement blocks. (multiple places)

+void eqnx_close(struct tty_struct *tty, struct file *filp)

static?

+{
+
+	/* channel validity checks */
+	if (tty == (struct tty_struct *)NULL)

Cast not needed, please drop it.

+		return;
+	mpc = (struct mpchan *)tty->driver_data;
+	if (mpc == (struct mpchan *)NULL)
+		return;

ditto (+ multiple other places)

+	d = SSTMINOR(mpc->mpc_major, mpc->mpc_minor);
+	if (d > (eqnx_nssps * MAXCHNL_BRD))
+		return;
+
+	if ((mpd = mpc->mpc_mpd) == (struct mpdev *)NULL)
+		return;

Drop the cast.  And preferably split the if-test into 2 lines of
assignment + if-test.

+	nchan = MPCHAN(d);
+	if ((mpd >= &eqnx_dev[eqnx_nssps]) || (!(mpd->mpd_alive)) ||
+	    (nchan >= (int)mpd->mpd_nchan))
+		return;
+}
+
+static void eqnx_flush_chars_locked(struct tty_struct *tty)
+{
+	struct mpchan *mpc;
+	struct mpdev *mpd;
+	unsigned int d;
+
...
+	if (eqnx_txcooktty == NULL)
+		return;

Don't need this else + braces:

+	else {
+
+#ifdef	DEBUG_LOCKS
+		if (!(spin_is_locked(&mpd->mpd_lock)))
+			dev_dbg(mpd->dev, "eqnx_flush_chars_locked: mpd
"
+				"lock !locked\n");
+#endif
+
+		if (mpd->mpd_board_def->asic == SSP64) {
+			/* SSP64 */
+			unsigned int cooksize = eqnx_txcooksize;
+			eqnx_txcooksize = 0;
+			eqnx_txcookrealsize = 0;
+			eqnx_txcooktty = (struct tty_struct *)NULL;
+			if (cooksize == 0)
+				return;
+			dev_dbg(mpd->dev, "eqnx_flush_chars_locked: "
+				"calling eqnx_write_SSP64\n");
+			eqnx_write_SSP64(mpc, eqnx_txcookbuf, cooksize);
+		} else {
+			/* SSP4 */
+			if (mpc->xmit_cnt == 0)
+				return;
+			dev_dbg(mpd->dev, "eqnx_flush_chars_locked: "
+				"calling eqnx_write_SSP4\n");
+			eqnx_write_SSP4(mpc, EQNX_COOKED);
+		}
+	}
+}
+
+void eqnx_flush_chars(struct tty_struct *tty)
+{
+	struct mpchan *mpc;
+	struct mpdev *mpd;
+	unsigned int d;
+	unsigned long flags;
+
...
+	if (eqnx_txcooktty == NULL)
+		return;

Drop the else + braces?

+	else {
+
+#ifdef	DEBUG_LOCKS
+		if (spin_is_locked(&mpd->mpd_lock))
+			dev_dbg(mpd->dev, "eqnx_flush_chars: mpd lock "
+				"already held\n");
+#endif
+
+		spin_lock_irqsave(&mpd->mpd_lock, flags);
+		eqnx_flush_chars_locked(tty);
+		spin_unlock_irqrestore(&mpd->mpd_lock, flags);
+
+		if (eqnx_write_wakeup_deferred) {
+			eqnx_write_wakeup_deferred = 0;
+			tty->ldisc.write_wakeup(tty);
+		}
+	}
+}
+
+void eqnx_throttle(struct tty_struct *tty)

static?

+{
+	struct mpchan *mpc;
+	struct mpdev *mpd;
+	int d;
+
+	/* channel validity checks */
+	if (tty == (struct tty_struct *)NULL)
+		return;
+	mpc = (struct mpchan *)tty->driver_data;
+	if (mpc == (struct mpchan *)NULL)
+		return;
+
+	d = SSTMINOR(mpc->mpc_major, mpc->mpc_minor);
+	if (d > (eqnx_nssps * MAXCHNL_BRD))
+		return;

About all of these validity checks:  is it actually possible
for all of these functions to be called when they shouldn't be?
There seems to be an over-abundance of these checks.

+	mpd = mpc->mpc_mpd;
+	dev_dbg(mpd->dev, "eqnx_throttle: device %d\n",
+		SSTMINOR(mpc->mpc_major, mpc->mpc_minor));
+	mpc->mpc_block = true;
+}
+
+void eqnx_flush_buffer_locked(struct tty_struct *tty)

static?

+{
+	struct mpchan *mpc;
+	struct mpdev *mpd;
+	volatile struct icp_in_struct *icpi;
+	volatile struct icp_out_struct *icpo;
+	volatile struct cout_que_struct *icpq;
+	volatile struct cin_bnk_struct *icpb;

Use of volatile is highly frowned on.
Please read these volatile emails from Linus:

http://www.x86-64.org/lists/discuss/msg07347.html
http://www.x86-64.org/lists/discuss/msg07358.html

+	u8 cin;
+	u16 nxt_dma;
+	int d;
+
...
+#ifdef	DEBUG_LOCKS
+	if (!(spin_is_locked(&mpd->mpd_lock)))
+		dev_dbg(mpd->dev, "eqnx_flush_buffer_locked: mpd lock
not "
+			"locked\n");
+#endif
+
+	icpi = mpc->mpc_icpi;
+	icpo = mpc->mpc_icpo;
+	icpq = &icpo->cout_q0;
+
+	dev_dbg(mpd->dev, "eqnx_flush_buffer_locked: device = %d\n", d);
+	if (tty == eqnx_txcooktty) {
+		eqnx_txcooktty = (struct tty_struct *)NULL;
+		eqnx_txcooksize = 0;
+		eqnx_txcookrealsize = 0;
+	}
+
+	/* flush transmit buffers */
+	icpo->cout_lck_cntrl |= LCK_Q_ACT;
+	eqnx_chnl_sync(mpc);
+	icpo->cout_dma_stat_save = 0;
+	icpo->cout_intnl_fifo_ptrs = 0;
+	SSTWR16(icpq->q_data_count, 0);
+	mpc->mpc_count = 0;
+	SSTWR16(icpq->q_data_ptr_l, (mpc->mpc_txq.q_begin +
mpc->mpc_txbase));
+	mpc->mpc_txq.q_ptr = mpc->mpc_txq.q_begin;
+	mpc->xmit_cnt = mpc->xmit_head = mpc->xmit_tail = 0;
+	SSTWR16(icpo->cout_attn_enbl, EN_REG_UPDT_EV);
+	icpo->cout_lck_cntrl &= ~LCK_Q_ACT;
+	mpc->mpc_flags &= ~MPC_BUSY;
+
+	/* flush receiver queue */
+	/* set rx ptr = tail */
+	icpb = (icpi->cin_locks & LOCK_A) ? &icpi->cin_bank_b :
+	    &icpi->cin_bank_a;
+	/* wait for valid registers */
+	if (!(SSTRD16(icpb->bank_events) & EV_REG_UPDT))
+		eqnx_frame_wait(mpc, 2);
+	cin = icpi->cin_locks;
+	icpi->cin_locks = cin | (DIS_BANK_A | DIS_BANK_B);
+	eqnx_chnl_sync(mpc);
+	icpi->cin_bank_a.bank_fifo_lvl &= ~0x8f;
+	icpi->cin_bank_b.bank_fifo_lvl &= ~0x8f;
+	nxt_dma = SSTRD16(icpb->bank_nxt_dma);
+	SSTWR16(icpi->cin_tail_ptr_a, nxt_dma);
+	SSTWR16(icpi->cin_tail_ptr_b, nxt_dma);
+	mpc->mpc_rxq.q_ptr = nxt_dma;
+	icpi->cin_locks = cin;
+	mpc->mpc_flags |= MPC_RXFLUSH;
+	mpc->mpc_block = false;
+	wake_up_interruptible(&tty->write_wait);
+
+	/* signal write wakeup when safe to call ldisc */
+	if ((tty->flags & (1 << TTY_DO_WRITE_WAKEUP)) &&
+	    tty->ldisc.write_wakeup)
+		eqnx_write_wakeup_deferred++;
+}
+
+void eqnx_set_termios(struct tty_struct *tty, struct termios *old)
+{

static?  or is it called from other modules?

...
+}
+
+static void chanoff(struct mpchan *mpc)
+{
+	volatile struct icp_in_struct *icpi;
+	volatile struct icp_out_struct *icpo;
+	volatile struct cout_que_struct *icpq;
+	volatile struct cin_bnk_struct *icpb;
+	int loop, ok;
+	u8 cin;
+	u16 events, nxt_dma, attn_ena;
+
+	icpi = mpc->mpc_icpi;
+	icpo = mpc->mpc_icpo;
+	icpq = &icpo->cout_q0;
+	icpb = (icpi->cin_locks & LOCK_A) ? &icpi->cin_bank_b :
+	    &icpi->cin_bank_a;
+	/* make sure registers are valid */
+	if (!(SSTRD16(icpb->bank_events) & EV_REG_UPDT))
+		eqnx_frame_wait(mpc, 2);
+
+	mpc->mpc_param &= (IOCTLLB | IOCTCTS | IOCTLCK | IOCTRTS);
+
+	/* disable events */
+	attn_ena = SSTRD16(icpi->cin_attn_ena);
+	attn_ena &= (ENA_DCD_CNG | ENA_CTS_CNG | ENA_DSR_CNG |
ENA_RI_CNG);
+	if (!(mpc->mpc_chan % 16))
+		attn_ena |= ENA_LMX_CNG;
+	SSTWR16(icpi->cin_attn_ena, attn_ena);
+
+	/* lock output */
+	icpo->cout_lck_cntrl |= LCK_Q_ACT;
+	eqnx_chnl_sync(mpc);
+	icpo->cout_intnl_fifo_ptrs = 0;
+	icpo->cout_dma_stat_save = 0;
+	SSTWR16(icpq->q_data_count, 0);
+	SSTWR16(icpq->q_data_ptr_l, (mpc->mpc_txbase +
mpc->mpc_txq.q_begin));
+	mpc->mpc_txq.q_ptr = mpc->mpc_txq.q_begin;
+	mpc->mpc_count = 0;
+	mpc->carr_state = false;
+
+	/* unlock output and update */
+	icpo->cout_lck_cntrl &= ~LCK_Q_ACT;
+	eqnx_chnl_sync(mpc);
+	if ((icpo->cout_intnl_svd_toggls & TOGGLS_LSEND) ==
+	    (icpo->cout_cpu_req & CPU_SND_REQ))
+		icpo->cout_cpu_req ^= CPU_SND_REQ;
+
+	/* wait for ack */
+	loop = 0;
+	ok = false;
+	while (++loop < 100000) {

This should use some kind of timing/timer, not just a counter.

+		if (SSTRD16(icpo->cout_status) & TXSR_EV_B_ACT)
+			events = SSTRD16(icpo->cout_events_b);
+		else
+			events = SSTRD16(icpo->cout_events_a);
+		if (events & EV_CPU_REQ_DN) {
+			ok = true;
+			break;
+		}
+	}
+
+	if (!ok)
+		dev_warn(mpc->mpc_mpd->dev, "chanoff: cpu_req ack
failed.\n");
...
+}
+
+/*
+ * delay_jiffies(len)
+ *
+ * Delay by the specified number of jiffies.
+ *
+ * len	= jiffies to delay.
+ */
+static void delay_jiffies(int len)
+{
+	if (len > 0) {
+		msleep(jiffies_to_msecs(len));
+	}

No braces on 1-statement blocks.

+}


---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/