[PATCH v4] flexcan: add err_irq handler for flexcan

[qiang.zhao at freescale.com]

On 07/01/2014 06:04 PM, Marc Kleine-Budde wrote:
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl at pengutronix.de]
> Sent: Tuesday, July 01, 2014 6:04 PM
> To: Zhao Qiang-B45475; linuxppc-dev at lists.ozlabs.org; wg at grandegger.com;
> linux-can at 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 <B45475 at 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(&regs->esr);
> > +	reg_ctrl = flexcan_read(&regs->ctrl);
> > +
> > +	if (reg_esr & FLEXCAN_ESR_ALL_INT) {
> > +		if (reg_esr & FLEXCAN_ESR_ERR_INT)
> > +			flexcan_write(reg_ctrl & ~FLEXCAN_CTRL_ERR_MSK,
> > +				      &regs->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   |