[PATCH] provide functions for gpio configuration

[PATCH] provide functions for gpio configuration

[Phil Sutter]

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.

The function rb532_gpio_set_cfg is redundant with
rb532_gpio_direction_{input,output} but was added for simplicity's sake.
Maybe gpiolib support could be dropped completely as there are not many
users of it.

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

diff --git a/arch/mips/include/asm/mach-rc32434/rb.h b/arch/mips/include/asm/mach-rc32434/rb.h
index 0cb9466..f25a849 100644
--- a/arch/mips/include/asm/mach-rc32434/rb.h
+++ b/arch/mips/include/asm/mach-rc32434/rb.h
@@ -41,6 +41,7 @@
 #define BTCOMPARE	0x010044
 #define GPIOBASE	0x050000
 /* Offsets relative to GPIOBASE */
+#define GPIOFUNC	0x00
 #define GPIOCFG		0x04
 #define GPIOD		0x08
 #define GPIOILEVEL	0x0C
diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 70c4a67..f56f73b 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -287,6 +287,45 @@ static struct rb532_gpio_chip rb532_gpio_chip[] = {
 	},
 };
 
+static void rb532_do_gpio_set(int bit, unsigned gpio, void __iomem *addr)
+{
+	unsigned long flags;
+	u32 val;
+
+	local_irq_save(flags);
+	val = readl(addr);
+	if (bit)
+		val |= (1 << gpio);
+	else
+		val &= ~(1 << gpio);
+	writel(val, addr);
+	local_irq_restore(flags);
+}
+
+void  rb532_gpio_set_func(int bit, unsigned gpio)
+{
+	rb532_do_gpio_set(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
+}
+EXPORT_SYMBOL(rb532_gpio_set_func);
+
+void  rb532_gpio_set_cfg(int bit, unsigned gpio)
+{
+	rb532_do_gpio_set(bit, gpio, rb532_gpio_chip->regbase + GPIOCFG);
+}
+EXPORT_SYMBOL(rb532_gpio_set_cfg);
+
+void  rb532_gpio_set_ilevel(int bit, unsigned gpio)
+{
+	rb532_do_gpio_set(bit, gpio, rb532_gpio_chip->regbase + GPIOILEVEL);
+}
+EXPORT_SYMBOL(rb532_gpio_set_ilevel);
+
+void  rb532_gpio_set_istat(int bit, unsigned gpio)
+{
+	rb532_do_gpio_set(bit, gpio, rb532_gpio_chip->regbase + GPIOISTAT);
+}
+EXPORT_SYMBOL(rb532_gpio_set_istat);
+
 int __init rb532_gpio_init(void)
 {
 	struct resource *r;
-- 
1.5.6.4

Re: [PATCH] provide functions for gpio configuration

[Florian Fainelli]

Hi Phil,

Le Wednesday 29 October 2008 21:00:09 Phil Sutter, vous avez écrit :
> 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.

Right, but we do have interrupt level and status fuctions, registered as 
callbacks to an extended gpiochip structure. These two functions can remain 
static to the gpio.c file since we should perform interrupt status and level 
initialisation at gpiochip init time. Not sure which code you based your work 
on, but linux-queue tree at linux-mips.org has such code.

>
> The function rb532_gpio_set_cfg is redundant with
> rb532_gpio_direction_{input,output} but was added for simplicity's sake.
> Maybe gpiolib support could be dropped completely as there are not many
> users of it.

Sticking with the gpiolib has two advantages to me :
- you can use gpio_set_value/get_value for other 
- GPIO initialisation should be done right after gpiochip registering

I would be rather in favor of adding the other missing callbacks to the 
rb532_gpio_chip and make them look like the standard get/set functions. Just 
like what was done with the interrupt level and status functions.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] provide functions for gpio configuration

[Phil Sutter]

Hi Florian,

On Wed, Oct 29, 2008 at 09:07:42PM +0100, Florian Fainelli wrote:
> Le Wednesday 29 October 2008 21:00:09 Phil Sutter, vous avez écrit :
> > 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.
> 
> Right, but we do have interrupt level and status fuctions, registered as 
> callbacks to an extended gpiochip structure. These two functions can remain 
> static to the gpio.c file since we should perform interrupt status and level 
> initialisation at gpiochip init time. Not sure which code you based your work 
> on, but linux-queue tree at linux-mips.org has such code.

Yes it does, but that's not part of gpiolib itself. Accessing them needs
a combination of gpio_to_chip() and container_of() to be used, which I
doubt makes sense on a device with a single, platform gpio chip.

> - GPIO initialisation should be done right after gpiochip registering

I'm not sure if this is absolutely true. The original CompactFlash
driver e.g. clears interrupt level in cf_irq_handler() and sets it in
prepare_cf_irq(). The latter function is called more than once.

> I would be rather in favor of adding the other missing callbacks to the 
> rb532_gpio_chip and make them look like the standard get/set functions. Just 
> like what was done with the interrupt level and status functions.

That could be a solution. Having both methods accessing the same data is
no choice anyway. I'll prepare a patch when I have time to.

Greetings, Phil

Re: [PATCH] provide functions for gpio configuration

[Florian Fainelli]

Hi Phil,

Le Wednesday 29 October 2008 22:10:46 Phil Sutter, vous avez écrit :
> Yes it does, but that's not part of gpiolib itself. Accessing them needs
> a combination of gpio_to_chip() and container_of() to be used, which I
> doubt makes sense on a device with a single, platform gpio chip.

Yes, that makes it unexportable the way it is done yet. What I suggest is not 
overriding the struct rb532_gpio_chip with thoses callbacks, but do like you 
suggested initially.

> I'm not sure if this is absolutely true. The original CompactFlash
> driver e.g. clears interrupt level in cf_irq_handler() and sets it in
> prepare_cf_irq(). The latter function is called more than once.

This should be moved the IRQ handler, where a specific check for the IRQ being 
a GPIO one should set the interrupt status and level accordingly.

Thanks.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

Re: [PATCH] provide functions for gpio configuration

[Phil Sutter]

Hi,

On Thu, Oct 30, 2008 at 06:13:05PM +0100, Florian Fainelli wrote:
> Le Wednesday 29 October 2008 22:10:46 Phil Sutter, vous avez écrit :
> > Yes it does, but that's not part of gpiolib itself. Accessing them needs
> > a combination of gpio_to_chip() and container_of() to be used, which I
> > doubt makes sense on a device with a single, platform gpio chip.
> 
> Yes, that makes it unexportable the way it is done yet. What I suggest is not 
> overriding the struct rb532_gpio_chip with thoses callbacks, but do like you 
> suggested initially.
> 
> > I'm not sure if this is absolutely true. The original CompactFlash
> > driver e.g. clears interrupt level in cf_irq_handler() and sets it in
> > prepare_cf_irq(). The latter function is called more than once.
> 
> This should be moved the IRQ handler, where a specific check for the IRQ being 
> a GPIO one should set the interrupt status and level accordingly.

Sounds reasonable to me. So I'll prepare a patch which:
* removes the function pointers from rb532_gpio_chip
* exports getters and setters for interrupt status and level

The GPIO config and function registers should indeed only be accessed at
bootup, so the corresponding getters/setters will be only locally
accessible in arch/mips/rb532/gpio.c. Doing it this way also prevents
any driver from breaking others, as the complete GPIO configuration will
be done at a single place (i.e., inside gpio.c). IIRC pata-rb532-cf did
use them only at initialisation state and to prevent the described
situation.

I got the pata driver working by the way. I just wanted to get a
solution for the GPIO problem first, as it needs to access interrupt
level and status at least.

Greetings, Phil

Re: [PATCH] provide functions for gpio configuration

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

Hi Phil,

Le Thursday 30 October 2008 18:47:15 Phil Sutter, vous avez écrit :
> Sounds reasonable to me. So I'll prepare a patch which:
> * removes the function pointers from rb532_gpio_chip
> * exports getters and setters for interrupt status and level
>
> The GPIO config and function registers should indeed only be accessed at
> bootup, so the corresponding getters/setters will be only locally
> accessible in arch/mips/rb532/gpio.c. Doing it this way also prevents
> any driver from breaking others, as the complete GPIO configuration will
> be done at a single place (i.e., inside gpio.c). IIRC pata-rb532-cf did
> use them only at initialisation state and to prevent the described
> situation.

Perfect.

>
> I got the pata driver working by the way. I just wanted to get a
> solution for the GPIO problem first, as it needs to access interrupt
> level and status at least.

Great :) Good job.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] provide functions for gpio configuration

