| Message ID | 1403229664-33912-1-git-send-email-B45475@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 <B45475@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? > +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? -Scott
On 06/20/2014 04:01 AM, 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 <B45475@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; > > struct clk *clk_ipg; > struct clk *clk_per; > @@ -744,6 +745,23 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +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) { When is the err_irq triggered? > + 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); poll_state() is does not work, if called from an IRQ handler.....Depending when err_irq is triggered, is it worth to just call the normal interrupt handler? > + } > + return IRQ_HANDLED; Please return IRQ_HANDLED, only if there really was an interrupt. > +} > + > static void flexcan_set_bittiming(struct net_device *dev) > { > const struct flexcan_priv *priv = netdev_priv(dev); > @@ -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; In case of an error, you don't free() dev->irq. > + > /* start chip and queuing */ > err = flexcan_chip_start(dev); > if (err) > @@ -1099,7 +1123,7 @@ static int flexcan_probe(struct platform_device *pdev) > struct resource *mem; > struct clk *clk_ipg = NULL, *clk_per = NULL; > void __iomem *base; > - int err, irq; > + int err, irq, err_irq; > u32 clock_freq = 0; > > if (pdev->dev.of_node) > @@ -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; > + > base = devm_ioremap_resource(&pdev->dev, mem); > if (IS_ERR(base)) > return PTR_ERR(base); > @@ -1149,6 +1177,7 @@ static int flexcan_probe(struct platform_device *pdev) > dev->flags |= IFF_ECHO; > > priv = netdev_priv(dev); > + priv->err_irq = err_irq; > priv->can.clock.freq = clock_freq; > priv->can.bittiming_const = &flexcan_bittiming_const; > priv->can.do_set_mode = flexcan_set_mode; > Marc
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-can@vger.kernel.org; > wg@grandegger.com; mkl@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 <B45475@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 >
On Mon, 2014-06-23 at 01:20 -0500, Zhao Qiang-B45475 wrote: > 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-can@vger.kernel.org; > > wg@grandegger.com; mkl@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 <B45475@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. So? irqs are plain "int" almost everywhere in the kernel. -Scott
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; struct clk *clk_ipg; struct clk *clk_per; @@ -744,6 +745,23 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id) return IRQ_HANDLED; } +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; +} + static void flexcan_set_bittiming(struct net_device *dev) { const struct flexcan_priv *priv = netdev_priv(dev); @@ -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; + /* start chip and queuing */ err = flexcan_chip_start(dev); if (err) @@ -1099,7 +1123,7 @@ static int flexcan_probe(struct platform_device *pdev) struct resource *mem; struct clk *clk_ipg = NULL, *clk_per = NULL; void __iomem *base; - int err, irq; + int err, irq, err_irq; u32 clock_freq = 0; if (pdev->dev.of_node) @@ -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; + base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(base)) return PTR_ERR(base); @@ -1149,6 +1177,7 @@ static int flexcan_probe(struct platform_device *pdev) dev->flags |= IFF_ECHO; priv = netdev_priv(dev); + priv->err_irq = err_irq; priv->can.clock.freq = clock_freq; priv->can.bittiming_const = &flexcan_bittiming_const; priv->can.do_set_mode = flexcan_set_mode;
when flexcan is not physically linked, command 'cantest' will trigger an err_irq, add err_irq handler for it. Signed-off-by: Zhao Qiang <B45475@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(-)