Fwd: [Socketcan-users] sja1000_platform interrupt sharing

Fwd: [Socketcan-users] sja1000_platform interrupt sharing

[Marc Kleine-Budde]

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

Hello Sven,

please use our new mailinglist linux-can@vger.kernel.org (Cc'ed), and please don't send HTML emails.

-------- Original Message --------
Subject: [Socketcan-users] sja1000_platform interrupt sharing
Date: Thu, 9 Aug 2012 12:43:38 +0200
From: Schmitt, Sven (EVM/8) <Sven.Schmitt@volkswagen.de>
To: <socketcan-users@lists.berlios.de>

Hi,

I've found something confusing when I wanted to use more than one
sja1000 attached to a platform bus with interrupt sharing. The flag for
the sja1000 resources I wanted to set first was the
IORESOURCE_IRQ_SHAREABLE* flag because of the other IORESOURCE_IRQ_XXX
flags. But this flag is ignored by the sja1000_platform driver because
it has another position. If I use the generic IRQF_SHARED** flag
everything works fine. Does it make sense to patch the driver in a way
like that:

--- linux-3.5/drivers/net/can/sja1000/sja1000_platform.c.orig
2012-08-09 10:38:57.405381025 +0200
+++ linux-3.5/drivers/net/can/sja1000/sja1000_platform.c
2012-08-09 10:57:22.693364557 +0200
@@ -109,6 +109,8 @@
 	priv = netdev_priv(dev);
 
 	dev->irq = res_irq->start;
+	if(res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
+		priv->irq_flags |= IRQF_SHARED;
 	priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK |
IRQF_SHARED);
 	priv->reg_base = addr;
 	/* The CAN clock frequency is half the oscillator clock
frequency */



*
include/linux/ioport.h:
#define IORESOURCE_IRQ_SHAREABLE
<http://lxr.free-electrons.com/ident?i=IORESOURCE_IRQ_SHAREABLE>
(1<<4)

**
include/linux/interrupt.h:
#define IRQF_SHARED <http://lxr.free-electrons.com/ident?i=IRQF_SHARED>
0x00000080 


Best regards,
Sven Schmitt


-- 
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: 262 bytes --]

Re: Fwd: [Socketcan-users] sja1000_platform interrupt sharing

[Marc Kleine-Budde]

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

On 08/09/2012 01:21 PM, Marc Kleine-Budde wrote:
> I've found something confusing when I wanted to use more than one

It's not confusing, it's a bug. Good catch.

> sja1000 attached to a platform bus with interrupt sharing. The flag for
> the sja1000 resources I wanted to set first was the
> IORESOURCE_IRQ_SHAREABLE* flag because of the other IORESOURCE_IRQ_XXX
> flags. But this flag is ignored by the sja1000_platform driver because
> it has another position. If I use the generic IRQF_SHARED** flag
> everything works fine. Does it make sense to patch the driver in a way
> like that:
> 
> --- linux-3.5/drivers/net/can/sja1000/sja1000_platform.c.orig
> 2012-08-09 10:38:57.405381025 +0200
> +++ linux-3.5/drivers/net/can/sja1000/sja1000_platform.c
> 2012-08-09 10:57:22.693364557 +0200
> @@ -109,6 +109,8 @@
>  	priv = netdev_priv(dev);
>  
>  	dev->irq = res_irq->start;
> +	if(res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
> +		priv->irq_flags |= IRQF_SHARED;

Please adjust to kernel coding style, space between if and opening bracket.

>  	priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK |
> IRQF_SHARED);

Please remove the IRQF_SHARED here, as IRQF_SHARED is wrong here and
means something different.

>  	priv->reg_base = addr;
>  	/* The CAN clock frequency is half the oscillator clock
> frequency */

If you have a look at the history of the file, it shows that this flag
was added Yegor Yefremov <yegor_sub1@visionsystems.de> in commit

abde89d can: sja1000: allow shared interrupt definition

Please resend your patch, with the problems fixed, use git send-email
for this, add Yegor on Cc and Sign your work [1]. If this is too
complicated for you, I'll fix it myself.

Marc

[1]
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L298
-- 
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: 262 bytes --]

Re: Fwd: [Socketcan-users] sja1000_platform interrupt sharing

[Marc Kleine-Budde]

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

On 08/09/2012 03:13 PM, Yegor Yefremov wrote:
[...]

>>> O.K. Nice catch. I just picked the wrong header file
>>> include/linux/interrupt.h, it should be include/linux/ioport.h. Now
>>> I'll have to change my board file as well.
>> A quick serach showed no mainline board using shared irq, have I missed one?
> 
> You've missed one board in our own repos. We had to much of a custom
> code to try to mainline this board file.

No mainline no breakage :)


>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> You mean Acked-by?
> 
> Yes.
> 
> Acked-by: Yegor Yefremov <yegorslists@googlemail.com>

Thanks, 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: 262 bytes --]

Re: Fwd: [Socketcan-users] sja1000_platform interrupt sharing

