* [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
@ 2009-05-26 10:16 Mike Frysinger
2009-05-26 11:06 ` Baruch Siach
0 siblings, 1 reply; 11+ messages in thread
From: Mike Frysinger @ 2009-05-26 10:16 UTC (permalink / raw
To: netdev; +Cc: Michael Hennerich, Bryan Wu, Baruch Siach
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 10:16 [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default Mike Frysinger
@ 2009-05-26 11:06 ` Baruch Siach
2009-05-26 11:24 ` David Brownell
0 siblings, 1 reply; 11+ messages in thread
From: Baruch Siach @ 2009-05-26 11:06 UTC (permalink / raw
To: Mike Frysinger; +Cc: netdev, Michael Hennerich, Bryan Wu, David Brownell
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 -
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 11:06 ` Baruch Siach
@ 2009-05-26 11:24 ` David Brownell
2009-05-26 12:24 ` Hennerich, Michael
2009-05-26 16:53 ` Mike Frysinger
0 siblings, 2 replies; 11+ messages in thread
From: David Brownell @ 2009-05-26 11:24 UTC (permalink / raw
To: Baruch Siach, Michael Hennerich; +Cc: Mike Frysinger, netdev, Bryan Wu
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 -
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 11:24 ` David Brownell
@ 2009-05-26 12:24 ` Hennerich, Michael
2009-05-26 19:40 ` David Brownell
2009-05-26 16:53 ` Mike Frysinger
1 sibling, 1 reply; 11+ messages in thread
From: Hennerich, Michael @ 2009-05-26 12:24 UTC (permalink / raw
To: David Brownell, Baruch Siach; +Cc: Mike Frysinger, netdev, Bryan Wu
>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
-
>>
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 12:24 ` Hennerich, Michael
@ 2009-05-26 19:40 ` David Brownell
0 siblings, 0 replies; 11+ messages in thread
From: David Brownell @ 2009-05-26 19:40 UTC (permalink / raw
To: Hennerich, Michael; +Cc: Baruch Siach, Mike Frysinger, netdev, Bryan Wu
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 11:24 ` David Brownell
2009-05-26 12:24 ` Hennerich, Michael
@ 2009-05-26 16:53 ` Mike Frysinger
2009-05-26 18:21 ` David Brownell
2009-05-27 15:18 ` Hennerich, Michael
1 sibling, 2 replies; 11+ messages in thread
From: Mike Frysinger @ 2009-05-26 16:53 UTC (permalink / raw
To: David Brownell; +Cc: Baruch Siach, Michael Hennerich, netdev, Bryan Wu
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 16:53 ` Mike Frysinger
@ 2009-05-26 18:21 ` David Brownell
2009-05-26 21:43 ` Mike Frysinger
2009-05-27 15:18 ` Hennerich, Michael
1 sibling, 1 reply; 11+ messages in thread
From: David Brownell @ 2009-05-26 18:21 UTC (permalink / raw
To: Mike Frysinger; +Cc: Baruch Siach, Michael Hennerich, netdev, Bryan Wu
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 18:21 ` David Brownell
@ 2009-05-26 21:43 ` Mike Frysinger
2009-05-27 1:17 ` David Brownell
0 siblings, 1 reply; 11+ messages in thread
From: Mike Frysinger @ 2009-05-26 21:43 UTC (permalink / raw
To: David Brownell; +Cc: Baruch Siach, Michael Hennerich, netdev, Bryan Wu
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 21:43 ` Mike Frysinger
@ 2009-05-27 1:17 ` David Brownell
2009-05-27 3:53 ` David Miller
0 siblings, 1 reply; 11+ messages in thread
From: David Brownell @ 2009-05-27 1:17 UTC (permalink / raw
To: Mike Frysinger; +Cc: Baruch Siach, Michael Hennerich, netdev, Bryan Wu
> > > > 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* ...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-27 1:17 ` David Brownell
@ 2009-05-27 3:53 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2009-05-27 3:53 UTC (permalink / raw
To: david-b; +Cc: vapier, baruch, michael.hennerich, netdev, cooloney
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default
2009-05-26 16:53 ` Mike Frysinger
2009-05-26 18:21 ` David Brownell
@ 2009-05-27 15:18 ` Hennerich, Michael
1 sibling, 0 replies; 11+ messages in thread
From: Hennerich, Michael @ 2009-05-27 15:18 UTC (permalink / raw
To: Mike Frysinger, David Brownell; +Cc: Baruch Siach, netdev, Bryan Wu
>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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-05-27 15:22 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-05-26 10:16 [PATCH] netdev: enc28j60: use IRQF_TRIGGER_FALLING for IRQ by default Mike Frysinger
2009-05-26 11:06 ` Baruch Siach
2009-05-26 11:24 ` David Brownell
2009-05-26 12:24 ` Hennerich, Michael
2009-05-26 19:40 ` David Brownell
2009-05-26 16:53 ` Mike Frysinger
2009-05-26 18:21 ` David Brownell
2009-05-26 21:43 ` Mike Frysinger
2009-05-27 1:17 ` David Brownell
2009-05-27 3:53 ` David Miller
2009-05-27 15:18 ` Hennerich, Michael
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.