Re: [patch 2/3] sc16is7x2: use threaded irqs

[Andrew Morton <akpm@linux-foundation.org>]

On Sat, 2 Oct 2010 01:37:53 +0200
Micha__ Miros__aw <mirq@rere.qmqm.pl> wrote:

> On Fri, Oct 01, 2010 at 02:18:04PM -0700, akpm@linux-foundation.org wrote:
> > From: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > 
> > Signed-off-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
> > Cc: Greg KH <greg@kroah.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  drivers/serial/sc16is7x2.c |  115 ++++++++++++-----------------------
> >  1 file changed, 40 insertions(+), 75 deletions(-)
> > 
> > diff -puN drivers/serial/sc16is7x2.c~sc16is7x2-use-threaded-irqs drivers/serial/sc16is7x2.c
> > --- a/drivers/serial/sc16is7x2.c~sc16is7x2-use-threaded-irqs
> > +++ a/drivers/serial/sc16is7x2.c
> [...]
> > @@ -1066,21 +1051,18 @@ static bool sc16is7x2_handle_channel(str
> >  	return (chan->iir & UART_IIR_NO_INT) == 0x00;
> >  }
> >  
> > -static void sc16is7x2_work(struct work_struct *w)
> > +static irqreturn_t sc16is7x2_work(int irq, void *data)
> >  {
> > -	struct sc16is7x2_chip *ts =
> > -			container_of(w, struct sc16is7x2_chip, work);
> > +	struct sc16is7x2_chip *ts = (struct sc16is7x2_chip *)data;
> 
> You don't need to cast from (void *).
> 
> [...]
> >  
> > -static irqreturn_t sc16is7x2_interrupt(int irq, void *dev_id)
> > +static irqreturn_t sc16is7x2_irq(int irq, void *data)
> >  {
> > -	struct sc16is7x2_chip *ts = dev_id;
> > -
> > -	dev_dbg(&ts->spi->dev, "%s\n", __func__);
> > +	struct sc16is7x2_chip *ts = (struct sc16is7x2_channel *)data;
> 
> This cast seems wrong. Better to just drop it, like in sc16is7x2_work().
> 
> >  
> > -	if (!ts->force_end_work && !work_pending(&ts->work) &&
> > -	    !freezing(current) && !ts->suspending)
> > -		queue_work(ts->workqueue, &ts->work);
> > -
> > -	return IRQ_HANDLED;
> > +	/* It takes too long to read the regs over SPI,
> > +	 * so just wake up the thread */
> > +	if (ts->channel[0].active || ts->channel[1].active)
> > +		return IRQ_WAKE_THREAD;
> > +	else
> > +		return IRQ_NONE;
> >  }
> >  
> [...]
> 
> Since this is fixing up patch #1, wouldn't it be better to merge the two?

I think I'll drop them both and will ask for a resend.

I have a note here that Greg mentioned the need to #define
PORT_SC16IS7X2 - please ensure that that is addressed in the next
version, thanks.