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

[Grygorii Strashko]

On 10/02/2017 02:55 AM, Linus Walleij wrote:
> On Thu, Sep 28, 2017 at 4:22 PM, Grygorii Strashko
> <grygorii.strashko@xxxxxx> 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).

-- 
regards,
-grygorii