mbox series

[v2,00/16] gpio: Tight IRQ chip integration and banked infrastructure

Message ID 20170928095628.21966-1-thierry.reding@gmail.com
Headers show
Series gpio: Tight IRQ chip integration and banked infrastructure | expand

Message

Thierry Reding Sept. 28, 2017, 9:56 a.m. UTC
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.

Changes in v2:
- rename pins to lines for consistent terminology
- rename gpio_irq_chip_banked_handler() to
  gpio_irq_chip_banked_chained_handler()

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       |   5 +-
 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, 1505 insertions(+), 348 deletions(-)
 create mode 100644 drivers/gpio/gpio-tegra186.c

Comments

Grygorii Strashko Sept. 28, 2017, 2:22 p.m. UTC | #1
On 09/28/2017 04:56 AM, Thierry Reding wrote:
> 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.

Hm. So you've ignored my comments [1].

Sry, but I do not agree with this series.
- no prof that it can be re-used by other drivers than tegra
 (at least I do not see reasons to re-use it for any TI drivers)
- no split
- all GPIO IRQs mapped statically
- irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
  which is waste of memory
- DT binding changes not documented and no DT examples
- below is ugly ;)
+	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;

[1] https://lkml.org/lkml/2017/9/15/442

> 
> Changes in v2:
> - rename pins to lines for consistent terminology
> - rename gpio_irq_chip_banked_handler() to
>    gpio_irq_chip_banked_chained_handler()
> 
> 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       |   5 +-
>   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, 1505 insertions(+), 348 deletions(-)
>   create mode 100644 drivers/gpio/gpio-tegra186.c
>
Linus Walleij Oct. 2, 2017, 7:55 a.m. UTC | #2
On Thu, Sep 28, 2017 at 4:22 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Sry, but I do not agree with this series.
> - no prof that it can be re-used by other drivers than tegra
>  (at least I do not see reasons to re-use it for any TI drivers)

This is not necessarily a blocker if it can be shown that others than
TI/OMAP can reuse it.

I've looked at things like the imagination pistachio:

