Re: MIPS: RB532: Provide functions for gpio configuration

Re: MIPS: RB532: Provide functions for gpio configuration

[Phil Sutter]

Hi!

I fullquote here because of the lists and people added to Cc.

On Thu, Nov 20, 2008 at 08:02:15PM -0800, David Brownell wrote:
> I just noticed:
> 
> > commit 2e373952cc893207a8b47a5e68c2f5155f912449
> > Author: Phil Sutter <n0-1@freewrt.org>
> > Date:   Sat Nov 1 15:13:21 2008 +0100
> >
> >     MIPS: RB532: Provide functions for gpio configuration
> >
> >     As gpiolib doesn't support pin multiplexing, it provides no way to
> >     access the GPIOFUNC register. Also there is no support for setting
> >     interrupt status and level. These functions provide access to them and
> >     are needed by the CompactFlash driver.
> >
> >     Signed-off-by: Phil Sutter <n0-1@freewrt.org>
> >     Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> 
> The conventional way to do most of that is through the irq_chip
> associated with that block of IRQ-capable GPIOs.
> 
> 
> So ...
> 
> > -       /* Set the interrupt status and level for the CF pin */
> > -       rb532_gpio_set_int_level(&rb532_gpio_chip->chip, CF_GPIO_NUM, 1);
> > -       rb532_gpio_set_int_status(&rb532_gpio_chip->chip, CF_GPIO_NUM, 0);
> > +       /* configure CF_GPIO_NUM as CFRDY IRQ source */
> > +       rb532_gpio_set_func(0, CF_GPIO_NUM);
> 
> ... the pinmux would indeed be a SOC-specific mechanism, kicked in
> only for boards that use that GPIO in that way, but ...
> 
> 
> > +       rb532_gpio_direction_input(&rb532_gpio_chip->chip, CF_GPIO_NUM);
> 
> ... normal gpio_request() + gpio_direction_input() would set the pin up, and ...
> 
> 
> > +       rb532_gpio_set_ilevel(1, CF_GPIO_NUM);
> > +       rb532_gpio_set_istat(0, CF_GPIO_NUM);
> 
> 	status = request_irq(gpio_to_irq(CF_GPIO_NUM), ... )
> 
> with appropriate IRQF_TRIGGER_* flags should solve that problem.
> Or even just set_irq_type().  (At least for one of the two
> registers updated there ... I can't guess what "istat" would be.)
> 
> Just FYI at this point.  Maybe you have a reason not to fit into
> the genirq framework.
> 
> - Dave

Thanks for your hints, Dave. I already had a patch flying around adding
a set_type() function to the irq_chip (merely a dummy to shut up
warnings in the boot log). After extending it to cover the needs of the
mapped GPIO pins and some other work, I could drop that CompactFlash
initialisation code in gpio.c completely.

As a side effect, many of my changes to pata-rb532-cf got unnecessary
which I consider a very good sign.

I'll reply to this mail with the relevant patches for each list and
maintainer. 

Greetings, Phil

[PATCH] MIPS: rb532: add set_type() function to IRQ struct

[Phil Sutter]

Interrupt Group 4 mapps the GPIO pins enabled as interrupt sources;
add defines to make this clear when addressing them later in code.

The mapped GPIOs support triggering on either level high or low. To
achieve this, the set_type() function calls rb532_gpio_set_ilevel() for
interrupts of the above mentioned group.

As there is no way to alter the triggering characteristics of the other
interrupts, accept level triggering on status high only. (This is just a
guess; but as the system boots fine and interrupt-driven devices (e.g.
serial console) work with no implications, it seems to be right.)

To clear a GPIO mapped IRQ, the source has to be cleared (i.e., the
interrupt status bit of the corresponding GPIO pin). This is done inside
rb532_disable_irq().

After applying these changes I could undo most of my former "fixes" to
pata-rb532-cf. Particularly all interrupt handling can be done
generically via set_irq_type() as it was before.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/include/asm/mach-rc32434/irq.h |    3 +++
 arch/mips/rb532/irq.c                    |   26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/mips/include/asm/mach-rc32434/irq.h b/arch/mips/include/asm/mach-rc32434/irq.h
index 56738d8..023a5b1 100644
--- a/arch/mips/include/asm/mach-rc32434/irq.h
+++ b/arch/mips/include/asm/mach-rc32434/irq.h
@@ -30,4 +30,7 @@
 #define ETH0_RX_OVR_IRQ   	(GROUP3_IRQ_BASE + 9)
 #define ETH0_TX_UND_IRQ   	(GROUP3_IRQ_BASE + 10)
 
+#define GPIO_MAPPED_IRQ_BASE	GROUP4_IRQ_BASE
+#define GPIO_MAPPED_IRQ_GROUP	4
+
 #endif  /* __ASM_RC32434_IRQ_H */
diff --git a/arch/mips/rb532/irq.c b/arch/mips/rb532/irq.c
index 549b46d..e265af5 100644
--- a/arch/mips/rb532/irq.c
+++ b/arch/mips/rb532/irq.c
@@ -46,6 +46,7 @@
 #include <asm/system.h>
 
 #include <asm/mach-rc32434/irq.h>
+#include <asm/mach-rc32434/gpio.h>
 
 struct intr_group {
 	u32 mask;	/* mask of valid bits in pending/mask registers */
@@ -150,6 +151,9 @@ static void rb532_disable_irq(unsigned int irq_nr)
 		mask |= intr_bit;
 		WRITE_MASK(addr, mask);
 
+		if (group == GPIO_MAPPED_IRQ_GROUP)
+			rb532_gpio_set_istat(0, irq_nr - GPIO_MAPPED_IRQ_BASE);
+
 		/*
 		 * if there are no more interrupts enabled in this
 		 * group, disable corresponding IP
@@ -165,12 +169,34 @@ static void rb532_mask_and_ack_irq(unsigned int irq_nr)
 	ack_local_irq(group_to_ip(irq_to_group(irq_nr)));
 }
 
+static int rb532_set_type(unsigned int irq_nr, unsigned type)
+{
+	int gpio = irq_nr - GPIO_MAPPED_IRQ_BASE;
+	int group = irq_to_group(irq_nr);
+
+	if (group != GPIO_MAPPED_IRQ_GROUP)
+		return (type == IRQ_TYPE_LEVEL_HIGH) ? 0 : -EINVAL;
+
+	switch (type) {
+		case IRQ_TYPE_LEVEL_HIGH:
+			rb532_gpio_set_ilevel(1, gpio);
+			break;
+		case IRQ_TYPE_LEVEL_LOW:
+			rb532_gpio_set_ilevel(0, gpio);
+			break;
+		default:
+			return -EINVAL;
+	}
+	return 0;
+}
+
 static struct irq_chip rc32434_irq_type = {
 	.name		= "RB532",
 	.ack		= rb532_disable_irq,
 	.mask		= rb532_disable_irq,
 	.mask_ack	= rb532_mask_and_ack_irq,
 	.unmask		= rb532_enable_irq,
+	.set_type	= rb532_set_type,
 };
 
 void __init arch_init_irq(void)
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: add set_type() function to IRQ struct

[Ralf Baechle]

On Fri, Nov 28, 2008 at 08:45:10PM +0100, Phil Sutter wrote:

> +	switch (type) {
> +		case IRQ_TYPE_LEVEL_HIGH:
> +			rb532_gpio_set_ilevel(1, gpio);
> +			break;
> +		case IRQ_TYPE_LEVEL_LOW:
> +			rb532_gpio_set_ilevel(0, gpio);
> +			break;
> +		default:
> +			return -EINVAL;
> +	}

Linux coding style - the case and default labels should have the same
indentation level as the switch statement.  I fixed that.

Applied, thanks.

  Ralf

[PATCH] MIPS: rb532: auto disable GPIO alternate function

[Phil Sutter]

When a driver calls gpio_set_direction_{input,output}(), it obviously
doesn't want the alternate function for that pin to be active (as the
direction would not matter in that case). This patch ensures alternate
function is disabled when the direction is being changed.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index e35cb75..f5b15a1 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -169,8 +169,8 @@ static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
 
-	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
-		return 1;	/* alternate function, GPIOCFG is ignored */
+	/* disable alternate function in case it's set */
+	rb532_set_bit(0, offset, gpch->regbase + GPIOFUNC);
 
 	rb532_set_bit(0, offset, gpch->regbase + GPIOCFG);
 	return 0;
@@ -186,8 +186,8 @@ static int rb532_gpio_direction_output(struct gpio_chip *chip,
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
 
-	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
-		return 1;	/* alternate function, GPIOCFG is ignored */
+	/* disable alternate function in case it's set */
+	rb532_set_bit(0, offset, gpch->regbase + GPIOFUNC);
 
 	/* set the initial output value */
 	rb532_set_bit(value, offset, gpch->regbase + GPIOD);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: auto disable GPIO alternate function

[Ralf Baechle]

On Fri, Nov 28, 2008 at 08:46:09PM +0100, Phil Sutter wrote:

> When a driver calls gpio_set_direction_{input,output}(), it obviously
> doesn't want the alternate function for that pin to be active (as the
> direction would not matter in that case). This patch ensures alternate
> function is disabled when the direction is being changed.

Applied.  Thanks,

  Ralf

[PATCH] MIPS: rb532: remove useless CF GPIO initialisation

[Phil Sutter]

As the pata-rb532-cf driver calls gpio_set_direction_input(), the calls
to rb532_gpio_set_func() and rb532_gpio_direction_input() are not needed
since the alternate function is automatically being disabled when
changing the GPIO pin direction.
The later two calls to rb532_gpio_set_{ilevel,istat}() are implicitly
being done by the IRQ initialisation of pata-rb532-cf.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index f5b15a1..b195f79 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -259,12 +259,6 @@ int __init rb532_gpio_init(void)
 		return -ENXIO;
 	}
 
-	/* configure CF_GPIO_NUM as CFRDY IRQ source */
-	rb532_gpio_set_func(0, CF_GPIO_NUM);
-	rb532_gpio_direction_input(&rb532_gpio_chip->chip, CF_GPIO_NUM);
-	rb532_gpio_set_ilevel(1, CF_GPIO_NUM);
-	rb532_gpio_set_istat(0, CF_GPIO_NUM);
-
 	return 0;
 }
 arch_initcall(rb532_gpio_init);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: remove useless CF GPIO initialisation

