RE: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan
From: Wood Scott-B07421
Sent: Wednesday, June 25, 2014 1:34 AM
To: Zhao Qiang-B45475
Cc: linuxppc-dev@lists.ozlabs.org; linux-...@vger.kernel.org;
w...@grandegger.com; m...@pengutronix.de
Subject: Re: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan
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-...@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
> > > ---
> > > 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.
OK, I will change it.
-Zhao
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan
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-...@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
> > > ---
> > > 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan
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
> > ---
> > 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
Re: [PATCH v2 1/2] flexcan: add err_irq handler for flexcan
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
> ---
> 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
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
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
> ---
> 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
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