pinctrl@18101C00 {
        compatible = "img,pistachio-system-pinctrl";
        reg = <0x18101C00 0x400>;

        gpio0: gpio0 {
                interrupts = <GIC_SHARED 71 IRQ_TYPE_LEVEL_HIGH>;

                gpio-controller;
                #gpio-cells = <2>;

                interrupt-controller;
                #interrupt-cells = <2>;
        };

        ...

        gpio5: gpio5 {
                interrupts = <GIC_SHARED 76 IRQ_TYPE_LEVEL_HIGH>;

                gpio-controller;
                #gpio-cells = <2>;

                interrupt-controller;
                #interrupt-cells = <2>;
        };

This looks like a clear candidate.
CC: to  Andrew Bresticker, can you look into this?

Another example would be gpio-tz1090, notably with interrupt optional:

       gpios: gpio-controller@02005800 {
                #address-cells = <1>;
                #size-cells = <0>;
                compatible = "img,tz1090-gpio";
                reg = <0x02005800 0x90>;

                /* bank 0 with an interrupt */
                gpios0: bank@0 {
                        #gpio-cells = <2>;
                        #interrupt-cells = <2>;
                        reg = <0>;
                        interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
                        gpio-controller;
                        gpio-ranges = <&pinctrl 0 0 30>;
                        interrupt-controller;
                };

                /* bank 2 without interrupt */
                gpios2: bank@2 {
                        #gpio-cells = <2>;
                        reg = <2>;
                        gpio-controller;
                        gpio-ranges = <&pinctrl 0 60 30>;
                };
        };

CC to James Hogan to look at the series from this perspective.

A third is gpio-mxs.c:

pinctrl@80018000 {
        compatible = "fsl,imx28-pinctrl", "simple-bus";
        reg = <0x80018000 2000>;

        gpio0: gpio@0 {
                compatible = "fsl,imx28-gpio";
                interrupts = <127>;
                gpio-controller;
                #gpio-cells = <2>;
                interrupt-controller;
                #interrupt-cells = <2>;
        };

        gpio1: gpio@1 {
                compatible = "fsl,imx28-gpio";
                interrupts = <126>;
                gpio-controller;
                #gpio-cells = <2>;
                interrupt-controller;
                #interrupt-cells = <2>;
        };

        gpio2: gpio@2 {
                compatible = "fsl,imx28-gpio";
                interrupts = <125>;
                gpio-controller;
                #gpio-cells = <2>;
                interrupt-controller;
                #interrupt-cells = <2>;
        };
(...)

CC to Shawn Guo to look into this.

So in short I think there can be others that can make good use of this
infrastructure.

> - all GPIO IRQs mapped statically

This really needs to be fixed.

> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>   which is waste of memory
> - DT binding changes not documented and no DT examples
> - below is ugly ;)
> +       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;

These should be fixable quite easily I think. Thierry?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko Oct. 3, 2017, 6:26 p.m. UTC | #3
On 10/02/2017 02:55 AM, Linus Walleij wrote:
> On Thu, Sep 28, 2017 at 4:22 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> Sry, but I do not agree with this series.
>> - no prof that it can be re-used by other drivers than tegra
>>   (at least I do not see reasons to re-use it for any TI drivers)
> 
> This is not necessarily a blocker if it can be shown that others than
> TI/OMAP can reuse it.

sure. My point is - this is big change in gpiolib, which is > 1000 lines,
but current re-usability just 2 drivers (I'm comparing with your work when
gpio irq infra was introduced - you did it bottom-up, by refactoring
existing drivers and moving common code in gpiolib, so re usability is great). 

> 
> I've looked at things like the imagination pistachio:
> 
> pinctrl@18101C00 {
>          compatible = "img,pistachio-system-pinctrl";
>          reg = <0x18101C00 0x400>;
> 
>          gpio0: gpio0 {
>                  interrupts = <GIC_SHARED 71 IRQ_TYPE_LEVEL_HIGH>;
> 
>                  gpio-controller;
>                  #gpio-cells = <2>;
> 
>                  interrupt-controller;
>                  #interrupt-cells = <2>;
>          };
> 
>          ...
> 
>          gpio5: gpio5 {
>                  interrupts = <GIC_SHARED 76 IRQ_TYPE_LEVEL_HIGH>;
> 
>                  gpio-controller;
>                  #gpio-cells = <2>;
> 
>                  interrupt-controller;
>                  #interrupt-cells = <2>;
>          };
> 
> This looks like a clear candidate.
> CC: to  Andrew Bresticker, can you look into this?
> 

[...]

		gpio: gpio@226000 {
			compatible = "ti,dm6441-gpio";
			gpio-controller;
			#gpio-cells = <2>;
			reg = <0x226000 0x1000>;
			interrupts = <42 IRQ_TYPE_EDGE_BOTH
				43 IRQ_TYPE_EDGE_BOTH 44 IRQ_TYPE_EDGE_BOTH
				45 IRQ_TYPE_EDGE_BOTH 46 IRQ_TYPE_EDGE_BOTH
				47 IRQ_TYPE_EDGE_BOTH 48 IRQ_TYPE_EDGE_BOTH
				49 IRQ_TYPE_EDGE_BOTH 50 IRQ_TYPE_EDGE_BOTH>;
			ti,ngpio = <144>;
			ti,davinci-gpio-unbanked = <0>;
			status = "disabled";
			interrupt-controller;
			#interrupt-cells = <2>;
		};

FYI. Above is gpio-dvinci example which defines the same, but without coding
gpio banks in DT (note 2 IRQ lines per bank, bank 32 pins).

> 
> CC to Shawn Guo to look into this.
> 
> So in short I think there can be others that can make good use of this
> infrastructure.
> 
>> - all GPIO IRQs mapped statically
> 
> This really needs to be fixed.
> 
>> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>>    which is waste of memory
>> - DT binding changes not documented and no DT examples
>> - below is ugly ;)
>> +       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;
> 
> These should be fixable quite easily I think. Thierry?

What I'm trying to understand is how GPIO client bindings will look like?
Now it is: gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; (pistachio_marduk.dts)

But as per of_gpio_banked_xlate() it expected to be
gpios = <&gpio [Linear gpio num] GPIO_ACTIVE_LOW>;
Wouldn't this break DT compatibility and prevent re-using of this feature
for pistachio, for example? (or i'm missing smth).
Linus Walleij Oct. 5, 2017, 11:14 a.m. UTC | #4
On Mon, Oct 2, 2017 at 9:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

> This is not necessarily a blocker if it can be shown that others than
> TI/OMAP can reuse it.
>
> I've looked at things like the imagination pistachio:
>
> pinctrl@18101C00 {
>         compatible = "img,pistachio-system-pinctrl";
>         reg = <0x18101C00 0x400>;
>
>         gpio0: gpio0 {
>                 interrupts = <GIC_SHARED 71 IRQ_TYPE_LEVEL_HIGH>;
>
>                 gpio-controller;
>                 #gpio-cells = <2>;
>
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>         };
>
>         ...
>
>         gpio5: gpio5 {
>                 interrupts = <GIC_SHARED 76 IRQ_TYPE_LEVEL_HIGH>;
>
>                 gpio-controller;
>                 #gpio-cells = <2>;
>
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>         };
>
> This looks like a clear candidate.
> CC: to  Andrew Bresticker, can you look into this?
>
> Another example would be gpio-tz1090, notably with interrupt optional:
>
>        gpios: gpio-controller@02005800 {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 compatible = "img,tz1090-gpio";
>                 reg = <0x02005800 0x90>;
>
>                 /* bank 0 with an interrupt */
>                 gpios0: bank@0 {
>                         #gpio-cells = <2>;
>                         #interrupt-cells = <2>;
>                         reg = <0>;
>                         interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
>                         gpio-controller;
>                         gpio-ranges = <&pinctrl 0 0 30>;
>                         interrupt-controller;
>                 };
>
>                 /* bank 2 without interrupt */
>                 gpios2: bank@2 {
>                         #gpio-cells = <2>;
>                         reg = <2>;
>                         gpio-controller;
>                         gpio-ranges = <&pinctrl 0 60 30>;
>                 };
>         };
>
> CC to James Hogan to look at the series from this perspective.
>
> A third is gpio-mxs.c:
>
> pinctrl@80018000 {
>         compatible = "fsl,imx28-pinctrl", "simple-bus";
>         reg = <0x80018000 2000>;
>
>         gpio0: gpio@0 {
>                 compatible = "fsl,imx28-gpio";
>                 interrupts = <127>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>         };
>
>         gpio1: gpio@1 {
>                 compatible = "fsl,imx28-gpio";
>                 interrupts = <126>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>         };
>
>         gpio2: gpio@2 {
>                 compatible = "fsl,imx28-gpio";
>                 interrupts = <125>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>         };
> (...)
>
> CC to Shawn Guo to look into this.
>
> So in short I think there can be others that can make good use of this
> infrastructure.

It also looks like the "ports" in the gpio-dwapb can be modeled using this
bank infrastructure, cutting down on per-driver overhead and adding
good much needed abstraction for these ports/banks.

Paging Hoan Tran on this as well, to keep an eye on the series.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 5, 2017, 11:19 a.m. UTC | #5
On Tue, Oct 3, 2017 at 8:26 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 10/02/2017 02:55 AM, Linus Walleij wrote:
>> On Thu, Sep 28, 2017 at 4:22 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>
>>> Sry, but I do not agree with this series.
>>> - no prof that it can be re-used by other drivers than tegra
>>>   (at least I do not see reasons to re-use it for any TI drivers)
>>
>> This is not necessarily a blocker if it can be shown that others than
>> TI/OMAP can reuse it.
>
> sure. My point is - this is big change in gpiolib, which is > 1000 lines,
> but current re-usability just 2 drivers (I'm comparing with your work when
> gpio irq infra was introduced - you did it bottom-up, by refactoring
> existing drivers and moving common code in gpiolib, so re usability is great).

Yes I am leaning toward adding this infrastructure also with switching as many
candidates as possible over to using the new infrastructure. So I'm trying
to align a few maintainers to this cause. Maybe OMAP will not be one
of them as I thought initially :/

>                 gpio: gpio@226000 {
>                         compatible = "ti,dm6441-gpio";
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         reg = <0x226000 0x1000>;
>                         interrupts = <42 IRQ_TYPE_EDGE_BOTH
>                                 43 IRQ_TYPE_EDGE_BOTH 44 IRQ_TYPE_EDGE_BOTH
>                                 45 IRQ_TYPE_EDGE_BOTH 46 IRQ_TYPE_EDGE_BOTH
>                                 47 IRQ_TYPE_EDGE_BOTH 48 IRQ_TYPE_EDGE_BOTH
>                                 49 IRQ_TYPE_EDGE_BOTH 50 IRQ_TYPE_EDGE_BOTH>;
>                         ti,ngpio = <144>;
>                         ti,davinci-gpio-unbanked = <0>;
>                         status = "disabled";
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>                 };
>
> FYI. Above is gpio-dvinci example which defines the same, but without coding
> gpio banks in DT (note 2 IRQ lines per bank, bank 32 pins).

Yeah. This case would be nice to cover too.

>>> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>>>    which is waste of memory
>>> - DT binding changes not documented and no DT examples
>>> - below is ugly ;)
>>> +       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;
>>
>> These should be fixable quite easily I think. Thierry?
>
> What I'm trying to understand is how GPIO client bindings will look like?
> Now it is: gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; (pistachio_marduk.dts)
>
> But as per of_gpio_banked_xlate() it expected to be
> gpios = <&gpio [Linear gpio num] GPIO_ACTIVE_LOW>;
> Wouldn't this break DT compatibility and prevent re-using of this feature
> for pistachio, for example? (or i'm missing smth).

I was hoping we could introduce infrastructure that can be used
by the existing in-tree banked/port GPIO drivers without any
changes to the consumer side of bindings.

So that is the patch set I'm imagining.

Else we're not really getting reusability.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Oct. 6, 2017, 11:07 a.m. UTC | #6
On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote:
> 
> 
> On 09/28/2017 04:56 AM, Thierry Reding wrote:
> > 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.
> 
> Hm. So you've ignored my comments [1].
> 
> Sry, but I do not agree with this series.
> - no prof that it can be re-used by other drivers than tegra
>  (at least I do not see reasons to re-use it for any TI drivers)

I had done some research based on Linus' feedback from an earlier series
and identified the following potential candidates[0] that could move to
this new infrastructure:

	- gpio-intel-mid.c
	- gpio-merrifield.c
	- gpio-pca953x.c
	- gpio-stmpe.c
	- gpio-tc3589x.c
	- gpio-ws16c48.c

Note that this is based on code inspection rather than DT inspection,
because that's fundamentally flawed. If you look at this from a DT
perspective you're going to be tempted to change the DT bindings, but
you can't do that because of backwards compatiblity. This new framework
also doesn't address the issues at that level, but rather tries to be
some common code that is otherwise duplicated in one way or another in
various drivers and therefore hard to maintain. This is what Linus had
originally requested, and that's what the series does.

> - no split

What does this mean? The series is nicely split into separate patches,
so each one individually is easy to review. I've also gone through quite
some trouble to make sure everything builds fine after each patch, so
it's possible to apply individual bits of the series. For example we
could opt to apply everything up to the banked GPIO support if that's
still contentious.

> - all GPIO IRQs mapped statically

This series predates your work on the dynamic IRQ mapping, so I hadn't
picked up those changes. This should be easily solved by the attached
patch, though.

> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>   which is waste of memory

It's the only way to generically do this. Also I don't think this wastes
that much memory. We have one unsigned int per pin, which even for very
large GPIO controllers is unlikely to exceed one 4 KiB page.

> - DT binding changes not documented and no DT examples

That's because this is completely orthogonal to DT bindings. We can't
make any changes to the bindings because of ABI stability.

> - below is ugly ;)
> +	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;

If by ugly you mean wrong, then yes, it's actually the wrong way around.
It should be:

	bank = (spec[0] >> gc->of_gpio_bank_shift) & gc->of_gpio_bank_mask;
	line = (spec[0] >> gc->of_gpio_line_shift) & gc->of_gpio_line_mask;

Thierry
Thierry Reding Oct. 6, 2017, 11:11 a.m. UTC | #7
On Fri, Oct 06, 2017 at 01:07:49PM +0200, Thierry Reding wrote:
> On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote:
[...]
> > - all GPIO IRQs mapped statically
> 
> This series predates your work on the dynamic IRQ mapping, so I hadn't
> picked up those changes. This should be easily solved by the attached
> patch, though.

Here's the patch.

Thierry

--- >8 ---
commit 139c254bf963bf373d83970e530a56599f1832cc
Author: Thierry Reding <treding@nvidia.com>
Date:   Fri Oct 6 12:12:27 2017 +0200

    fixup! gpio: Implement tighter IRQ chip integration

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b3bd19b793d3..2e450afe61b3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1708,9 +1708,23 @@ static void gpiochip_irq_relres(struct irq_data *d)
 
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
+	unsigned int irq;
+	int err;
+
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
 		return -ENXIO;
-	return irq_create_mapping(chip->irq.domain, offset);
+
+	irq = irq_create_mapping(chip->irq.domain, offset);
+	if (!irq)
+		return 0;
+
+	if (chip->irq.map) {
+		err = irq_set_parent(irq, chip->irq.map[offset]);
+		if (err < 0)
+			return err;
+	}
+
+	return irq;
 }
 
 /**
@@ -1856,27 +1870,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
 		gpiochip->irq.nested = true;
 	}
 
-	/*
-	 * Prepare the mapping since the IRQ chip shall be orthogonal to any
-	 * GPIO chip calls.
-	 */
-	for (i = 0; i < gpiochip->ngpio; i++) {
-		unsigned int irq;
-
-		if (!gpiochip_irqchip_irq_valid(gpiochip, i))
-			continue;
-
-		irq = irq_create_mapping(gpiochip->irq.domain, i);
-		if (!irq) {
-			chip_err(gpiochip,
-				 "failed to create IRQ mapping for GPIO#%u\n",
-				 i);
-			continue;
-		}
-
-		irq_set_parent(irq, gpiochip->irq.map[i]);
-	}
-
 	acpi_gpiochip_request_interrupts(gpiochip);
 
 	return 0;
Grygorii Strashko Oct. 9, 2017, 9:56 p.m. UTC | #8
On 10/06/2017 06:07 AM, Thierry Reding wrote:
> On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote:
>>
>>
>> On 09/28/2017 04:56 AM, Thierry Reding wrote:
>>> 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.
>>
>> Hm. So you've ignored my comments [1].
>>
>> Sry, but I do not agree with this series.
>> - no prof that it can be re-used by other drivers than tegra
>>   (at least I do not see reasons to re-use it for any TI drivers)
> 
> I had done some research based on Linus' feedback from an earlier series
> and identified the following potential candidates[0] that could move to
> this new infrastructure:
> 

below based on code check:

> 	- gpio-intel-mid.c

one irq per all gpios in controller

> 	- gpio-merrifield.c

one irq per all gpios in controller

> 	- gpio-pca953x.c
one irq per all gpios in controller

> 	- gpio-stmpe.c
one irq per all gpios in controller
> 	- gpio-tc3589x.c
one irq per all gpios in controller
> 	- gpio-ws16c48.c

one irq per all gpios in controller

> 
> Note that this is based on code inspection rather than DT inspection,
> because that's fundamentally flawed. If you look at this from a DT
> perspective you're going to be tempted to change the DT bindings, but
> you can't do that because of backwards compatiblity. This new framework
> also doesn't address the issues at that level, but rather tries to be
> some common code that is otherwise duplicated in one way or another in
> various drivers and therefore hard to maintain. This is what Linus had
> originally requested, and that's what the series does.

I've looked at this again, and again. I've looked on drivers listed above.
Sry, I do not see how this change can improve/simplify above drivers :(
May be it will clean up my doubts, if it will be possible to convert more drivers?

> 
>> - no split
> 
> What does this mean? The series is nicely split into separate patches,
> so each one individually is easy to review. I've also gone through quite
> some trouble to make sure everything builds fine after each patch, so
> it's possible to apply individual bits of the series. For example we
> could opt to apply everything up to the banked GPIO support if that's
> still contentious.

i've commented it in [1]. copy paste here

>>
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). 
>>

This will reduce size of your series and concentrate review attention on actual functional changes.


[1] https://lkml.org/lkml/2017/9/15/442

> 
>> - all GPIO IRQs mapped statically
> 
> This series predates your work on the dynamic IRQ mapping, so I hadn't
> picked up those changes. This should be easily solved by the attached
> patch, though.
> 
>> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>>    which is waste of memory
> 
> It's the only way to generically do this. Also I don't think this wastes
> that much memory. We have one unsigned int per pin, which even for very
> large GPIO controllers is unlikely to exceed one 4 KiB page.

for system with <128M of memory even 4k is a win.


> 
>> - DT binding changes not documented and no DT examples
> 
> That's because this is completely orthogonal to DT bindings. We can't
> make any changes to the bindings because of ABI stability.
> 
>> - below is ugly ;)
>> +	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;
> 
> If by ugly you mean wrong, then yes, it's actually the wrong way around.
> It should be:
> 
> 	bank = (spec[0] >> gc->of_gpio_bank_shift) & gc->of_gpio_bank_mask;
> 	line = (spec[0] >> gc->of_gpio_line_shift) & gc->of_gpio_line_mask;

 
Wrong yep. And No. What I do not like is encoding bank & line in the same field.
It creates some not clear DT standard bindings requirements as for me, comparing to the
current well known GPIO bindings 
 gpios = <&[controller] [line number in controller] [flags]>;
line number in controller ::= [0..max lines]

Actually, as per gpio.txt:
"Note that gpio-specifier length is controller dependent.  In the
above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
only uses one.", 
so, if this going to be part of gpiolib it should be
 described in bindings/gpio/gpio.txt (or some other documents), as
 above note will not be exactly correct and new "banked" gpio controllers
will be expected to use thin new binding.
Thierry Reding Oct. 10, 2017, 11:27 a.m. UTC | #9
On Mon, Oct 09, 2017 at 04:56:57PM -0500, Grygorii Strashko wrote:
> 
> 
> On 10/06/2017 06:07 AM, Thierry Reding wrote:
> > On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote:
> >>
> >>
> >> On 09/28/2017 04:56 AM, Thierry Reding wrote:
> >>> 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.
> >>
> >> Hm. So you've ignored my comments [1].
> >>
> >> Sry, but I do not agree with this series.
> >> - no prof that it can be re-used by other drivers than tegra
> >>   (at least I do not see reasons to re-use it for any TI drivers)
> > 
> > I had done some research based on Linus' feedback from an earlier series
> > and identified the following potential candidates[0] that could move to
> > this new infrastructure:
> > 
> 
> below based on code check:
> 
> > 	- gpio-intel-mid.c
> 
> one irq per all gpios in controller
> 
> > 	- gpio-merrifield.c
> 
> one irq per all gpios in controller
> 
> > 	- gpio-pca953x.c
> one irq per all gpios in controller
> 
> > 	- gpio-stmpe.c
> one irq per all gpios in controller
> > 	- gpio-tc3589x.c
> one irq per all gpios in controller
> > 	- gpio-ws16c48.c
> 
> one irq per all gpios in controller

I explained in another subthread how we could cater for this particular
use-case.

> > Note that this is based on code inspection rather than DT inspection,
> > because that's fundamentally flawed. If you look at this from a DT
> > perspective you're going to be tempted to change the DT bindings, but
> > you can't do that because of backwards compatiblity. This new framework
> > also doesn't address the issues at that level, but rather tries to be
> > some common code that is otherwise duplicated in one way or another in
> > various drivers and therefore hard to maintain. This is what Linus had
> > originally requested, and that's what the series does.
> 
> I've looked at this again, and again. I've looked on drivers listed above.
> Sry, I do not see how this change can improve/simplify above drivers :(
> May be it will clean up my doubts, if it will be possible to convert more
> drivers?

I'm reluctant to spend more work on this and get Tegra186 GPIO support
delayed indefinitely until every other driver has been converted. The
Tegra186 GPIO driver is now more than a year old and I've got a few
dozen patches that depend on GPIO functionality that we currently can't
merge because this unification work has been going on for more than 6
months. This has been a very frustrating experience overall.

I think this series is in good enough shape for now. It improves the
situation in general. I also don't see anything in here that couldn't be
further improved should the need arise.

If this really isn't useful at all, can we please at least get patches
1-11 merged? T hey are independent of the banked infrastructure work in
the last couple of patches.

> >> - no split
> > 
> > What does this mean? The series is nicely split into separate patches,
> > so each one individually is easy to review. I've also gone through quite
> > some trouble to make sure everything builds fine after each patch, so
> > it's possible to apply individual bits of the series. For example we
> > could opt to apply everything up to the banked GPIO support if that's
> > still contentious.
> 
> i've commented it in [1]. copy paste here
> 
> >>
> 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). 
> >>
> 
> This will reduce size of your series and concentrate review attention on actual functional changes.
> 
> 
> [1] https://lkml.org/lkml/2017/9/15/442

I could of course split the series into multiple parts, but then it
becomes difficult to represent dependencies. The changes you mention are
already split into separate commits and I even made sure that they keep
bisectibility. I've sent them together primarily to keep them in the
order that they need to be applied in to simplify things.

I don't see how splitting up the series any further is going to simplify
review. If you want to only review the banked infrastructure change, can
you not just focus on that patch and ignore the first ten since they are
not actually functional changes?

> 
> > 
> >> - all GPIO IRQs mapped statically
> > 
> > This series predates your work on the dynamic IRQ mapping, so I hadn't
> > picked up those changes. This should be easily solved by the attached
> > patch, though.
> > 
> >> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
> >>    which is waste of memory
> > 
> > It's the only way to generically do this. Also I don't think this wastes
> > that much memory. We have one unsigned int per pin, which even for very
> > large GPIO controllers is unlikely to exceed one 4 KiB page.
> 
> for system with <128M of memory even 4k is a win.

I suspect that such systems won't have GPIOs where this infrastructure
would be used, or where the number of GPIOs would be problematic.

Also, this is supposed to be generic infrastructure and that usually
comes at some price. I don't know how to do this without the mapping,
but I'm certainly open to suggestions.

> >> - DT binding changes not documented and no DT examples
> > 
> > That's because this is completely orthogonal to DT bindings. We can't
> > make any changes to the bindings because of ABI stability.
> > 
> >> - below is ugly ;)
> >> +	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;
> > 
> > If by ugly you mean wrong, then yes, it's actually the wrong way around.
> > It should be:
> > 
> > 	bank = (spec[0] >> gc->of_gpio_bank_shift) & gc->of_gpio_bank_mask;
> > 	line = (spec[0] >> gc->of_gpio_line_shift) & gc->of_gpio_line_mask;
> 
>  
> Wrong yep. And No. What I do not like is encoding bank & line in the same field.

