On Sat, 2014-06-21 at 12:19, Wood Scott wrote: > -----Original Message----- > From: Wood Scott-B07421 > Sent: Saturday, June 21, 2014 12:19 AM > To: Zhao Qiang-B45475 > Cc: linuxppc-dev@lists.ozlabs.org; linux-...@vger.kernel.org; > w...@grandegger.com; m...@pengutronix.de; Wood Scott-B07421 > Subject: Re: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan > > On Fri, 2014-06-20 at 10:01 +0800, Zhao Qiang wrote: > > when flexcan is not physically linked, command 'cantest' will trigger > > an err_irq, add err_irq handler for it. > > > > Signed-off-by: Zhao Qiang <b45...@freescale.com> > > --- > > Changes for v2: > > - use a space instead of tab > > - use flexcan_poll_state instead of print > > > > drivers/net/can/flexcan.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index f425ec2..7432ba4 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -208,6 +208,7 @@ struct flexcan_priv { > > void __iomem *base; > > u32 reg_esr; > > u32 reg_ctrl_default; > > + unsigned int err_irq; > > Why unsigned? Err_irq is from 0. > > > +static irqreturn_t flexcan_err_irq(int irq, void *dev_id) { > > + struct net_device *dev = dev_id; > > + struct flexcan_priv *priv = netdev_priv(dev); > > + struct flexcan_regs __iomem *regs = priv->base; > > + u32 reg_ctrl, reg_esr; > > + > > + reg_esr = flexcan_read(®s->esr); > > + reg_ctrl = flexcan_read(®s->ctrl); > > + if (reg_esr & FLEXCAN_ESR_TX_WRN) { > > + flexcan_write(reg_esr & ~FLEXCAN_ESR_TX_WRN, ®s->esr); > > + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK, ®s->ctrl); > > + flexcan_poll_state(dev, reg_esr); > > + } > > + return IRQ_HANDLED; > > +} > > You should only return IRQ_HANDLED if there was something to handle. > > > @@ -944,6 +962,12 @@ static int flexcan_open(struct net_device *dev) > > if (err) > > goto out_close; > > > > + if (priv->err_irq) > > + err = request_irq(priv->err_irq, flexcan_err_irq, IRQF_SHARED, > > + dev->name, dev); > > + if (err) > > + goto out_close; > > Is this really a fatal error? And why do you check err outside the "if > (priv->err_irq)" block? What if some previous code left err non-zero > (either now or after some future code change)? > > > @@ -1126,6 +1150,10 @@ static int flexcan_probe(struct platform_device > *pdev) > > if (irq <= 0) > > return -ENODEV; > > > > + err_irq = platform_get_irq(pdev, 1); > > + if (err_irq <= 0) > > + err_irq = 0; > > + > > Why is this <= 0 check needed?
Interrupt[1] is optional. If there is not interrupt[1] in dtb, err_irq will be <=0. > > -Scott > _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev