[PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[Mike Frysinger]

From: Michael Hennerich <michael.hennerich@analog.com>

Rather than have the default IRQ flags not be usable, use the logical
settings which match the hardware.  Since the part is /INT, it makes sense
to have it default to a falling trigger.  If a board truly needs something
different, it still can tweak the defaults in its own board file.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
CC: Baruch Siach <baruch@tkos.co.il>
---
 drivers/net/enc28j60.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/enc28j60.c b/drivers/net/enc28j60.c
index 36cb6e9..fbe2012 100644
--- a/drivers/net/enc28j60.c
+++ b/drivers/net/enc28j60.c
@@ -1562,7 +1562,7 @@ static int __devinit enc28j60_probe(struct spi_device *spi)
 	/* Board setup must set the relevant edge trigger type;
 	 * level triggers won't currently work.
 	 */
-	ret = request_irq(spi->irq, enc28j60_irq, 0, DRV_NAME, priv);
+	ret = request_irq(spi->irq, enc28j60_irq, IRQF_TRIGGER_FALLING, DRV_NAME, priv);
 	if (ret < 0) {
 		if (netif_msg_probe(priv))
 			dev_err(&spi->dev, DRV_NAME ": request irq %d failed "
-- 
1.6.3.1

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[Baruch Siach]

Hi Michael,

On Tue, May 26, 2009 at 06:16:47AM -0400, Mike Frysinger wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Rather than have the default IRQ flags not be usable, use the logical
> settings which match the hardware.  Since the part is /INT, it makes sense
> to have it default to a falling trigger.  If a board truly needs something
> different, it still can tweak the defaults in its own board file.

You revert David Brownell's commit here 
(c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.

> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Bryan Wu <cooloney@kernel.org>
> CC: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/net/enc28j60.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/enc28j60.c b/drivers/net/enc28j60.c
> index 36cb6e9..fbe2012 100644
> --- a/drivers/net/enc28j60.c
> +++ b/drivers/net/enc28j60.c
> @@ -1562,7 +1562,7 @@ static int __devinit enc28j60_probe(struct spi_device *spi)
>  	/* Board setup must set the relevant edge trigger type;
>  	 * level triggers won't currently work.
>  	 */
> -	ret = request_irq(spi->irq, enc28j60_irq, 0, DRV_NAME, priv);
> +	ret = request_irq(spi->irq, enc28j60_irq, IRQF_TRIGGER_FALLING, DRV_NAME, priv);
>  	if (ret < 0) {
>  		if (netif_msg_probe(priv))
>  			dev_err(&spi->dev, DRV_NAME ": request irq %d failed "

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[David Brownell]

On Tuesday 26 May 2009, Baruch Siach wrote:
> Hi Michael,
> 
> On Tue, May 26, 2009 at 06:16:47AM -0400, Mike Frysinger wrote:
> > From: Michael Hennerich <michael.hennerich@analog.com>
> > 
> > Rather than have the default IRQ flags not be usable, use the logical
> > settings which match the hardware.  Since the part is /INT, it makes sense
> > to have it default to a falling trigger.  If a board truly needs something
> > different, it still can tweak the defaults in its own board file.
> 
> You revert David Brownell's commit here 
> (c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.

Right ... the appended patch will *undo* the setting
established in the board init code via set_irq_type().

Moreover, it completely prevents working on systems which
only support "both edges" triggering.

So I'd NAK this, on the grounds that it causes severe
breakage.



> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > Signed-off-by: Bryan Wu <cooloney@kernel.org>
> > CC: Baruch Siach <baruch@tkos.co.il>
> > ---
> >  drivers/net/enc28j60.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/enc28j60.c b/drivers/net/enc28j60.c
> > index 36cb6e9..fbe2012 100644
> > --- a/drivers/net/enc28j60.c
> > +++ b/drivers/net/enc28j60.c
> > @@ -1562,7 +1562,7 @@ static int __devinit enc28j60_probe(struct spi_device *spi)
> >  	/* Board setup must set the relevant edge trigger type;
> >  	 * level triggers won't currently work.
> >  	 */
> > -	ret = request_irq(spi->irq, enc28j60_irq, 0, DRV_NAME, priv);
> > +	ret = request_irq(spi->irq, enc28j60_irq, IRQF_TRIGGER_FALLING, DRV_NAME, priv);
> >  	if (ret < 0) {
> >  		if (netif_msg_probe(priv))
> >  			dev_err(&spi->dev, DRV_NAME ": request irq %d failed "
> 
> baruch
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
> 
> 

RE: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[Hennerich, Michael]

>From: David Brownell [mailto:david-b@pacbell.net]
>Sent: Tuesday, May 26, 2009 1:24 PM
>On Tuesday 26 May 2009, Baruch Siach wrote:
>> Hi Michael,
>>
>> On Tue, May 26, 2009 at 06:16:47AM -0400, Mike Frysinger wrote:
>> > From: Michael Hennerich <michael.hennerich@analog.com>
>> >
>> > Rather than have the default IRQ flags not be usable, use the
logical
>> > settings which match the hardware.  Since the part is /INT, it
makes sense
>> > to have it default to a falling trigger.  If a board truly needs
something
>> > different, it still can tweak the defaults in its own board file.
>>
>> You revert David Brownell's commit here
>> (c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.
>
>Right ... the appended patch will *undo* the setting
>established in the board init code via set_irq_type().
>
>Moreover, it completely prevents working on systems which
>only support "both edges" triggering.
>
>So I'd NAK this, on the grounds that it causes severe
>breakage.

David,

The comment block above request_irq states:

>> >  	 * level triggers won't currently work.

And I guess it's because IRQ handling is deferred to a work queue.
I wonder how IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING will work in
such a case.
Since the ENC28J60 /INT will de-assert during the work handler executes,
thus causing a second spurious interrupt.

-Michael 


>
>
>
>> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> > Signed-off-by: Bryan Wu <cooloney@kernel.org>
>> > CC: Baruch Siach <baruch@tkos.co.il>
>> > ---
>> >  drivers/net/enc28j60.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/net/enc28j60.c b/drivers/net/enc28j60.c
>> > index 36cb6e9..fbe2012 100644
>> > --- a/drivers/net/enc28j60.c
>> > +++ b/drivers/net/enc28j60.c
>> > @@ -1562,7 +1562,7 @@ static int __devinit enc28j60_probe(struct
spi_device *spi)
>> >  	/* Board setup must set the relevant edge trigger type;
>> >  	 * level triggers won't currently work.
>> >  	 */
>> > -	ret = request_irq(spi->irq, enc28j60_irq, 0, DRV_NAME, priv);
>> > +	ret = request_irq(spi->irq, enc28j60_irq, IRQF_TRIGGER_FALLING,
DRV_NAME, priv);
>> >  	if (ret < 0) {
>> >  		if (netif_msg_probe(priv))
>> >  			dev_err(&spi->dev, DRV_NAME ": request irq %d
failed "
>>
>> baruch
>>
>> --
>>                                                      ~. .~   Tk Open
Systems
>>
=}------------------------------------------------ooO--U--Ooo-----------
-{=
>>    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il
-
>>
>>
>
>

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[Mike Frysinger]

On Tuesday 26 May 2009 07:24:17 David Brownell wrote:
> On Tuesday 26 May 2009, Baruch Siach wrote:
> > On Tue, May 26, 2009 at 06:16:47AM -0400, Mike Frysinger wrote:
> > > From: Michael Hennerich <michael.hennerich@analog.com>
> > >
> > > Rather than have the default IRQ flags not be usable, use the logical
> > > settings which match the hardware.  Since the part is /INT, it makes
> > > sense to have it default to a falling trigger.  If a board truly needs
> > > something different, it still can tweak the defaults in its own board
> > > file.
> >
> > You revert David Brownell's commit here
> > (c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.
>
> Right ... the appended patch will *undo* the setting
> established in the board init code via set_irq_type().
>
> Moreover, it completely prevents working on systems which
> only support "both edges" triggering.

how can it provide breakage when people are forced to use set_irq_type() ?

> So I'd NAK this, on the grounds that it causes severe
> breakage.

i see no consumers of this driver at all in mainline, so i dont think it 
causes any in tree breakage at all
-mike

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[David Brownell]

On Tuesday 26 May 2009, Mike Frysinger wrote:
> > > You revert David Brownell's commit here
> > > (c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.
> >
> > Right ... the appended patch will *undo* the setting
> > established in the board init code via set_irq_type().
> >
> > Moreover, it completely prevents working on systems which
> > only support "both edges" triggering.
> 
> how can it provide breakage when people are forced to use set_irq_type() ?

It reverses the IRQ setup done by the board code ...
the code which you said they'd have to use to work
around the breakage your patch adds.

If you're really craving some change to the driver,
you should make it take the relevant trigger type
from the platform resource ... instead of hard-wiring
it to a value which is *known* to break systems.

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[David Brownell]

On Tuesday 26 May 2009, Hennerich, Michael wrote:
> The comment block above request_irq states:
> 
> >> >     * level triggers won't currently work.
> 
> And I guess it's because IRQ handling is deferred to a work queue.

The IRQ needs to no longer be pending when the (hard) handler returns.
For level triggers, that means disabling the IRQ ... then re-enabling
it only when the (soft/threaded) handler returns.


> I wonder how IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING will work in
> such a case.

It works just fine, since when the (hard) handler returns there is
no longer "IRQ pending" status in the IRQ controller.


> Since the ENC28J60 /INT will de-assert during the work handler executes,
> thus causing a second spurious interrupt.

No problem.  The Ethernet adapter has no pending (unless another IRQ
triggered meanwhile); its workqueue/threaded handler returns immediately.

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[Mike Frysinger]

On Tuesday 26 May 2009 14:21:53 David Brownell wrote:
> On Tuesday 26 May 2009, Mike Frysinger wrote:
> > > > You revert David Brownell's commit here
> > > > (c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.
> > >
> > > Right ... the appended patch will *undo* the setting
> > > established in the board init code via set_irq_type().
> > >
> > > Moreover, it completely prevents working on systems which
> > > only support "both edges" triggering.
> >
> > how can it provide breakage when people are forced to use set_irq_type()
> > ?
>
> It reverses the IRQ setup done by the board code ...

how so ?  this code does the irq requesting which means the set_irq_type() 
needs to come after it anyways since the request_irq() will set any flags 
passed to it, right ?

> the code which you said they'd have to use to work
> around the breakage your patch adds.

umm, i'm referring to code the driver and you says should be put in place -- a 
call to set_irq_type() somewhere in the board init code.

> If you're really craving some change to the driver,
> you should make it take the relevant trigger type
> from the platform resource ... instead of hard-wiring
> it to a value which is *known* to break systems.

without any in-tree consumers, it's hard to see how things are supposed to be 
working and how they could possibly break
-mike

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[David Brownell]

> > > > Right ... the appended patch will *undo* the setting
> > > > established in the board init code via set_irq_type().
> > > >
> > > > Moreover, it completely prevents working on systems which
> > > > only support "both edges" triggering.
> > >
> > > how can it provide breakage when people are forced to use set_irq_type()
> > > ?
> >
> > It reverses the IRQ setup done by the board code ...
> 
> how so ?  this code does the irq requesting which means the set_irq_type() 
> needs to come after it

No; set_irq_type() can happen at any time.  It just does setup.
One policy sets IRQ types beforehand.

Equivalently, the driver may run on hardware which only supports
one policy ... *NOT* the policy you propose the driver be changed
to require.


> anyways since the request_irq() will set any flags  
> passed to it, right ?

Wrong.  A set_irq_type() call ensures the IRQ will trigger as
specified by its parameters (unless it fails).  The current
request_irq() leaves that "default" setup alone.

Your patch changes that configuration, making it request
a trigger mode that (a) the hardware may not support, so
the request_irq() will fail; else if it succeeds, then
(b) will clobber whatever mode that was already set up.
The first certainly breaks things; the second probably
does so.


> > the code which you said they'd have to use to work
> > around the breakage your patch adds.
> 
> umm, i'm referring to code the driver and you says should be put in place -- a 
> call to set_irq_type() somewhere in the board init code.

Which is well before the driver request_irq().  As noted
above, you're trying to make the driver request a specific
IRQ type, which is *NOT* one that can necessarily be
supported.

On the other hand, the current IRQ_NONE (type == 0) is by
definition be supported *everywhere* ...

Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Tue, 26 May 2009 18:17:06 -0700

> Wrong.  A set_irq_type() call ensures the IRQ will trigger as
> specified by its parameters (unless it fails).  The current
> request_irq() leaves that "default" setup alone.
> 
> Your patch changes that configuration, making it request
> a trigger mode that (a) the hardware may not support, so
> the request_irq() will fail; else if it succeeds, then
> (b) will clobber whatever mode that was already set up.
> The first certainly breaks things; the second probably
> does so.

I totally agree with David, this patch seems to be wrong on
just about every possible level.

RE: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default

[Hennerich, Michael]

>From: Mike Frysinger [mailto:vapier@gentoo.org]
>On Tuesday 26 May 2009 07:24:17 David Brownell wrote:
>> On Tuesday 26 May 2009, Baruch Siach wrote:
>> > On Tue, May 26, 2009 at 06:16:47AM -0400, Mike Frysinger wrote:
>> > > From: Michael Hennerich <michael.hennerich@analog.com>
>> > >
>> > > Rather than have the default IRQ flags not be usable, use the
logical
>> > > settings which match the hardware.  Since the part is /INT, it
makes
>> > > sense to have it default to a falling trigger.  If a board truly
needs
>> > > something different, it still can tweak the defaults in its own
board
>> > > file.
>> >
>> > You revert David Brownell's commit here
>> > (c7b7b042068cd12b8b155722d24686f70b88ced1). Added to CC.
>>
>> Right ... the appended patch will *undo* the setting
>> established in the board init code via set_irq_type().
>>
>> Moreover, it completely prevents working on systems which
>> only support "both edges" triggering.
>
>how can it provide breakage when people are forced to use
set_irq_type() ?

set_irq_type() is typically called before request_irq().
In case be give the IRQF_TRIGGER flag with request_irq() it will
overwrite the settings done by set_irq_type().

-Michael
>
>> So I'd NAK this, on the grounds that it causes severe
>> breakage.
>
>i see no consumers of this driver at all in mainline, so i dont think
it
>causes any in tree breakage at all
>-mike