[Phil Sutter]

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.

Also do a complete configuration of the GPIO pin used by the cf driver,
which also serves as example for future drivers.

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

diff --git a/arch/mips/include/asm/mach-rc32434/rb.h b/arch/mips/include/asm/mach-rc32434/rb.h
index 0cb9466..f25a849 100644
--- a/arch/mips/include/asm/mach-rc32434/rb.h
+++ b/arch/mips/include/asm/mach-rc32434/rb.h
@@ -41,6 +41,7 @@
 #define BTCOMPARE	0x010044
 #define GPIOBASE	0x050000
 /* Offsets relative to GPIOBASE */
+#define GPIOFUNC	0x00
 #define GPIOCFG		0x04
 #define GPIOD		0x08
 #define GPIOILEVEL	0x0C
diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 70c4a67..de29aba 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -39,10 +39,6 @@
 struct rb532_gpio_chip {
 	struct gpio_chip chip;
 	void __iomem	 *regbase;
-	void		(*set_int_level)(struct gpio_chip *chip, unsigned offset, int value);
-	int		(*get_int_level)(struct gpio_chip *chip, unsigned offset);
-	void		(*set_int_status)(struct gpio_chip *chip, unsigned offset, int value);
-	int		(*get_int_status)(struct gpio_chip *chip, unsigned offset);
 };
 
 struct mpmc_device dev3;
