2011-02-10 23:47:31

by Thomas Gleixner

[permalink] [raw]
Subject: [patch 34/75] arm: ep93xx: Kill another instance of broken irq_desc fiddling

1. This is a copy of the borked code in gpiolib
2. If you need information about irq state which is not exposed, then talk
to the maintainer of that code instead of adding totaly horrible open
coded access.

Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Hartley Sweeten <[email protected]>
Cc: Russell King <[email protected]>
---
arch/arm/mach-ep93xx/gpio.c | 38 --------------------------------------
1 file changed, 38 deletions(-)

Index: linux-2.6-tip/arch/arm/mach-ep93xx/gpio.c
===================================================================
--- linux-2.6-tip.orig/arch/arm/mach-ep93xx/gpio.c
+++ linux-2.6-tip/arch/arm/mach-ep93xx/gpio.c
@@ -354,44 +354,6 @@ static void ep93xx_gpio_dbg_show(struct
is_out ? "out" : "in ",
(data_reg & (1 << i)) ? "hi" : "lo");

- if (!is_out) {
- int irq = gpio_to_irq(gpio);
- struct irq_desc *desc = irq_desc + irq;
-
- if (irq >= 0 && desc->action) {
- char *trigger;
-
- switch (desc->status & IRQ_TYPE_SENSE_MASK) {
- case IRQ_TYPE_NONE:
- trigger = "(default)";
- break;
- case IRQ_TYPE_EDGE_FALLING:
- trigger = "edge-falling";
- break;
- case IRQ_TYPE_EDGE_RISING:
- trigger = "edge-rising";
- break;
- case IRQ_TYPE_EDGE_BOTH:
- trigger = "edge-both";
- break;
- case IRQ_TYPE_LEVEL_HIGH:
- trigger = "level-high";
- break;
- case IRQ_TYPE_LEVEL_LOW:
- trigger = "level-low";
- break;
- default:
- trigger = "?trigger?";
- break;
- }
-
- seq_printf(s, " irq-%d %s%s",
- irq, trigger,
- (desc->status & IRQ_WAKEUP)
- ? " wakeup" : "");
- }
- }
-
seq_printf(s, "\n");
}
}


2011-02-11 00:07:44

by Hartley Sweeten

[permalink] [raw]
Subject: RE: [patch 34/75] arm: ep93xx: Kill another instance of broken irq_desc fiddling

On Thursday, February 10, 2011 4:37 PM, Thomas Gleixner wrote:
>
> 1. This is a copy of the borked code in gpiolib
> 2. If you need information about irq state which is not exposed, then talk
> to the maintainer of that code instead of adding totaly horrible open
> coded access.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Hartley Sweeten <[email protected]>
> Cc: Russell King <[email protected]>

As stated, this was following what was provided in gpiolib.

The information was only provided by debugfs to help development. It's
not necessarily needed.

Minor note below. But otherwise,

Acked-by: H Hartley Sweeten <[email protected]>

> ---
> arch/arm/mach-ep93xx/gpio.c | 38 --------------------------------------
> 1 file changed, 38 deletions(-)
>
> Index: linux-2.6-tip/arch/arm/mach-ep93xx/gpio.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/arm/mach-ep93xx/gpio.c
> +++ linux-2.6-tip/arch/arm/mach-ep93xx/gpio.c
> @@ -354,44 +354,6 @@ static void ep93xx_gpio_dbg_show(struct
> is_out ? "out" : "in ",
> (data_reg & (1 << i)) ? "hi" : "lo");
>
> - if (!is_out) {
> - int irq = gpio_to_irq(gpio);
> - struct irq_desc *desc = irq_desc + irq;
> -
> - if (irq >= 0 && desc->action) {
> - char *trigger;
> -
> - switch (desc->status & IRQ_TYPE_SENSE_MASK) {
> - case IRQ_TYPE_NONE:
> - trigger = "(default)";
> - break;
> - case IRQ_TYPE_EDGE_FALLING:
> - trigger = "edge-falling";
> - break;
> - case IRQ_TYPE_EDGE_RISING:
> - trigger = "edge-rising";
> - break;
> - case IRQ_TYPE_EDGE_BOTH:
> - trigger = "edge-both";
> - break;
> - case IRQ_TYPE_LEVEL_HIGH:
> - trigger = "level-high";
> - break;
> - case IRQ_TYPE_LEVEL_LOW:
> - trigger = "level-low";
> - break;
> - default:
> - trigger = "?trigger?";
> - break;
> - }
> -
> - seq_printf(s, " irq-%d %s%s",
> - irq, trigger,
> - (desc->status & IRQ_WAKEUP)
> - ? " wakeup" : "");
> - }
> - }
> -
> seq_printf(s, "\n");

