[PATCH v4] flexcan: add err_irq handler for flexcan

[PATCH v4] flexcan: add err_irq handler for flexcan

[Zhao Qiang]

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
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);
+		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);
+			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;
-- 
1.8.5

Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3896 bytes --]

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@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?

> +		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()?

> +			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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

RE: [PATCH v4] flexcan: add err_irq handler for flexcan

[qiang.zhao]

T24gMDcvMDEvMjAxNCAwNjowNCBQTSwgTWFyYyBLbGVpbmUtQnVkZGUgd3JvdGU6DQo+IC0tLS0t
T3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IE1hcmMgS2xlaW5lLUJ1ZGRlIFttYWlsdG86
bWtsQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiBUdWVzZGF5LCBKdWx5IDAxLCAyMDE0IDY6MDQg
UE0NCj4gVG86IFpoYW8gUWlhbmctQjQ1NDc1OyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9y
Zzsgd2dAZ3JhbmRlZ2dlci5jb207DQo+IGxpbnV4LWNhbkB2Z2VyLmtlcm5lbC5vcmc7IFdvb2Qg
U2NvdHQtQjA3NDIxDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjRdIGZsZXhjYW46IGFkZCBlcnJf
aXJxIGhhbmRsZXIgZm9yIGZsZXhjYW4NCj4gDQo+IE9uIDA3LzAxLzIwMTQgMTA6MDMgQU0sIFpo
YW8gUWlhbmcgd3JvdGU6DQo+ID4gd2hlbiBmbGV4Y2FuIGlzIG5vdCBwaHlzaWNhbGx5IGxpbmtl
ZCwgY29tbWFuZCAnY2FudGVzdCcgd2lsbCB0cmlnZ2VyDQo+ID4gYW4gZXJyX2lycSwgYWRkIGVy
cl9pcnEgaGFuZGxlciBmb3IgaXQuDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBaaGFvIFFpYW5n
IDxCNDU0NzVAZnJlZXNjYWxlLmNvbT4NCj4gPiAtLS0NCj4gPiBDaGFuZ2VzIGZvciB2MjoNCj4g
PiAJLSB1c2UgYSBzcGFjZSBpbnN0ZWFkIG9mIHRhYg0KPiA+IAktIHVzZSBmbGV4Y2FuX3BvbGxf
c3RhdGUgaW5zdGVhZCBvZiBwcmludCBDaGFuZ2VzIGZvciB2MzoNCj4gPiAJLSByZXR1cm4gSVJR
X0hBTkRMRUQgaWYgZXJyIGlzIHRyaWdnZXJlZA0KPiA+IAktIHN0b3AgdHJhbnNtaXR0ZWQgcGFj
a2V0cyB3aGVuIHRoZXJlIGlzIGFuIGVycl9pbnRlcnJ1cHQgQ2hhbmdlcw0KPiBmb3INCj4gPiB2
NDoNCj4gPiAJLSBjYWxsIGZsZXhjYW5faXJxDQo+ID4NCj4gPiAgZHJpdmVycy9uZXQvY2FuL2Zs
ZXhjYW4uYyB8IDM4ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKystDQo+ID4g
IDEgZmlsZSBjaGFuZ2VkLCAzNyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pDQo+ID4NCj4g
PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9uZXQvY2FuL2ZsZXhjYW4uYyBiL2RyaXZlcnMvbmV0L2Nh
bi9mbGV4Y2FuLmMNCj4gPiBpbmRleCBmNDI1ZWMyLi4wOThmY2FjIDEwMDY0NA0KPiA+IC0tLSBh
L2RyaXZlcnMvbmV0L2Nhbi9mbGV4Y2FuLmMNCj4gPiArKysgYi9kcml2ZXJzL25ldC9jYW4vZmxl
eGNhbi5jDQo+ID4gQEAgLTIwOCw2ICsyMDgsNyBAQCBzdHJ1Y3QgZmxleGNhbl9wcml2IHsNCj4g
PiAgCXZvaWQgX19pb21lbSAqYmFzZTsNCj4gPiAgCXUzMiByZWdfZXNyOw0KPiA+ICAJdTMyIHJl
Z19jdHJsX2RlZmF1bHQ7DQo+ID4gKwlpbnQgZXJyX2lycTsNCj4gPg0KPiA+ICAJc3RydWN0IGNs
ayAqY2xrX2lwZzsNCj4gPiAgCXN0cnVjdCBjbGsgKmNsa19wZXI7DQo+ID4gQEAgLTc0NCw2ICs3
NDUsMjcgQEAgc3RhdGljIGlycXJldHVybl90IGZsZXhjYW5faXJxKGludCBpcnEsIHZvaWQNCj4g
KmRldl9pZCkNCj4gPiAgCXJldHVybiBJUlFfSEFORExFRDsNCj4gPiAgfQ0KPiA+DQo+ID4gK3N0
YXRpYyBpcnFyZXR1cm5fdCBmbGV4Y2FuX2Vycl9pcnEoaW50IGlycSwgdm9pZCAqZGV2X2lkKSB7
DQo+ID4gKwlzdHJ1Y3QgbmV0X2RldmljZSAqZGV2ID0gZGV2X2lkOw0KPiA+ICsJc3RydWN0IGZs
ZXhjYW5fcHJpdiAqcHJpdiA9IG5ldGRldl9wcml2KGRldik7DQo+ID4gKwlzdHJ1Y3QgZmxleGNh
bl9yZWdzIF9faW9tZW0gKnJlZ3MgPSBwcml2LT5iYXNlOw0KPiA+ICsJdTMyIHJlZ19jdHJsLCBy
ZWdfZXNyOw0KPiA+ICsNCj4gPiArCXJlZ19lc3IgPSBmbGV4Y2FuX3JlYWQoJnJlZ3MtPmVzcik7
DQo+ID4gKwlyZWdfY3RybCA9IGZsZXhjYW5fcmVhZCgmcmVncy0+Y3RybCk7DQo+ID4gKw0KPiA+
ICsJaWYgKHJlZ19lc3IgJiBGTEVYQ0FOX0VTUl9BTExfSU5UKSB7DQo+ID4gKwkJaWYgKHJlZ19l
c3IgJiBGTEVYQ0FOX0VTUl9FUlJfSU5UKQ0KPiA+ICsJCQlmbGV4Y2FuX3dyaXRlKHJlZ19jdHJs
ICYgfkZMRVhDQU5fQ1RSTF9FUlJfTVNLLA0KPiA+ICsJCQkJICAgICAgJnJlZ3MtPmN0cmwpOw0K
PiA+ICsJCWZsZXhjYW5faXJxKGlycSwgZGV2KTsNCj4gDQo+IEkgc3RpbGwgZG9uJ3QgdW5kZXJz
dGFuZCB3aHkgeW91IG5lZWQgYSBzcGVjaWFsIGZsZXhjYW5fZXJyX2lycSgpDQo+IGZ1bmN0aW9u
LiBXaHkgZG9uJ3QgeW91IGp1c3QgY2FsbDoNCj4gDQo+IHJlcXVlc3RfaXJxKHByaXYtPmVycl9p
cnEsIGZsZXhjYW5faXJxLCBJUlFGX1NIQVJFRCwgZGV2LT5uYW1lLCBkZXYpOw0KPiANCj4gaW5z
dGVhZD8NCg0KRmxleGNhbl9pcnEgaXMgZm9yIGZsZXhjYW4gbm9ybWFsIGludGVycnVwdChzdWNo
IGFzIE1lc3NhZ2UgQnVmZmVyLCBXYWtlIHVwIGFuZCBzbyBvbikuDQpBbmQgaXQgd2lsbCByZXR1
cm4gSVJRX0hBTkRMRUQgaWYgZmxleGNhbl9pcnEgaXMgdHJpZ2dlcmVkLg0KQnV0IGVycl9pcnEg
aXMgc2hhcmVkIHdpdGggb3RoZXIgZGV2aWNlcywgaXQgc2hvdWxkIHJldHVybiBJUlFfSEFORExF
RCB3aGVuIHRoZSBpbnRlcnJ1cHQgDQppcyB0cmlnZ2VyZWQgYnkgZmxleGNhbiBkZXZpY2UsIGlm
IG5vdCByZXR1cm4gSVJRX05PTkUuDQoNCj4gDQo+ID4gKwkJcmV0dXJuIElSUV9IQU5ETEVEOw0K
PiA+ICsJfQ0KPiA+ICsNCj4gPiArCXJldHVybiBJUlFfTk9ORTsNCj4gPiArfQ0KPiA+ICsNCj4g
PiAgc3RhdGljIHZvaWQgZmxleGNhbl9zZXRfYml0dGltaW5nKHN0cnVjdCBuZXRfZGV2aWNlICpk
ZXYpICB7DQo+ID4gIAljb25zdCBzdHJ1Y3QgZmxleGNhbl9wcml2ICpwcml2ID0gbmV0ZGV2X3By
aXYoZGV2KTsgQEAgLTk0NCw2DQo+ID4gKzk2NiwxNSBAQCBzdGF0aWMgaW50IGZsZXhjYW5fb3Bl
bihzdHJ1Y3QgbmV0X2RldmljZSAqZGV2KQ0KPiA+ICAJaWYgKGVycikNCj4gPiAgCQlnb3RvIG91
dF9jbG9zZTsNCj4gPg0KPiA+ICsJaWYgKHByaXYtPmVycl9pcnEpIHsNCj4gPiArCQllcnIgPSBy
ZXF1ZXN0X2lycShwcml2LT5lcnJfaXJxLCBmbGV4Y2FuX2Vycl9pcnEsIElSUUZfU0hBUkVELA0K
PiA+ICsJCQkJICBkZXYtPm5hbWUsIGRldik7DQo+ID4gKwkJaWYgKGVycikgew0KPiA+ICsJCQlm
cmVlX2lycShwcml2LT5lcnJfaXJxLCBkZXYpOw0KPiANCj4gV2h5IGRvIHlvdSB3YW50IHRvIGZy
ZWUgdGhlIGVycl9pcnEsIGlmIHJlcXVlc3RpbmcgdGhlIGVycl9pcnEgZmFpbHM/DQo+IEhvd2V2
ZXIgeW91IHNob3VsZCBhZGp1c3QgYWxsIGVycm9yIGNsZWFudXAgcGF0aCBmb2xsb3dpbmcgdGhp
cyByZXF1ZXN0DQo+IGlycSwgc28gdGhhdCBpdCB3aWxsIGJlIGZyZWVkLg0KPiANCj4gQlRXOiB3
aGVyZSBpcyB0aGUgY29ycmVzcG9uZGluZyBmcmVlX2lycSgpIGluIGZsZXhjYW5fY2xvc2UoKT8N
Cg0KSGVyZSwgSSB0aGluayB5b3UgYXJlIHJpZ2h0LiBUaGFua3MgZm9yIHlvdXIgcmVtaW5kZXIu
DQpJIHdpbGwgbW9kaWZ5IGl0Lg0KDQo+IA0KPiA+ICsJCQlnb3RvIG91dF9mcmVlX2lycTsNCj4g
PiArCQl9DQo+ID4gKwl9DQo+ID4gKw0KPiA+ICAJLyogc3RhcnQgY2hpcCBhbmQgcXVldWluZyAq
Lw0KPiA+ICAJZXJyID0gZmxleGNhbl9jaGlwX3N0YXJ0KGRldik7DQo+ID4gIAlpZiAoZXJyKQ0K
PiA+IEBAIC0xMDk5LDcgKzExMzAsNyBAQCBzdGF0aWMgaW50IGZsZXhjYW5fcHJvYmUoc3RydWN0
IHBsYXRmb3JtX2RldmljZQ0KPiAqcGRldikNCj4gPiAgCXN0cnVjdCByZXNvdXJjZSAqbWVtOw0K
PiA+ICAJc3RydWN0IGNsayAqY2xrX2lwZyA9IE5VTEwsICpjbGtfcGVyID0gTlVMTDsNCj4gPiAg
CXZvaWQgX19pb21lbSAqYmFzZTsNCj4gPiAtCWludCBlcnIsIGlycTsNCj4gPiArCWludCBlcnIs
IGlycSwgZXJyX2lycTsNCj4gPiAgCXUzMiBjbG9ja19mcmVxID0gMDsNCj4gPg0KPiA+ICAJaWYg
KHBkZXYtPmRldi5vZl9ub2RlKQ0KPiA+IEBAIC0xMTI2LDYgKzExNTcsMTAgQEAgc3RhdGljIGlu
dCBmbGV4Y2FuX3Byb2JlKHN0cnVjdCBwbGF0Zm9ybV9kZXZpY2UNCj4gKnBkZXYpDQo+ID4gIAlp
ZiAoaXJxIDw9IDApDQo+ID4gIAkJcmV0dXJuIC1FTk9ERVY7DQo+ID4NCj4gPiArCWVycl9pcnEg
PSBwbGF0Zm9ybV9nZXRfaXJxKHBkZXYsIDEpOw0KPiA+ICsJaWYgKGVycl9pcnEgPD0gMCkNCj4g
PiArCQllcnJfaXJxID0gMDsNCj4gPiArDQo+ID4gIAliYXNlID0gZGV2bV9pb3JlbWFwX3Jlc291
cmNlKCZwZGV2LT5kZXYsIG1lbSk7DQo+ID4gIAlpZiAoSVNfRVJSKGJhc2UpKQ0KPiA+ICAJCXJl
dHVybiBQVFJfRVJSKGJhc2UpOw0KPiA+IEBAIC0xMTQ5LDYgKzExODQsNyBAQCBzdGF0aWMgaW50
IGZsZXhjYW5fcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZQ0KPiAqcGRldikNCj4gPiAgCWRl
di0+ZmxhZ3MgfD0gSUZGX0VDSE87DQo+ID4NCj4gPiAgCXByaXYgPSBuZXRkZXZfcHJpdihkZXYp
Ow0KPiA+ICsJcHJpdi0+ZXJyX2lycSA9IGVycl9pcnE7DQo+ID4gIAlwcml2LT5jYW4uY2xvY2su
ZnJlcSA9IGNsb2NrX2ZyZXE7DQo+ID4gIAlwcml2LT5jYW4uYml0dGltaW5nX2NvbnN0ID0gJmZs
ZXhjYW5fYml0dGltaW5nX2NvbnN0Ow0KPiA+ICAJcHJpdi0+Y2FuLmRvX3NldF9tb2RlID0gZmxl
eGNhbl9zZXRfbW9kZTsNCj4gPg0KPiANCj4gTWFyYw0KPiANCj4gLS0NCj4gUGVuZ3V0cm9uaXgg
ZS5LLiAgICAgICAgICAgICAgICAgIHwgTWFyYyBLbGVpbmUtQnVkZGUgICAgICAgICAgIHwNCj4g
SW5kdXN0cmlhbCBMaW51eCBTb2x1dGlvbnMgICAgICAgIHwgUGhvbmU6ICs0OS0yMzEtMjgyNi05
MjQgICAgIHwNCj4gVmVydHJldHVuZyBXZXN0L0RvcnRtdW5kICAgICAgICAgIHwgRmF4OiAgICs0
OS01MTIxLTIwNjkxNy01NTU1IHwNCj4gQW10c2dlcmljaHQgSGlsZGVzaGVpbSwgSFJBIDI2ODYg
IHwgaHR0cDovL3d3dy5wZW5ndXRyb25peC5kZSAgIHwNCg0K

Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

