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 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.
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
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 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.
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 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(regs-esr);
+ reg_ctrl = flexcan_read(regs-ctrl);
+ if (reg_esr FLEXCAN_ESR_TX_WRN) {
+ flexcan_write(reg_esr ~FLEXCAN_ESR_TX_WRN, regs-esr);
+ flexcan_write(reg_ctrl ~FLEXCAN_CTRL_ERR_MSK, regs-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 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;
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(regs-esr);
+ reg_ctrl = flexcan_read(regs-ctrl);
+ if (reg_esr FLEXCAN_ESR_TX_WRN) {
When is the err_irq triggered?
+ flexcan_write(reg_esr ~FLEXCAN_ESR_TX_WRN, regs-esr);
+ flexcan_write(reg_ctrl ~FLEXCAN_CTRL_ERR_MSK, regs-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 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?
+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_TX_WRN) {
+ flexcan_write(reg_esr ~FLEXCAN_ESR_TX_WRN, regs-esr);
+ flexcan_write(reg_ctrl ~FLEXCAN_CTRL_ERR_MSK, regs-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
