| Message ID | 20170901185736.28051-1-thierry.reding@gmail.com |
|---|---|
| Headers | show |
| Series | gpio: Tight IRQ chip integration and banked infrastructure | expand |
On Fri, Sep 1, 2017 at 8:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > here's the latest series of patches that implement the tighter IRQ chip > integration as well as the banked GPIO infrastructure that we had > discussed a couple of weeks/months back. Yes it has become really tasty now, don't you think :) I really like the series. Banks are handled in the core, exactly as I wanted. I will likely go in and change some things I don't like, like switching num_pins in the bank to num_lines. I have preferred that terminology to avoid confusion with pin control. So GPIO chips have lines, not pins. But it's so minor that I can fix it up if you don't want to. We also need to go in and patch Documentation/gpio/driver.txt to represent the current best practice. But that can be later, separate patch. > The first couple of patches are mostly preparatory work in order to > consolidate all IRQ chip related fields in a new structure and create > the base functionality for adding IRQ chips. > > After that, I've added the Tegra186 GPIO support patch that makes use of > the new tight integration. > > To round things off the new banked GPIO infrastructure is added (along > with some more preparatory work), followed by the conversion of the two > Tegra GPIO drivers to the new infrastructure. I have put all on a branch for pushing to the test builders to begin with. Then I plan to make one branch with all infrastructure patches (patches 1-10, 12-14) and pull that into devel, then apply patch 11 and 15-16 directly on devel. That way other subsystems (pinctrl ...) can pull in the infrastructure for people adding new gpiochips this cycle. > Any thoughts on this? I'd like to target 4.15 with this, Me, too. > unless you'd be > willing to take this into 4.14, which I doubt at this point. The absence > of a GPIO driver has been hampering Tegra186 support upstream for a > while now, so it'd be good to make progress on this. Sorry about that. Let's move ahead with this now, it is neat and clean. What I want (as maintainer) is a bit of fingerpointing at the drivers that need to be converted to use the new banking infrastructure so they don't stay with their old crappy design pattern. OMAP is a clear candidate right? (Added Tony to CC...) Who else? 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
On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote: > On Fri, Sep 1, 2017 at 8:57 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > > > here's the latest series of patches that implement the tighter IRQ chip > > integration as well as the banked GPIO infrastructure that we had > > discussed a couple of weeks/months back. > > Yes it has become really tasty now, don't you think :) > > I really like the series. > > Banks are handled in the core, exactly as I wanted. > > I will likely go in and change some things I don't like, like switching > num_pins in the bank to num_lines. I have preferred that terminology > to avoid confusion with pin control. So GPIO chips have lines, not pins. > But it's so minor that I can fix it up if you don't want to. I rebased this on today's linux-next and noticed that there was a small conflict. I can rebase and work in the changes that you requested. I'm travelling this week and next, so it may take until after -rc2 that I can send out a new version that's properly build-tested. > We also need to go in and patch Documentation/gpio/driver.txt > to represent the current best practice. But that can be later, > separate patch. > > > The first couple of patches are mostly preparatory work in order to > > consolidate all IRQ chip related fields in a new structure and create > > the base functionality for adding IRQ chips. > > > > After that, I've added the Tegra186 GPIO support patch that makes use of > > the new tight integration. > > > > To round things off the new banked GPIO infrastructure is added (along > > with some more preparatory work), followed by the conversion of the two > > Tegra GPIO drivers to the new infrastructure. > > I have put all on a branch for pushing to the test builders to begin with. > > Then I plan to make one branch with all infrastructure patches > (patches 1-10, 12-14) and pull that into devel, then apply patch > 11 and 15-16 directly on devel. > > That way other subsystems (pinctrl ...) can pull in the infrastructure > for people adding new gpiochips this cycle. Sounds good. > > Any thoughts on this? I'd like to target 4.15 with this, > > Me, too. > > > unless you'd be > > willing to take this into 4.14, which I doubt at this point. The absence > > of a GPIO driver has been hampering Tegra186 support upstream for a > > while now, so it'd be good to make progress on this. > > Sorry about that. Let's move ahead with this now, it is neat and > clean. > > What I want (as maintainer) is a bit of fingerpointing at the drivers > that need to be converted to use the new banking infrastructure > so they don't stay with their old crappy design pattern. OMAP is > a clear candidate right? (Added Tony to CC...) OMAP should be able to use this infrastructure, but it may not want to because the semantics would change slightly. Currently OMAP registers a GPIO chip for each bank, whereas this infrastructure exposes multiple banks via a single chip. There might be some userspace that relies on the existence of multiple chips, but Tony can probably knows that better than I. > Who else? gpio-intel-mid.c and gpio-merrifield.c look like they could use this new infrastructure. So do gpio-pca953x.c, gpio-stmpe.c and gpio-tc3589x.c. gpio-ws16c48.c is another one that uses a similar pattern. Thierry
* Thierry Reding <thierry.reding@gmail.com> [170915 08:10]: > On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote: > > Sorry about that. Let's move ahead with this now, it is neat and > > clean. > > > > What I want (as maintainer) is a bit of fingerpointing at the drivers > > that need to be converted to use the new banking infrastructure > > so they don't stay with their old crappy design pattern. OMAP is > > a clear candidate right? (Added Tony to CC...) > > OMAP should be able to use this infrastructure, but it may not want to > because the semantics would change slightly. Currently OMAP registers a > GPIO chip for each bank, whereas this infrastructure exposes multiple > banks via a single chip. Oh so you don't have separate interrupts for the instances? Thanks for clarifying that. > There might be some userspace that relies on the existence of multiple > chips, but Tony can probably knows that better than I. On omaps, each bank is a separate driver instance with it's own interrupt. Maybe really all we need to do is get rid of the "bank" naming, I think that's left over from 15 years ago when we did not have separate driver instances. It seems we should s/bank/ddata/ on the driver to avoid confusion. Grygorii, any comments? Regards, Tony -- 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
On 09/15/2017 11:57 AM, Tony Lindgren wrote: > * Thierry Reding <thierry.reding@gmail.com> [170915 08:10]: >> On Thu, Sep 14, 2017 at 03:54:56PM +0200, Linus Walleij wrote: >>> Sorry about that. Let's move ahead with this now, it is neat and >>> clean. >>> >>> What I want (as maintainer) is a bit of fingerpointing at the drivers >>> that need to be converted to use the new banking infrastructure >>> so they don't stay with their old crappy design pattern. OMAP is >>> a clear candidate right? (Added Tony to CC...) >> >> OMAP should be able to use this infrastructure, but it may not want to >> because the semantics would change slightly. Currently OMAP registers a >> GPIO chip for each bank, whereas this infrastructure exposes multiple >> banks via a single chip. > > Oh so you don't have separate interrupts for the instances? > Thanks for clarifying that. > >> There might be some userspace that relies on the existence of multiple >> chips, but Tony can probably knows that better than I. > > On omaps, each bank is a separate driver instance with it's own > interrupt. Maybe really all we need to do is get rid of the "bank" > naming, I think that's left over from 15 years ago when we did not > have separate driver instances. It seems we should s/bank/ddata/ > on the driver to avoid confusion. > > Grygorii, any comments? Sry, for delayed reply - I've saw this series, but, honestly, it's very big change for review :( So, can it be split? I think, patches which reorganize gpio irqchip specific fields placement and move them in gpio_irq_chip can be considered separately if they will not introduce functional changes. Also, omap changes can be considered separately. (Pay attention that new fields introduced in patch 1). Regarding OMAP GPIO - right now I do not see how it can be applied for OMAP :( Each OMAP GPIO bank is standalone device which can be enabled/disabled, powered on/off in Linux. There are no contiguous MMIO space and each GPIO bank have separate MMIO space. I really, need more time to review this idea and I think that it can be done more easily if series size will be reduced. Few more notes: - pay attention on commit dc749a0 "gpiolib: allow gpio irqchip to map irqs dynamically" - good to see binding and DT examples - not sure if I've got idea of encoding bank&pin in spec[0] :( + bank = (spec[0] >> gc->of_gpio_bank_mask) & gc->of_gpio_bank_shift; + pin = (spec[0] >> gc->of_gpio_pin_mask) & gc->of_gpio_pin_shift; - irq "mapping" inside gpio_irq_chip is not clear :( does it static? does it require one array item/per pin - sry, this is waste of memory? irq->map[offset + j] = irq->parents[parent]; Potentially, this feature can be applied to Davinci GPIO driver, which is GPIO controller divided in multiple logical banks and which also have set of common registers for all logical banks. But, again, not sure how effective this implementation is - need more time. As of now, we perfectly handle this in Davinci GPIO driver by creating only ONE gpio_chip which hides HW details in driver and still uses standard irq DT mappings: <&gpio0 140 GPIO_ACTIVE_LOW>; By the way Patch 14 adds 300 lines, while patch changes 200 lines, so in terms of code lines this feature seems is not very efficient. (same for Patch 15, 16)
On Fri, Sep 15, 2017 at 6:57 PM, Tony Lindgren <tony@atomide.com> wrote: > On omaps, each bank is a separate driver instance with it's own > interrupt. Maybe really all we need to do is get rid of the "bank" > naming, I think that's left over from 15 years ago when we did not > have separate driver instances. It seems we should s/bank/ddata/ > on the driver to avoid confusion. OK sorry maybe OMAP is not a target for this, I just thought so since it was one of the platforms that is patches in the patch series. But I'm pretty sure we have chips with banking like this: separate interrupts but a single device. I would have to read through them all I guess. 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
From: Thierry Reding <treding@nvidia.com> Hi Linus, here's the latest series of patches that implement the tighter IRQ chip integration as well as the banked GPIO infrastructure that we had discussed a couple of weeks/months back. The first couple of patches are mostly preparatory work in order to consolidate all IRQ chip related fields in a new structure and create the base functionality for adding IRQ chips. After that, I've added the Tegra186 GPIO support patch that makes use of the new tight integration. To round things off the new banked GPIO infrastructure is added (along with some more preparatory work), followed by the conversion of the two Tegra GPIO drivers to the new infrastructure. Any thoughts on this? I'd like to target 4.15 with this, unless you'd be willing to take this into 4.14, which I doubt at this point. The absence of a GPIO driver has been hampering Tegra186 support upstream for a while now, so it'd be good to make progress on this. All of the patches are bisectible, at least to the point where I was able to compile (there are a couple of odd users of linux/gpio/driver.h that I either couldn't figure out the right Kconfig options to enable or didn't have a cross-compiler for). I've carried these patches in my development tree for a couple of weeks now, though, and the 0-day builder hasn't had any complaints in a while. I also have a local cocci patch for the move of fields to struct gpio_irq_chip and it gives an empty patch when run on top of these patches on top of linux-next, so I am fairly confident that this is all good to go. Thierry Thierry Reding (16): gpio: Implement tighter IRQ chip integration gpio: Move irqchip into struct gpio_irq_chip gpio: Move irqdomain into struct gpio_irq_chip gpio: Move irq_base to struct gpio_irq_chip gpio: Move irq_handler to struct gpio_irq_chip gpio: Move irq_default_type to struct gpio_irq_chip gpio: Move irq_chained_parent to struct gpio_irq_chip gpio: Move irq_nested into struct gpio_irq_chip gpio: Move irq_valid_mask into struct gpio_irq_chip gpio: Move lock_key into struct gpio_irq_chip gpio: Add Tegra186 support gpio: omap: Fix checkpatch warnings gpio: omap: Rename struct gpio_bank to struct omap_gpio_bank gpio: Add support for banked GPIO controllers gpio: tegra: Use banked GPIO infrastructure gpio: tegra186: Use banked GPIO infrastructure Documentation/gpio/driver.txt | 6 +- drivers/bcma/driver_gpio.c | 2 +- drivers/gpio/Kconfig | 10 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-104-dio-48e.c | 2 +- drivers/gpio/gpio-104-idi-48.c | 2 +- drivers/gpio/gpio-104-idio-16.c | 2 +- drivers/gpio/gpio-adnp.c | 2 +- drivers/gpio/gpio-altera.c | 4 +- drivers/gpio/gpio-aspeed.c | 6 +- drivers/gpio/gpio-ath79.c | 2 +- drivers/gpio/gpio-brcmstb.c | 2 +- drivers/gpio/gpio-crystalcove.c | 2 +- drivers/gpio/gpio-dln2.c | 2 +- drivers/gpio/gpio-ftgpio010.c | 2 +- drivers/gpio/gpio-ingenic.c | 2 +- drivers/gpio/gpio-intel-mid.c | 2 +- drivers/gpio/gpio-lynxpoint.c | 2 +- drivers/gpio/gpio-max732x.c | 2 +- drivers/gpio/gpio-merrifield.c | 2 +- drivers/gpio/gpio-omap.c | 222 ++++++----- drivers/gpio/gpio-pca953x.c | 2 +- drivers/gpio/gpio-pcf857x.c | 2 +- drivers/gpio/gpio-pci-idio-16.c | 2 +- drivers/gpio/gpio-pl061.c | 2 +- drivers/gpio/gpio-rcar.c | 2 +- drivers/gpio/gpio-reg.c | 4 +- drivers/gpio/gpio-stmpe.c | 6 +- drivers/gpio/gpio-tc3589x.c | 2 +- drivers/gpio/gpio-tegra.c | 203 +++++----- drivers/gpio/gpio-tegra186.c | 571 ++++++++++++++++++++++++++++ drivers/gpio/gpio-vf610.c | 2 +- drivers/gpio/gpio-wcove.c | 2 +- drivers/gpio/gpio-ws16c48.c | 2 +- drivers/gpio/gpio-xgene-sb.c | 2 +- drivers/gpio/gpio-xlp.c | 2 +- drivers/gpio/gpio-zx.c | 2 +- drivers/gpio/gpio-zynq.c | 2 +- drivers/gpio/gpiolib-of.c | 101 +++++ drivers/gpio/gpiolib.c | 320 ++++++++++++++-- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 4 +- drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 2 +- drivers/pinctrl/intel/pinctrl-baytrail.c | 6 +- drivers/pinctrl/intel/pinctrl-cherryview.c | 6 +- drivers/pinctrl/intel/pinctrl-intel.c | 2 +- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 4 +- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 4 +- drivers/pinctrl/pinctrl-amd.c | 2 +- drivers/pinctrl/pinctrl-at91.c | 2 +- drivers/pinctrl/pinctrl-coh901.c | 2 +- drivers/pinctrl/pinctrl-mcp23s08.c | 2 +- drivers/pinctrl/pinctrl-oxnas.c | 2 +- drivers/pinctrl/pinctrl-pic32.c | 2 +- drivers/pinctrl/pinctrl-pistachio.c | 2 +- drivers/pinctrl/pinctrl-st.c | 2 +- drivers/pinctrl/pinctrl-sx150x.c | 2 +- drivers/pinctrl/qcom/pinctrl-msm.c | 2 +- drivers/pinctrl/sirf/pinctrl-atlas7.c | 2 +- drivers/pinctrl/sirf/pinctrl-sirf.c | 2 +- drivers/pinctrl/spear/pinctrl-plgpio.c | 2 +- drivers/platform/x86/intel_int0002_vgpio.c | 6 +- include/linux/gpio/driver.h | 272 +++++++++++-- include/linux/of_gpio.h | 10 + 63 files changed, 1504 insertions(+), 348 deletions(-) create mode 100644 drivers/gpio/gpio-tegra186.c