This could just be merged into the previous seq_printf().

For that matter, the is_out variable could be removed and just code the test
into the seq_printf().

> }
> }

2011-02-11 00:22:19

by Ryan Mallon

[permalink] [raw]
Subject: Re: [patch 34/75] arm: ep93xx: Kill another instance of broken irq_desc fiddling

On 02/11/2011 12:37 PM, Thomas Gleixner wrote:
> 1. This is a copy of the borked code in gpiolib
> 2. If you need information about irq state which is not exposed, then talk
> to the maintainer of that code instead of adding totaly horrible open
> coded access.

This code got added simply because it is sometimes helpful to be able to
see how various gpio/irq pins are configured. I'm happy to drop the
functionality (see below), but is there a better way to get this
information? Is it already available somewhere else (proc, sys)?

> Signed-off-by: Thomas Gleixner <[email protected]>
> Cc: Hartley Sweeten <[email protected]>
> Cc: Russell King <[email protected]>
> ---
> arch/arm/mach-ep93xx/gpio.c | 38 --------------------------------------
> 1 file changed, 38 deletions(-)
>
> Index: linux-2.6-tip/arch/arm/mach-ep93xx/gpio.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/arm/mach-ep93xx/gpio.c
> +++ linux-2.6-tip/arch/arm/mach-ep93xx/gpio.c
> @@ -354,44 +354,6 @@ static void ep93xx_gpio_dbg_show(struct
> is_out ? "out" : "in ",
> (data_reg & (1 << i)) ? "hi" : "lo");
>
> - if (!is_out) {
> - int irq = gpio_to_irq(gpio);
> - struct irq_desc *desc = irq_desc + irq;
> -
> - if (irq >= 0 && desc->action) {

Would be nice to at least keep the fact that the gpio is configured as
an interrupt. Something like:

if (!is_out) {
int irq;

irq = gpio_to_irq(gpio);
if (irq >= 0)
seq_printf(s, " (irq %d)", irq);
}

I'm okay with this patch as-is though. We can add a corrected patch
later if we decided that it is still useful to have this information.

~Ryan

--
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon 5 Amuri Park, 404 Barbadoes St
[email protected] PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com New Zealand
Phone: +64 3 3779127 Freecall: Australia 1800 148 751
Fax: +64 3 3779135 USA 1800 261 2934

2011-02-11 11:42:50

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [patch 34/75] arm: ep93xx: Kill another instance of broken irq_desc fiddling

On Fri, 11 Feb 2011, Ryan Mallon wrote:

> On 02/11/2011 12:37 PM, Thomas Gleixner wrote:
> > 1. This is a copy of the borked code in gpiolib
> > 2. If you need information about irq state which is not exposed, then talk
> > to the maintainer of that code instead of adding totaly horrible open
> > coded access.
>
> This code got added simply because it is sometimes helpful to be able to
> see how various gpio/irq pins are configured. I'm happy to drop the
> functionality (see below), but is there a better way to get this
> information? Is it already available somewhere else (proc, sys)?

No, but we can add that if it's required in a sane form.

I don't have objections to expose debug informations in general, but
I have objections that random code does this especially, when it's
duplicated random code :)

Thanks,

tglx

2011-02-11 20:16:56

by Ryan Mallon

[permalink] [raw]
Subject: Re: [patch 34/75] arm: ep93xx: Kill another instance of broken irq_desc fiddling

On 12/02/11 00:42, Thomas Gleixner wrote:
> On Fri, 11 Feb 2011, Ryan Mallon wrote:
>
>> On 02/11/2011 12:37 PM, Thomas Gleixner wrote:
>>> 1. This is a copy of the borked code in gpiolib
>>> 2. If you need information about irq state which is not exposed, then talk
>>> to the maintainer of that code instead of adding totaly horrible open
>>> coded access.
>> This code got added simply because it is sometimes helpful to be able to
>> see how various gpio/irq pins are configured. I'm happy to drop the
>> functionality (see below), but is there a better way to get this
>> information? Is it already available somewhere else (proc, sys)?
> No, but we can add that if it's required in a sane form.
>
> I don't have objections to expose debug informations in general, but
> I have objections that random code does this especially, when it's
> duplicated random code :)
>

