| Message ID | 4B27D87D.5070908@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
>interruptTransmit, -Receive and -Error are unsigned so the >tests did not work. > >This also changes that if irq_of_parse_and_map(np, 0) returns >NO_IRQ, that >gfar_parse_group() returns an -EINVAL. > >Signed-off-by: Roel Kluin <roel.kluin@gmail.com> >--- >Found using coccinelle: http://coccinelle.lip6.fr/ > >diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c >index 6850dc0..c411682 100644 >--- a/drivers/net/gianfar.c >+++ b/drivers/net/gianfar.c >@@ -522,6 +522,7 @@ static int gfar_parse_group(struct >device_node *np, { > u32 *queue_mask; > u64 addr, size; >+ int irq; > > addr = of_translate_address(np, > of_get_address(np, 0, &size, NULL)); @@ >-530,20 +531,22 @@ static int gfar_parse_group(struct device_node *np, > if (!priv->gfargrp[priv->num_grps].regs) > return -ENOMEM; > >- priv->gfargrp[priv->num_grps].interruptTransmit = >- irq_of_parse_and_map(np, 0); >+ irq = irq_of_parse_and_map(np, 0); >+ if (irq < 0) >+ return -EINVAL; >+ priv->gfargrp[priv->num_grps].interruptTransmit = irq; > > /* If we aren't the FEC we have multiple interrupts */ > if (model && strcasecmp(model, "FEC")) { >- priv->gfargrp[priv->num_grps].interruptReceive = >- irq_of_parse_and_map(np, 1); >- priv->gfargrp[priv->num_grps].interruptError = >- irq_of_parse_and_map(np,2); >- if >(priv->gfargrp[priv->num_grps].interruptTransmit < 0 || >- >priv->gfargrp[priv->num_grps].interruptReceive < 0 || >- >priv->gfargrp[priv->num_grps].interruptError < 0) { >+ irq = irq_of_parse_and_map(np, 1); >+ if (irq < 0) > return -EINVAL; >- } >+ priv->gfargrp[priv->num_grps].interruptReceive = irq; >+ >+ irq = irq_of_parse_and_map(np, 2); >+ if (irq < 0) >+ return -EINVAL; >+ priv->gfargrp[priv->num_grps].interruptError = irq; > } > > priv->gfargrp[priv->num_grps].grp_id = priv->num_grps; Won't it be better to change types of interruptTransmit, -Receive and -Error in struct gfar_priv_grp itself. Something like this: struct gfar_priv_grp { .... - unsigned int interruptTransmit; - unsigned int interruptReceive; - unsigned int interruptError; + unsigned int interruptTransmit; + unsigned int interruptReceive; + unsigned int interruptError; .... } -- Thanks Sandeep -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>Won't it be better to change types of interruptTransmit, >-Receive and -Error in struct gfar_priv_grp itself. Something >like this: > >struct gfar_priv_grp { >.... >- unsigned int interruptTransmit; >- unsigned int interruptReceive; >- unsigned int interruptError; > >+ unsigned int interruptTransmit; >+ unsigned int interruptReceive; >+ unsigned int interruptError; >.... >} > Sorry for my earlier mail, What I meant is something like this: struct gfar_priv_grp { .... - unsigned int interruptTransmit; - unsigned int interruptReceive; - unsigned int interruptError; + int interruptTransmit; + int interruptReceive; + int interruptError; .... } -- Thanks Sandeep -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 16, 2009 at 6:04 AM, Kumar Gopalpet-B05799 <B05799@freescale.com> wrote: > >>Won't it be better to change types of interruptTransmit, >>-Receive and -Error in struct gfar_priv_grp itself. Something >>like this: > struct gfar_priv_grp { > .... > - unsigned int interruptTransmit; > - unsigned int interruptReceive; > - unsigned int interruptError; > > + int interruptTransmit; > + int interruptReceive; > + int interruptError; > .... > } > > -- > > Thanks > Sandeep I don't think that's better. Only the error returned is signed, If none occurred the irq thereafter is unsigned. Thanks, Roel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c index 6850dc0..c411682 100644 --- a/drivers/net/gianfar.c +++ b/drivers/net/gianfar.c @@ -522,6 +522,7 @@ static int gfar_parse_group(struct device_node *np, { u32 *queue_mask; u64 addr, size; + int irq; addr = of_translate_address(np, of_get_address(np, 0, &size, NULL)); @@ -530,20 +531,22 @@ static int gfar_parse_group(struct device_node *np, if (!priv->gfargrp[priv->num_grps].regs) return -ENOMEM; - priv->gfargrp[priv->num_grps].interruptTransmit = - irq_of_parse_and_map(np, 0); + irq = irq_of_parse_and_map(np, 0); + if (irq < 0) + return -EINVAL; + priv->gfargrp[priv->num_grps].interruptTransmit = irq; /* If we aren't the FEC we have multiple interrupts */ if (model && strcasecmp(model, "FEC")) { - priv->gfargrp[priv->num_grps].interruptReceive = - irq_of_parse_and_map(np, 1); - priv->gfargrp[priv->num_grps].interruptError = - irq_of_parse_and_map(np,2); - if (priv->gfargrp[priv->num_grps].interruptTransmit < 0 || - priv->gfargrp[priv->num_grps].interruptReceive < 0 || - priv->gfargrp[priv->num_grps].interruptError < 0) { + irq = irq_of_parse_and_map(np, 1); + if (irq < 0) return -EINVAL; - } + priv->gfargrp[priv->num_grps].interruptReceive = irq; + + irq = irq_of_parse_and_map(np, 2); + if (irq < 0) + return -EINVAL; + priv->gfargrp[priv->num_grps].interruptError = irq; } priv->gfargrp[priv->num_grps].grp_id = priv->num_grps;
interruptTransmit, -Receive and -Error are unsigned so the tests did not work. This also changes that if irq_of_parse_and_map(np, 0) returns NO_IRQ, that gfar_parse_group() returns an -EINVAL. Signed-off-by: Roel Kluin <roel.kluin@gmail.com> --- Found using coccinelle: http://coccinelle.lip6.fr/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html