[PATCH 1/3] max3100: added raise_threaded_irq

[PATCH 1/3] max3100: added raise_threaded_irq

[Christian Pellegrin]

raise_threaded_irq schedules the execution of an interrupt thread

Signed-off-by: Christian Pellegrin <chripell-VaTbYqLCNhc@public.gmane.org>
---
 include/linux/interrupt.h |    3 +++
 kernel/irq/manage.c       |   27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..14c0c13 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -144,6 +144,9 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
 static inline void exit_irq_thread(void) { }
 #endif
 
+extern int raise_threaded_irq(unsigned int irq);
+
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb6078c..a7d21e0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1088,3 +1088,30 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return retval;
 }
 EXPORT_SYMBOL(request_threaded_irq);
+
+/**
+ *	raise_threaded_irq - triggers a threded interrupt
+ *	@irq: Interrupt line to trigger
+ */
+int raise_threaded_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	struct irqaction *action;
+
+	if (!desc)
+		return -ENOENT;
+	action = desc->action;
+	if (!action)
+		return -ENOENT;
+	if (unlikely(!action->thread_fn))
+		return -EINVAL;
+	if (likely(!test_bit(IRQTF_DIED,
+			     &action->thread_flags))) {
+		set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
+		wake_up_process(action->thread);
+	} else {
+		return -ECHILD;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(raise_threaded_irq);
-- 
1.5.6.5


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

[PATCH 1/3] max3100: added raise_threaded_irq

[Christian Pellegrin]

raise_threaded_irq schedules the execution of an interrupt thread

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 include/linux/interrupt.h |    3 +++
 kernel/irq/manage.c       |   27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 75f3f00..14c0c13 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -144,6 +144,9 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
 static inline void exit_irq_thread(void) { }
 #endif
 
+extern int raise_threaded_irq(unsigned int irq);
+
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb6078c..a7d21e0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1088,3 +1088,30 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	return retval;
 }
 EXPORT_SYMBOL(request_threaded_irq);
+
+/**
+ *	raise_threaded_irq - triggers a threded interrupt
+ *	@irq: Interrupt line to trigger
+ */
+int raise_threaded_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	struct irqaction *action;
+
+	if (!desc)
+		return -ENOENT;
+	action = desc->action;
+	if (!action)
+		return -ENOENT;
+	if (unlikely(!action->thread_fn))
+		return -EINVAL;
+	if (likely(!test_bit(IRQTF_DIED,
+			     &action->thread_flags))) {
+		set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
+		wake_up_process(action->thread);
+	} else {
+		return -ECHILD;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(raise_threaded_irq);
-- 
1.5.6.5

[PATCH 0/3] max3100: improvements

[christian pellegrin]

Hi all,

first of all I'm sending this patch to all the people that were in CC
to the original Feng's email. Let me know if this bothers you.

This bunch of patches should solve the problems noted by Feng to the
max3100 driver. It was tested on an S3C2440 system which has a rather
bad SPI master controller. The efficiency is very good on zmodem
receive (near 100% at 115200) and not so bad on zmodem send (around
95% at 115200). I guess the reason is the TX buffer of the MAX3100
being just one byte deep. All the tests were done with two MAX3100
running on the same SPI bus: the first one as a console and under
test, the second one happily receiving a 4800 bit/s NMEA stream from a
GPS (with a process checking we don't get wrong sentences). I also did
the simple "cut&paste a screenful of charters" test and it worked! The
console was stressed as much I could (things like putting the S3C to
suspend during a zmodem transfer and checking it restarts after
resume).

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

[PATCH 2/3] max3100: moved to threaded interrupt

[Christian Pellegrin]

The driver was reworked to use threaded interrupts instead of workqueus.
This is a major boost in performance because the former are scheduled as
SCHED_FIFO processes. As a side effect suspend/resume code was greatly
simplified.

Signed-off-by: Christian Pellegrin <chripell-VaTbYqLCNhc@public.gmane.org>
---
 drivers/serial/max3100.c |  102 ++++++++++++++-------------------------------
 1 files changed, 32 insertions(+), 70 deletions(-)

diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 3c30c56..0c972c6 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -45,7 +45,9 @@
 #include <linux/serial_core.h>
 #include <linux/serial.h>
 #include <linux/spi/spi.h>
-#include <linux/freezer.h>
+#include <linux/kthread.h>
+#include <linux/interrupt.h>
+#include <linux/bitops.h>
 
 #include <linux/serial_max3100.h>
 
@@ -113,18 +115,15 @@ struct max3100_port {
 
 	int irq;		/* irq assigned to the max3100 */
 
+	/* the workqueue is needed because we cannot schedule
+	   a threaded interrupt during PM
+	 */
+	struct work_struct resume_work;
+
 	int minor;		/* minor number */
 	int crystal;		/* 1 if 3.6864Mhz crystal 0 for 1.8432 */
 	int loopback;		/* 1 if we are in loopback mode */
 
-	/* for handling irqs: need workqueue since we do spi_sync */
-	struct workqueue_struct *workqueue;
-	struct work_struct work;
-	/* set to 1 to make the workhandler exit as soon as possible */
-	int  force_end_work;
-	/* need to know we are suspending to avoid deadlock on workqueue */
-	int suspending;
-
 	/* hook for suspending MAX3100 via dedicated pin */
 	void (*max3100_hw_suspend) (int suspend);
 
@@ -171,13 +170,12 @@ static void max3100_calc_parity(struct max3100_port *s, u16 *c)
 		*c |= max3100_do_parity(s, *c) << 8;
 }
 
-static void max3100_work(struct work_struct *w);
-
-static void max3100_dowork(struct max3100_port *s)
+static void max3100_resume_work(struct work_struct *w)
 {
-	if (!s->force_end_work && !work_pending(&s->work) &&
-	    !freezing(current) && !s->suspending)
-		queue_work(s->workqueue, &s->work);
+	struct max3100_port *s = container_of(w, struct max3100_port,
+					      resume_work);
+
+	raise_threaded_irq(s->irq);
 }
 
 static void max3100_timeout(unsigned long data)
@@ -185,7 +183,7 @@ static void max3100_timeout(unsigned long data)
 	struct max3100_port *s = (struct max3100_port *)data;
 
 	if (s->port.state) {
-		max3100_dowork(s);
+		raise_threaded_irq(s->irq);
 		mod_timer(&s->timer, jiffies + s->poll_time);
 	}
 }
@@ -255,9 +253,9 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
 	return ret;
 }
 
-static void max3100_work(struct work_struct *w)
+static irqreturn_t max3100_ist(int irq, void *dev_id)
 {
-	struct max3100_port *s = container_of(w, struct max3100_port, work);
+	struct max3100_port *s = dev_id;
 	int rxchars;
 	u16 tx, rx;
 	int conf, cconf, rts, crts;
@@ -314,23 +312,14 @@ static void max3100_work(struct work_struct *w)
 		if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 			uart_write_wakeup(&s->port);
 
-	} while (!s->force_end_work &&
-		 !freezing(current) &&
+	} while (!kthread_should_stop() &&
 		 ((rx & MAX3100_R) ||
 		  (!uart_circ_empty(xmit) &&
 		   !uart_tx_stopped(&s->port))));
 
 	if (rxchars > 0 && s->port.state->port.tty != NULL)
 		tty_flip_buffer_push(s->port.state->port.tty);
-}
-
-static irqreturn_t max3100_irq(int irqno, void *dev_id)
-{
-	struct max3100_port *s = dev_id;
-
-	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	max3100_dowork(s);
 	return IRQ_HANDLED;
 }
 
@@ -353,7 +342,7 @@ static void max3100_start_tx(struct uart_port *port)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 }
 
 static void max3100_stop_rx(struct uart_port *port)
@@ -369,7 +358,7 @@ static void max3100_stop_rx(struct uart_port *port)
 	s->conf &= ~MAX3100_RM;
 	s->conf_commit = 1;
 	spin_unlock(&s->conf_lock);
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 }
 
 static unsigned int max3100_tx_empty(struct uart_port *port)
@@ -381,7 +370,7 @@ static unsigned int max3100_tx_empty(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	/* may not be truly up-to-date */
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	return s->tx_empty;
 }
 
@@ -394,7 +383,7 @@ static unsigned int max3100_get_mctrl(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	/* may not be truly up-to-date */
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	/* always assert DCD and DSR since these lines are not wired */
 	return (s->cts ? TIOCM_CTS : 0) | TIOCM_DSR | TIOCM_CAR;
 }
@@ -414,7 +403,7 @@ static void max3100_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (s->rts != rts) {
 		s->rts = rts;
 		s->rts_commit = 1;
-		max3100_dowork(s);
+		raise_threaded_irq(s->irq);
 	}
 	spin_unlock(&s->conf_lock);
 }
@@ -528,7 +517,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 			MAX3100_STATUS_PE | MAX3100_STATUS_FE |
 			MAX3100_STATUS_OE;
 
-	/* we are sending char from a workqueue so enable */
+	/* we are sending char from a threded irq so enable */
 	s->port.state->port.tty->low_latency = 1;
 
 	if (s->poll_time > 0)
@@ -541,7 +530,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 	s->conf_commit = 1;
 	s->parity = parity;
 	spin_unlock(&s->conf_lock);
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 
 	if (UART_ENABLE_MS(&s->port, termios->c_cflag))
 		max3100_enable_ms(&s->port);
@@ -555,19 +544,11 @@ static void max3100_shutdown(struct uart_port *port)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	if (s->suspending)
-		return;
-
-	s->force_end_work = 1;
-
 	if (s->poll_time > 0)
 		del_timer_sync(&s->timer);
 
-	if (s->workqueue) {
-		flush_workqueue(s->workqueue);
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
-	}
+	flush_work(&s->resume_work);
+
 	if (s->irq)
 		free_irq(s->irq, s);
 
@@ -587,7 +568,6 @@ static int max3100_startup(struct uart_port *port)
 	struct max3100_port *s = container_of(port,
 					      struct max3100_port,
 					      port);
-	char b[12];
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -595,27 +575,15 @@ static int max3100_startup(struct uart_port *port)
 	s->baud = s->crystal ? 230400 : 115200;
 	s->rx_enabled = 1;
 
-	if (s->suspending)
-		return 0;
-
-	s->force_end_work = 0;
 	s->parity = 0;
 	s->rts = 0;
 
-	sprintf(b, "max3100-%d", s->minor);
-	s->workqueue = create_freezeable_workqueue(b);
-	if (!s->workqueue) {
-		dev_warn(&s->spi->dev, "cannot create workqueue\n");
-		return -EBUSY;
-	}
-	INIT_WORK(&s->work, max3100_work);
+	INIT_WORK(&s->resume_work, max3100_resume_work);
 
-	if (request_irq(s->irq, max3100_irq,
+	if (request_threaded_irq(s->irq, NULL, max3100_ist,
 			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
-		dev_warn(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
+		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
 		s->irq = 0;
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
 		return -EBUSY;
 	}
 
@@ -628,7 +596,7 @@ static int max3100_startup(struct uart_port *port)
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(0);
 	s->conf_commit = 1;
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	/* wait for clock to settle */
 	msleep(50);
 
@@ -857,9 +825,6 @@ static int max3100_suspend(struct spi_device *spi, pm_message_t state)
 
 	disable_irq(s->irq);
 
-	s->suspending = 1;
-	uart_suspend_port(&max3100_uart_driver, &s->port);
-
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(1);
 	else {
@@ -880,14 +845,11 @@ static int max3100_resume(struct spi_device *spi)
 
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(0);
-	uart_resume_port(&max3100_uart_driver, &s->port);
-	s->suspending = 0;
 
 	enable_irq(s->irq);
-
+	/* must reconfigure if power was cut-off the chip during suspend */
 	s->conf_commit = 1;
-	if (s->workqueue)
-		max3100_dowork(s);
+	schedule_work(&s->resume_work);
 
 	return 0;
 }
-- 
1.5.6.5


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

[PATCH 2/3] max3100: moved to threaded interrupt

[Christian Pellegrin]

The driver was reworked to use threaded interrupts instead of workqueus.
This is a major boost in performance because the former are scheduled as
SCHED_FIFO processes. As a side effect suspend/resume code was greatly
simplified.

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 drivers/serial/max3100.c |  102 ++++++++++++++-------------------------------
 1 files changed, 32 insertions(+), 70 deletions(-)

diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 3c30c56..0c972c6 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -45,7 +45,9 @@
 #include <linux/serial_core.h>
 #include <linux/serial.h>
 #include <linux/spi/spi.h>
-#include <linux/freezer.h>
+#include <linux/kthread.h>
+#include <linux/interrupt.h>
+#include <linux/bitops.h>
 
 #include <linux/serial_max3100.h>
 
@@ -113,18 +115,15 @@ struct max3100_port {
 
 	int irq;		/* irq assigned to the max3100 */
 
+	/* the workqueue is needed because we cannot schedule
+	   a threaded interrupt during PM
+	 */
+	struct work_struct resume_work;
+
 	int minor;		/* minor number */
 	int crystal;		/* 1 if 3.6864Mhz crystal 0 for 1.8432 */
 	int loopback;		/* 1 if we are in loopback mode */
 
-	/* for handling irqs: need workqueue since we do spi_sync */
-	struct workqueue_struct *workqueue;
-	struct work_struct work;
-	/* set to 1 to make the workhandler exit as soon as possible */
-	int  force_end_work;
-	/* need to know we are suspending to avoid deadlock on workqueue */
-	int suspending;
-
 	/* hook for suspending MAX3100 via dedicated pin */
 	void (*max3100_hw_suspend) (int suspend);
 
@@ -171,13 +170,12 @@ static void max3100_calc_parity(struct max3100_port *s, u16 *c)
 		*c |= max3100_do_parity(s, *c) << 8;
 }
 
-static void max3100_work(struct work_struct *w);
-
-static void max3100_dowork(struct max3100_port *s)
+static void max3100_resume_work(struct work_struct *w)
 {
-	if (!s->force_end_work && !work_pending(&s->work) &&
-	    !freezing(current) && !s->suspending)
-		queue_work(s->workqueue, &s->work);
+	struct max3100_port *s = container_of(w, struct max3100_port,
+					      resume_work);
+
+	raise_threaded_irq(s->irq);
 }
 
 static void max3100_timeout(unsigned long data)
@@ -185,7 +183,7 @@ static void max3100_timeout(unsigned long data)
 	struct max3100_port *s = (struct max3100_port *)data;
 
 	if (s->port.state) {
-		max3100_dowork(s);
+		raise_threaded_irq(s->irq);
 		mod_timer(&s->timer, jiffies + s->poll_time);
 	}
 }
@@ -255,9 +253,9 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
 	return ret;
 }
 
-static void max3100_work(struct work_struct *w)
+static irqreturn_t max3100_ist(int irq, void *dev_id)
 {
-	struct max3100_port *s = container_of(w, struct max3100_port, work);
+	struct max3100_port *s = dev_id;
 	int rxchars;
 	u16 tx, rx;
 	int conf, cconf, rts, crts;
@@ -314,23 +312,14 @@ static void max3100_work(struct work_struct *w)
 		if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 			uart_write_wakeup(&s->port);
 
-	} while (!s->force_end_work &&
-		 !freezing(current) &&
+	} while (!kthread_should_stop() &&
 		 ((rx & MAX3100_R) ||
 		  (!uart_circ_empty(xmit) &&
 		   !uart_tx_stopped(&s->port))));
 
 	if (rxchars > 0 && s->port.state->port.tty != NULL)
 		tty_flip_buffer_push(s->port.state->port.tty);
-}
-
-static irqreturn_t max3100_irq(int irqno, void *dev_id)
-{
-	struct max3100_port *s = dev_id;
-
-	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	max3100_dowork(s);
 	return IRQ_HANDLED;
 }
 
@@ -353,7 +342,7 @@ static void max3100_start_tx(struct uart_port *port)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 }
 
 static void max3100_stop_rx(struct uart_port *port)
@@ -369,7 +358,7 @@ static void max3100_stop_rx(struct uart_port *port)
 	s->conf &= ~MAX3100_RM;
 	s->conf_commit = 1;
 	spin_unlock(&s->conf_lock);
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 }
 
 static unsigned int max3100_tx_empty(struct uart_port *port)
@@ -381,7 +370,7 @@ static unsigned int max3100_tx_empty(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	/* may not be truly up-to-date */
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	return s->tx_empty;
 }
 
@@ -394,7 +383,7 @@ static unsigned int max3100_get_mctrl(struct uart_port *port)
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
 	/* may not be truly up-to-date */
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	/* always assert DCD and DSR since these lines are not wired */
 	return (s->cts ? TIOCM_CTS : 0) | TIOCM_DSR | TIOCM_CAR;
 }
@@ -414,7 +403,7 @@ static void max3100_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (s->rts != rts) {
 		s->rts = rts;
 		s->rts_commit = 1;
-		max3100_dowork(s);
+		raise_threaded_irq(s->irq);
 	}
 	spin_unlock(&s->conf_lock);
 }