On 07/02/2014 04:00 AM, qiang.zhao@freescale.com wrote:
>>> +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.

What about fixing flexcan_irq() first and the make use of it?

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

RE: [PATCH v4] flexcan: add err_irq handler for flexcan

[qiang.zhao]

T24gMDcvMDIvMjAxNCAwMzowNCBQTSwgTWFyYyBLbGVpbmUtQnVkZGUgd3JvdGU6DQo+IC0tLS0t
T3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IE1hcmMgS2xlaW5lLUJ1ZGRlIFttYWlsdG86
bWtsQHBlbmd1dHJvbml4LmRlXQ0KPiBTZW50OiBXZWRuZXNkYXksIEp1bHkgMDIsIDIwMTQgMzow
NCBQTQ0KPiBUbzogWmhhbyBRaWFuZy1CNDU0NzU7IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMu
b3JnOyB3Z0BncmFuZGVnZ2VyLmNvbTsNCj4gbGludXgtY2FuQHZnZXIua2VybmVsLm9yZzsgV29v
ZCBTY290dC1CMDc0MjENCj4gU3ViamVjdDogUmU6IFtQQVRDSCB2NF0gZmxleGNhbjogYWRkIGVy
cl9pcnEgaGFuZGxlciBmb3IgZmxleGNhbg0KPiANCj4gT24gMDcvMDIvMjAxNCAwNDowMCBBTSwg
cWlhbmcuemhhb0BmcmVlc2NhbGUuY29tIHdyb3RlOg0KPiA+Pj4gK3N0YXRpYyBpcnFyZXR1cm5f
dCBmbGV4Y2FuX2Vycl9pcnEoaW50IGlycSwgdm9pZCAqZGV2X2lkKSB7DQo+ID4+PiArCXN0cnVj
dCBuZXRfZGV2aWNlICpkZXYgPSBkZXZfaWQ7DQo+ID4+PiArCXN0cnVjdCBmbGV4Y2FuX3ByaXYg
KnByaXYgPSBuZXRkZXZfcHJpdihkZXYpOw0KPiA+Pj4gKwlzdHJ1Y3QgZmxleGNhbl9yZWdzIF9f
aW9tZW0gKnJlZ3MgPSBwcml2LT5iYXNlOw0KPiA+Pj4gKwl1MzIgcmVnX2N0cmwsIHJlZ19lc3I7
DQo+ID4+PiArDQo+ID4+PiArCXJlZ19lc3IgPSBmbGV4Y2FuX3JlYWQoJnJlZ3MtPmVzcik7DQo+
ID4+PiArCXJlZ19jdHJsID0gZmxleGNhbl9yZWFkKCZyZWdzLT5jdHJsKTsNCj4gPj4+ICsNCj4g
Pj4+ICsJaWYgKHJlZ19lc3IgJiBGTEVYQ0FOX0VTUl9BTExfSU5UKSB7DQo+ID4+PiArCQlpZiAo
cmVnX2VzciAmIEZMRVhDQU5fRVNSX0VSUl9JTlQpDQo+ID4+PiArCQkJZmxleGNhbl93cml0ZShy
ZWdfY3RybCAmIH5GTEVYQ0FOX0NUUkxfRVJSX01TSywNCj4gPj4+ICsJCQkJICAgICAgJnJlZ3Mt
PmN0cmwpOw0KPiA+Pj4gKwkJZmxleGNhbl9pcnEoaXJxLCBkZXYpOw0KPiA+Pg0KPiA+PiBJIHN0
aWxsIGRvbid0IHVuZGVyc3RhbmQgd2h5IHlvdSBuZWVkIGEgc3BlY2lhbCBmbGV4Y2FuX2Vycl9p
cnEoKQ0KPiA+PiBmdW5jdGlvbi4gV2h5IGRvbid0IHlvdSBqdXN0IGNhbGw6DQo+ID4+DQo+ID4+
IHJlcXVlc3RfaXJxKHByaXYtPmVycl9pcnEsIGZsZXhjYW5faXJxLCBJUlFGX1NIQVJFRCwgZGV2
LT5uYW1lLCBkZXYpOw0KPiA+Pg0KPiA+PiBpbnN0ZWFkPw0KPiA+DQo+ID4gRmxleGNhbl9pcnEg
aXMgZm9yIGZsZXhjYW4gbm9ybWFsIGludGVycnVwdChzdWNoIGFzIE1lc3NhZ2UgQnVmZmVyLA0K
PiBXYWtlIHVwIGFuZCBzbyBvbikuDQo+ID4gQW5kIGl0IHdpbGwgcmV0dXJuIElSUV9IQU5ETEVE
IGlmIGZsZXhjYW5faXJxIGlzIHRyaWdnZXJlZC4NCj4gPiBCdXQgZXJyX2lycSBpcyBzaGFyZWQg
d2l0aCBvdGhlciBkZXZpY2VzLCBpdCBzaG91bGQgcmV0dXJuIElSUV9IQU5ETEVEDQo+ID4gd2hl
biB0aGUgaW50ZXJydXB0IGlzIHRyaWdnZXJlZCBieSBmbGV4Y2FuIGRldmljZSwgaWYgbm90IHJl
dHVybg0KPiBJUlFfTk9ORS4NCj4gDQo+IFdoYXQgYWJvdXQgZml4aW5nIGZsZXhjYW5faXJxKCkg
Zmlyc3QgYW5kIHRoZSBtYWtlIHVzZSBvZiBpdD8NCg0KRXJyX2lycSBpcyBhIHNoYXJlZCBpbnRl
cnJ1cHQgd2l0aCBvdGhlciBkZXZpY2UsIA0KSSBob3BlIHRoYXQgaXRzIGhhbmRsZXIgaXMgaW5k
ZXBlbmRlbnQuDQpIb3dldmVyLCBpZiB5b3UgcGVyc2lzdCBpbiB5b3VyIG9waW5pb24sIEkgd2ls
bCBkbyBpdCBhcyB5b3Ugc2FpZC4NCg0KPiANCj4gTWFyYw0KPiANCj4gLS0NCj4gUGVuZ3V0cm9u
aXggZS5LLiAgICAgICAgICAgICAgICAgIHwgTWFyYyBLbGVpbmUtQnVkZGUgICAgICAgICAgIHwN
Cj4gSW5kdXN0cmlhbCBMaW51eCBTb2x1dGlvbnMgICAgICAgIHwgUGhvbmU6ICs0OS0yMzEtMjgy
Ni05MjQgICAgIHwNCj4gVmVydHJldHVuZyBXZXN0L0RvcnRtdW5kICAgICAgICAgIHwgRmF4OiAg
ICs0OS01MTIxLTIwNjkxNy01NTU1IHwNCj4gQW10c2dlcmljaHQgSGlsZGVzaGVpbSwgSFJBIDI2
ODYgIHwgaHR0cDovL3d3dy5wZW5ndXRyb25peC5kZSAgIHwNCg0K