@@ -111,15 +107,47 @@ unsigned char get_latch_u5(void)
 }
 EXPORT_SYMBOL(get_latch_u5);
 
+/* rb532_set_bit - sanely set a bit
+ * 
+ * bitval: new value for the bit
+ * offset: bit index in the 4 byte address range
+ * ioaddr: 4 byte aligned address being altered
+ */
+static inline void rb532_set_bit(unsigned bitval,
+		unsigned offset, void __iomem *ioaddr)
+{
+	unsigned long flags;
+	u32 val;
+
+	bitval = !!bitval;              /* map parameter to {0,1} */
+
+	local_irq_save(flags);
+
+	val = readl(ioaddr);
+	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
+	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	writel(val, ioaddr);
+
+	local_irq_restore(flags);
+}
+
+/* rb532_get_bit - read a bit
+ *
+ * returns the boolean state of the bit, which may be > 1
+ */
+static inline int rb532_get_bit(unsigned offset, void __iomem *ioaddr)
+{
+	return (readl(ioaddr) & (1 << offset));
+}
+
 /*
  * Return GPIO level */
 static int rb532_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	u32			mask = 1 << offset;
 	struct rb532_gpio_chip	*gpch;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	return readl(gpch->regbase + GPIOD) & mask;
+	return rb532_get_bit(offset, gpch->regbase + GPIOD);
 }
 
 /*
@@ -128,23 +156,10 @@ static int rb532_gpio_get(struct gpio_chip *chip, unsigned offset)
 static void rb532_gpio_set(struct gpio_chip *chip,
 				unsigned offset, int value)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
 	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpvr;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpvr = gpch->regbase + GPIOD;
-
-	local_irq_save(flags);
-	tmp = readl(gpvr);
-	if (value)
-		tmp |= mask;
-	else
-		tmp &= ~mask;
-	writel(tmp, gpvr);
-	local_irq_restore(flags);
+	rb532_set_bit(value, offset, gpch->regbase + GPIOD);
 }
 
 /*
@@ -152,21 +167,14 @@ static void rb532_gpio_set(struct gpio_chip *chip,
  */
 static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			value;
 	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpdr;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpdr = gpch->regbase + GPIOCFG;
 
-	local_irq_save(flags);
-	value = readl(gpdr);
-	value &= ~mask;
-	writel(value, gpdr);
-	local_irq_restore(flags);
+	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
+		return 1;	/* alternate function, GPIOCFG is ignored */
 
+	rb532_set_bit(0, offset, gpch->regbase + GPIOCFG);
 	return 0;
 }
 
