| Message ID | 1414454528-24240-3-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Add gpiolib driver for gpio pins placed on the LoCoMo GA. > > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> (...) > +static int locomo_gpio_get(struct gpio_chip *chip, > + unsigned offset) > +{ > + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); > + > + return readw(lg->regs + LOCOMO_GPL) & (1 << offset); Do this: #include <linux/bitops.h> return !!(readw(lg->regs + LOCOMO_GPL) & BIT(offset)); So you clamp the returned value to a bool. > +static void __locomo_gpio_set(struct gpio_chip *chip, > + unsigned offset, int value) > +{ > + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); > + u16 r; > + > + r = readw(lg->regs + LOCOMO_GPO); > + if (value) > + r |= 1 << offset; r |= BIT(offset); > + else > + r &= ~(1 << offset); r &= BIT(offset); (etc, everywhere this pattern occurs). > +static void locomo_gpio_set(struct gpio_chip *chip, > + unsigned offset, int value) > +{ > + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); > + unsigned long flags; > + > + spin_lock_irqsave(&lg->lock, flags); > + > + __locomo_gpio_set(chip, offset, value); > + > + spin_unlock_irqrestore(&lg->lock, flags); If you actually always have to be getting and releasing a spin lock around the register writes, contemplate using regmap-mmio because that is part of what it does. But is this locking really necessary? > +static int locomo_gpio_remove(struct platform_device *pdev) > +{ > + struct locomo_gpio *lg = platform_get_drvdata(pdev); > + int ret; > + > + ret = gpiochip_remove(&lg->gpio); > + if (ret) { > + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret); > + return ret; > + } The return value from gpiochip_remove() has been removed in v3.18-rc1 so this will not compile. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-10-31 10:48 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov > <dbaryshkov@gmail.com> wrote: > >> Add gpiolib driver for gpio pins placed on the LoCoMo GA. >> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> > [skipped] > (etc, everywhere this pattern occurs). >> +static void locomo_gpio_set(struct gpio_chip *chip, >> + unsigned offset, int value) >> +{ >> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&lg->lock, flags); >> + >> + __locomo_gpio_set(chip, offset, value); >> + >> + spin_unlock_irqrestore(&lg->lock, flags); > > If you actually always have to be getting and releasing a spin lock around > the register writes, contemplate using regmap-mmio because that > is part of what it does. > > But is this locking really necessary? I have a custom of doing such locking and never having to think about somebody breaking into RMW cycles. Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock around each register read/write/RMW? >> +static int locomo_gpio_remove(struct platform_device *pdev) >> +{ >> + struct locomo_gpio *lg = platform_get_drvdata(pdev); >> + int ret; >> + >> + ret = gpiochip_remove(&lg->gpio); >> + if (ret) { >> + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret); >> + return ret; >> + } > > The return value from gpiochip_remove() has been removed in v3.18-rc1 > so this will not compile. Yes, the fix will be in the next iteration. This patchset was based on 3.17
On Fri, Oct 31, 2014 at 10:39 AM, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > 2014-10-31 10:48 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: >> On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov >> <dbaryshkov@gmail.com> wrote: >> >>> Add gpiolib driver for gpio pins placed on the LoCoMo GA. >>> >>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >> > > [skipped] > >> (etc, everywhere this pattern occurs). >>> +static void locomo_gpio_set(struct gpio_chip *chip, >>> + unsigned offset, int value) >>> +{ >>> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&lg->lock, flags); >>> + >>> + __locomo_gpio_set(chip, offset, value); >>> + >>> + spin_unlock_irqrestore(&lg->lock, flags); >> >> If you actually always have to be getting and releasing a spin lock around >> the register writes, contemplate using regmap-mmio because that >> is part of what it does. >> >> But is this locking really necessary? > > I have a custom of doing such locking and never having to think about > somebody breaking into RMW cycles. > > Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock > around each register read/write/RMW? Yes that's the point: if you use regmap mmio you get the RMW-locking for free, with the regmap implementation. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-11-03 16:43 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > On Fri, Oct 31, 2014 at 10:39 AM, Dmitry Eremin-Solenikov > <dbaryshkov@gmail.com> wrote: >> 2014-10-31 10:48 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: >>> On Tue, Oct 28, 2014 at 1:01 AM, Dmitry Eremin-Solenikov >>> <dbaryshkov@gmail.com> wrote: >>> >>>> Add gpiolib driver for gpio pins placed on the LoCoMo GA. >>>> >>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> >>> >> >> [skipped] >> >>> (etc, everywhere this pattern occurs). >>>> +static void locomo_gpio_set(struct gpio_chip *chip, >>>> + unsigned offset, int value) >>>> +{ >>>> + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); >>>> + unsigned long flags; >>>> + >>>> + spin_lock_irqsave(&lg->lock, flags); >>>> + >>>> + __locomo_gpio_set(chip, offset, value); >>>> + >>>> + spin_unlock_irqrestore(&lg->lock, flags); >>> >>> If you actually always have to be getting and releasing a spin lock around >>> the register writes, contemplate using regmap-mmio because that >>> is part of what it does. >>> >>> But is this locking really necessary? >> >> I have a custom of doing such locking and never having to think about >> somebody breaking into RMW cycles. >> >> Also isn't regmap an overkill here? Wouldn't regmap also do a lock/unlock >> around each register read/write/RMW? > > Yes that's the point: if you use regmap mmio you get the RMW-locking > for free, with the regmap implementation. Just to be more concrete. Currently locomo_gpio_ack_irq() function uses one lock and one unlock for doing 3 consecutive RMW I I convert locomo to regmap, will that be 3 lock/unlock calls or still one? (Or maybe I'm trying to be over-protective here and adding more lock/unlock cycles won't matter that much?) Next question: if I have to export regmap to several subdrivers, is it better to have one big regmap or to have one-map-per-driver approach?
On Thu, Nov 06, 2014 at 01:33:24AM +0400, Dmitry Eremin-Solenikov wrote: > 2014-11-03 16:43 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > > Yes that's the point: if you use regmap mmio you get the RMW-locking > > for free, with the regmap implementation. > Just to be more concrete. Currently locomo_gpio_ack_irq() function uses > one lock and one unlock for doing 3 consecutive RMW I I convert locomo > to regmap, will that be 3 lock/unlock calls or still one? (Or maybe I'm > trying to be over-protective here and adding more lock/unlock cycles > won't matter that much?) You'll get three lock/unlocks, we could add an interface for bulk updates under one lock if it's important though. > Next question: if I have to export regmap to several subdrivers, is it better > to have one big regmap or to have one-map-per-driver approach? One regmap for the physical register map which is shared between all the users.
2014-11-06 9:03 GMT+03:00 Mark Brown <broonie@kernel.org>: > On Thu, Nov 06, 2014 at 01:33:24AM +0400, Dmitry Eremin-Solenikov wrote: >> 2014-11-03 16:43 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > >> > Yes that's the point: if you use regmap mmio you get the RMW-locking >> > for free, with the regmap implementation. > >> Just to be more concrete. Currently locomo_gpio_ack_irq() function uses >> one lock and one unlock for doing 3 consecutive RMW I I convert locomo >> to regmap, will that be 3 lock/unlock calls or still one? (Or maybe I'm >> trying to be over-protective here and adding more lock/unlock cycles >> won't matter that much?) > > You'll get three lock/unlocks, we could add an interface for bulk > updates under one lock if it's important though. > >> Next question: if I have to export regmap to several subdrivers, is it better >> to have one big regmap or to have one-map-per-driver approach? > > One regmap for the physical register map which is shared between all the > users. Mark, Linus, Just to better understand your suggestions: do you want me to convert to regmap only gpio driver or do you suggest to convert all LoCoMo drivers? That is doable, of course, but the amount of changes is overwhelming. Also I'm concerned about the performance impact from going through regmap layers.
On Tue, Nov 11, 2014 at 05:16:38PM +0400, Dmitry Eremin-Solenikov wrote: > Just to better understand your suggestions: do you want me to convert > to regmap only gpio driver or do you suggest to convert all LoCoMo drivers? > That is doable, of course, but the amount of changes is overwhelming. > Also I'm concerned about the performance impact from going through > regmap layers. I don't care.
On Tue, Nov 11, 2014 at 2:16 PM, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > Just to better understand your suggestions: do you want me to convert > to regmap only gpio driver or do you suggest to convert all LoCoMo drivers? Um... I was just thinking about this one usecase. It's no big deal, the other review comments are more important. > That is doable, of course, but the amount of changes is overwhelming. > Also I'm concerned about the performance impact from going through > regmap layers. Is it on a critical path? The current locking isn't any less invasive AFAICT. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-11-14 13:11 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Nov 11, 2014 at 2:16 PM, Dmitry Eremin-Solenikov > <dbaryshkov@gmail.com> wrote: > >> Just to better understand your suggestions: do you want me to convert >> to regmap only gpio driver or do you suggest to convert all LoCoMo drivers? > > Um... I was just thinking about this one usecase. I ended up converting all drivers. It allowed me to clean up several points in the driver. > > It's no big deal, the other review comments are more important. Fixed most of the comments. Last remaining issue is factoring out m62332 interface. >> That is doable, of course, but the amount of changes is overwhelming. >> Also I'm concerned about the performance impact from going through >> regmap layers. > > Is it on a critical path? The current locking isn't any less invasive > AFAICT. > > Yours, > Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0959ca9..11c03d4 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -457,6 +457,13 @@ config GPIO_TB10X select GENERIC_IRQ_CHIP select OF_GPIO +config GPIO_LOCOMO + bool "Sharp LoCoMo GPIO support" + depends on MFD_LOCOMO + help + Select this to support GPIO pins on Sharp LoCoMo Grid Array found + in Sharp Zaurus collie and poodle models. + comment "I2C GPIO expanders:" config GPIO_ARIZONA diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index e5d346c..ed73f63 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_GPIO_JANZ_TTL) += gpio-janz-ttl.o obj-$(CONFIG_GPIO_KEMPLD) += gpio-kempld.o obj-$(CONFIG_ARCH_KS8695) += gpio-ks8695.o obj-$(CONFIG_GPIO_INTEL_MID) += gpio-intel-mid.o +obj-$(CONFIG_GPIO_LOCOMO) += gpio-locomo.o obj-$(CONFIG_GPIO_LP3943) += gpio-lp3943.o obj-$(CONFIG_ARCH_LPC32XX) += gpio-lpc32xx.o obj-$(CONFIG_GPIO_LYNXPOINT) += gpio-lynxpoint.o diff --git a/drivers/gpio/gpio-locomo.c b/drivers/gpio/gpio-locomo.c new file mode 100644 index 0000000..3b54b07 --- /dev/null +++ b/drivers/gpio/gpio-locomo.c @@ -0,0 +1,228 @@ +/* + * Sharp LoCoMo support for GPIO + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This file contains all generic LoCoMo support. + * + * All initialization functions provided here are intended to be called + * from machine specific code with proper arguments when required. + */ +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/gpio.h> +#include <linux/io.h> +#include <linux/spinlock.h> +#include <linux/mfd/locomo.h> + +struct locomo_gpio { + void __iomem *regs; + + spinlock_t lock; + struct gpio_chip gpio; + + u16 rising_edge; + u16 falling_edge; + + u16 save_gpo; + u16 save_gpe; +}; + +static int locomo_gpio_get(struct gpio_chip *chip, + unsigned offset) +{ + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); + + return readw(lg->regs + LOCOMO_GPL) & (1 << offset); +} + +static void __locomo_gpio_set(struct gpio_chip *chip, + unsigned offset, int value) +{ + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); + u16 r; + + r = readw(lg->regs + LOCOMO_GPO); + if (value) + r |= 1 << offset; + else + r &= ~(1 << offset); + writew(r, lg->regs + LOCOMO_GPO); +} + +static void locomo_gpio_set(struct gpio_chip *chip, + unsigned offset, int value) +{ + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); + unsigned long flags; + + spin_lock_irqsave(&lg->lock, flags); + + __locomo_gpio_set(chip, offset, value); + + spin_unlock_irqrestore(&lg->lock, flags); +} + +static int locomo_gpio_direction_input(struct gpio_chip *chip, + unsigned offset) +{ + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); + unsigned long flags; + u16 r; + + spin_lock_irqsave(&lg->lock, flags); + + r = readw(lg->regs + LOCOMO_GPD); + r |= (1 << offset); + writew(r, lg->regs + LOCOMO_GPD); + + r = readw(lg->regs + LOCOMO_GPE); + r |= (1 << offset); + writew(r, lg->regs + LOCOMO_GPE); + + spin_unlock_irqrestore(&lg->lock, flags); + + return 0; +} + +static int locomo_gpio_direction_output(struct gpio_chip *chip, + unsigned offset, int value) +{ + struct locomo_gpio *lg = container_of(chip, struct locomo_gpio, gpio); + unsigned long flags; + u16 r; + + spin_lock_irqsave(&lg->lock, flags); + + __locomo_gpio_set(chip, offset, value); + + r = readw(lg->regs + LOCOMO_GPD); + r &= ~(1 << offset); + writew(r, lg->regs + LOCOMO_GPD); + + r = readw(lg->regs + LOCOMO_GPE); + r &= ~(1 << offset); + writew(r, lg->regs + LOCOMO_GPE); + + spin_unlock_irqrestore(&lg->lock, flags); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int locomo_gpio_suspend(struct device *dev) +{ + struct locomo_gpio *lg = dev_get_drvdata(dev); + unsigned long flags; + + spin_lock_irqsave(&lg->lock, flags); + + lg->save_gpo = readw(lg->regs + LOCOMO_GPO); + writew(0x00, lg->regs + LOCOMO_GPO); + + lg->save_gpo = readw(lg->regs + LOCOMO_GPE); + writew(0x00, lg->regs + LOCOMO_GPE); + + spin_unlock_irqrestore(&lg->lock, flags); + return 0; +} + +static int locomo_gpio_resume(struct device *dev) +{ + struct locomo_gpio *lg = dev_get_drvdata(dev); + unsigned long flags; + + spin_lock_irqsave(&lg->lock, flags); + + writew(lg->save_gpo, lg->regs + LOCOMO_GPO); + + writew(lg->save_gpe, lg->regs + LOCOMO_GPE); + + spin_unlock_irqrestore(&lg->lock, flags); + return 0; +} +static SIMPLE_DEV_PM_OPS(locomo_gpio_pm, + locomo_gpio_suspend, locomo_gpio_resume); +#define LOCOMO_GPIO_PM (&locomo_gpio_pm) +#else +#define LOCOMO_GPIO_PM NULL +#endif + +static int locomo_gpio_probe(struct platform_device *pdev) +{ + struct resource *res; + struct locomo_gpio *lg; + int ret; + struct locomo_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + + lg = devm_kzalloc(&pdev->dev, sizeof(struct locomo_gpio), + GFP_KERNEL); + if (!lg) + return -ENOMEM; + + lg->regs = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(lg->regs)) + return PTR_ERR(lg->regs); + + spin_lock_init(&lg->lock); + + platform_set_drvdata(pdev, lg); + + writew(0, lg->regs + LOCOMO_GPO); + writew(0, lg->regs + LOCOMO_GPE); + writew(0, lg->regs + LOCOMO_GPD); + writew(0, lg->regs + LOCOMO_GIE); + + lg->gpio.base = pdata ? pdata->gpio_base : -1; + lg->gpio.label = "locomo-gpio"; + lg->gpio.ngpio = 16; + lg->gpio.set = locomo_gpio_set; + lg->gpio.get = locomo_gpio_get; + lg->gpio.direction_input = locomo_gpio_direction_input; + lg->gpio.direction_output = locomo_gpio_direction_output; + + ret = gpiochip_add(&lg->gpio); + if (ret) + return ret; + + return 0; +} + +static int locomo_gpio_remove(struct platform_device *pdev) +{ + struct locomo_gpio *lg = platform_get_drvdata(pdev); + int ret; + + ret = gpiochip_remove(&lg->gpio); + if (ret) { + dev_err(&pdev->dev, "Can't remove gpio chip: %d\n", ret); + return ret; + } + + return 0; +} + +static struct platform_driver locomo_gpio_driver = { + .probe = locomo_gpio_probe, + .remove = locomo_gpio_remove, + .driver = { + .name = "locomo-gpio", + .owner = THIS_MODULE, + .pm = LOCOMO_GPIO_PM, + }, +}; +module_platform_driver(locomo_gpio_driver); + +MODULE_DESCRIPTION("Sharp LoCoMo GPIO driver"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>"); +MODULE_ALIAS("platform:locomo-gpio");
Add gpiolib driver for gpio pins placed on the LoCoMo GA. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- drivers/gpio/Kconfig | 7 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-locomo.c | 228 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 drivers/gpio/gpio-locomo.c