On Fri, Mar 19, 2010 at 2:39 AM, Christian Pellegrin <chripell@xxxxxxxx> 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@xxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
