Re: [PATCH 12/23] move twl4030-gpio to drivers/gpio

Re: [PATCH 12/23] move twl4030-gpio to drivers/gpio

[David Brownell]

On Monday 29 September 2008, Felipe Balbi wrote:
>  drivers/{i2c/chips => gpio}/twl4030-gpio.c |   37 +++++++++++++++++++++++----

Best to avoid sending this type of patch except as backup
to a git pull request ... it's unusable to folk who use
quilt and thus don't have access to GIT extensions.

Where "unusable" means "can't apply the patch".

- dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/23] move twl4030-gpio to drivers/gpio

[Felipe Balbi]

On Mon, Sep 29, 2008 at 01:06:19PM -0700, David Brownell wrote:
> On Monday 29 September 2008, Felipe Balbi wrote:
> >  drivers/{i2c/chips => gpio}/twl4030-gpio.c |   37 +++++++++++++++++++++++----
> 
> Best to avoid sending this type of patch except as backup
> to a git pull request ... it's unusable to folk who use
> quilt and thus don't have access to GIT extensions.
> 
> Where "unusable" means "can't apply the patch".

The good thing with the git extensions for rename detection is that we
really see what has changed when you renamed the file and that's what
really matters, right ?

But you use quilt, so when I send the final version, I'll drop the -C
flag.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/23] move twl4030-gpio to drivers/gpio

[David Brownell]

On Monday 29 September 2008, Felipe Balbi wrote:
> On Mon, Sep 29, 2008 at 01:06:19PM -0700, David Brownell wrote:
> > On Monday 29 September 2008, Felipe Balbi wrote:
> > >  drivers/{i2c/chips => gpio}/twl4030-gpio.c |   37 +++++++++++++++++++++++----
> > 
> > Best to avoid sending this type of patch except as backup
> > to a git pull request ... it's unusable to folk who use
> > quilt and thus don't have access to GIT extensions.
> > 
> > Where "unusable" means "can't apply the patch".
> 
> The good thing with the git extensions for rename detection is that we
> really see what has changed when you renamed the file and that's what
> really matters, right ?

That's not all that matters, although I certainly liked being
able to see that specific diff and confirm "yes, that's the
stuff I had intended to change".

A more basic point of posting patches to lists is that they be
usable for testing.  And the git rename stuff prevents that.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/23] move twl4030-gpio to drivers/gpio

[Felipe Balbi]

On Mon, Sep 29, 2008 at 01:45:59PM -0700, David Brownell wrote:
> On Monday 29 September 2008, Felipe Balbi wrote:
> > On Mon, Sep 29, 2008 at 01:06:19PM -0700, David Brownell wrote:
> > > On Monday 29 September 2008, Felipe Balbi wrote:
> > > >  drivers/{i2c/chips => gpio}/twl4030-gpio.c |   37 +++++++++++++++++++++++----
> > > 
> > > Best to avoid sending this type of patch except as backup
> > > to a git pull request ... it's unusable to folk who use
> > > quilt and thus don't have access to GIT extensions.
> > > 
> > > Where "unusable" means "can't apply the patch".
> > 
> > The good thing with the git extensions for rename detection is that we
> > really see what has changed when you renamed the file and that's what
> > really matters, right ?
> 
> That's not all that matters, although I certainly liked being
> able to see that specific diff and confirm "yes, that's the
> stuff I had intended to change".
> 
> A more basic point of posting patches to lists is that they be
> usable for testing.  And the git rename stuff prevents that.

Well that's one point, yes. But only prevent for those who doesn't use
git, right ? Although we can't really push everybody to drop quilt,
cogito, "patch -p1", own scripts, etc.

Then again, i really can't see why people still use quilt when we have
stgit, which gives you quilt functionality with all git's goodies.
Anyways, everybody should be able to choose a tool that fits better.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/23] twl4030-gpio: irq and other cleanup

[David Brownell]

