Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver

[Felipe Balbi <felipe.balbi@nokia.com>]

On Fri, Jun 12, 2009 at 04:50:09PM +0200, ext Pandita, Vikram wrote:
> 
> 
> >-----Original Message-----
> >From: Menon, Nishanth
> >Sent: Friday, June 12, 2009 9:46 AM
> >To: felipe.balbi@nokia.com
> >Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >
> >> -----Original Message-----
> >> From: Felipe Balbi [mailto:felipe.balbi@nokia.com]
> >> Sent: Friday, June 12, 2009 1:38 AM
> >> To: Menon, Nishanth
> >> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >>
> >> On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
> >> >
> >> > > -----Original Message-----
> >> > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> > > owner@vger.kernel.org] On Behalf Of Pandita, Vikram
> >> > > Sent: Thursday, June 11, 2009 7:50 PM
> >> > > To: linux-omap@vger.kernel.org
> >> > > Cc: Pandita, Vikram
> >> > > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >> > >
> >> > >
> >> > > +	/* Get IRQ Trigger Flag */
> >> > > +	if (up->port.flags & UPF_IRQ_TRIG_RISING)
> >> > > +		irq_flags |= IRQF_TRIGGER_RISING;
> >> > > +	else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
> >> > > +		irq_flags |= IRQF_TRIGGER_FALLING;
> >> > > +	else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
> >> > > +		irq_flags |= IRQF_TRIGGER_HIGH;
> >> > > +	else if (up->port.flags & UPF_IRQ_TRIG_LOW)
> >> > > +		irq_flags |= IRQF_TRIGGER_LOW;
> >> > > +
> >> > Blame it on my dislike for nested if elseif, but...
> >> > irq_flags |=
> >> > 	(up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
> >> > 	(up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
> >> > 	(up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
> >> > 	(up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
> >> >
> >> > Makes sense?
> >>
> >> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
> >> case UPF_IRQ_TRIG_RISING:
> >> 	irq_flags |= IRQF_TRIGGER_RISING;
> >> 	break;
> >> case UPF_IRQ_TRIG_FALLING:
> >> 	irq_flags |= IRQF_TRIGGER_FALLING;
> >> 	break;
> >> case UPF_IRQ_TRIG_HIGH:
> >> 	irq_flags |= IRQF_TRIGGER_HIGH;
> >> 	break;
> >> case UPF_IRQ_TRIG_LOW:
> >> 	irq_flags |= IRQF_TRIGGER_LOW;
> >> 	break;
> >> default:
> >> 	printk(KERN_ERR "Unsupported flag\n");
> >> 	return -EINVAL;
> >> }
> >>
> >Better :) infact, if the port.flags = UPF_IRQ_TRIG_LOW | UPF_IRQ_TRIG_HIGH it might be better to
> >return -EINVAL, which Felipe's change does :).
> 
> Needs more investigation as to how current boards using 8250 driver do not pass any flag (maybe just SHARED flag) and they still work. Maybe some default taken by request_irq()
> 
> In short NO_ need to return failure as no flag is a valid use case.

remove the default part:

switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
case UPF_IRQ_TRIG_RISING:
	irq_flags |= IRQF_TRIGGER_RISING;
	break;
case UPF_IRQ_TRIG_FALLING:
	irq_flags |= IRQF_TRIGGER_FALLING;
	break;
case UPF_IRQ_TRIG_HIGH:
	irq_flags |= IRQF_TRIGGER_HIGH;
	break;
case UPF_IRQ_TRIG_LOW:
	irq_flags |= IRQF_TRIGGER_LOW;
	break;
default:
	break;
}

-- 
balbi