@@ -176,117 +184,60 @@ static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int rb532_gpio_direction_output(struct gpio_chip *chip,
 					unsigned offset, int value)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
 	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpdr;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	writel(mask, gpch->regbase + GPIOD);
-	gpdr = gpch->regbase + GPIOCFG;
 
-	local_irq_save(flags);
-	tmp = readl(gpdr);
-	tmp |= mask;
-	writel(tmp, gpdr);
-	local_irq_restore(flags);
+	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
+		return 1;	/* alternate function, GPIOCFG is ignored */
+
+	/* set the initial output value */
+	rb532_set_bit(value, offset, gpch->regbase + GPIOD);
 
+	rb532_set_bit(1, offset, gpch->regbase + GPIOCFG);
 	return 0;
 }
 
-/*
- * Set the GPIO interrupt level
- */
-static void rb532_gpio_set_int_level(struct gpio_chip *chip,
-					unsigned offset, int value)
-{
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
-	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpil;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpil = gpch->regbase + GPIOILEVEL;
-
-	local_irq_save(flags);
-	tmp = readl(gpil);
-	if (value)
-		tmp |= mask;
-	else
-		tmp &= ~mask;
-	writel(tmp, gpil);
-	local_irq_restore(flags);
-}
+static struct rb532_gpio_chip rb532_gpio_chip[] = {
+	[0] = {
+		.chip = {
+			.label			= "gpio0",
+			.direction_input	= rb532_gpio_direction_input,
+			.direction_output	= rb532_gpio_direction_output,
+			.get			= rb532_gpio_get,
+			.set			= rb532_gpio_set,
+			.base			= 0,
+			.ngpio			= 32,
+		},
+	},
+};
 
 /*
- * Get the GPIO interrupt level
+ * Set GPIO interrupt level
  */
-static int rb532_gpio_get_int_level(struct gpio_chip *chip, unsigned offset)
+void rb532_gpio_set_ilevel(int bit, unsigned gpio)
 {
-	u32			mask = 1 << offset;
-	struct rb532_gpio_chip	*gpch;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	return readl(gpch->regbase + GPIOILEVEL) & mask;
+	rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOILEVEL);
 }
+EXPORT_SYMBOL(rb532_gpio_set_ilevel);
 
 /*
- * Set the GPIO interrupt status
+ * Set GPIO interrupt status 
  */
-static void rb532_gpio_set_int_status(struct gpio_chip *chip,
-				unsigned offset, int value)
+void rb532_gpio_set_istat(int bit, unsigned gpio)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
-	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpis;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpis = gpch->regbase + GPIOISTAT;
-
-	local_irq_save(flags);
-	tmp = readl(gpis);
-	if (value)
-		tmp |= mask;
-	else
-		tmp &= ~mask;
-	writel(tmp, gpis);
-	local_irq_restore(flags);
+	rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOISTAT);
 }
+EXPORT_SYMBOL(rb532_gpio_set_istat);
 
 /*
- * Get the GPIO interrupt status
+ * Configure GPIO alternate function
  */
-static int rb532_gpio_get_int_status(struct gpio_chip *chip, unsigned offset)
+static void rb532_gpio_set_func(int bit, unsigned gpio)
 {
-	u32			mask = 1 << offset;
-	struct rb532_gpio_chip	*gpch;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	return readl(gpch->regbase + GPIOISTAT) & mask;
+       rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
 }
 
-static struct rb532_gpio_chip rb532_gpio_chip[] = {
-	[0] = {
-		.chip = {
-			.label			= "gpio0",
-			.direction_input	= rb532_gpio_direction_input,
-			.direction_output	= rb532_gpio_direction_output,
-			.get			= rb532_gpio_get,
-			.set			= rb532_gpio_set,
-			.base			= 0,
-			.ngpio			= 32,
-		},
-		.get_int_level		= rb532_gpio_get_int_level,
-		.set_int_level		= rb532_gpio_set_int_level,
-		.get_int_status		= rb532_gpio_get_int_status,
-		.set_int_status		= rb532_gpio_set_int_status,
-	},
-};
-
 int __init rb532_gpio_init(void)
 {
 	struct resource *r;
@@ -310,9 +261,11 @@ int __init rb532_gpio_init(void)
 		return -ENXIO;
 	}
 