Ok. Does it make sense to expose the interrupt configuration for all
interrupts via proc, sys, debug, etc? Is this information useful to
enough people to warrant it? I would rather have a global debug rather
than individual platforms and drivers implementing it.

For the time being can we fix up the ep93xx gpio code with the amended
patch below. It keeps the information that the pin is also configured as
an interrupt and cleans the code up a bit.

Signed-off-by: Ryan Mallon <[email protected]>
---

diff --git a/arch/arm/mach-ep93xx/gpio.c b/arch/arm/mach-ep93xx/gpio.c
index bec34b8..d0bcce4 100644
--- a/arch/arm/mach-ep93xx/gpio.c
+++ b/arch/arm/mach-ep93xx/gpio.c
@@ -347,52 +347,14 @@ static void ep93xx_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
gpio = ep93xx_chip->chip.base;
for (i = 0; i< chip->ngpio; i++, gpio++) {
int is_out = data_dir_reg& (1<< i);
+ int irq = gpio_to_irq(gpio);

- seq_printf(s, " %s%d gpio-%-3d (%-12s) %s %s",
+ seq_printf(s, " %s%d gpio-%-3d (%-12s) %s %s %s\n",
chip->label, i, gpio,
gpiochip_is_requested(chip, i) ? : "",
is_out ? "out" : "in ",
- (data_reg& (1<< i)) ? "hi" : "lo");
-
- if (!is_out) {
- int irq = gpio_to_irq(gpio);
- struct irq_desc *desc = irq_desc + irq;
-
- if (irq>= 0&& desc->action) {
- char *trigger;
-
- switch (desc->status& IRQ_TYPE_SENSE_MASK) {
- case IRQ_TYPE_NONE:
- trigger = "(default)";
- break;
- case IRQ_TYPE_EDGE_FALLING:
- trigger = "edge-falling";
- break;
- case IRQ_TYPE_EDGE_RISING:
- trigger = "edge-rising";
- break;
- case IRQ_TYPE_EDGE_BOTH:
- trigger = "edge-both";
- break;
- case IRQ_TYPE_LEVEL_HIGH:
- trigger = "level-high";
- break;
- case IRQ_TYPE_LEVEL_LOW:
- trigger = "level-low";
- break;
- default:
- trigger = "?trigger?";
- break;
- }
-
- seq_printf(s, " irq-%d %s%s",
- irq, trigger,
- (desc->status& IRQ_WAKEUP)
- ? " wakeup" : "");
- }
- }
-
- seq_printf(s, "\n");
+ (data_reg& (1<< i)) ? "hi" : "lo",
+ (!is_out&& irq>= 0) ? "(interrupt)" : "");
}
}