This is not about liking or disliking the encoding. The encoding is
already defined in the bindings for Tegra and Tegra186, so this isn't up
for discussion.

> It creates some not clear DT standard bindings requirements as for me, comparing to the
> current well known GPIO bindings 
>  gpios = <&[controller] [line number in controller] [flags]>;
> line number in controller ::= [0..max lines]

Yes, that's because this is specifically for banked controllers. It
seems to me like most other bindings for banked controllers simply use a
linear mapping, in which case the simple example above would work.

Tegra seems somewhat of an exception because the DT bindings use the
(bank, line) pair encoding to reflect the names given to the lines in
technical reference manuals. I like this because it makes the DTS files
very easy to read and relate to schematics.

> Actually, as per gpio.txt:
> "Note that gpio-specifier length is controller dependent.  In the
> above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
> only uses one.", 
> so, if this going to be part of gpiolib it should be
>  described in bindings/gpio/gpio.txt (or some other documents), as
>  above note will not be exactly correct and new "banked" gpio controllers
> will be expected to use thin new binding.

Yeah, that makes sense. I'll work on a patch that updates the binding
documentation to include this particular encoding. Again, this is not
intended to replace existing bindings because they may not be
compatible. However, bindings for new banked controllers may be able
to reuse this if it suits their needs.