On Monday 29 September 2008, Felipe Balbi wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Remove some ARM and OMAP specific stuff from the twl4030 GPIO code,
> and some unused/undesirable CPP symbols.  Add minor checkpatch fixes.

This one and #0021 share a problem.  Looks like what I tested
wasn't the version I thought I'd tested ...


> @@ -847,9 +839,9 @@ static int __devinit gpio_twl4030_probe(struct platform_device *pdev)
>  		/* install an irq handler for each of the gpio interrupts */
>  		for (irq = twl4030_gpio_irq_base; irq < twl4030_gpio_irq_end;
>  				irq++) {
> -			set_irq_chip(irq, &twl4030_gpio_irq_chip);
> -			set_irq_handler(irq, do_twl4030_gpio_irq);

The problem is right here:

> -			set_irq_flags(irq, IRQF_VALID);
> +			set_irq_noprobe(irq);

What's going on is that set_irq_flags() is ARM-specific; so using it
will break builds everywhere else; it can't be used without adding
undesirable dependencies on ARM.  (Interface changes done in mainline
may well overlook such stuff.  And if this mechanism were important,
it would be needed on other platforms...)

That normal usage (just mark as "valid"):

  - Clears the IRQ_NOREQUEST flag ... the troublesome bit
  - Sets IRQ_NOPROBE ... like the standard set_irq_noprobe() call
  - Clears IRQ_NOAUTOEN ... set for some PCMCIA irqs and little else

Thing is, ARM also *sets* IRQ_NOREQUEST (and NOPROBE) for all IRQs,
very early.  And it's effectively the only architecture which uses
that flag.  (PowerPC doesn't quite count, since you won't even get
an IRQ number until it's been mapped and that flag is cleared.)

... So it looks like I'm going to have to come up with a different
portability fix, sigh.



> +			set_irq_chip_and_handler(irq, &twl4030_gpio_irq_chip,
> +					do_twl4030_gpio_irq);
>  		}
>  
>  		/* gpio module IRQ */

Re: [PATCH 20/23] twl4030-gpio: irq and other cleanup

[David Brownell]

This update (to this patch) resolves that problem ... please
merge this into the next version.
--- beagle.orig/drivers/gpio/twl4030-gpio.c	2008-09-29 17:19:37.000000000 -0700
+++ beagle/drivers/gpio/twl4030-gpio.c	2008-09-29 17:16:04.000000000 -0700
@@ -41,9 +41,22 @@
 #include <linux/i2c/twl4030-gpio.h>
 
 
+static inline void activate_irq(int irq)
+{
+#ifdef CONFIG_ARM
+	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
+	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
+	 */
+	set_irq_flags(irq, IRQF_VALID);
+#else
+	/* same effect on other architectures */
+	set_irq_noprobe(irq);
+#endif
+}
+
+static struct gpio_chip twl_gpiochip;
 static int twl4030_gpio_irq_base;
 static int twl4030_gpio_irq_end;
-static struct gpio_chip twl_gpiochip;
 
 /* genirq interfaces are not available to modules */
 #ifdef MODULE
@@ -839,9 +852,9 @@ static int __devinit gpio_twl4030_probe(
 		/* install an irq handler for each of the gpio interrupts */
 		for (irq = twl4030_gpio_irq_base; irq < twl4030_gpio_irq_end;
 				irq++) {
-			set_irq_noprobe(irq);
 			set_irq_chip_and_handler(irq, &twl4030_gpio_irq_chip,
 					do_twl4030_gpio_irq);
+			activate_irq(irq);
 		}
 
 		/* gpio module IRQ */

Re: [PATCH 21/23] twl4030-core: portability updates

[David Brownell]

Similar fix needs merging into patch 21 too.

--- beagle.orig/drivers/i2c/chips/twl4030-core.c	2008-09-29 17:21:37.000000000 -0700
+++ beagle/drivers/i2c/chips/twl4030-core.c	2008-09-29 17:21:25.000000000 -0700
@@ -75,6 +75,19 @@
 #define twl_has_usb()	false
 #endif
 
+static inline void activate_irq(int irq)
+{
+#ifdef CONFIG_ARM
+	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
+	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
+	 */
+	set_irq_flags(irq, IRQF_VALID);
+#else
+	/* same effect on other architectures */
+	set_irq_noprobe(irq);
+#endif
+}
+
 /* Primary Interrupt Handler on TWL4030 Registers */
 
 /* Register Definitions */
@@ -1052,9 +1065,9 @@ static void twl_init_irq(int irq_num, un
 
 	/* install an irq handler for each of the PIH modules */
 	for (i = irq_base; i < irq_end; i++) {
-		set_irq_noprobe(i);
 		set_irq_chip_and_handler(i, &twl4030_irq_chip,
 				do_twl4030_module_irq);
+		activate_irq(i);
 	}
 
 	/* install an irq handler to demultiplex the TWL4030 interrupt */

Re: [PATCH 20/23] twl4030-gpio: irq and other cleanup

[Felipe Balbi]

On Mon, Sep 29, 2008 at 05:30:45PM -0700, David Brownell wrote:
> This update (to this patch) resolves that problem ... please
> merge this into the next version.
> --- beagle.orig/drivers/gpio/twl4030-gpio.c	2008-09-29 17:19:37.000000000 -0700
> +++ beagle/drivers/gpio/twl4030-gpio.c	2008-09-29 17:16:04.000000000 -0700
> @@ -41,9 +41,22 @@
>  #include <linux/i2c/twl4030-gpio.h>
>  
>  
> +static inline void activate_irq(int irq)
> +{
> +#ifdef CONFIG_ARM
> +	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
> +	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
> +	 */
> +	set_irq_flags(irq, IRQF_VALID);
> +#else
> +	/* same effect on other architectures */
> +	set_irq_noprobe(irq);
> +#endif
> +}

couldn't this be done by set_type() so you could use
set_irq_type(irq, IRQF_VALID); ?

i'm merging this already

-- 
balbi

Re: [PATCH 20/23] twl4030-gpio: irq and other cleanup

[David Brownell]

On Monday 29 September 2008, Felipe Balbi wrote:
> couldn't this be done by set_type() so you could use
> set_irq_type(irq, IRQF_VALID); ?

No; type is for trigger modes (edges, levels).

[PATCH 20/23] twl4030-gpio: irq and other cleanup

[Felipe Balbi]

From: David Brownell <dbrownell@users.sourceforge.net>

Remove some ARM and OMAP specific stuff from the twl4030 GPIO code,
and some unused/undesirable CPP symbols.  Add minor checkpatch fixes.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/gpio/twl4030-gpio.c |   36 +++++++++++++++++++-----------------
 include/linux/i2c/twl4030.h |    6 ------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/gpio/twl4030-gpio.c b/drivers/gpio/twl4030-gpio.c
index 404cb34..04a58cb 100644
--- a/drivers/gpio/twl4030-gpio.c
+++ b/drivers/gpio/twl4030-gpio.c
@@ -40,20 +40,25 @@
 #include <linux/i2c/twl4030.h>
 #include <linux/i2c/twl4030-gpio.h>
 
-#include <mach/irqs.h>
-#include <asm/mach/irq.h>
-#include <mach/gpio.h>
-#include <mach/mux.h>
 
-
-/* REVISIT when these symbols vanish elsewhere, remove them here too */
-/* #undef TWL4030_GPIO_IRQ_BASE */
-/* #undef TWL4030_GPIO_IRQ_END */
+static inline void activate_irq(int irq)
+{
+#ifdef CONFIG_ARM
+	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
+	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
+	 */
+	set_irq_flags(irq, IRQF_VALID);
+#else
+	/* same effect on other architectures */
+	set_irq_noprobe(irq);
+#endif
+}
 
 static struct gpio_chip twl_gpiochip;
 static int twl4030_gpio_irq_base;
 static int twl4030_gpio_irq_end;
 
+/* genirq interfaces are not available to modules */
 #ifdef MODULE
 #define is_module()	true
 #else
@@ -530,7 +535,7 @@ static int twl4030_set_gpio_edge_ctrl(int gpio, int edge)
 	if (ret >= 0) {
 		/* clear the previous rising/falling values */
 		reg = (u8) ret;
-		reg &= ~( MASK_GPIO_EDR1_GPIOxFALLING(c_off)
+		reg &= ~(MASK_GPIO_EDR1_GPIOxFALLING(c_off)
 			| MASK_GPIO_EDR1_GPIOxRISING(c_off));
 		reg |= c_msk;
 		ret = gpio_twl4030_write(base, reg);
@@ -598,7 +603,7 @@ int twl4030_set_gpio_card_detect(int gpio, int enable)
 		ret = gpio_twl4030_write(REG_GPIO_CTRL, reg);
 	}
 	mutex_unlock(&gpio_lock);
-	return (ret);
+	return ret;
 }
 #endif
 
@@ -847,9 +852,9 @@ static int __devinit gpio_twl4030_probe(struct platform_device *pdev)
 		/* install an irq handler for each of the gpio interrupts */
 		for (irq = twl4030_gpio_irq_base; irq < twl4030_gpio_irq_end;
 				irq++) {
-			set_irq_chip(irq, &twl4030_gpio_irq_chip);
-			set_irq_handler(irq, do_twl4030_gpio_irq);
-			set_irq_flags(irq, IRQF_VALID);
+			set_irq_chip_and_handler(irq, &twl4030_gpio_irq_chip,
+					do_twl4030_gpio_irq);
+			activate_irq(irq);
 		}
 
 		/* gpio module IRQ */
@@ -918,13 +923,10 @@ static int __devexit gpio_twl4030_remove(struct platform_device *pdev)
 	/* uninstall the gpio demultiplexing interrupt handler */
 	irq = platform_get_irq(pdev, 0);
 	set_irq_handler(irq, NULL);
-	set_irq_flags(irq, 0);
 
 	/* uninstall the irq handler for each of the gpio interrupts */
-	for (irq = twl4030_gpio_irq_base; irq < twl4030_gpio_irq_end; irq++) {
+	for (irq = twl4030_gpio_irq_base; irq < twl4030_gpio_irq_end; irq++)
 		set_irq_handler(irq, NULL);
-		set_irq_flags(irq, 0);
-	}
 
 	/* stop the gpio unmask kernel thread */
 	if (gpio_unmask_thread) {
diff --git a/include/linux/i2c/twl4030.h b/include/linux/i2c/twl4030.h
index f26fda1..56a9415 100644
--- a/include/linux/i2c/twl4030.h
+++ b/include/linux/i2c/twl4030.h
@@ -138,17 +138,11 @@ struct twl4030_platform_data {
 
 /* TWL4030 GPIO interrupt definitions */
 
-#define TWL4030_GPIO_MIN		0
 #define TWL4030_GPIO_MAX		18
-#define TWL4030_GPIO_MAX_CD		2
 #define TWL4030_GPIO_IRQ_NO(n)		(TWL4030_GPIO_IRQ_BASE + (n))
-#define TWL4030_GPIO_IS_INPUT		1
-#define TWL4030_GPIO_IS_OUTPUT		0
 #define TWL4030_GPIO_IS_ENABLE		1
-#define TWL4030_GPIO_IS_DISABLE		0
 #define TWL4030_GPIO_PULL_UP		0
 #define TWL4030_GPIO_PULL_DOWN		1
-#define TWL4030_GPIO_PULL_NONE		2
 
 /* Functions to read and write from TWL4030 */
 
-- 
1.6.0.2.307.gc427