-	/* 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);
+	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;
 }
-- 
1.5.6.4

Re: [PATCH] provide functions for gpio configuration

[Florian Fainelli]

Le Thursday 30 October 2008 21:20:00 Phil Sutter, vous avez écrit :
> 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.
>
> Also do a complete configuration of the GPIO pin used by the cf driver,
> which also serves as example for future drivers.

Great work, thanks !

>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>

Acked-by: Florian Fainelli <florian@openwrt.org>

[PATCH] add prototypes for the exported symbols

[Phil Sutter]

Forgot to include them in the original patch.

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

diff --git a/arch/mips/include/asm/mach-rc32434/gpio.h b/arch/mips/include/asm/mach-rc32434/gpio.h
index c8e554e..b5cf645 100644
--- a/arch/mips/include/asm/mach-rc32434/gpio.h
+++ b/arch/mips/include/asm/mach-rc32434/gpio.h
@@ -84,5 +84,7 @@ extern void set_434_reg(unsigned reg_offs, unsigned bit, unsigned len, unsigned
 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);
 
 #endif /* _RC32434_GPIO_H_ */
-- 
1.5.6.4

[PATCH] provide functions for gpio configuration

[Phil Sutter]

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.

This is a combined version of the first two patches send in this thread
on the linux-mips mailinglist. It's also addressing the conflict
generated due to the change in arch/mips/include/asm/mach-rc32434/rb.h.
Though this patch depends on it, it has been moved to another
(yet uncommitted) patch changing the same part of rb.h.

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

diff --git a/arch/mips/include/asm/mach-rc32434/gpio.h b/arch/mips/include/asm/mach-rc32434/gpio.h
index c8e554e..b5cf645 100644
--- a/arch/mips/include/asm/mach-rc32434/gpio.h
+++ b/arch/mips/include/asm/mach-rc32434/gpio.h
@@ -84,5 +84,7 @@ extern void set_434_reg(unsigned reg_offs, unsigned bit, unsigned len, unsigned
 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);
 
 #endif /* _RC32434_GPIO_H_ */
diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 76a7fd9..de29aba 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -39,10 +39,6 @@
 struct rb532_gpio_chip {
 	struct gpio_chip chip;
 	void __iomem	 *regbase;
-	void		(*set_int_level)(struct gpio_chip *chip, unsigned offset, int value);
-	int		(*get_int_level)(struct gpio_chip *chip, unsigned offset);
-	void		(*set_int_status)(struct gpio_chip *chip, unsigned offset, int value);
-	int		(*get_int_status)(struct gpio_chip *chip, unsigned offset);
 };
 
 struct mpmc_device dev3;
@@ -111,15 +107,47 @@ unsigned char get_latch_u5(void)
 }
 EXPORT_SYMBOL(get_latch_u5);
 
+/* rb532_set_bit - sanely set a bit
+ * 
+ * bitval: new value for the bit
+ * offset: bit index in the 4 byte address range
+ * ioaddr: 4 byte aligned address being altered
+ */
+static inline void rb532_set_bit(unsigned bitval,
+		unsigned offset, void __iomem *ioaddr)
+{
+	unsigned long flags;
+	u32 val;
+
+	bitval = !!bitval;              /* map parameter to {0,1} */
+
+	local_irq_save(flags);
+
+	val = readl(ioaddr);
+	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
+	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	writel(val, ioaddr);
+
+	local_irq_restore(flags);
+}
+
+/* rb532_get_bit - read a bit
+ *
+ * returns the boolean state of the bit, which may be > 1
+ */
+static inline int rb532_get_bit(unsigned offset, void __iomem *ioaddr)
+{
+	return (readl(ioaddr) & (1 << offset));
+}
+
 /*
  * Return GPIO level */
 static int rb532_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	u32			mask = 1 << offset;
 	struct rb532_gpio_chip	*gpch;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	return readl(gpch->regbase + GPIOD) & mask;
+	return rb532_get_bit(offset, gpch->regbase + GPIOD);
 }
 
 /*
@@ -128,23 +156,10 @@ static int rb532_gpio_get(struct gpio_chip *chip, unsigned offset)
 static void rb532_gpio_set(struct gpio_chip *chip,
 				unsigned offset, int value)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
 	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpvr;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpvr = gpch->regbase + GPIOD;
-
-	local_irq_save(flags);
-	tmp = readl(gpvr);
-	if (value)
-		tmp |= mask;
-	else
-		tmp &= ~mask;
-	writel(tmp, gpvr);
-	local_irq_restore(flags);
+	rb532_set_bit(value, offset, gpch->regbase + GPIOD);
 }
 
 /*
@@ -152,21 +167,14 @@ static void rb532_gpio_set(struct gpio_chip *chip,
  */
 static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			value;
 	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpdr;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpdr = gpch->regbase + GPIOCFG;
 
-	local_irq_save(flags);
-	value = readl(gpdr);
-	value &= ~mask;
-	writel(value, gpdr);
-	local_irq_restore(flags);
+	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
+		return 1;	/* alternate function, GPIOCFG is ignored */
 
+	rb532_set_bit(0, offset, gpch->regbase + GPIOCFG);
 	return 0;
 }
 
@@ -176,117 +184,60 @@ static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int rb532_gpio_direction_output(struct gpio_chip *chip,
 					unsigned offset, int value)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
 	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpdr;
 
 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	writel(mask, gpch->regbase + GPIOD);
-	gpdr = gpch->regbase + GPIOCFG;
 
-	local_irq_save(flags);
-	tmp = readl(gpdr);
-	tmp |= mask;
-	writel(tmp, gpdr);
-	local_irq_restore(flags);
+	if (rb532_get_bit(offset, gpch->regbase + GPIOFUNC))
+		return 1;	/* alternate function, GPIOCFG is ignored */
+
+	/* set the initial output value */
+	rb532_set_bit(value, offset, gpch->regbase + GPIOD);
 
+	rb532_set_bit(1, offset, gpch->regbase + GPIOCFG);
 	return 0;
 }
 
-/*
- * Set the GPIO interrupt level
- */
-static void rb532_gpio_set_int_level(struct gpio_chip *chip,
-					unsigned offset, int value)
-{
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
-	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpil;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpil = gpch->regbase + GPIOILEVEL;
-
-	local_irq_save(flags);
-	tmp = readl(gpil);
-	if (value)
-		tmp |= mask;
-	else
-		tmp &= ~mask;
-	writel(tmp, gpil);
-	local_irq_restore(flags);
-}
+static struct rb532_gpio_chip rb532_gpio_chip[] = {
+	[0] = {
+		.chip = {
+			.label			= "gpio0",
+			.direction_input	= rb532_gpio_direction_input,
+			.direction_output	= rb532_gpio_direction_output,
+			.get			= rb532_gpio_get,
+			.set			= rb532_gpio_set,
+			.base			= 0,
+			.ngpio			= 32,
+		},
+	},
+};
 
 /*
- * Get the GPIO interrupt level
+ * Set GPIO interrupt level
  */
-static int rb532_gpio_get_int_level(struct gpio_chip *chip, unsigned offset)
+void rb532_gpio_set_ilevel(int bit, unsigned gpio)
 {
-	u32			mask = 1 << offset;
-	struct rb532_gpio_chip	*gpch;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	return readl(gpch->regbase + GPIOILEVEL) & mask;
+	rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOILEVEL);
 }
+EXPORT_SYMBOL(rb532_gpio_set_ilevel);
 
 /*
- * Set the GPIO interrupt status
+ * Set GPIO interrupt status 
  */
-static void rb532_gpio_set_int_status(struct gpio_chip *chip,
-				unsigned offset, int value)
+void rb532_gpio_set_istat(int bit, unsigned gpio)
 {
-	unsigned long		flags;
-	u32			mask = 1 << offset;
-	u32			tmp;
-	struct rb532_gpio_chip	*gpch;
-	void __iomem		*gpis;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	gpis = gpch->regbase + GPIOISTAT;
-
-	local_irq_save(flags);
-	tmp = readl(gpis);
-	if (value)
-		tmp |= mask;
-	else
-		tmp &= ~mask;
-	writel(tmp, gpis);
-	local_irq_restore(flags);
+	rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOISTAT);
 }
