Hi Anatolij,
Looks pretty good, but some comments below...
On Sat, Aug 7, 2010 at 7:28 AM, Anatolij Gustschin <ag...@denx.de> wrote:
> Signed-off-by: Anatolij Gustschin <ag...@denx.de>
You haven't written a patch description. Give some details about how
the 512x gpio controller is different from the 8xxx one.
> ---
> arch/powerpc/platforms/Kconfig | 7 ++--
> arch/powerpc/sysdev/mpc8xxx_gpio.c | 54
> +++++++++++++++++++++++++++++++++++-
> 2 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index d1663db..471115a 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -304,13 +304,14 @@ config OF_RTC
> source "arch/powerpc/sysdev/bestcomm/Kconfig"
>
> config MPC8xxx_GPIO
> - bool "MPC8xxx GPIO support"
> - depends on PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || FSL_SOC_BOOKE
> || PPC_86xx
> + bool "MPC512x/MPC8xxx GPIO support"
> + depends on PPC_MPC512x || PPC_MPC831x || PPC_MPC834x || PPC_MPC837x
> || \
> + FSL_SOC_BOOKE || PPC_86xx
> select GENERIC_GPIO
> select ARCH_REQUIRE_GPIOLIB
> help
> Say Y here if you're going to use hardware that connects to the
> - MPC831x/834x/837x/8572/8610 GPIOs.
> + MPC512x/831x/834x/837x/8572/8610 GPIOs.
>
> config SIMPLE_GPIO
> bool "Support for simple, memory-mapped GPIO controllers"
> diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c
> b/arch/powerpc/sysdev/mpc8xxx_gpio.c
> index 2b69084..f5b4959 100644
> --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
> +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
> @@ -1,5 +1,5 @@
> /*
> - * GPIOs on MPC8349/8572/8610 and compatible
> + * GPIOs on MPC512x/8349/8572/8610 and compatible
> *
> * Copyright (C) 2008 Peter Korsgaard <jac...@sunsite.dk>
> *
> @@ -26,6 +26,7 @@
> #define GPIO_IER 0x0c
> #define GPIO_IMR 0x10
> #define GPIO_ICR 0x14
> +#define GPIO_ICR2 0x18
>
> struct mpc8xxx_gpio_chip {
> struct of_mm_gpio_chip mm_gc;
> @@ -215,6 +216,51 @@ static int mpc8xxx_irq_set_type(unsigned int virq,
> unsigned int flow_type)
> return 0;
> }
>
> +static int mpc512x_irq_set_type(unsigned int virq, unsigned int flow_type)
> +{
> + struct mpc8xxx_gpio_chip *mpc8xxx_gc = get_irq_chip_data(virq);
> + struct of_mm_gpio_chip *mm = &mpc8xxx_gc->mm_gc;
> + unsigned long gpio = virq_to_hw(virq);
> + void __iomem *reg;
> + unsigned int shift;
> + unsigned long flags;
> +
> + if (gpio < 16) {
> + reg = mm->regs + GPIO_ICR;
> + shift = (15 - gpio) * 2;
> + } else {
> + reg = mm->regs + GPIO_ICR2;
> + shift = (15 - (gpio % 16)) * 2;
> + }
> +
> + switch (flow_type) {
> + case IRQ_TYPE_EDGE_FALLING:
> + case IRQ_TYPE_LEVEL_LOW:
> + spin_lock_irqsave(&mpc8xxx_gc->lock, flags);
> + clrsetbits_be32(reg, 3 << shift, 2 << shift);
> + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags);
> + break;
> +
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_LEVEL_HIGH:
> + spin_lock_irqsave(&mpc8xxx_gc->lock, flags);
> + clrsetbits_be32(reg, 3 << shift, 1 << shift);
> + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags);
> + break;
> +
> + case IRQ_TYPE_EDGE_BOTH:
> + spin_lock_irqsave(&mpc8xxx_gc->lock, flags);
> + clrbits32(reg, 3 << shift);
> + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags);
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static struct irq_chip mpc8xxx_irq_chip = {
> .name = "mpc8xxx-gpio",
> .unmask = mpc8xxx_irq_unmask,
> @@ -226,6 +272,9 @@ static struct irq_chip mpc8xxx_irq_chip = {
> static int mpc8xxx_gpio_irq_map(struct irq_host *h, unsigned int virq,
> irq_hw_number_t hw)
> {
> + if (of_device_is_compatible(h->of_node, "fsl,mpc5121-gpio"))
> + mpc8xxx_irq_chip.set_type = mpc512x_irq_set_type;
> +
You can put the set type hook into the of_match_table data which you
will need for of_find_matching_node() (see below).
> set_irq_chip_data(virq, h->host_data);
> set_irq_chip_and_handler(virq, &mpc8xxx_irq_chip, handle_level_irq);
> set_irq_type(virq, IRQ_TYPE_NONE);
> @@ -330,6 +379,9 @@ static int __init mpc8xxx_add_gpiochips(void)
> for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio")
> mpc8xxx_add_controller(np);
>
> + for_each_compatible_node(np, NULL, "fsl,mpc5121-gpio")
> + mpc8xxx_add_controller(np);
> +
This list is getting too long. Refactor this function to use
for_each_matching_node(). Also doing it this way is dangerous because
if say a 5121 gpio node claims compatibility with a 8610 gpio node,
then the gpio controller will get registered twice.
> return 0;
> }
> arch_initcall(mpc8xxx_add_gpiochips);
> --
> 1.7.0.4
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
