On Fri, Jun 12, 2009 at 10:01 PM, Felipe Balbi<felipe.balbi@xxxxxxxxx> wrote: > 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@xxxxxxxxx >> >Cc: Pandita, Vikram; linux-omap@xxxxxxxxxxxxxxx >> >Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver >> > >> >> -----Original Message----- >> >> From: Felipe Balbi [mailto:felipe.balbi@xxxxxxxxx] >> >> Sent: Friday, June 12, 2009 1:38 AM >> >> To: Menon, Nishanth >> >> Cc: Pandita, Vikram; linux-omap@xxxxxxxxxxxxxxx >> >> 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@xxxxxxxxxxxxxxx [mailto:linux-omap- >> >> > > owner@xxxxxxxxxxxxxxx] On Behalf Of Pandita, Vikram >> >> > > Sent: Thursday, June 11, 2009 7:50 PM >> >> > > To: linux-omap@xxxxxxxxxxxxxxx >> >> > > 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; How about? irq_flags |= IRQF_TRIGGER_RISING; break; As far as I can tell Linux code-style tries to use as less lines of code as possible while retaining sane readability. > 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; There's no need for the final break, is it? > } Cheers. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