+EXPORT_SYMBOL(rb532_gpio_set_istat);
 
 /*
- * Get the GPIO interrupt status
+ * Configure GPIO alternate function
  */
-static int rb532_gpio_get_int_status(struct gpio_chip *chip, unsigned offset)
+static void rb532_gpio_set_func(int bit, unsigned gpio)
 {
-	u32			mask = 1 << offset;
-	struct rb532_gpio_chip	*gpch;
-
-	gpch = container_of(chip, struct rb532_gpio_chip, chip);
-	return readl(gpch->regbase + GPIOISTAT) & mask;
+       rb532_set_bit(bit, gpio, rb532_gpio_chip->regbase + GPIOFUNC);
 }
 
-static struct rb532_gpio_chip rb532_gpio_chip[] = {
-	[0] = {
-		.chip = {
-			.label			= "gpio0",
-			.direction_input	= rb532_gpio_direction_input,
-			.direction_output	= rb532_gpio_direction_output,
-			.get			= rb532_gpio_get,
-			.set			= rb532_gpio_set,
-			.base			= 0,
-			.ngpio			= 32,
-		},
-		.get_int_level		= rb532_gpio_get_int_level,
-		.set_int_level		= rb532_gpio_set_int_level,
-		.get_int_status		= rb532_gpio_get_int_status,
-		.set_int_status		= rb532_gpio_set_int_status,
-	},
-};
-
 int __init rb532_gpio_init(void)
 {
 	struct resource *r;
@@ -310,6 +261,12 @@ 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] provide functions for gpio configuration

[Sergei Shtylyov]

Hello.

Phil Sutter wrote:

> 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.
>
> This is a combined version of the first two patches send in this thread
> on the linux-mips mailinglist. It's also addressing the conflict
> generated due to the change in arch/mips/include/asm/mach-rc32434/rb.h.
> Though this patch depends on it, it has been moved to another
> (yet uncommitted) patch changing the same part of rb.h.
>
> Signed-off-by: Phil Sutter <n0-1@freewrt.org>
>   

   NAK, some of the code is wrong.

> diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
> index 76a7fd9..de29aba 100644
> --- a/arch/mips/rb532/gpio.c
> +++ b/arch/mips/rb532/gpio.c
>   
[...]
> @@ -111,15 +107,47 @@ unsigned char get_latch_u5(void)
>  }
>  EXPORT_SYMBOL(get_latch_u5);
>  
> +/* rb532_set_bit - sanely set a bit
> + * 
> + * bitval: new value for the bit
> + * offset: bit index in the 4 byte address range
> + * ioaddr: 4 byte aligned address being altered
> + */
> +static inline void rb532_set_bit(unsigned bitval,
> +		unsigned offset, void __iomem *ioaddr)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	bitval = !!bitval;              /* map parameter to {0,1} */
> +
> +	local_irq_save(flags);
> +
> +	val = readl(ioaddr);
> +	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
>   

   If bitval == 0, ~bitval evaluates to all ones, and the final AND mask 
~(0xffffffff << offset) clears 32-offset MSBs. What you want is simply 
~(1 << offset).

> +	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
>   

   And don't put spaces before ) and after (, checkpatch.pl wouldn't 
approve that. ;-)