Also, note that the rest of the banked infrastructure can be used
independently of the ->xlate() callback. Drivers are free to use the
of_gpio_simple_xlate() with the rest of the banked infrastructure to
simplify the driver code.

Thierry
Grygorii Strashko Oct. 10, 2017, 10:56 p.m. UTC | #10
On 10/10/2017 06:27 AM, Thierry Reding wrote:
> On Mon, Oct 09, 2017 at 04:56:57PM -0500, Grygorii Strashko wrote:
>>
>>
>> On 10/06/2017 06:07 AM, Thierry Reding wrote:
>>> On Thu, Sep 28, 2017 at 09:22:17AM -0500, Grygorii Strashko wrote:
>>>>
>>>>
>>>> On 09/28/2017 04:56 AM, Thierry Reding wrote:
>>>>> 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.
>>>>
>>>> Hm. So you've ignored my comments [1].
>>>>
>>>> Sry, but I do not agree with this series.
>>>> - no prof that it can be re-used by other drivers than tegra
>>>>    (at least I do not see reasons to re-use it for any TI drivers)
>>>
>>> I had done some research based on Linus' feedback from an earlier series
>>> and identified the following potential candidates[0] that could move to
>>> this new infrastructure:
>>>
>>
>> below based on code check:
>>
>>> 	- gpio-intel-mid.c
>>
>> one irq per all gpios in controller
>>
>>> 	- gpio-merrifield.c
>>
>> one irq per all gpios in controller
>>
>>> 	- gpio-pca953x.c
>> one irq per all gpios in controller
>>
>>> 	- gpio-stmpe.c
>> one irq per all gpios in controller
>>> 	- gpio-tc3589x.c
>> one irq per all gpios in controller
>>> 	- gpio-ws16c48.c
>>
>> one irq per all gpios in controller
> 
> I explained in another subthread how we could cater for this particular
> use-case.
> 
>>> Note that this is based on code inspection rather than DT inspection,
>>> because that's fundamentally flawed. If you look at this from a DT
>>> perspective you're going to be tempted to change the DT bindings, but
>>> you can't do that because of backwards compatiblity. This new framework
>>> also doesn't address the issues at that level, but rather tries to be
>>> some common code that is otherwise duplicated in one way or another in
>>> various drivers and therefore hard to maintain. This is what Linus had
>>> originally requested, and that's what the series does.
>>
>> I've looked at this again, and again. I've looked on drivers listed above.
>> Sry, I do not see how this change can improve/simplify above drivers :(
>> May be it will clean up my doubts, if it will be possible to convert more
>> drivers?
> 
> I'm reluctant to spend more work on this and get Tegra186 GPIO support
> delayed indefinitely until every other driver has been converted. The
> Tegra186 GPIO driver is now more than a year old and I've got a few
> dozen patches that depend on GPIO functionality that we currently can't
> merge because this unification work has been going on for more than 6
> months. This has been a very frustrating experience overall.
> 
> I think this series is in good enough shape for now. It improves the
> situation in general. I also don't see anything in here that couldn't be
> further improved should the need arise.
> 
> If this really isn't useful at all, can we please at least get patches
> 1-11 merged? T hey are independent of the banked infrastructure work in
> the last couple of patches.

I agree with patches 10 in general (with comments applied) and, honestly.
Do not see reasons why 11 can't go (but it any way up to Linus). 
That why I've proposed split.

> 
>>>> - no split
>>>
>>> What does this mean? The series is nicely split into separate patches,
>>> so each one individually is easy to review. I've also gone through quite
>>> some trouble to make sure everything builds fine after each patch, so
>>> it's possible to apply individual bits of the series. For example we
>>> could opt to apply everything up to the banked GPIO support if that's
>>> still contentious.
>>
>> i've commented it in [1]. copy paste here
>>
>>>>
>> 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).
>>>>
>>
>> This will reduce size of your series and concentrate review attention on actual functional changes.
>>
>>
>> [1] https://lkml.org/lkml/2017/9/15/442
> 
> I could of course split the series into multiple parts, but then it
> becomes difficult to represent dependencies. The changes you mention are
> already split into separate commits and I even made sure that they keep
> bisectibility. I've sent them together primarily to keep them in the
> order that they need to be applied in to simplify things.
> 
> I don't see how splitting up the series any further is going to simplify
> review. If you want to only review the banked infrastructure change, can
> you not just focus on that patch and ignore the first ten since they are
> not actually functional changes?

