Re: [PATCH] powerpc/4xx: Add 4xx gpio support

Hi Stefan,

On Thu, Oct 09, 2008 at 04:53:34PM +0200, Stefan Roese wrote:

This patch adds a 4xx gpio driver. Tested on a 405EP and a 440EPx
platform.

Signed-off-by: Stefan Roese <sr@denx.de>
---

This driver probably needs some final polishing. E.g. some parts can
be extraced from ppc4xx_gpio_dir_in()/ppc4xx_gpio_dir_out() and moved
into seperate functions to make the code more compact. Also some
documentation would be handy. I didn't include it for now since I
wanted this driver version out now for comments. Documentation
will follow in a later version.

Other suggestion are welcome. :)

Few comments below.

I heard that we tend to put platform GPIO Kconfig symbols into
the arch/powerpc/platforms/Kconfig. Otherwise user-selectable
options appear at the top-level menu in the `menuconfig'.

dd

+#include <linux/stddef.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>

interrupts?

+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>

#include <linux/gpio.h>
#include <linux/types.h>
#include <linux/spinlock.h>

+#include <asm/dcr.h>
+#include <asm/dcr-regs.h>
+#include <asm/reg.h>
+#include <asm/io.h>

linux/io.h

+
+#define GPIO_ALT1_SEL	0x40000000
+#define GPIO_ALT2_SEL	0x80000000
+#define GPIO_ALT3_SEL	0xc0000000
+#define GPIO_IN_SEL	0x40000000
+#define GPIO_MASK	0xc0000000
+
+#define GPIO_MAX	32
+#define GPIO_VAL(gpio)	(0x80000000 >> (gpio))
+
+struct ppc4xx_gpio {
+	u32 or;		/* 0x00: Output */

I think __be32 here would be better.

+	u32 tcr;	/* 0x04: Three-State Control */
+	u32 osrl;	/* 0x08: Output Select (Low) */
+	u32 osrh;	/* 0x0c: Output Select (High) */
+	u32 tsrl;	/* 0x10: Three-State Select (Low) */
+	u32 tsrh;	/* 0x14: Three-State Select (High) */
+	u32 odr;	/* 0x18: Open Drain */
+	u32 ir;		/* 0x1c: Input */
+	u32 rr1;	/* 0x20: Receive Register 1 */
+	u32 rr2;	/* 0x24: Receive Register 2 */
+	u32 rr3;	/* 0x28: Receive Register 3 */
+	u32 res;
+	u32 isr1l;	/* 0x30: Input Select 1 (Low) */
+	u32 isr1h;	/* 0x34: Input Select 1 (High) */
+	u32 isr2l;	/* 0x38: Input Select 1 (Low) */
+	u32 isr2h;	/* 0x3c: Input Select 1 (High) */
+	u32 isr3l;	/* 0x40: Input Select 1 (Low) */
+	u32 isr3h;	/* 0x44: Input Select 1 (High) */
+};
+
+struct ppc4xx_gpio_chip {
+	struct of_mm_gpio_chip mmchip;
+	spinlock_t lock;

Um, no shadowed registers.. can cause problems with open drain
pins, no?

+};
+
+static inline struct ppc4xx_gpio_chip *
+to_ppc4xx_gpio_chip(struct of_mm_gpio_chip *mm_gc)
+{
+	return container_of(mm_gc, struct ppc4xx_gpio_chip, mmchip);
+}
+
+static int ppc4xx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct ppc4xx_gpio __iomem *regs = mm_gc->regs;
+
+	return (in_be32(&regs->ir) & GPIO_VAL(gpio) ? 1 : 0);

Outermost parenthesis aren't necessary. Plus no need for ? 1 : 0.
gpio_get returns 0 for low state, and any value != 0 for high state.

+}
+
+static inline void __ppc4xx_gpio_set(struct gpio_chip *gc,
+				     unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct ppc4xx_gpio __iomem *regs = mm_gc->regs;
+
+	if (val)
+		out_be32(&regs->or, in_be32(&regs->or) | GPIO_VAL(gpio));
+	else
+		out_be32(&regs->or, in_be32(&regs->or) & ~GPIO_VAL(gpio));
+}
+
+static void ppc4xx_gpio_set(struct gpio_chip *gc,
+			    unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct ppc4xx_gpio_chip *chip = to_ppc4xx_gpio_chip(mm_gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&chip->lock, flags);
+	__ppc4xx_gpio_set(gc, gpio, val);
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
+}
+
+static int ppc4xx_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct ppc4xx_gpio_chip *chip = to_ppc4xx_gpio_chip(mm_gc);
+	struct ppc4xx_gpio __iomem *regs = mm_gc->regs;
+	unsigned long flags;
+	u32 offs, mask, mask2, val32;

I'd write is as

u32 offs;
u32 mask;
...

+	u32 gpio2 = 0;
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	if (gpio >= GPIO_MAX / 2) {
+		offs = 0x4;
+		gpio2 = (gpio - GPIO_MAX / 2) << 1;
+	}
+
+	mask = 0x80000000 >> gpio;
+	mask2 = 0xc0000000 >> gpio2;
+
+	/* first set TCR to 0 */
+	out_be32(&regs->tcr, in_be32(&regs->tcr) & ~mask);
+
+	/* now set the direction */
+	val32 = in_be32(&regs->isr1l + offs) & ~mask2;
+	val32 |= GPIO_IN_SEL >> gpio2;
+	out_be32(&regs->isr1l + offs, val32);

clrsetbits32() would make this look nicer.

+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	return 0;
+}
+
+static int ppc4xx_gpio_dir_out(struct gpio_chip *gc,
+			       unsigned int gpio, int val)
+{
+	struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+	struct ppc4xx_gpio_chip *chip = to_ppc4xx_gpio_chip(mm_gc);
+	struct ppc4xx_gpio __iomem *regs = mm_gc->regs;
+	unsigned long flags;
+	u32 offs, mask, mask2, val32;
+	u32 gpio2 = 0;
+
+	spin_lock_irqsave(&chip->lock, flags);
+
+	if (gpio >= GPIO_MAX / 2) {
+		offs = 0x4;
+		gpio2 = (gpio - GPIO_MAX / 2) << 1;
+	}
+
+	mask = 0x80000000 >> gpio;
+	mask2 = 0xc0000000 >> gpio2;
+
+	/* first set TCR to 0 */
+	out_be32(&regs->tcr, in_be32(&regs->tcr) & ~mask);

clrbits32()

+
+	/* set initial value */
+	__ppc4xx_gpio_set(gc, gpio, val);
+
+	/* now set the direction */
+	val32 = in_be32(&regs->osrl + offs) & ~mask2;
+	out_be32(&regs->osrl + offs, val32);

clrbits32()

+
+	/* now configure TCR to drive output */
+	out_be32(&regs->tcr, in_be32(&regs->tcr) | mask);

setbits32()

+
+	spin_unlock_irqrestore(&chip->lock, flags);
+
+	pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val);
+
+	return 0;
+}
+
+static int __devinit ppc4xx_gpiochip_probe(struct of_device *ofdev,
+					   const struct of_device_id *match)
+{
+	struct ppc4xx_gpio_chip *chip;
+	struct of_gpio_chip *ofchip;
+	int ret;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	ofchip = &chip->mmchip.of_gc;
+
+	spin_lock_init(&chip->lock);
+
+	ofchip->gpio_cells = 2;
+	ofchip->gc.ngpio = 32;
+	ofchip->gc.direction_input = ppc4xx_gpio_dir_in;
+	ofchip->gc.direction_output = ppc4xx_gpio_dir_out;
+	ofchip->gc.get = ppc4xx_gpio_get;
+	ofchip->gc.set = ppc4xx_gpio_set;
+
+	ret = of_mm_gpiochip_add(ofdev->node, &chip->mmchip);
+	if (ret) {
+		free(chip);

kfree()

+		return ret;
+	}
+
+	return 0;
+}
+
+static int ppc4xx_gpiochip_remove(struct of_device *ofdev)
+{
+	return -EBUSY;
+}
+
+static const struct of_device_id ppc4xx_gpiochip_match[] = {
+	{
+		.compatible = "ibm,gpio",

Seems too generic, but I really don't know the 4xx enough to tell
that for sure...

+	},
+	{}
+};
+
+static struct of_platform_driver ppc4xx_gpiochip_driver = {
+	.name = "gpio",

This is too generic for sure.

+	.match_table = ppc4xx_gpiochip_match,
+	.probe = ppc4xx_gpiochip_probe,
+	.remove = ppc4xx_gpiochip_remove,
+};
+
+static int __init ppc4xx_gpio_init(void)
+{
+	if (of_register_platform_driver(&ppc4xx_gpiochip_driver))
+		printk(KERN_ERR "%s: Unable to register GPIO driver\n", __func__);
+
+	return 0;
+}
+arch_initcall(ppc4xx_gpio_init);

There is no point of doing this in arch_initcall since most boards
call of_platform_bus_probe() at device_initcall time... It would be
better to get rid of the of_platform_driver() and just walk the device
tree via for_each_compatible_node() { register chips } in this
arch_initcall.


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2