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

[Grant Likely <grant.likely@secretlab.ca>]

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