Patch 1 need to be updated as for me. while 2 - 10 are very simple.
It will allow to get rid 10 patches from your series if
- first introduce struct gpio_irq_chip
- second move all current IRQ specific field in it
- last add gpiochip_add_irqchip() and num_parents, parents, map

this will allow to update drivers and drop gpiochip_irqchip_add() by filling
gpio_irq_chip structure.

> 
>>
>>>
>>>> - all GPIO IRQs mapped statically
>>>
>>> This series predates your work on the dynamic IRQ mapping, so I hadn't
>>> picked up those changes. This should be easily solved by the attached
>>> patch, though.
>>>
>>>> - irq->map[offset + j] = irq->parents[parent]; holds IRQs for all pins
>>>>     which is waste of memory
>>>
>>> It's the only way to generically do this. Also I don't think this wastes
>>> that much memory. We have one unsigned int per pin, which even for very
>>> large GPIO controllers is unlikely to exceed one 4 KiB page.
>>
>> for system with <128M of memory even 4k is a win.
> 
> I suspect that such systems won't have GPIOs where this infrastructure
> would be used, or where the number of GPIOs would be problematic.
> 
> Also, this is supposed to be generic infrastructure and that usually
> comes at some price. I don't know how to do this without the mapping,
> but I'm certainly open to suggestions.
> 
>>>> - DT binding changes not documented and no DT examples
>>>
>>> That's because this is completely orthogonal to DT bindings. We can't
>>> make any changes to the bindings because of ABI stability.
>>>
>>>> - below is ugly ;)
>>>> +	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;
>>>
>>> If by ugly you mean wrong, then yes, it's actually the wrong way around.
>>> It should be:
>>>
>>> 	bank = (spec[0] >> gc->of_gpio_bank_shift) & gc->of_gpio_bank_mask;
>>> 	line = (spec[0] >> gc->of_gpio_line_shift) & gc->of_gpio_line_mask;
>>
>>   
>> Wrong yep. And No. What I do not like is encoding bank & line in the same field.
> 
> This is not about liking or disliking the encoding. The encoding is
> already defined in the bindings for Tegra and Tegra186, so this isn't up
> for discussion.
> 
>> It creates some not clear DT standard bindings requirements as for me, comparing to the
>> current well known GPIO bindings
>>   gpios = <&[controller] [line number in controller] [flags]>;
>> line number in controller ::= [0..max lines]
> 
> Yes, that's because this is specifically for banked controllers. It
> seems to me like most other bindings for banked controllers simply use a
> linear mapping, in which case the simple example above would work.
> 
> Tegra seems somewhat of an exception because the DT bindings use the
> (bank, line) pair encoding to reflect the names given to the lines in
> technical reference manuals. I like this because it makes the DTS files
> very easy to read and relate to schematics.

I did some googling and saw tegra binding accepted already. 
Juts curious why not to use two cells <bank/port> <pin>
it as for me much more readable and no masks/shifts 

> 
>> Actually, as per gpio.txt:
>> "Note that gpio-specifier length is controller dependent.  In the
>> above example, &gpio1 uses 2 cells to specify a gpio, while &gpio2
>> only uses one.",
>> so, if this going to be part of gpiolib it should be
>>   described in bindings/gpio/gpio.txt (or some other documents), as
>>   above note will not be exactly correct and new "banked" gpio controllers
>> will be expected to use thin new binding.
> 
> Yeah, that makes sense. I'll work on a patch that updates the binding
> documentation to include this particular encoding. Again, this is not
> intended to replace existing bindings because they may not be
> compatible. However, bindings for new banked controllers may be able
> to reuse this if it suits their needs.
> 
> Also, note that the rest of the banked infrastructure can be used
> independently of the ->xlate() callback. Drivers are free to use the
> of_gpio_simple_xlate() with the rest of the banked infrastructure to
> simplify the driver code.