@@ -528,7 +517,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 			MAX3100_STATUS_PE | MAX3100_STATUS_FE |
 			MAX3100_STATUS_OE;
 
-	/* we are sending char from a workqueue so enable */
+	/* we are sending char from a threded irq so enable */
 	s->port.state->port.tty->low_latency = 1;
 
 	if (s->poll_time > 0)
@@ -541,7 +530,7 @@ max3100_set_termios(struct uart_port *port, struct ktermios *termios,
 	s->conf_commit = 1;
 	s->parity = parity;
 	spin_unlock(&s->conf_lock);
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 
 	if (UART_ENABLE_MS(&s->port, termios->c_cflag))
 		max3100_enable_ms(&s->port);
@@ -555,19 +544,11 @@ static void max3100_shutdown(struct uart_port *port)
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
-	if (s->suspending)
-		return;
-
-	s->force_end_work = 1;
-
 	if (s->poll_time > 0)
 		del_timer_sync(&s->timer);
 
-	if (s->workqueue) {
-		flush_workqueue(s->workqueue);
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
-	}
+	flush_work(&s->resume_work);
+
 	if (s->irq)
 		free_irq(s->irq, s);
 
@@ -587,7 +568,6 @@ static int max3100_startup(struct uart_port *port)
 	struct max3100_port *s = container_of(port,
 					      struct max3100_port,
 					      port);
-	char b[12];
 
 	dev_dbg(&s->spi->dev, "%s\n", __func__);
 
@@ -595,27 +575,15 @@ static int max3100_startup(struct uart_port *port)
 	s->baud = s->crystal ? 230400 : 115200;
 	s->rx_enabled = 1;
 
-	if (s->suspending)
-		return 0;
-
-	s->force_end_work = 0;
 	s->parity = 0;
 	s->rts = 0;
 
-	sprintf(b, "max3100-%d", s->minor);
-	s->workqueue = create_freezeable_workqueue(b);
-	if (!s->workqueue) {
-		dev_warn(&s->spi->dev, "cannot create workqueue\n");
-		return -EBUSY;
-	}
-	INIT_WORK(&s->work, max3100_work);
+	INIT_WORK(&s->resume_work, max3100_resume_work);
 
-	if (request_irq(s->irq, max3100_irq,
+	if (request_threaded_irq(s->irq, NULL, max3100_ist,
 			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
-		dev_warn(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
+		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
 		s->irq = 0;
-		destroy_workqueue(s->workqueue);
-		s->workqueue = NULL;
 		return -EBUSY;
 	}
 
@@ -628,7 +596,7 @@ static int max3100_startup(struct uart_port *port)
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(0);
 	s->conf_commit = 1;
-	max3100_dowork(s);
+	raise_threaded_irq(s->irq);
 	/* wait for clock to settle */
 	msleep(50);
 
@@ -857,9 +825,6 @@ static int max3100_suspend(struct spi_device *spi, pm_message_t state)
 
 	disable_irq(s->irq);
 
-	s->suspending = 1;
-	uart_suspend_port(&max3100_uart_driver, &s->port);
-
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(1);
 	else {
@@ -880,14 +845,11 @@ static int max3100_resume(struct spi_device *spi)
 
 	if (s->max3100_hw_suspend)
 		s->max3100_hw_suspend(0);
-	uart_resume_port(&max3100_uart_driver, &s->port);
-	s->suspending = 0;
 
 	enable_irq(s->irq);
-
+	/* must reconfigure if power was cut-off the chip during suspend */
 	s->conf_commit = 1;
-	if (s->workqueue)
-		max3100_dowork(s);
+	schedule_work(&s->resume_work);
 
 	return 0;
 }
-- 
1.5.6.5

[PATCH 3/3] max3100: adds console support for MAX3100

[Christian Pellegrin]

This patch adds console support for the MAX3100 UART
(console=ttyMAX0,11500). The SPI subsystem and an
suitable SPI master driver have to be compiled in the kernel.

Signed-off-by: Christian Pellegrin <chripell-VaTbYqLCNhc@public.gmane.org>
---
 drivers/serial/Kconfig   |   20 +++++
 drivers/serial/max3100.c |  184 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 187 insertions(+), 17 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9ff47db..bc72e84 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -547,6 +547,26 @@ config SERIAL_MAX3100
 	help
 	  MAX3100 chip support
 
+config SERIAL_MAX3100_CONSOLE
+	bool "Support console on MAX3100"
+	depends on SERIAL_MAX3100=y
+	select SERIAL_CORE_CONSOLE
+	help
+	  Console on MAX3100
+
+config SERIAL_MAX3100_CONSOLE_N
+	int "Number of MAX3100 chips to wait before registering console"
+	depends on SERIAL_MAX3100_CONSOLE=y
+	default "1"
+	help
+	  Unfortunately due to the async nature of Linux boot we must
+	  know in advance when to register the console. If we do it
+	  too early not all the MAX3100 chips are known to the system
+	  yet. And we just cannot know how many MAX3100 will be in the
+	  system so it's impossible to wait for the last one.  If you
+	  just want to have the console on the first MAX3100 chip
+	  probed (ttyMAX0) it's safe to leave this to 1.
+
 config SERIAL_DZ
 	bool "DECstation DZ serial driver"
 	depends on MACH_DECSTATION && 32BIT
diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 0c972c6..f6d250d 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -48,6 +48,7 @@
 #include <linux/kthread.h>
 #include <linux/interrupt.h>
 #include <linux/bitops.h>
+#include <linux/console.h>
 
 #include <linux/serial_max3100.h>
 
@@ -131,6 +132,10 @@ struct max3100_port {
 	int poll_time;
 	/* and its timer */
 	struct timer_list	timer;
+
+	int console_flags;
+	/* is this port a console? */
+#define MAX3100_IS_CONSOLE   (1 << 0)
 };
 
 static struct max3100_port *max3100s[MAX_MAX3100]; /* the chips */
@@ -175,7 +180,8 @@ static void max3100_resume_work(struct work_struct *w)
 	struct max3100_port *s = container_of(w, struct max3100_port,
 					      resume_work);
 
-	raise_threaded_irq(s->irq);
+	if (s->irq)
+		raise_threaded_irq(s->irq);
 }
 
 static void max3100_timeout(unsigned long data)
@@ -552,14 +558,16 @@ static void max3100_shutdown(struct uart_port *port)
 	if (s->irq)
 		free_irq(s->irq, s);
 
-	/* set shutdown mode to save power */
-	if (s->max3100_hw_suspend)
-		s->max3100_hw_suspend(1);
-	else  {
-		u16 tx, rx;
+	if (!(s->console_flags & MAX3100_IS_CONSOLE)) {
+		/* set shutdown mode to save power */
+		if (s->max3100_hw_suspend)
+			s->max3100_hw_suspend(1);
+		else  {
+			u16 tx, rx;
 
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(s, tx, &rx);
+			tx = MAX3100_WC | MAX3100_SHDN;
+			max3100_sr(s, tx, &rx);
+		}
 	}
 }
 
@@ -578,10 +586,8 @@ static int max3100_startup(struct uart_port *port)
 	s->parity = 0;
 	s->rts = 0;
 
-	INIT_WORK(&s->resume_work, max3100_resume_work);
-
 	if (request_threaded_irq(s->irq, NULL, max3100_ist,
-			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
+				 IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
 		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
 		s->irq = 0;
 		return -EBUSY;
@@ -699,6 +705,136 @@ static struct uart_ops max3100_ops = {
 	.verify_port	= max3100_verify_port,
 };
 
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+
+static void max3100_console_putchar(struct uart_port *port, int ch)
+{
+	struct max3100_port *s = container_of(port, struct max3100_port, port);
+	unsigned long tout = 10 * HZ / 1000; /* 10ms */
+	unsigned long start;
+	u16 tx, rx;
+
+	if (tout == 0)
+		tout = 1;
+	start = jiffies;
+	do {
+		tx = MAX3100_RC;
+		max3100_sr(s, tx, &rx);
+	} while ((rx & MAX3100_T) == 0 &&
+		 !time_after(jiffies, start + tout));
+	tx = ch;
+	max3100_calc_parity(s, &tx);
+	tx |= MAX3100_WD | MAX3100_RTS;
+	max3100_sr(s, tx, &rx);
+}
+
+static void max3100_console_write(struct console *co,
+				  const char *str, unsigned int count)
+{
+	struct max3100_port *s = max3100s[co->index];;
+
+	uart_console_write(&s->port, str, count, max3100_console_putchar);
+}
+
+static int max3100_console_setup(struct console *co, char *options)
+{
+	struct max3100_port *s;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+	int ret;
+	u16 tx, rx;
+
+	if (co->index == -1 || co->index >= MAX_MAX3100)
+		co->index = 0;
+	s = max3100s[co->index];
+	if (!s)
+		return -ENOENT;
+	s->console_flags |= MAX3100_IS_CONSOLE;
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+	ret = uart_set_options(&s->port, co, baud, parity, bits, flow);
+
+	tx = 0;
+	switch (baud) {
+	case 300:
+		tx = 15;
+		break;
+	case 600:
+		tx = 14 + s->crystal;
+		break;
+	case 1200:
+		tx = 13 + s->crystal;
+		break;
+	case 2400:
+		tx = 12 + s->crystal;
+		break;
+	case 4800:
+		tx = 11 + s->crystal;
+		break;
+	case 9600:
+		tx = 10 + s->crystal;
+		break;
+	case 19200:
+		tx = 9 + s->crystal;
+		break;
+	case 38400:
+		tx = 8 + s->crystal;
+		break;
+	case 57600:
+		tx = 1 + s->crystal;
+		break;
+	case 115200:
+		tx = 0 + s->crystal;
+		break;
+	case 230400:
+		tx = 0;
+		break;
+	}
+
+	if (bits == 8) {
+		tx &= ~MAX3100_L;
+		s->parity &= ~MAX3100_7BIT;
+	} else {
+		tx |= MAX3100_L;
+		s->parity |= MAX3100_7BIT;
+	}
+
+	if (parity == 'o' || parity == 'e') {
+		tx |= MAX3100_PE;
+		parity |= MAX3100_PARITY_ON;
+	} else {
+		tx &= ~MAX3100_PE;
+		parity &= ~MAX3100_PARITY_ON;
+	}
+
+	if (parity == 'o')
+		parity |= MAX3100_PARITY_ODD;
+	else
+		parity &= ~MAX3100_PARITY_ODD;
+
+
+	tx |= MAX3100_WC;
+	max3100_sr(s, tx, &rx);
+
+	msleep(50);
+	return ret;
+}
+
+static struct uart_driver max3100_uart_driver;
+static struct console max3100_console = {
+	.name		= "ttyMAX",
+	.write		= max3100_console_write,
+	.device		= uart_console_device,
+	.setup		= max3100_console_setup,
+	.flags		= CON_PRINTBUFFER,
+	.index		= -1,
+	.data		= &max3100_uart_driver,
+};
+#endif
+
 static struct uart_driver max3100_uart_driver = {
 	.owner          = THIS_MODULE,
 	.driver_name    = "ttyMAX",
@@ -706,6 +842,9 @@ static struct uart_driver max3100_uart_driver = {
 	.major          = MAX3100_MAJOR,
 	.minor          = MAX3100_MINOR,
 	.nr             = MAX_MAX3100,
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	.cons           = &max3100_console,
+#endif
 };
 static int uart_driver_registered;
 
@@ -768,18 +907,26 @@ static int __devinit max3100_probe(struct spi_device *spi)
 	max3100s[i]->port.line = i;
 	max3100s[i]->port.type = PORT_MAX3100;
 	max3100s[i]->port.dev = &spi->dev;
+	INIT_WORK(&max3100s[i]->resume_work, max3100_resume_work);
 	retval = uart_add_one_port(&max3100_uart_driver, &max3100s[i]->port);
 	if (retval < 0)
 		dev_warn(&spi->dev,
 			 "uart_add_one_port failed for line %d with error %d\n",
 			 i, retval);
 
-	/* set shutdown mode to save power. Will be woken-up on open */
-	if (max3100s[i]->max3100_hw_suspend)
-		max3100s[i]->max3100_hw_suspend(1);
-	else {
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(max3100s[i], tx, &rx);
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	if (i == (CONFIG_SERIAL_MAX3100_CONSOLE_N - 1))
+		register_console(&max3100_console);
+#endif
+
+	if (!(max3100s[i]->console_flags & MAX3100_IS_CONSOLE)) {
+		/* set shutdown mode to save power. Will be woken-up on open */
+		if (max3100s[i]->max3100_hw_suspend)
+			max3100s[i]->max3100_hw_suspend(1);
+		else {
+			tx = MAX3100_WC | MAX3100_SHDN;
+			max3100_sr(max3100s[i], tx, &rx);
+		}
 	}
 	mutex_unlock(&max3100s_lock);
 	return 0;
@@ -810,6 +957,9 @@ static int __devexit max3100_remove(struct spi_device *spi)
 		}
 	pr_debug("removing max3100 driver\n");
 	uart_unregister_driver(&max3100_uart_driver);
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	unregister_console(&max3100_console);
+#endif
 
 	mutex_unlock(&max3100s_lock);
 	return 0;
-- 
1.5.6.5


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

[PATCH 3/3] max3100: adds console support for MAX3100

[Christian Pellegrin]

This patch adds console support for the MAX3100 UART
(console=ttyMAX0,11500). The SPI subsystem and an
suitable SPI master driver have to be compiled in the kernel.

Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
---
 drivers/serial/Kconfig   |   20 +++++
 drivers/serial/max3100.c |  184 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 187 insertions(+), 17 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 9ff47db..bc72e84 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -547,6 +547,26 @@ config SERIAL_MAX3100
 	help
 	  MAX3100 chip support
 
+config SERIAL_MAX3100_CONSOLE
+	bool "Support console on MAX3100"
+	depends on SERIAL_MAX3100=y
+	select SERIAL_CORE_CONSOLE
+	help
+	  Console on MAX3100
+
+config SERIAL_MAX3100_CONSOLE_N
+	int "Number of MAX3100 chips to wait before registering console"
+	depends on SERIAL_MAX3100_CONSOLE=y
+	default "1"
+	help
+	  Unfortunately due to the async nature of Linux boot we must
+	  know in advance when to register the console. If we do it
+	  too early not all the MAX3100 chips are known to the system
+	  yet. And we just cannot know how many MAX3100 will be in the
+	  system so it's impossible to wait for the last one.  If you
+	  just want to have the console on the first MAX3100 chip
+	  probed (ttyMAX0) it's safe to leave this to 1.
+
 config SERIAL_DZ
 	bool "DECstation DZ serial driver"
 	depends on MACH_DECSTATION && 32BIT
diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
index 0c972c6..f6d250d 100644
--- a/drivers/serial/max3100.c
+++ b/drivers/serial/max3100.c
@@ -48,6 +48,7 @@
 #include <linux/kthread.h>
 #include <linux/interrupt.h>
 #include <linux/bitops.h>
+#include <linux/console.h>
 
 #include <linux/serial_max3100.h>
 
@@ -131,6 +132,10 @@ struct max3100_port {
 	int poll_time;
 	/* and its timer */
 	struct timer_list	timer;
+
+	int console_flags;
+	/* is this port a console? */
+#define MAX3100_IS_CONSOLE   (1 << 0)
 };
 
 static struct max3100_port *max3100s[MAX_MAX3100]; /* the chips */
@@ -175,7 +180,8 @@ static void max3100_resume_work(struct work_struct *w)
 	struct max3100_port *s = container_of(w, struct max3100_port,
 					      resume_work);
 
-	raise_threaded_irq(s->irq);
+	if (s->irq)
+		raise_threaded_irq(s->irq);
 }
 
 static void max3100_timeout(unsigned long data)
@@ -552,14 +558,16 @@ static void max3100_shutdown(struct uart_port *port)
 	if (s->irq)
 		free_irq(s->irq, s);
 
-	/* set shutdown mode to save power */
-	if (s->max3100_hw_suspend)
-		s->max3100_hw_suspend(1);
-	else  {
-		u16 tx, rx;
+	if (!(s->console_flags & MAX3100_IS_CONSOLE)) {
+		/* set shutdown mode to save power */
+		if (s->max3100_hw_suspend)
+			s->max3100_hw_suspend(1);
+		else  {
+			u16 tx, rx;
 
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(s, tx, &rx);
+			tx = MAX3100_WC | MAX3100_SHDN;
+			max3100_sr(s, tx, &rx);
+		}
 	}
 }
 
@@ -578,10 +586,8 @@ static int max3100_startup(struct uart_port *port)
 	s->parity = 0;
 	s->rts = 0;
 
-	INIT_WORK(&s->resume_work, max3100_resume_work);
-
 	if (request_threaded_irq(s->irq, NULL, max3100_ist,
-			IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
+				 IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
 		dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
 		s->irq = 0;
 		return -EBUSY;
@@ -699,6 +705,136 @@ static struct uart_ops max3100_ops = {
 	.verify_port	= max3100_verify_port,
 };
 
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+
+static void max3100_console_putchar(struct uart_port *port, int ch)
+{
+	struct max3100_port *s = container_of(port, struct max3100_port, port);
+	unsigned long tout = 10 * HZ / 1000; /* 10ms */
+	unsigned long start;
+	u16 tx, rx;
+
+	if (tout == 0)
+		tout = 1;
+	start = jiffies;
+	do {
+		tx = MAX3100_RC;
+		max3100_sr(s, tx, &rx);
+	} while ((rx & MAX3100_T) == 0 &&
+		 !time_after(jiffies, start + tout));
+	tx = ch;
+	max3100_calc_parity(s, &tx);
+	tx |= MAX3100_WD | MAX3100_RTS;
+	max3100_sr(s, tx, &rx);
+}
+
+static void max3100_console_write(struct console *co,
+				  const char *str, unsigned int count)
+{
+	struct max3100_port *s = max3100s[co->index];;
+
+	uart_console_write(&s->port, str, count, max3100_console_putchar);
+}
+
+static int max3100_console_setup(struct console *co, char *options)
+{
+	struct max3100_port *s;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+	int ret;
+	u16 tx, rx;
+
+	if (co->index == -1 || co->index >= MAX_MAX3100)
+		co->index = 0;
+	s = max3100s[co->index];
+	if (!s)
+		return -ENOENT;
+	s->console_flags |= MAX3100_IS_CONSOLE;
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+	ret = uart_set_options(&s->port, co, baud, parity, bits, flow);
+
+	tx = 0;
+	switch (baud) {
+	case 300:
+		tx = 15;
+		break;
+	case 600:
+		tx = 14 + s->crystal;
+		break;
+	case 1200:
+		tx = 13 + s->crystal;
+		break;
+	case 2400:
+		tx = 12 + s->crystal;
+		break;
+	case 4800:
+		tx = 11 + s->crystal;
+		break;
+	case 9600:
+		tx = 10 + s->crystal;
+		break;
+	case 19200:
+		tx = 9 + s->crystal;
+		break;
+	case 38400:
+		tx = 8 + s->crystal;
+		break;
+	case 57600:
+		tx = 1 + s->crystal;
+		break;
+	case 115200:
+		tx = 0 + s->crystal;
+		break;
+	case 230400:
+		tx = 0;
+		break;
+	}
+
+	if (bits == 8) {
+		tx &= ~MAX3100_L;
+		s->parity &= ~MAX3100_7BIT;
+	} else {
+		tx |= MAX3100_L;
+		s->parity |= MAX3100_7BIT;
+	}
+
+	if (parity == 'o' || parity == 'e') {
+		tx |= MAX3100_PE;
+		parity |= MAX3100_PARITY_ON;
+	} else {
+		tx &= ~MAX3100_PE;
+		parity &= ~MAX3100_PARITY_ON;
+	}
+
+	if (parity == 'o')
+		parity |= MAX3100_PARITY_ODD;
+	else
+		parity &= ~MAX3100_PARITY_ODD;
+
+
+	tx |= MAX3100_WC;
+	max3100_sr(s, tx, &rx);
+
+	msleep(50);
+	return ret;
+}
+
+static struct uart_driver max3100_uart_driver;
+static struct console max3100_console = {
+	.name		= "ttyMAX",
+	.write		= max3100_console_write,
+	.device		= uart_console_device,
+	.setup		= max3100_console_setup,
+	.flags		= CON_PRINTBUFFER,
+	.index		= -1,
+	.data		= &max3100_uart_driver,
+};
+#endif
+
 static struct uart_driver max3100_uart_driver = {
 	.owner          = THIS_MODULE,
 	.driver_name    = "ttyMAX",
@@ -706,6 +842,9 @@ static struct uart_driver max3100_uart_driver = {
 	.major          = MAX3100_MAJOR,
 	.minor          = MAX3100_MINOR,
 	.nr             = MAX_MAX3100,
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	.cons           = &max3100_console,
+#endif
 };
 static int uart_driver_registered;
 
@@ -768,18 +907,26 @@ static int __devinit max3100_probe(struct spi_device *spi)
 	max3100s[i]->port.line = i;
 	max3100s[i]->port.type = PORT_MAX3100;
 	max3100s[i]->port.dev = &spi->dev;
+	INIT_WORK(&max3100s[i]->resume_work, max3100_resume_work);
 	retval = uart_add_one_port(&max3100_uart_driver, &max3100s[i]->port);
 	if (retval < 0)
 		dev_warn(&spi->dev,
 			 "uart_add_one_port failed for line %d with error %d\n",
 			 i, retval);
 
-	/* set shutdown mode to save power. Will be woken-up on open */
-	if (max3100s[i]->max3100_hw_suspend)
-		max3100s[i]->max3100_hw_suspend(1);
-	else {
-		tx = MAX3100_WC | MAX3100_SHDN;
-		max3100_sr(max3100s[i], tx, &rx);
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	if (i == (CONFIG_SERIAL_MAX3100_CONSOLE_N - 1))
+		register_console(&max3100_console);
+#endif
+
+	if (!(max3100s[i]->console_flags & MAX3100_IS_CONSOLE)) {
+		/* set shutdown mode to save power. Will be woken-up on open */
+		if (max3100s[i]->max3100_hw_suspend)
+			max3100s[i]->max3100_hw_suspend(1);
+		else {
+			tx = MAX3100_WC | MAX3100_SHDN;
+			max3100_sr(max3100s[i], tx, &rx);
+		}
 	}
 	mutex_unlock(&max3100s_lock);
 	return 0;
@@ -810,6 +957,9 @@ static int __devexit max3100_remove(struct spi_device *spi)
 		}
 	pr_debug("removing max3100 driver\n");
 	uart_unregister_driver(&max3100_uart_driver);
+#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
+	unregister_console(&max3100_console);
+#endif
 
 	mutex_unlock(&max3100s_lock);
 	return 0;
-- 
1.5.6.5

Re: [PATCH 1/3] max3100: added raise_threaded_irq

[Grant Likely]

On Fri, Mar 19, 2010 at 2:38 AM, Christian Pellegrin <chripell@fsfe.org> wrote:
> raise_threaded_irq schedules the execution of an interrupt thread
>
> Signed-off-by: Christian Pellegrin <chripell@fsfe.org>

You should cc: Thomas and Ingo and lkml (which I just did) on patches
to the threaded interrupt code.

g.

> ---
>  include/linux/interrupt.h |    3 +++
>  kernel/irq/manage.c       |   27 +++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 75f3f00..14c0c13 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -144,6 +144,9 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
>  static inline void exit_irq_thread(void) { }
>  #endif
>
> +extern int raise_threaded_irq(unsigned int irq);
> +
> +
>  extern void free_irq(unsigned int, void *);
>
>  struct device;
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index eb6078c..a7d21e0 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1088,3 +1088,30 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
>        return retval;
>  }
>  EXPORT_SYMBOL(request_threaded_irq);
> +
> +/**
> + *     raise_threaded_irq - triggers a threded interrupt
> + *     @irq: Interrupt line to trigger
> + */
> +int raise_threaded_irq(unsigned int irq)
> +{
> +       struct irq_desc *desc = irq_to_desc(irq);
> +       struct irqaction *action;
> +
> +       if (!desc)
> +               return -ENOENT;
> +       action = desc->action;
> +       if (!action)
> +               return -ENOENT;
> +       if (unlikely(!action->thread_fn))
> +               return -EINVAL;
> +       if (likely(!test_bit(IRQTF_DIED,
> +                            &action->thread_flags))) {
> +               set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +               wake_up_process(action->thread);
> +       } else {
> +               return -ECHILD;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL(raise_threaded_irq);
> --
> 1.5.6.5
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/3] max3100: added raise_threaded_irq

[Grant Likely]

On Fri, Mar 19, 2010 at 2:38 AM, Christian Pellegrin <chripell@fsfe.org> wrote:
> raise_threaded_irq schedules the execution of an interrupt thread
>
> Signed-off-by: Christian Pellegrin <chripell@fsfe.org>

You should cc: Thomas and Ingo and lkml (which I just did) on patches
to the threaded interrupt code.

g.

> ---
>  include/linux/interrupt.h |    3 +++
>  kernel/irq/manage.c       |   27 +++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 75f3f00..14c0c13 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -144,6 +144,9 @@ request_threaded_irq(unsigned int irq, irq_handler_t handler,
>  static inline void exit_irq_thread(void) { }
>  #endif
>
> +extern int raise_threaded_irq(unsigned int irq);
> +
> +
>  extern void free_irq(unsigned int, void *);
>
>  struct device;
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index eb6078c..a7d21e0 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1088,3 +1088,30 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
>        return retval;
>  }
>  EXPORT_SYMBOL(request_threaded_irq);
> +
> +/**
> + *     raise_threaded_irq - triggers a threded interrupt
> + *     @irq: Interrupt line to trigger
> + */
> +int raise_threaded_irq(unsigned int irq)
> +{
> +       struct irq_desc *desc = irq_to_desc(irq);
> +       struct irqaction *action;
> +
> +       if (!desc)
> +               return -ENOENT;
> +       action = desc->action;
> +       if (!action)
> +               return -ENOENT;
> +       if (unlikely(!action->thread_fn))
> +               return -EINVAL;
> +       if (likely(!test_bit(IRQTF_DIED,
> +                            &action->thread_flags))) {
> +               set_bit(IRQTF_RUNTHREAD, &action->thread_flags);
> +               wake_up_process(action->thread);
> +       } else {
> +               return -ECHILD;
> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL(raise_threaded_irq);
> --
> 1.5.6.5
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] max3100: moved to threaded interrupt

[Grant Likely]

On Fri, Mar 19, 2010 at 2:39 AM, Christian Pellegrin <chripell@fsfe.org> wrote:
> The driver was reworked to use threaded interrupts instead of workqueus.
> This is a major boost in performance because the former are scheduled as
> SCHED_FIFO processes. As a side effect suspend/resume code was greatly
> simplified.
>
> Signed-off-by: Christian Pellegrin <chripell@fsfe.org>

On a quick parse, it seems mostly okay.  I haven't dug into the driver
execution flow though.  A few minor comments below.

> ---
>  drivers/serial/max3100.c |  102 ++++++++++++++-------------------------------
>  1 files changed, 32 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
> index 3c30c56..0c972c6 100644
> --- a/drivers/serial/max3100.c
> +++ b/drivers/serial/max3100.c
> @@ -45,7 +45,9 @@
>  #include <linux/serial_core.h>
>  #include <linux/serial.h>
>  #include <linux/spi/spi.h>
> -#include <linux/freezer.h>
> +#include <linux/kthread.h>
> +#include <linux/interrupt.h>
> +#include <linux/bitops.h>
>
>  #include <linux/serial_max3100.h>
>
> @@ -113,18 +115,15 @@ struct max3100_port {
>
>        int irq;                /* irq assigned to the max3100 */
>
> +       /* the workqueue is needed because we cannot schedule
> +          a threaded interrupt during PM
> +        */
> +       struct work_struct resume_work;
> +
>        int minor;              /* minor number */
>        int crystal;            /* 1 if 3.6864Mhz crystal 0 for 1.8432 */
>        int loopback;           /* 1 if we are in loopback mode */
>
> -       /* for handling irqs: need workqueue since we do spi_sync */
> -       struct workqueue_struct *workqueue;
> -       struct work_struct work;
> -       /* set to 1 to make the workhandler exit as soon as possible */
> -       int  force_end_work;
> -       /* need to know we are suspending to avoid deadlock on workqueue */
> -       int suspending;
> -
>        /* hook for suspending MAX3100 via dedicated pin */
>        void (*max3100_hw_suspend) (int suspend);
>
> @@ -171,13 +170,12 @@ static void max3100_calc_parity(struct max3100_port *s, u16 *c)
>                *c |= max3100_do_parity(s, *c) << 8;
>  }
>
> -static void max3100_work(struct work_struct *w);
> -
> -static void max3100_dowork(struct max3100_port *s)
> +static void max3100_resume_work(struct work_struct *w)
>  {
> -       if (!s->force_end_work && !work_pending(&s->work) &&
> -           !freezing(current) && !s->suspending)
> -               queue_work(s->workqueue, &s->work);
> +       struct max3100_port *s = container_of(w, struct max3100_port,
> +                                             resume_work);

container_of is used a lot.  Maybe consider a follow-on patch to
create a to_max3100_port() static inline.

> +
> +       raise_threaded_irq(s->irq);
>  }
>
>  static void max3100_timeout(unsigned long data)
> @@ -185,7 +183,7 @@ static void max3100_timeout(unsigned long data)
>        struct max3100_port *s = (struct max3100_port *)data;
>
>        if (s->port.state) {
> -               max3100_dowork(s);
> +               raise_threaded_irq(s->irq);
>                mod_timer(&s->timer, jiffies + s->poll_time);
>        }
>  }
> @@ -255,9 +253,9 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
>        return ret;
>  }
>
> -static void max3100_work(struct work_struct *w)
> +static irqreturn_t max3100_ist(int irq, void *dev_id)

'ist'?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] max3100: adds console support for MAX3100

[Grant Likely]

Hi Christian,

Comments below.

g.

On Fri, Mar 19, 2010 at 2:39 AM, Christian Pellegrin <chripell@fsfe.org> wrote:
> This patch adds console support for the MAX3100 UART
> (console=ttyMAX0,11500). The SPI subsystem and an
> suitable SPI master driver have to be compiled in the kernel.
>
> Signed-off-by: Christian Pellegrin <chripell@fsfe.org>
> ---
>  drivers/serial/Kconfig   |   20 +++++
>  drivers/serial/max3100.c |  184 +++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 187 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> index 9ff47db..bc72e84 100644
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -547,6 +547,26 @@ config SERIAL_MAX3100
>        help
>          MAX3100 chip support
>
> +config SERIAL_MAX3100_CONSOLE
> +       bool "Support console on MAX3100"
> +       depends on SERIAL_MAX3100=y
> +       select SERIAL_CORE_CONSOLE
> +       help
> +         Console on MAX3100
> +
> +config SERIAL_MAX3100_CONSOLE_N
> +       int "Number of MAX3100 chips to wait before registering console"
> +       depends on SERIAL_MAX3100_CONSOLE=y
> +       default "1"
> +       help
> +         Unfortunately due to the async nature of Linux boot we must
> +         know in advance when to register the console. If we do it
> +         too early not all the MAX3100 chips are known to the system
> +         yet. And we just cannot know how many MAX3100 will be in the
> +         system so it's impossible to wait for the last one.  If you
> +         just want to have the console on the first MAX3100 chip
> +         probed (ttyMAX0) it's safe to leave this to 1.

This seems wrong and fragile.  It certainly isn't multiplatform safe
to have it as a CONFIG setting because every board will have a
different number of devices that it needs to wait for.  I don't know
the console code very well though, but it seems to me that there
should be a way to register the console regardless and then be able to
hold off output until the actual backing device shows up.

Anybody know the right thing to do here?

> +
>  config SERIAL_DZ
>        bool "DECstation DZ serial driver"
>        depends on MACH_DECSTATION && 32BIT
> diff --git a/drivers/serial/max3100.c b/drivers/serial/max3100.c
> index 0c972c6..f6d250d 100644
> --- a/drivers/serial/max3100.c
> +++ b/drivers/serial/max3100.c
> @@ -48,6 +48,7 @@
>  #include <linux/kthread.h>
>  #include <linux/interrupt.h>
>  #include <linux/bitops.h>
> +#include <linux/console.h>
>
>  #include <linux/serial_max3100.h>
>
> @@ -131,6 +132,10 @@ struct max3100_port {
>        int poll_time;
>        /* and its timer */
>        struct timer_list       timer;
> +
> +       int console_flags;
> +       /* is this port a console? */
> +#define MAX3100_IS_CONSOLE   (1 << 0)
>  };
>
>  static struct max3100_port *max3100s[MAX_MAX3100]; /* the chips */
> @@ -175,7 +180,8 @@ static void max3100_resume_work(struct work_struct *w)
>        struct max3100_port *s = container_of(w, struct max3100_port,
>                                              resume_work);
>
> -       raise_threaded_irq(s->irq);
> +       if (s->irq)
> +               raise_threaded_irq(s->irq);
>  }
>
>  static void max3100_timeout(unsigned long data)
> @@ -552,14 +558,16 @@ static void max3100_shutdown(struct uart_port *port)
>        if (s->irq)
>                free_irq(s->irq, s);
>
> -       /* set shutdown mode to save power */
> -       if (s->max3100_hw_suspend)
> -               s->max3100_hw_suspend(1);
> -       else  {
> -               u16 tx, rx;
> +       if (!(s->console_flags & MAX3100_IS_CONSOLE)) {
> +               /* set shutdown mode to save power */
> +               if (s->max3100_hw_suspend)
> +                       s->max3100_hw_suspend(1);
> +               else  {
> +                       u16 tx, rx;

inconsistent braces.  If you use {} on the else side, then please use
{} on the if side too.

>
> -               tx = MAX3100_WC | MAX3100_SHDN;
> -               max3100_sr(s, tx, &rx);
> +                       tx = MAX3100_WC | MAX3100_SHDN;
> +                       max3100_sr(s, tx, &rx);
> +               }
>        }
>  }
>
> @@ -578,10 +586,8 @@ static int max3100_startup(struct uart_port *port)
>        s->parity = 0;
>        s->rts = 0;
>
> -       INIT_WORK(&s->resume_work, max3100_resume_work);
> -
>        if (request_threaded_irq(s->irq, NULL, max3100_ist,
> -                       IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
> +                                IRQF_TRIGGER_FALLING, "max3100", s) < 0) {

try to avoid unrelated whitespace changes, it adds noise when reviewing.

>                dev_err(&s->spi->dev, "cannot allocate irq %d\n", s->irq);
>                s->irq = 0;
>                return -EBUSY;
> @@ -699,6 +705,136 @@ static struct uart_ops max3100_ops = {
>        .verify_port    = max3100_verify_port,
>  };
>
> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +
> +static void max3100_console_putchar(struct uart_port *port, int ch)
> +{
> +       struct max3100_port *s = container_of(port, struct max3100_port, port);
> +       unsigned long tout = 10 * HZ / 1000; /* 10ms */
> +       unsigned long start;
> +       u16 tx, rx;
> +
> +       if (tout == 0)
> +               tout = 1;
> +       start = jiffies;
> +       do {
> +               tx = MAX3100_RC;
> +               max3100_sr(s, tx, &rx);
> +       } while ((rx & MAX3100_T) == 0 &&
> +                !time_after(jiffies, start + tout));
> +       tx = ch;
> +       max3100_calc_parity(s, &tx);
> +       tx |= MAX3100_WD | MAX3100_RTS;
> +       max3100_sr(s, tx, &rx);
> +}
> +
> +static void max3100_console_write(struct console *co,
> +                                 const char *str, unsigned int count)
> +{
> +       struct max3100_port *s = max3100s[co->index];;
> +
> +       uart_console_write(&s->port, str, count, max3100_console_putchar);
> +}
> +
> +static int max3100_console_setup(struct console *co, char *options)
> +{
> +       struct max3100_port *s;
> +       int baud = 115200;
> +       int bits = 8;
> +       int parity = 'n';
> +       int flow = 'n';
> +       int ret;
> +       u16 tx, rx;
> +
> +       if (co->index == -1 || co->index >= MAX_MAX3100)
> +               co->index = 0;
> +       s = max3100s[co->index];
> +       if (!s)
> +               return -ENOENT;
> +       s->console_flags |= MAX3100_IS_CONSOLE;
> +
> +       if (options)
> +               uart_parse_options(options, &baud, &parity, &bits, &flow);
> +       ret = uart_set_options(&s->port, co, baud, parity, bits, flow);
> +
> +       tx = 0;
> +       switch (baud) {
> +       case 300:
> +               tx = 15;
> +               break;
> +       case 600:
> +               tx = 14 + s->crystal;
> +               break;
> +       case 1200:
> +               tx = 13 + s->crystal;
> +               break;
> +       case 2400:
> +               tx = 12 + s->crystal;
> +               break;
> +       case 4800:
> +               tx = 11 + s->crystal;
> +               break;
> +       case 9600:
> +               tx = 10 + s->crystal;
> +               break;
> +       case 19200:
> +               tx = 9 + s->crystal;
> +               break;
> +       case 38400:
> +               tx = 8 + s->crystal;
> +               break;
> +       case 57600:
> +               tx = 1 + s->crystal;
> +               break;
> +       case 115200:
> +               tx = 0 + s->crystal;
> +               break;
> +       case 230400:
> +               tx = 0;
> +               break;
> +       }
> +
> +       if (bits == 8) {
> +               tx &= ~MAX3100_L;
> +               s->parity &= ~MAX3100_7BIT;
> +       } else {
> +               tx |= MAX3100_L;
> +               s->parity |= MAX3100_7BIT;
> +       }
> +
> +       if (parity == 'o' || parity == 'e') {
> +               tx |= MAX3100_PE;
> +               parity |= MAX3100_PARITY_ON;

s->parity?

> +       } else {
> +               tx &= ~MAX3100_PE;
> +               parity &= ~MAX3100_PARITY_ON;
> +       }
> +
> +       if (parity == 'o')
> +               parity |= MAX3100_PARITY_ODD;
> +       else
> +               parity &= ~MAX3100_PARITY_ODD;

ditto?


Also, these blocks are really verbose; maybe do this:
       tx &= ~(MAX3100_L | MAX3100_PE);
       s->parity &= ~(MAX3100_7BIT | MAX3100_PARITY_ON | MAX3100_PARITY_ODD);
       if (bits != 8) {
               tx |= MAX3100_L;
               s->parity |= MAX3100_7BIT;
       }

       if (parity == 'o' || parity == 'e') {
               tx |= MAX3100_PE;
               s->parity |= MAX3100_PARITY_ON;
       }

       if (parity == 'o')
               s->parity |= MAX3100_PARITY_ODD;

> +
> +
> +       tx |= MAX3100_WC;
> +       max3100_sr(s, tx, &rx);
> +
> +       msleep(50);

Why the sleep?  Shouldn't the console driver be able to handle the SPI
device not being ready yet?

> +       return ret;
> +}
> +
> +static struct uart_driver max3100_uart_driver;
> +static struct console max3100_console = {
> +       .name           = "ttyMAX",
> +       .write          = max3100_console_write,
> +       .device         = uart_console_device,
> +       .setup          = max3100_console_setup,
> +       .flags          = CON_PRINTBUFFER,
> +       .index          = -1,
> +       .data           = &max3100_uart_driver,
> +};
> +#endif
> +
>  static struct uart_driver max3100_uart_driver = {
>        .owner          = THIS_MODULE,
>        .driver_name    = "ttyMAX",
> @@ -706,6 +842,9 @@ static struct uart_driver max3100_uart_driver = {
>        .major          = MAX3100_MAJOR,
>        .minor          = MAX3100_MINOR,
>        .nr             = MAX_MAX3100,
> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +       .cons           = &max3100_console,
> +#endif
>  };
>  static int uart_driver_registered;
>
> @@ -768,18 +907,26 @@ static int __devinit max3100_probe(struct spi_device *spi)
>        max3100s[i]->port.line = i;
>        max3100s[i]->port.type = PORT_MAX3100;
>        max3100s[i]->port.dev = &spi->dev;
> +       INIT_WORK(&max3100s[i]->resume_work, max3100_resume_work);
>        retval = uart_add_one_port(&max3100_uart_driver, &max3100s[i]->port);
>        if (retval < 0)
>                dev_warn(&spi->dev,
>                         "uart_add_one_port failed for line %d with error %d\n",
>                         i, retval);
>
> -       /* set shutdown mode to save power. Will be woken-up on open */
> -       if (max3100s[i]->max3100_hw_suspend)
> -               max3100s[i]->max3100_hw_suspend(1);
> -       else {
> -               tx = MAX3100_WC | MAX3100_SHDN;
> -               max3100_sr(max3100s[i], tx, &rx);
> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +       if (i == (CONFIG_SERIAL_MAX3100_CONSOLE_N - 1))
> +               register_console(&max3100_console);
> +#endif
> +
> +       if (!(max3100s[i]->console_flags & MAX3100_IS_CONSOLE)) {
> +               /* set shutdown mode to save power. Will be woken-up on open */
> +               if (max3100s[i]->max3100_hw_suspend)
> +                       max3100s[i]->max3100_hw_suspend(1);
> +               else {
> +                       tx = MAX3100_WC | MAX3100_SHDN;
> +                       max3100_sr(max3100s[i], tx, &rx);
> +               }

Same comment on braces as for above.

>        }
>        mutex_unlock(&max3100s_lock);
>        return 0;
> @@ -810,6 +957,9 @@ static int __devexit max3100_remove(struct spi_device *spi)
>                }
>        pr_debug("removing max3100 driver\n");
>        uart_unregister_driver(&max3100_uart_driver);
> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +       unregister_console(&max3100_console);
> +#endif
>
>        mutex_unlock(&max3100s_lock);
>        return 0;
> --
> 1.5.6.5
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] max3100: added raise_threaded_irq

[christian pellegrin]

Hi,


On Fri, Mar 19, 2010 at 6:48 PM, Grant Likely <grant.likely@secretlab.ca> wrote:

> You should cc: Thomas and Ingo and lkml (which I just did) on patches
> to the threaded interrupt code.
>

ok, let me explain the reason for this function. The move from
worqueues to the threaded interrupts was motivated by a reduction of
latency in answering to RX buffer full interrupts. Threaded interrupts
are SCHED_FIFO instead worqueues being SCHED_OTHER. In the case of
MAX31x0 when we transmit a character we could have to receive one
(this is efficient because SPI transfers are quite always
bidirectional). The same routine is used both for tx and rx (an other
things like changing parameters): this makes locking really simple.
With this routine the thread interrupt handler could be the only kind
of deferred work a driver for a simple hardware may ever need.

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

Re: [PATCH 2/3] max3100: moved to threaded interrupt

[christian pellegrin]

Hi,

On Fri, Mar 19, 2010 at 6:58 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
> On a quick parse, it seems mostly okay.  I haven't dug into the driver
> execution flow though.  A few minor comments below.
>

thank you very much for the review. I'm waiting a bit for other people
to comment and then prepare another patch.


>> +                                             resume_work);
>
> container_of is used a lot.  Maybe consider a follow-on patch to
> create a to_max3100_port() static inline.
>

ack

>> +static irqreturn_t max3100_ist(int irq, void *dev_id)
>
> 'ist'?
>

interrupt service thread (compared to isr, interrupt service routine,
for the code executed in hard interrupt context). I know it's
windozece-speak and so *very lame*. Any suggestion for a better name
is welcome!


-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] max3100: adds console support for MAX3100

[christian pellegrin]

On Fri, Mar 19, 2010 at 8:31 PM, Grant Likely <grant.likely@secretlab.ca> wrote:

>> +config SERIAL_MAX3100_CONSOLE_N
>> +       int "Number of MAX3100 chips to wait before registering console"
>> +       depends on SERIAL_MAX3100_CONSOLE=y
>> +       default "1"
>> +       help
>> +         Unfortunately due to the async nature of Linux boot we must
>> +         know in advance when to register the console. If we do it
>> +         too early not all the MAX3100 chips are known to the system
>> +         yet. And we just cannot know how many MAX3100 will be in the
>> +         system so it's impossible to wait for the last one.  If you
>> +         just want to have the console on the first MAX3100 chip
>> +         probed (ttyMAX0) it's safe to leave this to 1.
>
> This seems wrong and fragile.  It certainly isn't multiplatform safe
> to have it as a CONFIG setting because every board will have a
> different number of devices that it needs to wait for.  I don't know
> the console code very well though, but it seems to me that there
> should be a way to register the console regardless and then be able to
> hold off output until the actual backing device shows up.
>
> Anybody know the right thing to do here?

I'll try to dig in the console code, any suggestion from others if
more than welcome.

>> +               else  {
>> +                       u16 tx, rx;
>
> inconsistent braces.  If you use {} on the else side, then please use
> {} on the if side too.

ack, strange checkpatch.pl didin't catch these.

>> +                                IRQF_TRIGGER_FALLING, "max3100", s) < 0) {
>
> try to avoid unrelated whitespace changes, it adds noise when reviewing.
>

ack

>> +               parity |= MAX3100_PARITY_ON;
>
> s->parity?
>

huh, well spotted. ack

>> +       } else {
>> +               tx &= ~MAX3100_PE;
>> +               parity &= ~MAX3100_PARITY_ON;
>> +       }
>> +
>> +       if (parity == 'o')
>> +               parity |= MAX3100_PARITY_ODD;
>> +       else
>> +               parity &= ~MAX3100_PARITY_ODD;
>
> ditto?

ack

>
>
> Also, these blocks are really verbose; maybe do this:

ack, I'll reorganize this chunk of code.

>
>> +
>> +       msleep(50);
>
> Why the sleep?  Shouldn't the console driver be able to handle the SPI
> device not being ready yet?

it's for the max3100 to settle. I'll double check the needed time and
add a comment.

>> +                       max3100_sr(max3100s[i], tx, &rx);
>> +               }
>
> Same comment on braces as for above.
>

ack

Thanks again for the review.

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] max3100: adds console support for MAX3100

[David Brownell]

On Sunday 21 March 2010, christian pellegrin wrote:
> > Anybody know the right thing to do here?

when to register consoles is a general mess.  I've seeen some
success in having it be board-specific data, at the level of
"port 3 should be a console ... or in this case, maybe having
platform data say which chip.

After all, board docs probably assign numbers to each of the
external connetors, and say "#1 is normally the onsole" (or
maybe #0, #3, etc).


But it's still kind of chaotic trying to set things up so that
for example $3 becomes /dev/ttyS3" and so forth.  Especially if
your chip has six serial ports but several of them aren't wired
up (and so should not get published to userspace even if they do
get autodetected...  (Heck, the controller may exist, but the
unavailability may as easily be "those pins are routed to some
other peripheral" as ""those pins aren't wired up to RS232 level
shifters or a DB9/RJ45/... connector.

.

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 3/3] max3100: adds console support for MAX3100

[Feng Tang]

On Fri, 19 Mar 2010 16:39:57 +0800
Christian Pellegrin <chripell@fsfe.org> wrote:

> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +
> +static void max3100_console_putchar(struct uart_port *port, int ch)
> +{
> +	struct max3100_port *s = container_of(port, struct
> max3100_port, port);
> +	unsigned long tout = 10 * HZ / 1000; /* 10ms */
> +	unsigned long start;
> +	u16 tx, rx;
> +
> +	if (tout == 0)
> +		tout = 1;
> +	start = jiffies;
> +	do {
> +		tx = MAX3100_RC;
> +		max3100_sr(s, tx, &rx);

Hi Christian,

You'd better not call max3110_sr (which calls spi_sync) here, a console's
putchar() func may be called in some atomic context.

I used similar method when I started to implement a console, but then I switch
to current method in my 3110 driver: save the printk buffer inside putchar,
and wake up a thread to really handle the spi transfer later.

Thanks,
Feng
> +	} while ((rx & MAX3100_T) == 0 &&
> +		 !time_after(jiffies, start + tout));
> +	tx = ch;
> +	max3100_calc_parity(s, &tx);
> +	tx |= MAX3100_WD | MAX3100_RTS;
> +	max3100_sr(s, tx, &rx);
> +}
> +
> +static void max3100_console_write(struct console *co,
> +				  const char *str, unsigned int
> count) +{
> +	struct max3100_port *s = max3100s[co->index];;
> +
> +	uart_console_write(&s->port, str, count,
> max3100_console_putchar); +}

Re: [PATCH 3/3] max3100: adds console support for MAX3100

[Feng Tang]

On Fri, 19 Mar 2010 16:39:57 +0800
Christian Pellegrin <chripell@fsfe.org> wrote:

> +#ifdef CONFIG_SERIAL_MAX3100_CONSOLE
> +
> +static void max3100_console_putchar(struct uart_port *port, int ch)
> +{
> +	struct max3100_port *s = container_of(port, struct
> max3100_port, port);
> +	unsigned long tout = 10 * HZ / 1000; /* 10ms */
> +	unsigned long start;
> +	u16 tx, rx;
> +
> +	if (tout == 0)
> +		tout = 1;
> +	start = jiffies;
> +	do {
> +		tx = MAX3100_RC;
> +		max3100_sr(s, tx, &rx);

Hi Christian,

You'd better not call max3110_sr (which calls spi_sync) here, a console's
putchar() func may be called in some atomic context.

I used similar method when I started to implement a console, but then I switch
to current method in my 3110 driver: save the printk buffer inside putchar,
and wake up a thread to really handle the spi transfer later.

Thanks,
Feng
> +	} while ((rx & MAX3100_T) == 0 &&
> +		 !time_after(jiffies, start + tout));
> +	tx = ch;
> +	max3100_calc_parity(s, &tx);
> +	tx |= MAX3100_WD | MAX3100_RTS;
> +	max3100_sr(s, tx, &rx);
> +}
> +
> +static void max3100_console_write(struct console *co,
> +				  const char *str, unsigned int
> count) +{
> +	struct max3100_port *s = max3100s[co->index];;
> +
> +	uart_console_write(&s->port, str, count,
> max3100_console_putchar); +}

Re: [PATCH 3/3] max3100: adds console support for MAX3100

[christian pellegrin]

On Mon, Mar 22, 2010 at 2:31 AM, Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>
> Hi Christian,

Hi,

>
> You'd better not call max3110_sr (which calls spi_sync) here, a console's
> putchar() func may be called in some atomic context.
>

ack, I didn't realize that. Unfortunately this makes reliable use of
MAX3100 as a console near impossible. Some SPI master driver use
workqueues (see the bit banging ones). I don't see any way to yield
CPU to them while somehow wait the output has finished (a busy loop
won't work: perhaps the thread calling the console has a higher
priority than the one doing actual SPI output so the system will
deadlock). The only way out I see is to provide a buffer large enough
for all practical purposes. Will try it in the next version of the
patch.

Thanks.

-- 
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev