| Message ID | 20080917175820.GA22539@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes:
Hi,
Anton> This patch implements GPIOLIB support for MPC8349-compatible SOC GPIOs.
Anton> MPC8610 adopted this GPIO unit, so let's place it into sysdev.
Anton> We'll need these gpios to support IrDA transceiver on MPC8610HPCD.
It seems basically the same as my patch (gpio driver for
mpc831x/mpc834x/mpc837x with OF bindings) from September 5th except
you don't have any documentation and the Kconfig doesn't depend on
831x/834x/837x.
Galak, did you have time to look at these patches yet?
Anton> +struct fsl_gpio_chip {
Anton> + struct of_mm_gpio_chip mm_gc;
Anton> + spinlock_t lock;
Anton> +
Anton> + /* shadowed data register to clear/set bits safely */
Anton> + u32 gpdat;
Why do you need this when you have the lock?
On Wed, Sep 17, 2008 at 09:30:16PM +0200, Peter Korsgaard wrote: > >>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes: > > Hi, > > Anton> This patch implements GPIOLIB support for MPC8349-compatible SOC GPIOs. > Anton> MPC8610 adopted this GPIO unit, so let's place it into sysdev. > > Anton> We'll need these gpios to support IrDA transceiver on MPC8610HPCD. > > It seems basically the same as my patch (gpio driver for > mpc831x/mpc834x/mpc837x with OF bindings) from September 5th except > you don't have any documentation Well, do we really need documentation for every trivial gpio controller? The bindings are the same, only different compatible entries... If we do need the documentation for this, I can readily include it. > and the Kconfig doesn't depend on > 831x/834x/837x. This is purposely. We also need support for 8610, and maybe later we'll find another chip with the same unit. So, to not touch the Kconfig for every new chip I just made it PPC32-wide. Other option is to depend on FSL_SOC, but the driver really does not depend on any fsl_soc stuff... > Galak, did you have time to look at these patches yet? > > Anton> +struct fsl_gpio_chip { > Anton> + struct of_mm_gpio_chip mm_gc; > Anton> + spinlock_t lock; > Anton> + > Anton> + /* shadowed data register to clear/set bits safely */ > Anton> + u32 gpdat; > > Why do you need this when you have the lock? See this discussion: http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg12299.html Thanks,
>>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes: Hi, >> It seems basically the same as my patch (gpio driver for >> mpc831x/mpc834x/mpc837x with OF bindings) from September 5th except >> you don't have any documentation Anton> Well, do we really need documentation for every trivial gpio Anton> controller? The bindings are the same, only different compatible Anton> entries... Well, we need some way of knowing that atleast. Having people grepping through arch/powerpc/boot/dts/ (and knowing that they should look for 8349 even though they might use 8313 or 8610) doesn't seem optimal to me - I take it that's why we created Documentation/powerpc/dts-bindings in the first place? Anton> If we do need the documentation for this, I can readily include it. >> and the Kconfig doesn't depend on >> 831x/834x/837x. Anton> This is purposely. We also need support for 8610, and maybe Anton> later we'll find another chip with the same unit. So, to not touch Anton> the Kconfig for every new chip I just made it PPC32-wide. Other Anton> option is to depend on FSL_SOC, but the driver really does not Anton> depend on any fsl_soc stuff... Adding another symbol to the Kconfig once it is verified that a new SoC is compatible doesn't seem like a big deal - Figuring out all the knobs we already have is, without having options for stuff that is known to be irrelevant for the SoC. The other 83xx specific drivers also depend on PPC_83xx. >> Why do you need this when you have the lock? Anton> See this discussion: Anton> http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg12299.html Ahh, for open drain outputs. gpiolib doesn't actually provide an interface to put the gpios in that mode, but it could ofcause have been set up in the bootloader. I'll fix that and send an updated patch.
On Fri, Sep 19, 2008 at 05:11:15PM +0200, Peter Korsgaard wrote: > >>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes: > > Hi, > > >> It seems basically the same as my patch (gpio driver for > >> mpc831x/mpc834x/mpc837x with OF bindings) from September 5th except > >> you don't have any documentation > > Anton> Well, do we really need documentation for every trivial gpio > Anton> controller? The bindings are the same, only different compatible > Anton> entries... > > Well, we need some way of knowing that atleast. Having people grepping > through arch/powerpc/boot/dts/ (and knowing that they should look for > 8349 even though they might use 8313 or 8610) doesn't seem optimal to > me - I take it that's why we created > Documentation/powerpc/dts-bindings in the first place? Ok. > Anton> If we do need the documentation for this, I can readily include it. > > >> and the Kconfig doesn't depend on > >> 831x/834x/837x. > > Anton> This is purposely. We also need support for 8610, and maybe > Anton> later we'll find another chip with the same unit. So, to not touch > Anton> the Kconfig for every new chip I just made it PPC32-wide. Other > Anton> option is to depend on FSL_SOC, but the driver really does not > Anton> depend on any fsl_soc stuff... > > Adding another symbol to the Kconfig once it is verified that a new > SoC is compatible doesn't seem like a big deal - Figuring out all the > knobs we already have is, without having options for stuff that is > known to be irrelevant for the SoC. > > The other 83xx specific drivers also depend on PPC_83xx. Lets wait for Kumar's comments. We've already had a PPC_* mess for the USB_EHCI_FSL symbol. What I've learned from it, is that huge PPC_* list isn't perfect either. See this discussion: http://www.mail-archive.com/linux-usb@vger.kernel.org/msg01926.html And this one: http://fixunix.com/kernel/397972-patch-1-4-spi-powerpc-spi_mpc83xx-handles-freescale-mpc8610-well.html > >> Why do you need this when you have the lock? > > Anton> See this discussion: > > Anton> http://www.mail-archive.com/linuxppc-dev@ozlabs.org/msg12299.html > > Ahh, for open drain outputs. gpiolib doesn't actually provide an > interface to put the gpios in that mode, but it could ofcause have > been set up in the bootloader. I'll fix that and send an updated > patch. Pardon? Who will send an updated patch, and what patch exactly? ;-)
On Sep 19, 2008, at 10:33 AM, Anton Vorontsov wrote: > On Fri, Sep 19, 2008 at 05:11:15PM +0200, Peter Korsgaard wrote: >>>>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes: >> >> Hi, >> >>>> It seems basically the same as my patch (gpio driver for >>>> mpc831x/mpc834x/mpc837x with OF bindings) from September 5th except >>>> you don't have any documentation >> >> Anton> Well, do we really need documentation for every trivial gpio >> Anton> controller? The bindings are the same, only different >> compatible >> Anton> entries... >> >> Well, we need some way of knowing that atleast. Having people >> grepping >> through arch/powerpc/boot/dts/ (and knowing that they should look for >> 8349 even though they might use 8313 or 8610) doesn't seem optimal to >> me - I take it that's why we created >> Documentation/powerpc/dts-bindings in the first place? > > Ok. > >> Anton> If we do need the documentation for this, I can readily >> include it. >> >>>> and the Kconfig doesn't depend on >>>> 831x/834x/837x. >> >> Anton> This is purposely. We also need support for 8610, and maybe >> Anton> later we'll find another chip with the same unit. So, to not >> touch >> Anton> the Kconfig for every new chip I just made it PPC32-wide. >> Other >> Anton> option is to depend on FSL_SOC, but the driver really does not >> Anton> depend on any fsl_soc stuff... >> >> Adding another symbol to the Kconfig once it is verified that a new >> SoC is compatible doesn't seem like a big deal - Figuring out all the >> knobs we already have is, without having options for stuff that is >> known to be irrelevant for the SoC. >> >> The other 83xx specific drivers also depend on PPC_83xx. > > Lets wait for Kumar's comments. We've already had a PPC_* mess > for the USB_EHCI_FSL symbol. What I've learned from it, is that > huge PPC_* list isn't perfect either. I've alone glanced over this, but some initial comments are.. lets rename the thing to not be 83xx specific since 8610 uses it and I'm sure we'll have other parts that do similar things. With regards to the binding, lets make it generic like 'fsl,mpc8xxx- gpio", "fsl,CHIP-gpio" and than we can use cpm1/cpm2/pq1/pq2 as prefixes to distinguish and major differences. - k
On Fri, Sep 19, 2008 at 01:02:11PM -0500, Kumar Gala wrote: [...] >>> Anton> This is purposely. We also need support for 8610, and maybe >>> Anton> later we'll find another chip with the same unit. So, to not >>> touch >>> Anton> the Kconfig for every new chip I just made it PPC32-wide. >>> Other >>> Anton> option is to depend on FSL_SOC, but the driver really does not >>> Anton> depend on any fsl_soc stuff... >>> >>> Adding another symbol to the Kconfig once it is verified that a new >>> SoC is compatible doesn't seem like a big deal - Figuring out all the >>> knobs we already have is, without having options for stuff that is >>> known to be irrelevant for the SoC. >>> >>> The other 83xx specific drivers also depend on PPC_83xx. >> >> Lets wait for Kumar's comments. We've already had a PPC_* mess >> for the USB_EHCI_FSL symbol. What I've learned from it, is that >> huge PPC_* list isn't perfect either. > > I've alone glanced over this, but some initial comments are.. lets > rename the thing to not be 83xx specific since 8610 uses it and I'm sure > we'll have other parts that do similar things. Ok, mpc8xxx_gpio.c would be fine? (Note that I'm agree with 8xxx, for the file name). > With regards to the binding, lets make it generic like 'fsl,mpc8xxx- > gpio", "fsl,CHIP-gpio" and than we can use cpm1/cpm2/pq1/pq2 as prefixes > to distinguish and major differences. But for compatible entry, shouldn't we use the last compatiblle entry as a generic one? Then fsl,mpc8349-gpio is perfectly valid. I.e., for MPC8610 chips we will have: "fsl,mpc8610-gpio", "fsl,mpc8349-gpio" The last entry is most generic, and 8610 is registers-compatible with the earlier (8349) chips. I thought that we tend to not do "made up" 8xxx things in the device tree... Am I wrong? Thanks!
On Sep 19, 2008, at 1:12 PM, Anton Vorontsov wrote: > On Fri, Sep 19, 2008 at 01:02:11PM -0500, Kumar Gala wrote: > [...] >>>> Anton> This is purposely. We also need support for 8610, and maybe >>>> Anton> later we'll find another chip with the same unit. So, to not >>>> touch >>>> Anton> the Kconfig for every new chip I just made it PPC32-wide. >>>> Other >>>> Anton> option is to depend on FSL_SOC, but the driver really does >>>> not >>>> Anton> depend on any fsl_soc stuff... >>>> >>>> Adding another symbol to the Kconfig once it is verified that a new >>>> SoC is compatible doesn't seem like a big deal - Figuring out all >>>> the >>>> knobs we already have is, without having options for stuff that is >>>> known to be irrelevant for the SoC. >>>> >>>> The other 83xx specific drivers also depend on PPC_83xx. >>> >>> Lets wait for Kumar's comments. We've already had a PPC_* mess >>> for the USB_EHCI_FSL symbol. What I've learned from it, is that >>> huge PPC_* list isn't perfect either. >> >> I've alone glanced over this, but some initial comments are.. lets >> rename the thing to not be 83xx specific since 8610 uses it and I'm >> sure >> we'll have other parts that do similar things. > > Ok, mpc8xxx_gpio.c would be fine? (Note that I'm agree with 8xxx, for > the file name). > >> With regards to the binding, lets make it generic like 'fsl,mpc8xxx- >> gpio", "fsl,CHIP-gpio" and than we can use cpm1/cpm2/pq1/pq2 as >> prefixes >> to distinguish and major differences. > > But for compatible entry, shouldn't we use the last compatiblle entry > as a generic one? Then fsl,mpc8349-gpio is perfectly valid. I.e., > for MPC8610 chips we will have: > > "fsl,mpc8610-gpio", "fsl,mpc8349-gpio" Yeah the order is correct.. should be: "fsl,CHIP-gpio", "fsl,mpc8xxx-gpio" The last entry is most generic, and 8610 is registers-compatible with > > the earlier (8349) chips. I thought that we tend to not do "made up" > 8xxx things in the device tree... Am I wrong? You are correct we try to avoid this, but than I suggest we do it by family. I think its confusing to show 8610 and 8349 in the same dev tree. What we do in PCI is use the first in family. So lets take 8379 as an example it would look like: "fsl,mpc8379-gpio", "fsl-mpc8349-gpio" and 8610 would look like: "fsl,mpc8610-gpio" This way the compatiable binding in the code just lists something like "fsl,mpc8349-gpio", "fsl,mpc8610-gpio", "fsl,mpc8572-gpio". Does that make sense? - k
>>>>> "Kumar" == Kumar Gala <galak@kernel.crashing.org> writes:
Hi,
Kumar> This way the compatiable binding in the code just lists something like
Kumar> "fsl,mpc8349-gpio", "fsl,mpc8610-gpio", "fsl,mpc8572-gpio".
Kumar> Does that make sense?
Yes, sounds fine - I'll adapt the patch and resend.
On Fri, Sep 19, 2008 at 01:46:49PM -0500, Kumar Gala wrote: > You are correct we try to avoid this, but than I suggest we do it by > family. I think its confusing to show 8610 and 8349 in the same dev > tree. I don't think it's confusing (at least, not moreso than picking an arbitrary chip within the same family), if it really is compatible and there is nothing new in the 86xx GPIO that warrants having a common compatible for them. -Scott
On Fri, Sep 19, 2008 at 05:11:15PM +0200, Peter Korsgaard wrote: > Anton> This is purposely. We also need support for 8610, and maybe > Anton> later we'll find another chip with the same unit. So, to not touch > Anton> the Kconfig for every new chip I just made it PPC32-wide. Other > Anton> option is to depend on FSL_SOC, but the driver really does not > Anton> depend on any fsl_soc stuff... > > Adding another symbol to the Kconfig once it is verified that a new > SoC is compatible doesn't seem like a big deal - Figuring out all the > knobs we already have is, without having options for stuff that is > known to be irrelevant for the SoC. I agree with Anton; it's a lot less of a headache to say "no" to some unfamiliar kconfig option than it is to hunt around and figure out why you're not being given the option. I'd be OK with depending on FSL_SOC. > The other 83xx specific drivers also depend on PPC_83xx. As pointed out above, this isn't 83xx-specific. -Scott
diff --git a/arch/powerpc/sysdev/Kconfig b/arch/powerpc/sysdev/Kconfig index 30d6e4d..4cbfda0 100644 --- a/arch/powerpc/sysdev/Kconfig +++ b/arch/powerpc/sysdev/Kconfig @@ -17,3 +17,13 @@ config OF_SIMPLE_GPIO These are usually BCSRs used to control board's switches, LEDs, chip-selects, Ethernet/USB PHY's power and various other small on-board peripherals. + +config FSL_MPC8349_GPIO + bool "Support for MPC8349-compatible GPIO controllers" + depends on PPC32 + select GENERIC_GPIO + select ARCH_REQUIRE_GPIOLIB + help + Say Y here to support Freescale MPC8349-compatible GPIO controllers. + The controllers can be found in MPC831x, MPC834x, MPC837x and + MPC8610 processors. diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile index 239d7e8..afbc3a4 100644 --- a/arch/powerpc/sysdev/Makefile +++ b/arch/powerpc/sysdev/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_4xx) += ppc4xx_pci.o endif obj-$(CONFIG_OF_SIMPLE_GPIO) += of_simple_gpio.o +obj-$(CONFIG_FSL_MPC8349_GPIO) += fsl_mpc8349_gpio.o # Temporary hack until we have migrated to asm-powerpc ifeq ($(ARCH),powerpc) diff --git a/arch/powerpc/sysdev/fsl_mpc8349_gpio.c b/arch/powerpc/sysdev/fsl_mpc8349_gpio.c new file mode 100644 index 0000000..c8c77f4 --- /dev/null +++ b/arch/powerpc/sysdev/fsl_mpc8349_gpio.c @@ -0,0 +1,161 @@ +/* + * MPC8349-compatible SOC GPIOs + * + * Copyright (c) MontaVista Software, Inc. 2008. + * + * Author: Anton Vorontsov <avorontsov@ru.mvista.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/init.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/compiler.h> +#include <linux/spinlock.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/gpio.h> + +#define FSL_GPIO_PINS 32 + +struct fsl_gpio_regs { + __be32 gpdir; + __be32 gpodr; + __be32 gpdat; + __be32 gpier; + __be32 gpimr; + __be32 gpicr; +}; + +struct fsl_gpio_chip { + struct of_mm_gpio_chip mm_gc; + spinlock_t lock; + + /* shadowed data register to clear/set bits safely */ + u32 gpdat; +}; + +static inline u32 pin2mask(unsigned int pin) +{ + return 1 << (FSL_GPIO_PINS - 1 - pin); +} + +static inline struct fsl_gpio_chip * +to_fsl_gpio_chip(struct of_mm_gpio_chip *mm_gc) +{ + return container_of(mm_gc, struct fsl_gpio_chip, mm_gc); +} + +static void fsl_gpio_save_regs(struct of_mm_gpio_chip *mm_gc) +{ + struct fsl_gpio_chip *fsl_gc = to_fsl_gpio_chip(mm_gc); + struct fsl_gpio_regs __iomem *regs = mm_gc->regs; + + fsl_gc->gpdat = in_be32(®s->gpdat); +} + +static int fsl_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct fsl_gpio_regs __iomem *regs = mm_gc->regs; + + return in_be32(®s->gpdat) & pin2mask(gpio); +} + +static void fsl_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 fsl_gpio_chip *fsl_gc = to_fsl_gpio_chip(mm_gc); + struct fsl_gpio_regs __iomem *regs = mm_gc->regs; + unsigned long flags; + + spin_lock_irqsave(&fsl_gc->lock, flags); + + if (val) + fsl_gc->gpdat |= pin2mask(gpio); + else + fsl_gc->gpdat &= ~pin2mask(gpio); + + out_be32(®s->gpdat, fsl_gc->gpdat); + + spin_unlock_irqrestore(&fsl_gc->lock, flags); +} + +static int fsl_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio) +{ + struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); + struct fsl_gpio_chip *fsl_gc = to_fsl_gpio_chip(mm_gc); + struct fsl_gpio_regs __iomem *regs = mm_gc->regs; + unsigned long flags; + + spin_lock_irqsave(&fsl_gc->lock, flags); + clrbits32(®s->gpdir, pin2mask(gpio)); + spin_unlock_irqrestore(&fsl_gc->lock, flags); + return 0; +} + +static int fsl_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 fsl_gpio_chip *fsl_gc = to_fsl_gpio_chip(mm_gc); + struct fsl_gpio_regs __iomem *regs = mm_gc->regs; + unsigned long flags; + + fsl_gpio_set(gc, gpio, val); + + spin_lock_irqsave(&fsl_gc->lock, flags); + setbits32(®s->gpdir, pin2mask(gpio)); + spin_unlock_irqrestore(&fsl_gc->lock, flags); + return 0; +} + +static int __init fsl_gpio_init(void) +{ + struct device_node *np; + + for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio-bank") { + int ret; + struct fsl_gpio_chip *fsl_gc; + struct of_mm_gpio_chip *mm_gc; + struct of_gpio_chip *of_gc; + struct gpio_chip *gc; + + fsl_gc = kzalloc(sizeof(*fsl_gc), GFP_KERNEL); + if (!fsl_gc) { + ret = -ENOMEM; + goto err; + } + + spin_lock_init(&fsl_gc->lock); + + mm_gc = &fsl_gc->mm_gc; + of_gc = &mm_gc->of_gc; + gc = &of_gc->gc; + + mm_gc->save_regs = fsl_gpio_save_regs; + of_gc->gpio_cells = 2; + gc->ngpio = FSL_GPIO_PINS; + gc->direction_input = fsl_gpio_dir_in; + gc->direction_output = fsl_gpio_dir_out; + gc->get = fsl_gpio_get; + gc->set = fsl_gpio_set; + + ret = of_mm_gpiochip_add(np, mm_gc); + if (ret) + goto err; + continue; +err: + pr_err("%s: registration failed with status %d\n", + np->full_name, ret); + kfree(fsl_gc); + /* try others anyway */ + } + return 0; +} +arch_initcall(fsl_gpio_init);
This patch implements GPIOLIB support for MPC8349-compatible SOC GPIOs. MPC8610 adopted this GPIO unit, so let's place it into sysdev. We'll need these gpios to support IrDA transceiver on MPC8610HPCD. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>