Re: [PATCH v4] flexcan: add err_irq handler for flexcan

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

On 07/02/2014 10:32 AM, qiang.zhao@freescale.com wrote:
> On 07/02/2014 03:04 PM, Marc Kleine-Budde wrote:
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
>> Sent: Wednesday, July 02, 2014 3:04 PM
>> To: Zhao Qiang-B45475; linuxppc-dev@lists.ozlabs.org; wg@grandegger.com;
>> linux-can@vger.kernel.org; Wood Scott-B07421
>> Subject: Re: [PATCH v4] flexcan: add err_irq handler for flexcan
>>
>> On 07/02/2014 04:00 AM, qiang.zhao@freescale.com wrote:
>>>>> +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.
>>
>> What about fixing flexcan_irq() first and the make use of it?
> 
> Err_irq is a shared interrupt with other device, 
> I hope that its handler is independent.
> However, if you persist in your opinion, I will do it as you said.

There is another option, you can move all of the error interrupt
handling code from flexcan_irq() to flexcan_irq_err(). To keep the ARM
SoCs supported, you need to call flexcan_irq_err() form the
flexcan_irq() handler.

What I don't want is a) code duplication and b) no fishy wrapper
functions around flexcan_irq() that work around flexcan_irq() returning
IRQ_HANDLED unconditionally.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]