Message ID | 4B1EF3E9.8050005@elphinstone.net (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Kumar Gala |
Headers | show |
On Dec 8, 2009, at 6:48 PM, Mark Ware wrote: > Port C interrupts can be either falling edge, or either edge. > Other external interrupts are either falling edge or active low. > > Signed-Off-By: Mark Ware <mware@elphinstone.net> > --- > Changed in v2: > - Disallow rising edge only on Port C > > arch/powerpc/sysdev/cpm2_pic.c | 30 +++++++++++++++++++++++------- > 1 files changed, 23 insertions(+), 7 deletions(-) Anton, mind taking a look at this for me. - k > > diff --git a/arch/powerpc/sysdev/cpm2_pic.c b/arch/powerpc/sysdev/cpm2_pic.c > index 78f1f7c..eba5f24 100644 > --- a/arch/powerpc/sysdev/cpm2_pic.c > +++ b/arch/powerpc/sysdev/cpm2_pic.c > @@ -141,13 +141,29 @@ static int cpm2_set_irq_type(unsigned int virq, unsigned int flow_type) > struct irq_desc *desc = get_irq_desc(virq); > unsigned int vold, vnew, edibit; > > - if (flow_type == IRQ_TYPE_NONE) > - flow_type = IRQ_TYPE_LEVEL_LOW; > - > - if (flow_type & IRQ_TYPE_EDGE_RISING) { > - printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", > - flow_type); > - return -EINVAL; > + /* Port C interrupts are either IRQ_TYPE_EDGE_FALLING or > + * IRQ_TYPE_EDGE_BOTH (default). All others are IRQ_TYPE_EDGE_FALLING > + * or IRQ_TYPE_LEVEL_LOW (default) > + */ > + if (src >= CPM2_IRQ_PORTC15 && src <= CPM2_IRQ_PORTC0) { > + if (flow_type == IRQ_TYPE_NONE) > + flow_type = IRQ_TYPE_EDGE_BOTH; > + > + if ((flow_type != IRQ_TYPE_EDGE_BOTH) && > + (flow_type != IRQ_TYPE_EDGE_FALLING)) { > + printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", > + flow_type); > + return -EINVAL; > + } > + } else { > + if (flow_type == IRQ_TYPE_NONE) > + flow_type = IRQ_TYPE_LEVEL_LOW; > + > + if (flow_type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)) { > + printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", > + flow_type); > + return -EINVAL; > + } > } > > desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL); > -- > 1.5.6.5
On Wed, Dec 09, 2009 at 11:48:41AM +1100, Mark Ware wrote: > Port C interrupts can be either falling edge, or either edge. > Other external interrupts are either falling edge or active low. > > Signed-Off-By: Mark Ware <mware@elphinstone.net> Looks correct (checked with 8272 and 8555 specs). Reviewed-by: Anton Vorontsov <avorontsov@ru.mvista.com> Cosmetic nitpicks below. I tend to think they don't desire another resend, but I couldn't resist making them anyway. ;-) [...] > + if (src >= CPM2_IRQ_PORTC15 && src <= CPM2_IRQ_PORTC0) { > + if (flow_type == IRQ_TYPE_NONE) > + flow_type = IRQ_TYPE_EDGE_BOTH; > + > + if ((flow_type != IRQ_TYPE_EDGE_BOTH) && > + (flow_type != IRQ_TYPE_EDGE_FALLING)) { I'd place one more tab here. And parenthesis aren't actually needed. > + printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", > + flow_type); > + return -EINVAL; > + } > + } else { > + if (flow_type == IRQ_TYPE_NONE) > + flow_type = IRQ_TYPE_LEVEL_LOW; > + > + if (flow_type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)) { > + printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", > + flow_type); pr_err is shorter. Also, this message is duplicated. Would be better to do something like: if (somethingwrong) goto err_sense; ... return 0; err_sense: pr_err("CPM2 PIC: sense type 0x%x not supported\n", flow_type); return -EINVAL; } Or we may don't print any errors at all. For internal interrupts we don't print them anyway, i.e. the current code has just return (flow_type & IRQ_TYPE_LEVEL_LOW) ? 0 : -EINVAL; Thanks,
Anton Vorontsov wrote: >> + if ((flow_type != IRQ_TYPE_EDGE_BOTH) && >> + (flow_type != IRQ_TYPE_EDGE_FALLING)) { > > I'd place one more tab here. Or better, align one "flow_type" with the other. -Scott
Scott Wood wrote: > Anton Vorontsov wrote: >>> + if ((flow_type != IRQ_TYPE_EDGE_BOTH) && + >>> (flow_type != IRQ_TYPE_EDGE_FALLING)) { >> >> I'd place one more tab here. > > Or better, align one "flow_type" with the other. > > -Scott > Thanks Anton and Scott. Scott's suggestion is also my preferred style, but a (brief) search through the source didn't reveal much support. I'll send a new version shortly including you suggestions. - Mark
diff --git a/arch/powerpc/sysdev/cpm2_pic.c b/arch/powerpc/sysdev/cpm2_pic.c index 78f1f7c..eba5f24 100644 --- a/arch/powerpc/sysdev/cpm2_pic.c +++ b/arch/powerpc/sysdev/cpm2_pic.c @@ -141,13 +141,29 @@ static int cpm2_set_irq_type(unsigned int virq, unsigned int flow_type) struct irq_desc *desc = get_irq_desc(virq); unsigned int vold, vnew, edibit; - if (flow_type == IRQ_TYPE_NONE) - flow_type = IRQ_TYPE_LEVEL_LOW; - - if (flow_type & IRQ_TYPE_EDGE_RISING) { - printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", - flow_type); - return -EINVAL; + /* Port C interrupts are either IRQ_TYPE_EDGE_FALLING or + * IRQ_TYPE_EDGE_BOTH (default). All others are IRQ_TYPE_EDGE_FALLING + * or IRQ_TYPE_LEVEL_LOW (default) + */ + if (src >= CPM2_IRQ_PORTC15 && src <= CPM2_IRQ_PORTC0) { + if (flow_type == IRQ_TYPE_NONE) + flow_type = IRQ_TYPE_EDGE_BOTH; + + if ((flow_type != IRQ_TYPE_EDGE_BOTH) && + (flow_type != IRQ_TYPE_EDGE_FALLING)) { + printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", + flow_type); + return -EINVAL; + } + } else { + if (flow_type == IRQ_TYPE_NONE) + flow_type = IRQ_TYPE_LEVEL_LOW; + + if (flow_type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH)) { + printk(KERN_ERR "CPM2 PIC: sense type 0x%x not supported\n", + flow_type); + return -EINVAL; + } } desc->status &= ~(IRQ_TYPE_SENSE_MASK | IRQ_LEVEL);
Port C interrupts can be either falling edge, or either edge. Other external interrupts are either falling edge or active low. Signed-Off-By: Mark Ware <mware@elphinstone.net> --- Changed in v2: - Disallow rising edge only on Port C arch/powerpc/sysdev/cpm2_pic.c | 30 +++++++++++++++++++++++------- 1 files changed, 23 insertions(+), 7 deletions(-)