[Ralf Baechle]

On Fri, Nov 28, 2008 at 08:46:22PM +0100, Phil Sutter wrote:

> As the pata-rb532-cf driver calls gpio_set_direction_input(), the calls
> to rb532_gpio_set_func() and rb532_gpio_direction_input() are not needed
> since the alternate function is automatically being disabled when
> changing the GPIO pin direction.
> The later two calls to rb532_gpio_set_{ilevel,istat}() are implicitly
> being done by the IRQ initialisation of pata-rb532-cf.
> 
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>

Applied.  Thanks!

  Ralf

[PATCH] MIPS: rb532: remove unused rb532_gpio_set_func()

[Phil Sutter]

Since disabling of the alternate function of a GPIO pin is being done
implicitly when changing it's direction, the above mentioned function is
not being called anymore and can be removed.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index b195f79..d75eb19 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -228,14 +228,6 @@ void rb532_gpio_set_istat(int bit, unsigned gpio)
 }
 EXPORT_SYMBOL(rb532_gpio_set_istat);
 
-/*
- * Configure GPIO alternate function
- */
-static void rb532_gpio_set_func(int bit, unsigned gpio)
-{
-       rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
-}
-
 int __init rb532_gpio_init(void)
 {
 	struct resource *r;
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: remove unused rb532_gpio_set_func()

[Phil Sutter]

Hi,

On Fri, Nov 28, 2008 at 08:46:29PM +0100, Phil Sutter wrote:
> Since disabling of the alternate function of a GPIO pin is being done
> implicitly when changing it's direction, the above mentioned function is
> not being called anymore and can be removed.

Please do not apply this patch. In fact, the function in question should
be exported as kernel symbol, as else there is no possibility for
drivers to turn on alternate function for a GPIO pin. This is of
particular use when trying to read the S1 button state, as the
corresponding GPIO pin is multiplexed with UART0 input.

Greetings, Phil

[PATCH] rb532: update headers

[Phil Sutter]

Remove the {set,get}_434_reg() prototypes, as the functions have been
removed. Also move the prototypes for {get,set}_latch_u5() to the
correct place.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/include/asm/mach-rc32434/gpio.h |    4 ----
 arch/mips/include/asm/mach-rc32434/rb.h   |    3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/mach-rc32434/gpio.h b/arch/mips/include/asm/mach-rc32434/gpio.h
index b5cf645..ca92c81 100644
--- a/arch/mips/include/asm/mach-rc32434/gpio.h
+++ b/arch/mips/include/asm/mach-rc32434/gpio.h
@@ -80,10 +80,6 @@ struct rb532_gpio_reg {
 /* Compact Flash GPIO pin */
 #define CF_GPIO_NUM		13
 
-extern void set_434_reg(unsigned reg_offs, unsigned bit, unsigned len, unsigned val);
-extern unsigned get_434_reg(unsigned reg_offs);
-extern void set_latch_u5(unsigned char or_mask, unsigned char nand_mask);
-extern unsigned char get_latch_u5(void);
 extern void rb532_gpio_set_ilevel(int bit, unsigned gpio);
 extern void rb532_gpio_set_istat(int bit, unsigned gpio);
 
diff --git a/arch/mips/include/asm/mach-rc32434/rb.h b/arch/mips/include/asm/mach-rc32434/rb.h
index f25a849..6dc5f8d 100644
--- a/arch/mips/include/asm/mach-rc32434/rb.h
+++ b/arch/mips/include/asm/mach-rc32434/rb.h
@@ -83,4 +83,7 @@ struct mpmc_device {
 	void __iomem 	*base;
 };
 
+extern void set_latch_u5(unsigned char or_mask, unsigned char nand_mask);
+extern unsigned char get_latch_u5(void);
+
 #endif  /* __ASM_RC32434_RB_H */
-- 
1.5.6.4

Re: [PATCH] rb532: update headers

[Ralf Baechle]

On Thu, Jan 22, 2009 at 07:28:50PM +0100, Phil Sutter wrote:

Applied.

  Ralf

[PATCH] MIPS: rb532: export rb532_gpio_set_func()

[Phil Sutter]

This kernel symbol provides a way for drivers to switch on alternate
function for a certain GPIO pin. Turning it off is done implicitly when
changing the GPIO direction, as that would be fixed when using the given
pin als alternate function.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/include/asm/mach-rc32434/gpio.h |    1 +
 arch/mips/rb532/gpio.c                    |    5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/include/asm/mach-rc32434/gpio.h b/arch/mips/include/asm/mach-rc32434/gpio.h
index ca92c81..3cb50d1 100644
--- a/arch/mips/include/asm/mach-rc32434/gpio.h
+++ b/arch/mips/include/asm/mach-rc32434/gpio.h
@@ -82,5 +82,6 @@ struct rb532_gpio_reg {
 
 extern void rb532_gpio_set_ilevel(int bit, unsigned gpio);
 extern void rb532_gpio_set_istat(int bit, unsigned gpio);
+extern void rb532_gpio_set_func(unsigned gpio);
 
 #endif /* _RC32434_GPIO_H_ */
diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index f338681..37de05d 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -174,10 +174,11 @@ EXPORT_SYMBOL(rb532_gpio_set_istat);
 /*
  * Configure GPIO alternate function
  */
-static void rb532_gpio_set_func(int bit, unsigned gpio)
+void rb532_gpio_set_func(unsigned gpio)
 {
-       rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
+       rb532_set_bit(1, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
 }
+EXPORT_SYMBOL(rb532_gpio_set_func);
 
 int __init rb532_gpio_init(void)
 {
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: export rb532_gpio_set_func()

[Ralf Baechle]

On Thu, Jan 22, 2009 at 07:32:43PM +0100, Phil Sutter wrote:

Applied.

  Ralf