| Message ID | 1306142730-7528-1-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Grant Likely |
| Headers | show |
Hi Grant, Ping. On Mon, 23 May 2011 11:25:30 +0200 Anatolij Gustschin <agust@denx.de> wrote: > The mpc52xx_gpio driver currently supports 8 wakeup GPIOs and 32 > simple GPIOs. Extend it to also support GPIO function of 8 simple > interrupt GPIOs controlled in the standard GPIO register module. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 117 ++++++++++++++++++++++++++++ > 1 files changed, 117 insertions(+), 0 deletions(-) ... Thanks, Anatolij
On Mon, May 23, 2011 at 11:25:30AM +0200, Anatolij Gustschin wrote: > The mpc52xx_gpio driver currently supports 8 wakeup GPIOs and 32 > simple GPIOs. Extend it to also support GPIO function of 8 simple > interrupt GPIOs controlled in the standard GPIO register module. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 117 ++++++++++++++++++++++++++++ I don't want to merge more open coded MMIO gpio driver code. This whole driver really needs to be converted to use GENERIC_GPIO. g. > 1 files changed, 117 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c > index 1757d1d..42a0759 100644 > --- a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c > +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c > @@ -35,6 +35,9 @@ struct mpc52xx_gpiochip { > unsigned int shadow_dvo; > unsigned int shadow_gpioe; > unsigned int shadow_ddr; > + unsigned char sint_shadow_dvo; > + unsigned char sint_shadow_gpioe; > + unsigned char sint_shadow_ddr; > }; > > /* > @@ -309,6 +312,100 @@ mpc52xx_simple_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) > return 0; > } > > +static int mpc52xx_sint_gpio_get(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > + struct mpc52xx_gpio __iomem *regs = mm_gc->regs; > + unsigned int ret; > + > + ret = (in_8(®s->sint_ival) >> (7 - gpio)) & 1; > + > + pr_debug("%s: gpio: %d ret: %d\n", __func__, gpio, ret); > + > + return ret; > +} > + > +static inline void > +__mpc52xx_sint_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 mpc52xx_gpiochip *chip = container_of(mm_gc, > + struct mpc52xx_gpiochip, mmchip); > + struct mpc52xx_gpio __iomem *regs = mm_gc->regs; > + > + if (val) > + chip->sint_shadow_dvo |= 1 << (7 - gpio); > + else > + chip->sint_shadow_dvo &= ~(1 << (7 - gpio)); > + > + out_8(®s->sint_dvo, chip->sint_shadow_dvo); > +} > + > +static void > +mpc52xx_sint_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&gpio_lock, flags); > + > + __mpc52xx_sint_gpio_set(gc, gpio, val); > + > + spin_unlock_irqrestore(&gpio_lock, flags); > + > + pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val); > +} > + > +static int mpc52xx_sint_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > + struct mpc52xx_gpiochip *chip = container_of(mm_gc, > + struct mpc52xx_gpiochip, mmchip); > + struct mpc52xx_gpio __iomem *regs = mm_gc->regs; > + unsigned long flags; > + > + spin_lock_irqsave(&gpio_lock, flags); > + > + /* set the direction */ > + chip->sint_shadow_ddr &= ~(1 << (7 - gpio)); > + out_8(®s->sint_ddr, chip->sint_shadow_ddr); > + > + /* and enable the pin */ > + chip->sint_shadow_gpioe |= 1 << (7 - gpio); > + out_8(®s->sint_gpioe, chip->sint_shadow_gpioe); > + > + spin_unlock_irqrestore(&gpio_lock, flags); > + > + return 0; > +} > + > +static int > +mpc52xx_sint_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 mpc52xx_gpio __iomem *regs = mm_gc->regs; > + struct mpc52xx_gpiochip *chip = container_of(mm_gc, > + struct mpc52xx_gpiochip, mmchip); > + unsigned long flags; > + > + spin_lock_irqsave(&gpio_lock, flags); > + > + __mpc52xx_sint_gpio_set(gc, gpio, val); > + > + /* Then set direction */ > + chip->sint_shadow_ddr |= 1 << (7 - gpio); > + out_8(®s->sint_ddr, chip->sint_shadow_ddr); > + > + /* Finally enable the pin */ > + chip->sint_shadow_gpioe |= 1 << (7 - gpio); > + out_8(®s->sint_gpioe, chip->sint_shadow_gpioe); > + > + spin_unlock_irqrestore(&gpio_lock, flags); > + > + pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val); > + > + return 0; > +} > + > static int __devinit mpc52xx_simple_gpiochip_probe(struct platform_device *ofdev) > { > struct mpc52xx_gpiochip *chip; > @@ -337,6 +434,26 @@ static int __devinit mpc52xx_simple_gpiochip_probe(struct platform_device *ofdev > chip->shadow_ddr = in_be32(®s->simple_ddr); > chip->shadow_dvo = in_be32(®s->simple_dvo); > > + chip = kzalloc(sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + gc = &chip->mmchip.gc; > + > + gc->ngpio = 8; > + gc->direction_input = mpc52xx_sint_gpio_dir_in; > + gc->direction_output = mpc52xx_sint_gpio_dir_out; > + gc->get = mpc52xx_sint_gpio_get; > + gc->set = mpc52xx_sint_gpio_set; > + > + ret = of_mm_gpiochip_add(ofdev->dev.of_node, &chip->mmchip); > + if (ret) > + return ret; > + > + regs = chip->mmchip.regs; > + chip->sint_shadow_gpioe = in_8(®s->sint_gpioe); > + chip->sint_shadow_ddr = in_8(®s->sint_ddr); > + chip->sint_shadow_dvo = in_8(®s->sint_dvo); > return 0; > } > > -- > 1.7.1 >
On Wed, 6 Jul 2011 11:52:45 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Mon, May 23, 2011 at 11:25:30AM +0200, Anatolij Gustschin wrote: > > The mpc52xx_gpio driver currently supports 8 wakeup GPIOs and 32 > > simple GPIOs. Extend it to also support GPIO function of 8 simple > > interrupt GPIOs controlled in the standard GPIO register module. > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > --- > > arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 117 ++++++++++++++++++++++++++++ > > I don't want to merge more open coded MMIO gpio driver code. This whole driver really needs to be converted to use GENERIC_GPIO. I'm not sure I understand what you mean. Do you mean the conversion to drop of_mm_* stuff? Thanks, Anatolij
On Sat, Jul 09, 2011 at 12:14:09PM +0200, Anatolij Gustschin wrote: > On Wed, 6 Jul 2011 11:52:45 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > > > On Mon, May 23, 2011 at 11:25:30AM +0200, Anatolij Gustschin wrote: > > > The mpc52xx_gpio driver currently supports 8 wakeup GPIOs and 32 > > > simple GPIOs. Extend it to also support GPIO function of 8 simple > > > interrupt GPIOs controlled in the standard GPIO register module. > > > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > > --- > > > arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 117 ++++++++++++++++++++++++++++ > > > > I don't want to merge more open coded MMIO gpio driver code. This whole driver really needs to be converted to use GENERIC_GPIO. > > I'm not sure I understand what you mean. Do you mean > the conversion to drop of_mm_* stuff? No, I mean conversion to use the generic gpio register access functions instead of creating new ones. g.
On Fri, 15 Jul 2011 14:08:01 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Sat, Jul 09, 2011 at 12:14:09PM +0200, Anatolij Gustschin wrote: > > On Wed, 6 Jul 2011 11:52:45 -0600 > > Grant Likely <grant.likely@secretlab.ca> wrote: > > > > > On Mon, May 23, 2011 at 11:25:30AM +0200, Anatolij Gustschin wrote: > > > > The mpc52xx_gpio driver currently supports 8 wakeup GPIOs and 32 > > > > simple GPIOs. Extend it to also support GPIO function of 8 simple > > > > interrupt GPIOs controlled in the standard GPIO register module. > > > > > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > > > --- > > > > arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 117 ++++++++++++++++++++++++++++ > > > > > > I don't want to merge more open coded MMIO gpio driver code. This whole driver really needs to be converted to use GENERIC_GPIO. > > > > I'm not sure I understand what you mean. Do you mean > > the conversion to drop of_mm_* stuff? > > No, I mean conversion to use the generic gpio register access > functions instead of creating new ones. Could you please explain which functions exactly should be generalized? All functions (32-bit gpio register access and 8-bit gpio register access for wakeup and simple interrupt gpio registers) by providing an access function and doing all simple/wakeup/simple-interrupt gpio specific stuff in it? Or only providing generic functions for 8-bit gpio registers and use them for both wakeup and simple-interrupt gpios? Which struct should be used for register offset calculation in accessors then? Should a generic 8-bit gpio register description struct be added for this purpose? Or some defines for generic register offsets? How to differentiate between shadow registers for wakeup and simple- interrupt gpios in generic accessors? By adding a type field to mpc52xx_gpiochip struct and checking for it in generic accessors? Thanks, Anatolij
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c index 1757d1d..42a0759 100644 --- a/arch/powerpc/platforms/52xx/mpc52xx_gpio.c +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c @@ -35,6 +35,9 @@ struct mpc52xx_gpiochip { unsigned int shadow_dvo; unsigned int shadow_gpioe; unsigned int shadow_ddr; + unsigned char sint_shadow_dvo; + unsigned char sint_shadow_gpioe; + unsigned char sint_shadow_ddr; }; /* @@ -309,6 +312,100 @@ mpc52xx_simple_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val) return 0; } +static int mpc52xx_sint_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct mpc52xx_gpio __iomem *regs = mm_gc->regs; + unsigned int ret; + + ret = (in_8(®s->sint_ival) >> (7 - gpio)) & 1; + + pr_debug("%s: gpio: %d ret: %d\n", __func__, gpio, ret); + + return ret; +} + +static inline void +__mpc52xx_sint_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 mpc52xx_gpiochip *chip = container_of(mm_gc, + struct mpc52xx_gpiochip, mmchip); + struct mpc52xx_gpio __iomem *regs = mm_gc->regs; + + if (val) + chip->sint_shadow_dvo |= 1 << (7 - gpio); + else + chip->sint_shadow_dvo &= ~(1 << (7 - gpio)); + + out_8(®s->sint_dvo, chip->sint_shadow_dvo); +} + +static void +mpc52xx_sint_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) +{ + unsigned long flags; + + spin_lock_irqsave(&gpio_lock, flags); + + __mpc52xx_sint_gpio_set(gc, gpio, val); + + spin_unlock_irqrestore(&gpio_lock, flags); + + pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val); +} + +static int mpc52xx_sint_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio) +{ + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct mpc52xx_gpiochip *chip = container_of(mm_gc, + struct mpc52xx_gpiochip, mmchip); + struct mpc52xx_gpio __iomem *regs = mm_gc->regs; + unsigned long flags; + + spin_lock_irqsave(&gpio_lock, flags); + + /* set the direction */ + chip->sint_shadow_ddr &= ~(1 << (7 - gpio)); + out_8(®s->sint_ddr, chip->sint_shadow_ddr); + + /* and enable the pin */ + chip->sint_shadow_gpioe |= 1 << (7 - gpio); + out_8(®s->sint_gpioe, chip->sint_shadow_gpioe); + + spin_unlock_irqrestore(&gpio_lock, flags); + + return 0; +} + +static int +mpc52xx_sint_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 mpc52xx_gpio __iomem *regs = mm_gc->regs; + struct mpc52xx_gpiochip *chip = container_of(mm_gc, + struct mpc52xx_gpiochip, mmchip); + unsigned long flags; + + spin_lock_irqsave(&gpio_lock, flags); + + __mpc52xx_sint_gpio_set(gc, gpio, val); + + /* Then set direction */ + chip->sint_shadow_ddr |= 1 << (7 - gpio); + out_8(®s->sint_ddr, chip->sint_shadow_ddr); + + /* Finally enable the pin */ + chip->sint_shadow_gpioe |= 1 << (7 - gpio); + out_8(®s->sint_gpioe, chip->sint_shadow_gpioe); + + spin_unlock_irqrestore(&gpio_lock, flags); + + pr_debug("%s: gpio: %d val: %d\n", __func__, gpio, val); + + return 0; +} + static int __devinit mpc52xx_simple_gpiochip_probe(struct platform_device *ofdev) { struct mpc52xx_gpiochip *chip; @@ -337,6 +434,26 @@ static int __devinit mpc52xx_simple_gpiochip_probe(struct platform_device *ofdev chip->shadow_ddr = in_be32(®s->simple_ddr); chip->shadow_dvo = in_be32(®s->simple_dvo); + chip = kzalloc(sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + gc = &chip->mmchip.gc; + + gc->ngpio = 8; + gc->direction_input = mpc52xx_sint_gpio_dir_in; + gc->direction_output = mpc52xx_sint_gpio_dir_out; + gc->get = mpc52xx_sint_gpio_get; + gc->set = mpc52xx_sint_gpio_set; + + ret = of_mm_gpiochip_add(ofdev->dev.of_node, &chip->mmchip); + if (ret) + return ret; + + regs = chip->mmchip.regs; + chip->sint_shadow_gpioe = in_8(®s->sint_gpioe); + chip->sint_shadow_ddr = in_8(®s->sint_ddr); + chip->sint_shadow_dvo = in_8(®s->sint_dvo); return 0; }
The mpc52xx_gpio driver currently supports 8 wakeup GPIOs and 32 simple GPIOs. Extend it to also support GPIO function of 8 simple interrupt GPIOs controlled in the standard GPIO register module. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 117 ++++++++++++++++++++++++++++ 1 files changed, 117 insertions(+), 0 deletions(-)