On 07/01/2014 06:04 PM, Marc Kleine-Budde wrote:
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:m...@pengutronix.de]
> Sent: Tuesday, July 01, 2014 6:04 PM
> To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; w...@grandegger.com;
> linux-...@vger.kernel.org; Wood Scott-B07421
> Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan
>
> On 07/01/2014 10:03 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 <b45...@freescale.com>
> > ---
> > Changes for v2:
> > - use a space instead of tab
> > - use flexcan_poll_state instead of print Changes for v3:
> > - return IRQ_HANDLED if err is triggered
> > - stop transmitted packets when there is an err_interrupt Changes
> for
> > v4:
> > - call flexcan_irq
> >
> > drivers/net/can/flexcan.c | 38 +++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index f425ec2..098fcac 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;
> > + int err_irq;
> >
> > struct clk *clk_ipg;
> > struct clk *clk_per;
> > @@ -744,6 +745,27 @@ 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_ALL_INT) {
> > + if (reg_esr & FLEXCAN_ESR_ERR_INT)
> > + flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
> > + ®s->ctrl);
> > + flexcan_irq(irq, dev);
>
> I still don't understand why you need a special flexcan_err_irq()
> function. Why don't you just call:
>
> request_irq(priv->err_irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
>
> instead?
Flexcan_irq is for flexcan normal interrupt(such as Message Buffer, Wake up and
so on).
And it will return IRQ_HANDLED if flexcan_irq is triggered.
But err_irq is shared with other devices, it should return IRQ_HANDLED when the
interrupt
is triggered by flexcan device, if not return IRQ_NONE.
>
> > + return IRQ_HANDLED;
> > + }
> > +
> > + return IRQ_NONE;
> > +}
> > +
> > static void flexcan_set_bittiming(struct net_device *dev) {
> > const struct flexcan_priv *priv = netdev_priv(dev); @@ -944,6
> > +966,15 @@ 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) {
> > + free_irq(priv->err_irq, dev);
>
> Why do you want to free the err_irq, if requesting the err_irq fails?
> However you should adjust all error cleanup path following this request
> irq, so that it will be freed.
>
> BTW: where is the corresponding free_irq() in flexcan_close()?
Here, I think you are right. Thanks for your reminder.
I will modify it.
>
> > + goto out_free_irq;
> > + }
> > + }
> > +
> > /* start chip and queuing */
> > err = flexcan_chip_start(dev);
> > if (err)
> > @@ -1099,7 +1130,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 +1157,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 +1184,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
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