[Yegor Yefremov]

Am 09.08.2012 15:01, schrieb Marc Kleine-Budde:
> On 08/09/2012 02:56 PM, Yegor Yefremov wrote:
>> Am 09.08.2012 14:46, schrieb Schmitt, Sven (EVM/8):
>>> Second try:
>>>
>>> Signed-off-by: Sven Schmitt <sven.schmitt@volkswagen.de>
>>>
>>> --- linux-3.5/drivers/net/can/sja1000/sja1000_platform.c.orig
>>> +++ linux-3.5/drivers/net/can/sja1000/sja1000_platform.c
>>> @@ -109,7 +109,9 @@ static int sp_probe(struct platform_devi
>>>  	priv = netdev_priv(dev);
>>>  
>>>  	dev->irq = res_irq->start;
>>> -	priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK |
>>> IRQF_SHARED);
>>> +	priv->irq_flags &= IRQF_TRIGGER_MASK;
>>> +	if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
>>> +		priv->irq_flags |= IRQF_SHARED;
>>>  	priv->reg_base = addr;
>>>  	/* The CAN clock frequency is half the oscillator clock
>>> frequency */
>>>  	priv->can.clock.freq = pdata->osc_freq / 2;
>>>
>>>
>>> note: patch is compile tested only.
>>>
>>> Sven
>> O.K. Nice catch. I just picked the wrong header file
>> include/linux/interrupt.h, it should be include/linux/ioport.h. Now
>> I'll have to change my board file as well.
> A quick serach showed no mainline board using shared irq, have I missed one?

You've missed one board in our own repos. We had to much of a custom code to try to mainline this board file.

>> You can add my
>>
>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> You mean Acked-by?

Yes.

Acked-by: Yegor Yefremov <yegorslists@googlemail.com>


Yegor

Re: Fwd: [Socketcan-users] sja1000_platform interrupt sharing

[Marc Kleine-Budde]

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

On 08/09/2012 02:56 PM, Yegor Yefremov wrote:
> Am 09.08.2012 14:46, schrieb Schmitt, Sven (EVM/8):
>> Second try:
>>
>> Signed-off-by: Sven Schmitt <sven.schmitt@volkswagen.de>
>>
>> --- linux-3.5/drivers/net/can/sja1000/sja1000_platform.c.orig
>> +++ linux-3.5/drivers/net/can/sja1000/sja1000_platform.c
>> @@ -109,7 +109,9 @@ static int sp_probe(struct platform_devi
>>  	priv = netdev_priv(dev);
>>  
>>  	dev->irq = res_irq->start;
>> -	priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK |
>> IRQF_SHARED);
>> +	priv->irq_flags &= IRQF_TRIGGER_MASK;
>> +	if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
>> +		priv->irq_flags |= IRQF_SHARED;
>>  	priv->reg_base = addr;
>>  	/* The CAN clock frequency is half the oscillator clock
>> frequency */
>>  	priv->can.clock.freq = pdata->osc_freq / 2;
>>
>>
>> note: patch is compile tested only.
>>
>> Sven
> 
> O.K. Nice catch. I just picked the wrong header file
> include/linux/interrupt.h, it should be include/linux/ioport.h. Now
> I'll have to change my board file as well.

A quick serach showed no mainline board using shared irq, have I missed one?

> You can add my
> 
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>

You mean Acked-by?

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: 262 bytes --]

Re: Fwd: [Socketcan-users] sja1000_platform interrupt sharing

[Yegor Yefremov]

Am 09.08.2012 14:46, schrieb Schmitt, Sven (EVM/8):
> Second try:
>
> Signed-off-by: Sven Schmitt <sven.schmitt@volkswagen.de>
>
> --- linux-3.5/drivers/net/can/sja1000/sja1000_platform.c.orig
> +++ linux-3.5/drivers/net/can/sja1000/sja1000_platform.c
> @@ -109,7 +109,9 @@ static int sp_probe(struct platform_devi
>  	priv = netdev_priv(dev);
>  
>  	dev->irq = res_irq->start;
> -	priv->irq_flags = res_irq->flags & (IRQF_TRIGGER_MASK |
> IRQF_SHARED);
> +	priv->irq_flags &= IRQF_TRIGGER_MASK;
> +	if (res_irq->flags & IORESOURCE_IRQ_SHAREABLE)
> +		priv->irq_flags |= IRQF_SHARED;
>  	priv->reg_base = addr;
>  	/* The CAN clock frequency is half the oscillator clock
> frequency */
>  	priv->can.clock.freq = pdata->osc_freq / 2;
>
>
> note: patch is compile tested only.
>
> Sven

O.K. Nice catch. I just picked the wrong header file include/linux/interrupt.h, it should be include/linux/ioport.h. Now I'll have to change my board file as well.

You can add my

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>

See how I manage patches with git: https://libbits.wordpress.com/2011/12/27/managing-patch-series-with-git/

Yegor