> @@ -176,117 +184,60 @@ static int rb532_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>  static int rb532_gpio_direction_output(struct gpio_chip *chip,
>  					unsigned offset, int value)
>  {
> -	unsigned long		flags;
> -	u32			mask = 1 << offset;
> -	u32			tmp;
>  	struct rb532_gpio_chip	*gpch;
> -	void __iomem		*gpdr;
>  
>  	gpch = container_of(chip, struct rb532_gpio_chip, chip);
> -	writel(mask, gpch->regbase + GPIOD);
>   

   Hm, the old code seems really borked here...

MBR, Sergei

Re: [PATCH] provide functions for gpio configuration

[Phil Sutter]

Hi,

On Mon, Nov 03, 2008 at 12:56:16AM +0300, Sergei Shtylyov wrote:
> >+	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
> >  
> 
>   If bitval == 0, ~bitval evaluates to all ones, and the final AND mask 
> ~(0xffffffff << offset) clears 32-offset MSBs. What you want is simply 
> ~(1 << offset).

Right, I messed up boolean and bitwise inverting. The correct line is:

| val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */

That's the same as ~(1 << offset) when bitval is zero, but turns into a
no op when it's one. That's what I want here, successfully removing the
need for the if-else case.

> >@@ -176,117 +184,60 @@ static int rb532_gpio_direction_input(struct 
> >gpio_chip *chip, unsigned offset)
> > static int rb532_gpio_direction_output(struct gpio_chip *chip,
> > 					unsigned offset, int value)
> > {
> >-	unsigned long		flags;
> >-	u32			mask = 1 << offset;
> >-	u32			tmp;
> > 	struct rb532_gpio_chip	*gpch;
> >-	void __iomem		*gpdr;
> > 
> > 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
> >-	writel(mask, gpch->regbase + GPIOD);
> >  
> 
>   Hm, the old code seems really borked here...

It's not as bad as this may look like. In fact I could just simplify
lots of the functions by having them call rb532_set_bit().

A patch fixing the above mentioned issue follows, thanks for your help!

Greetings, Phil

[PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

The algorithm works unconditionally. If bitval is one, the first line is
a no op and the second line sets the bit at offset position. Vice versa,
if bitval is zero, the first line clears the bit at offset position and
the second line is a no op.

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

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 0e84c8a..ba9a0c4 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -124,8 +124,8 @@ static inline void rb532_set_bit(unsigned bitval,
 	local_irq_save(flags);
 
 	val = readl(ioaddr);
-	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
-	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
+	val |=  (bitval << offset);   /* set bit if bitval == 1 */
 	writel(val, ioaddr);
 
 	local_irq_restore(flags);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Atsushi Nemoto]

On Mon,  3 Nov 2008 15:30:25 +0100, Phil Sutter <n0-1@freewrt.org> wrote:
> The algorithm works unconditionally. If bitval is one, the first line is
> a no op and the second line sets the bit at offset position. Vice versa,
> if bitval is zero, the first line clears the bit at offset position and
> the second line is a no op.

Well, the linux gpio framework uses 0 for low, _nonzero_ for high.
You should not assume the bitval is 0 or 1.

	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
	val |= (!!bitval << offset);   /* set bit if bitval != 0 */

would be safe here.  Or you should ensure the bitval is 0 or 1
somewhere.

---
Atsushi Nemoto

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

Hi,

On Mon, Nov 03, 2008 at 11:48:56PM +0900, Atsushi Nemoto wrote:
> Well, the linux gpio framework uses 0 for low, _nonzero_ for high.
> You should not assume the bitval is 0 or 1.

Good point. I was thinking about this potential problem, too. This is
why the mentioned function contains the following line (sadly too far
off to occur in the patch):

| bitval = !!bitval;              /* map parameter to {0,1} */

I put that separately to not make the below lines even more unreadable.

> 	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
> 	val |= (!!bitval << offset);   /* set bit if bitval != 0 */

But as a boolean inverse has to be used anyway, your solution looks more
elegant than mine.

Greetings, Phil

[PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

This is a simplified version of the original fix, thanks to Atsushi Nemoto for
the hint.

Greetings, Phil

---

The algorithm works unconditionally. If bitval is one, the first line is
a no op and the second line sets the bit at offset position. Vice versa,
if bitval is zero, the first line clears the bit at offset position and
the second line is a no op.

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

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 0e84c8a..e35cb75 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -119,13 +119,11 @@ static inline void rb532_set_bit(unsigned bitval,
 	unsigned long flags;
 	u32 val;
 
-	bitval = !!bitval;              /* map parameter to {0,1} */
-
 	local_irq_save(flags);
 
 	val = readl(ioaddr);
-	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
-	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
+	val |= (!!bitval << offset);   /* set bit if bitval == 1 */
 	writel(val, ioaddr);
 
 	local_irq_restore(flags);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Ralf Baechle]

On Wed, Nov 12, 2008 at 12:09:30AM +0100, Phil Sutter wrote:

thanks, applied.

  Ralf