gpio: msm7200a: Add gpiolib support for MSM chips.
in [Kernel]
|
Prev: [PATCH v2] xfs: Make fiemap works with sparse file.
Next: [PATCH 1/2] Remove preempt_enable/disable calls around sched_clock()
From: Ben Dooks on 11 Jun 2010 22:20 On Fri, Jun 11, 2010 at 12:58:51PM -0700, Gregory Bean wrote: > Add support for uniprocessor MSM chips whose TLMM/GPIO design > is the same as the MSM7200A. > This includes, but is not necessarily limited to, the: > MSM7200A, MSM7x25, MSM7x27, MSM7x30, QSD8x50, QSD8x50A > > Change-Id: I748d0e09f6b762f1711cde3c27a9a6e8de6d9454 > Signed-off-by: Gregory Bean <gbean(a)codeaurora.org> > --- > MAINTAINERS | 2 + > drivers/gpio/Kconfig | 8 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/msm7200a-gpio.c | 194 +++++++++++++++++++++++++++++++++++++++++ > include/linux/msm7200a-gpio.h | 44 +++++++++ > 5 files changed, 249 insertions(+), 0 deletions(-) > create mode 100644 drivers/gpio/msm7200a-gpio.c > create mode 100644 include/linux/msm7200a-gpio.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 6d119c9..bdfd31d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -819,6 +819,8 @@ F: drivers/mmc/host/msm_sdcc.c > F: drivers/mmc/host/msm_sdcc.h > F: drivers/serial/msm_serial.h > F: drivers/serial/msm_serial.c > +F: drivers/gpio/msm7200a-gpio.c > +F: include/linux/msm7200a-gpio.h > T: git git://codeaurora.org/quic/kernel/dwalker/linux-msm.git > S: Maintained > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 724038d..557738a 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -76,6 +76,14 @@ config GPIO_IT8761E > help > Say yes here to support GPIO functionality of IT8761E super I/O chip. > > +config GPIO_MSM7200A > + tristate "Qualcomm MSM7200A SoC GPIO support" > + depends on GPIOLIB > + help > + Say yes here to support GPIO functionality on Qualcomm's > + MSM chipsets which descend from the MSM7200a: > + MSM7x01(a), MSM7x25, MSM7x27, MSM7x30, QSD8x50(a). > + > config GPIO_PL061 > bool "PrimeCell PL061 GPIO support" > depends on ARM_AMBA > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 51c3cdd..2389c29 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_GPIO_MAX7301) += max7301.o > obj-$(CONFIG_GPIO_MAX732X) += max732x.o > obj-$(CONFIG_GPIO_MC33880) += mc33880.o > obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o > +obj-$(CONFIG_GPIO_MSM7200A) += msm7200a-gpio.o > obj-$(CONFIG_GPIO_PCA953X) += pca953x.o > obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o > obj-$(CONFIG_GPIO_PL061) += pl061.o Why not put this under arch/arm? > diff --git a/drivers/gpio/msm7200a-gpio.c b/drivers/gpio/msm7200a-gpio.c > new file mode 100644 > index 0000000..b31c25e > --- /dev/null > +++ b/drivers/gpio/msm7200a-gpio.c > @@ -0,0 +1,194 @@ > +/* > + * Driver for Qualcomm MSM7200a and related SoC GPIO. > + * Supported chipset families include: > + * MSM7x01(a), MSM7x25, MSM7x27, MSM7x30, QSD8x50(a) > + * > + * Copyright (C) 2007 Google, Inc. > + * Copyright (c) 2010, Code Aurora Forum. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > + * 02110-1301, USA. > + */ > +#include <linux/kernel.h> > +#include <linux/gpio.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/msm7200a-gpio.h> > + > +struct msm_gpio_dev { > + struct gpio_chip gpio_chip; > + spinlock_t lock; > + struct msm7200a_gpio_regs regs; > +}; > + > +#define TO_MSM_GPIO_DEV(c) container_of(c, struct msm_gpio_dev, gpio_chip) I'd say an inline function would be better, since it gives a typecheck on c. also, c is a bit of a vague name for a macro argument. > + > +static inline unsigned bit(unsigned offset) > +{ > + BUG_ON(offset >= sizeof(unsigned) * 8); > + return 1U << offset; > +} do you really need BUG_ON(), gpiolib tends to check the parameters that are given to it. personally i think this is silly. > +/* > + * This function assumes that msm_gpio_dev::lock is held. > + */ > +static inline void set_gpio_bit(unsigned n, void __iomem *reg) > +{ > + writel(readl(reg) | bit(n), reg); > +} > + > +/* > + * This function assumes that msm_gpio_dev::lock is held. > + */ > +static inline void clr_gpio_bit(unsigned n, void __iomem *reg) > +{ > + writel(readl(reg) & ~bit(n), reg); > +} > + > +/* > + * This function assumes that msm_gpio_dev::lock is held. > + */ > +static inline void > +msm_gpio_write(struct msm_gpio_dev *dev, unsigned n, unsigned on) > +{ > + if (on) > + set_gpio_bit(n, dev->regs.out); > + else > + clr_gpio_bit(n, dev->regs.out); > +} wouldn't it be easier to inline a set_to function and just role the set and clr bit functions into it, since they pretty much do the same thing. even better, on arm the code won't require a branch. > + > +static int gpio_chip_direction_input(struct gpio_chip *chip, unsigned offset) > +{ > + struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip); > + unsigned long irq_flags; > + > + spin_lock_irqsave(&msm_gpio->lock, irq_flags); > + clr_gpio_bit(offset, msm_gpio->regs.oe); > + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags); > + > + return 0; > +} > + > +static int > +gpio_chip_direction_output(struct gpio_chip *chip, unsigned offset, int value) > +{ > + struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip); > + unsigned long irq_flags; > + > + spin_lock_irqsave(&msm_gpio->lock, irq_flags); > + > + msm_gpio_write(msm_gpio, offset, value); > + set_gpio_bit(offset, msm_gpio->regs.oe); > + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags); > + > + return 0; > +} > + > +static int gpio_chip_get(struct gpio_chip *chip, unsigned offset) > +{ > + struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip); > + unsigned long irq_flags; > + int ret; > + > + spin_lock_irqsave(&msm_gpio->lock, irq_flags); > + ret = readl(msm_gpio->regs.in) & bit(offset) ? 1 : 0; > + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags); do you really need a lock to read a value? > + return ret; > +} > + > +static void gpio_chip_set(struct gpio_chip *chip, unsigned offset, int value) > +{ > + struct msm_gpio_dev *msm_gpio = TO_MSM_GPIO_DEV(chip); > + unsigned long irq_flags; > + > + spin_lock_irqsave(&msm_gpio->lock, irq_flags); > + msm_gpio_write(msm_gpio, offset, value); > + spin_unlock_irqrestore(&msm_gpio->lock, irq_flags); > +} > + > +static int msm_gpio_probe(struct platform_device *dev) > +{ > + struct msm_gpio_dev *msm_gpio; > + struct msm7200a_gpio_platform_data *pdata = > + (struct msm7200a_gpio_platform_data *)dev->dev.platform_data; no need to cast, void* goes to any non void * easily. > + int ret; > + > + if (!pdata) > + return -EINVAL; > + > + msm_gpio = kzalloc(sizeof(struct msm_gpio_dev), GFP_KERNEL); > + if (!msm_gpio) > + return -ENOMEM; > + > + spin_lock_init(&msm_gpio->lock); > + platform_set_drvdata(dev, msm_gpio); > + memcpy(&msm_gpio->regs, > + &pdata->regs, > + sizeof(struct msm7200a_gpio_regs)); you could have easily done msm_gpio->regs = *pdata->regs and got some free type-checking into the bargin. > + msm_gpio->gpio_chip.label = dev->name; > + msm_gpio->gpio_chip.base = pdata->gpio_base; > + msm_gpio->gpio_chip.ngpio = pdata->ngpio; > + msm_gpio->gpio_chip.direction_input = gpio_chip_direction_input; > + msm_gpio->gpio_chip.direction_output = gpio_chip_direction_output; > + msm_gpio->gpio_chip.get = gpio_chip_get; > + msm_gpio->gpio_chip.set = gpio_chip_set; > + > + ret = gpiochip_add(&msm_gpio->gpio_chip); > + if (ret < 0) > + goto err; > + > + return ret; > +err: > + kfree(msm_gpio); > + return ret; > +} > + > +static int msm_gpio_remove(struct platform_device *dev) > +{ > + struct msm_gpio_dev *msm_gpio = platform_get_drvdata(dev); > + int ret = gpiochip_remove(&msm_gpio->gpio_chip); > + > + if (ret == 0) > + kfree(msm_gpio); hmm, not sure if you really need to check the result here before kfrree() the memory. ~> + return ret; > +} > + > +static struct platform_driver msm_gpio_driver = { > + .probe = msm_gpio_probe, > + .remove = msm_gpio_remove, > + .driver = { > + .name = "msm7200a-gpio", > + .owner = THIS_MODULE, > + }, > +}; > + > +static int __init msm_gpio_init(void) > +{ > + return platform_driver_register(&msm_gpio_driver); > +} > + > +static void __exit msm_gpio_exit(void) > +{ > + platform_driver_unregister(&msm_gpio_driver); > +} > + > +postcore_initcall(msm_gpio_init); > +module_exit(msm_gpio_exit); > + > +MODULE_DESCRIPTION("Driver for Qualcomm MSM 7200a-family SoC GPIOs"); > +MODULE_LICENSE("GPLv2"); you left out a MODULE_ALIAS() for your platform device. you could also do with a MODULE_AUTHOUR() line as well > diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h > new file mode 100644 > index 0000000..3f1ef38 > --- /dev/null > +++ b/include/linux/msm7200a-gpio.h > @@ -0,0 +1,44 @@ > +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions are > + * met: > + * * Redistributions of source code must retain the above copyright > diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h > new file mode 100644 > index 0000000..3f1ef38 > --- /dev/null > +++ b/include/linux/msm7200a-gpio.h > @@ -0,0 +1,44 @@ > +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without ;5B> + * modification, are permitted provided that the following conditions are > + * met: > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials provided > + * with the distribution. > + * * Neither the name of Code Aurora Forum, Inc. nor the names of its > + * contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED "AS IS" AND ANY EXPRESS OR IMPLIED > + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS > + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, > + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE > + * OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN > + * IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + * > + */ Is this really GPL compatible? > +#ifndef __LINUX_MSM7200A_GPIO_H > +#define __LINUX_MSM7200A_GPIO_H > + > +struct msm7200a_gpio_regs { > + void __iomem *in; > + void __iomem *out; > + void __iomem *oe; > +}; Are the offsets of in/out/oe so different? would be nice to document this structure and the likely offsets you would see. > + > +struct msm7200a_gpio_platform_data { > + unsigned gpio_base; > + unsigned ngpio; > + struct msm7200a_gpio_regs regs; > +}; again, some documentaiton of this would be nice. -- Ben (ben(a)fluff.org, http://www.fluff.org/) 'a smiley only costs 4 bytes' -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bryan Huntsman on 11 Jun 2010 22:40 Ben Dooks wrote: > On Fri, Jun 11, 2010 at 12:58:51PM -0700, Gregory Bean wrote: ....snip... >> +MODULE_DESCRIPTION("Driver for Qualcomm MSM 7200a-family SoC GPIOs"); >> +MODULE_LICENSE("GPLv2"); > > you left out a MODULE_ALIAS() for your platform device. > you could also do with a MODULE_AUTHOUR() line as well Also, the string is "GPL v2". See include/linux/license.h. >> diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h >> new file mode 100644 >> index 0000000..3f1ef38 >> --- /dev/null >> +++ b/include/linux/msm7200a-gpio.h >> @@ -0,0 +1,44 @@ >> +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved. >> + * >> + * Redistribution and use in source and binary forms, with or without >> + * modification, are permitted provided that the following conditions are >> + * met: >> + * * Redistributions of source code must retain the above copyright >> diff --git a/include/linux/msm7200a-gpio.h b/include/linux/msm7200a-gpio.h >> new file mode 100644 >> index 0000000..3f1ef38 >> --- /dev/null >> +++ b/include/linux/msm7200a-gpio.h >> @@ -0,0 +1,44 @@ >> +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved. >> + * >> + * Redistribution and use in source and binary forms, with or without > ;5B> + * modification, are permitted provided that the following conditions are >> + * met: >> + * * Redistributions of source code must retain the above copyright >> + * notice, this list of conditions and the following disclaimer. >> + * * Redistributions in binary form must reproduce the above >> + * copyright notice, this list of conditions and the following >> + * disclaimer in the documentation and/or other materials provided >> + * with the distribution. >> + * * Neither the name of Code Aurora Forum, Inc. nor the names of its >> + * contributors may be used to endorse or promote products derived >> + * from this software without specific prior written permission. >> + * >> + * THIS SOFTWARE IS PROVIDED "AS IS" AND ANY EXPRESS OR IMPLIED >> + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF >> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT >> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS >> + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR >> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF >> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR >> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, >> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE >> + * OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN >> + * IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> + * >> + */ > > Is this really GPL compatible? It's BSD-style, so yes. -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Gregory Bean on 12 Jun 2010 01:40 > Why not put this under arch/arm? Is there an appropriate place for loadable device drivers under arch/arm? I don't know of one. >> +static inline void set_gpio_bit(unsigned n, void __iomem *reg) >> +{ >> + writel(readl(reg) | bit(n), reg); >> +} >> + >> +/* >> + * This function assumes that msm_gpio_dev::lock is held. >> + */ >> +static inline void clr_gpio_bit(unsigned n, void __iomem *reg) >> +{ >> + writel(readl(reg)& ~bit(n), reg); >> +} >> + >> +/* >> + * This function assumes that msm_gpio_dev::lock is held. >> + */ >> +static inline void >> +msm_gpio_write(struct msm_gpio_dev *dev, unsigned n, unsigned on) >> +{ >> + if (on) >> + set_gpio_bit(n, dev->regs.out); >> + else >> + clr_gpio_bit(n, dev->regs.out); >> +} > > wouldn't it be easier to inline a set_to function and just role the > set and clr bit functions into it, since they pretty much do the > same thing. even better, on arm the code won't require a branch. I'm not sure I understand you. Can you clarify? set_ and clr_gpio_bit are used in more places than just here, so they can't just be rolled into msm_gpio_write and disappear. >> +static int msm_gpio_remove(struct platform_device *dev) >> +{ >> + struct msm_gpio_dev *msm_gpio = platform_get_drvdata(dev); >> + int ret = gpiochip_remove(&msm_gpio->gpio_chip); >> + >> + if (ret == 0) >> + kfree(msm_gpio); > > hmm, not sure if you really need to check the result here before > kfrree() the memory. I feel that this is important. If any clients are still holding gpio lines, gpiochip_remove will fail. In those circumstances, is it not important that the device not be freed (which would leave clients with stale references) and that the remove call return a proper failure code? -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Ben Dooks on 13 Jun 2010 01:40 On Fri, Jun 11, 2010 at 10:37:18PM -0700, Gregory Bean wrote: >> Why not put this under arch/arm? > > Is there an appropriate place for loadable device drivers under > arch/arm? I don't know of one. > >>> +static inline void set_gpio_bit(unsigned n, void __iomem *reg) >>> +{ >>> + writel(readl(reg) | bit(n), reg); >>> +} >>> + >>> +/* >>> + * This function assumes that msm_gpio_dev::lock is held. >>> + */ >>> +static inline void clr_gpio_bit(unsigned n, void __iomem *reg) >>> +{ >>> + writel(readl(reg)& ~bit(n), reg); >>> +} >>> + >>> +/* >>> + * This function assumes that msm_gpio_dev::lock is held. >>> + */ >>> +static inline void >>> +msm_gpio_write(struct msm_gpio_dev *dev, unsigned n, unsigned on) >>> +{ >>> + if (on) >>> + set_gpio_bit(n, dev->regs.out); >>> + else >>> + clr_gpio_bit(n, dev->regs.out); >>> +} >> >> wouldn't it be easier to inline a set_to function and just role the >> set and clr bit functions into it, since they pretty much do the >> same thing. even better, on arm the code won't require a branch. > > I'm not sure I understand you. Can you clarify? set_ and clr_gpio_bit > are used in more places than just here, so they can't just be rolled > into msm_gpio_write and disappear. Hmm, thought the compiler was clever enough to inline and sort that out, I'll have a check later into whether this is true or not for a few versions of gcc. >>> +static int msm_gpio_remove(struct platform_device *dev) >>> +{ >>> + struct msm_gpio_dev *msm_gpio = platform_get_drvdata(dev); >>> + int ret = gpiochip_remove(&msm_gpio->gpio_chip); >>> + >>> + if (ret == 0) >>> + kfree(msm_gpio); >> >> hmm, not sure if you really need to check the result here before >> kfrree() the memory. > > I feel that this is important. If any clients are still holding gpio > lines, gpiochip_remove will fail. In those circumstances, is it not > important that the device not be freed (which would leave clients with > stale references) and that the remove call return a proper failure code? Right, confused holding onto the module to holding onto the device too. Maybe devices should be refcounted too so that holding an open gpio on via the driver would force the driver core to refuse to remove the device. -- Ben Q: What's a light-year? A: One-third less calories than a regular year. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mike Frysinger on 15 Jun 2010 17:20
On Tue, Jun 15, 2010 at 17:11, Gregory Bean wrote: > +config GPIO_MSM7200A > + tristate "Qualcomm MSM7200A SoC GPIO support" > + depends on GPIOLIB > + help > + Say yes here to support GPIO functionality on Qualcomm's > + MSM chipsets which descend from the MSM7200a: > + MSM7x01(a), MSM7x25, MSM7x27, MSM7x30, QSD8x50(a). you should mention the module name in the help text > +static int msm_gpio_probe(struct platform_device *dev) should have __devinit markings > +static int msm_gpio_remove(struct platform_device *dev) __devexit > + .remove = msm_gpio_remove, __devexit_p() > +postcore_initcall(msm_gpio_init); does this really need to be postcore rather than module ? -mike -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |