[PATCH v3 0/3] GPIO DT support for da850

[PATCH v3 0/3] GPIO DT support for da850

[Prabhakar Lad]

From: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>

This patch series
- adds dt binding support for gpio-davinci.
- da850 dt support goio.

This patch series is rebased on 3.12-rc2

Changes for v3:
1: Fixed review comments pointed by Sekhar

KV Sujith (3):
  gpio: davinci: add OF support
  ARM: davinci: da850: add GPIO DT node
  ARM: davinci: da850 evm: add GPIO pinumux entries DT node

 .../devicetree/bindings/gpio/gpio-davinci.txt      |   34 +++++++++++
 arch/arm/boot/dts/da850-evm.dts                    |   20 +++++++
 arch/arm/boot/dts/da850.dtsi                       |   15 +++++
 drivers/gpio/gpio-davinci.c                        |   60 +++++++++++++++++++-
 4 files changed, 126 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt

-- 
1.7.9.5

[PATCH v3 1/3] gpio: davinci: add OF support

[Prabhakar Lad]

From: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>

This patch adds OF parser support for davinci gpio
driver and also appropriate documentation in gpio-davinci.txt
located at Documentation/devicetree/bindings/gpio/.

Signed-off-by: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>
Signed-off-by: Philip Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
Acked-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
[prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org: simplified the OF code and also
                 the commit message]
Signed-off-by: Lad, Prabhakar <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/gpio/gpio-davinci.txt      |   34 +++++++++++
 drivers/gpio/gpio-davinci.c                        |   60 +++++++++++++++++++-
 2 files changed, 91 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
new file mode 100644
index 0000000..87abd3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
@@ -0,0 +1,34 @@
+Davinci GPIO controller bindings
+
+Required Properties:
+- compatible: should be "ti,dm6441-gpio"
+
+- reg: Physical base address of the controller and the size of memory mapped
+       registers.
+
+- gpio-controller : Marks the device node as a gpio controller.
+
+- interrupts: Array of GPIO interrupt number.
+
+- ngpio: The number of GPIO pins supported.
+
+- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering starts.
+
+- ti,davinci-gpio-unbanked: The number of GPIOs that have an individual interrupt
+		             line to processor.
+
+Example:
+
+gpio: gpio@1e26000 {
+	compatible = "ti,dm6441-gpio";
+	gpio-controller;
+	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>;
+	ngpio = <144>;
+	ti,davinci-gpio-irq-base = <101>;
+	ti,davinci-gpio-unbanked = <0>;
+};
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 8847adf..1e7c5a4 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -16,6 +16,9 @@
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/gpio-davinci.h>
 
@@ -133,6 +136,46 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	__raw_writel((1 << offset), value ? &g->set_data : &g->clr_data);
 }
 
+static struct davinci_gpio_platform_data *
+davinci_gpio_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *dn = pdev->dev.of_node;
+	struct davinci_gpio_platform_data *pdata;
+	int ret;
+	u32 val;
+
+	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+		return pdev->dev.platform_data;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	ret = of_property_read_u32(dn, "ngpio", &val);
+	if (ret)
+		goto of_err;
+
+	pdata->ngpio = val;
+
+	ret = of_property_read_u32(dn, "ti,davinci-gpio-unbanked", &val);
+	if (ret)
+		goto of_err;
+
+	pdata->gpio_unbanked = val;
+
+	ret = of_property_read_u32(dn, "ti,davinci-gpio-irq-base", &val);
+	if (ret)
+		goto of_err;
+
+	pdata->intc_irq_num = val;
+
+	return pdata;
+
+of_err:
+	dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
+	return NULL;
+}
+
 static int davinci_gpio_probe(struct platform_device *pdev)
 {
 	int i, base;
@@ -143,12 +186,14 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 
-	pdata = dev->platform_data;
+	pdata = davinci_gpio_get_pdata(pdev);
 	if (!pdata) {
 		dev_err(dev, "No platform data found\n");
 		return -EINVAL;
 	}
 
+	dev->platform_data = pdata;
+
 	/*
 	 * The gpio banks conceptually expose a segmented bitmap,
 	 * and "ngpio" is one more than the largest zero-based
@@ -490,11 +535,20 @@ done:
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id davinci_gpio_ids[] = {
+	{ .compatible = "ti,dm6441-gpio", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
+#endif
+
 static struct platform_driver davinci_gpio_driver = {
 	.probe		= davinci_gpio_probe,
 	.driver		= {
-		.name	= "davinci_gpio",
-		.owner	= THIS_MODULE,
+		.name		= "davinci_gpio",
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(davinci_gpio_ids),
 	},
 };
 
-- 
1.7.9.5

[PATCH v3 2/3] ARM: davinci: da850: add GPIO DT node

[Prabhakar Lad]

From: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>

Add DT node for Davinci GPIO driver.

Signed-off-by: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>
Signed-off-by: Philip Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
Signed-off-by: Lad, Prabhakar <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/da850.dtsi |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 8d17346..85697f666 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -8,6 +8,7 @@
  * option) any later version.
  */
 #include "skeleton.dtsi"
+#include <dt-bindings/interrupt-controller/irq.h>
 
 / {
 	arm {
@@ -256,6 +257,20 @@
 					36
 					>;
 		};
+		gpio: gpio@1e26000 {
+			compatible = "ti,dm6441-gpio";
+			gpio-controller;
+			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>;
+			ngpio = <144>;
+			ti,davinci-gpio-irq-base = <101>;
+			ti,davinci-gpio-unbanked = <0>;
+			status = "disabled";
+		};
 	};
 	nand_cs3@62000000 {
 		compatible = "ti,davinci-nand";
-- 
1.7.9.5

[PATCH v3 3/3] ARM: davinci: da850 evm: add GPIO pinumux entries DT node

[Prabhakar Lad]

From: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>

Add GPIO DT node and pinmux entries for DA850 EVM. GPIO is
configurable differently on different boards. So add GPIO
pinmuxing in dts file.

Signed-off-by: KV Sujith <sujithkv-l0cyMroinI0@public.gmane.org>
Signed-off-by: Philip Avinash <avinashphilip-l0cyMroinI0@public.gmane.org>
Signed-off-by: Lad, Prabhakar <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/da850-evm.dts |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 588ce58..f82c129 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -17,6 +17,21 @@
 	soc {
 		pmx_core: pinmux@1c14120 {
 			status = "okay";
+
+			gpio_pins: pinmux_gpio_pins {
+				pinctrl-single,bits = <
+					/* GPIO2_4 GPIO2_6 */
+					0x18 0x00008080 0x0000f0f0
+					/* GPIO2_8 GPIO2_15 */
+					0x14 0x80000008 0xf000000f
+					/* GPIO3_12 GPIO3_13 */
+					0x1C 0x00008800 0x0000ff00
+					/* GPIO4_0 GPIO4_1 */
+					0x28 0x88000000 0xff000000
+					/* GPIO6_9 GPIO6_10 GPIO6_13 */
+					0x34 0x08800800 0x0ff00f00
+				>;
+			};
 		};
 		serial0: serial@1c42000 {
 			status = "okay";
@@ -101,6 +116,11 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&mii_pins>;
 		};
+		gpio: gpio@1e26000 {
+			status = "okay";
+			pinctrl-names = "default";
+			pinctrl-0 = <&gpio_pins>;
+		};
 	};
 	nand_cs3@62000000 {
 		status = "okay";
-- 
1.7.9.5

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Linus Walleij]

On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:

> This patch adds OF parser support for davinci gpio
> driver and also appropriate documentation in gpio-davinci.txt
> located at Documentation/devicetree/bindings/gpio/.
>
> Signed-off-by: KV Sujith <sujithkv@ti.com>
> Signed-off-by: Philip Avinash <avinashphilip@ti.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

^Don't trust this guy.

> [prabhakar.csengg@gmail.com: simplified the OF code and also
>                  the commit message]
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
>  .../devicetree/bindings/gpio/gpio-davinci.txt      |   34 +++++++++++
>  drivers/gpio/gpio-davinci.c                        |   60 +++++++++++++++++++-
>  2 files changed, 91 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> new file mode 100644
> index 0000000..87abd3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> @@ -0,0 +1,34 @@
> +Davinci GPIO controller bindings
> +
> +Required Properties:
> +- compatible: should be "ti,dm6441-gpio"
> +
> +- reg: Physical base address of the controller and the size of memory mapped
> +       registers.
> +
> +- gpio-controller : Marks the device node as a gpio controller.
> +
> +- interrupts: Array of GPIO interrupt number.
> +
> +- ngpio: The number of GPIO pins supported.
> +
> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering starts.

What is this?

If I have ever ACKed this I have been drunk. I take it back.

This "base" is a Linux-specific thing and has no place in the
device tree, and shall not be there. You have to find some way to
avoid this, what do you think some other OS should do with
this value...

All IRQs in Linux are assumed to be dynamically assigned numbers
nowadays, with a property like this you can never switch on
SPARSE_IRQ for the DaVinci.

Yours,
Linus Walleij

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Prabhakar Lad]

Hi Linus ,

On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
> <prabhakar.csengg@gmail.com> wrote:
>
>> This patch adds OF parser support for davinci gpio
>> driver and also appropriate documentation in gpio-davinci.txt
>> located at Documentation/devicetree/bindings/gpio/.
>>
>> Signed-off-by: KV Sujith <sujithkv@ti.com>
>> Signed-off-by: Philip Avinash <avinashphilip@ti.com>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> ^Don't trust this guy.
>
>> [prabhakar.csengg@gmail.com: simplified the OF code and also
>>                  the commit message]
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> ---
>>  .../devicetree/bindings/gpio/gpio-davinci.txt      |   34 +++++++++++
>>  drivers/gpio/gpio-davinci.c                        |   60
>> +++++++++++++++++++-
>>  2 files changed, 91 insertions(+), 3 deletions(-)
>>  create mode 100644
>> Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> new file mode 100644
>> index 0000000..87abd3b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>> @@ -0,0 +1,34 @@
>> +Davinci GPIO controller bindings
>> +
>> +Required Properties:
>> +- compatible: should be "ti,dm6441-gpio"
>> +
>> +- reg: Physical base address of the controller and the size of memory
>> mapped
>> +       registers.
>> +
>> +- gpio-controller : Marks the device node as a gpio controller.
>> +
>> +- interrupts: Array of GPIO interrupt number.
>> +
>> +- ngpio: The number of GPIO pins supported.
>> +
>> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering
>> starts.
>
> What is this?
>
> If I have ever ACKed this I have been drunk. I take it back.
>
here is the ACK https://patchwork.kernel.org/patch/2721181/

> This "base" is a Linux-specific thing and has no place in the
> device tree, and shall not be there. You have to find some way to
> avoid this, what do you think some other OS should do with
> this value...
>
> All IRQs in Linux are assumed to be dynamically assigned numbers
> nowadays, with a property like this you can never switch on
> SPARSE_IRQ for the DaVinci.
>
Can you point to any alternative solution if you have any ?

-- 
Regards,
--Prabhakar Lad

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Linus Walleij]

On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>> <prabhakar.csengg@gmail.com> wrote:

>>> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering
>>> starts.
>>
>> What is this?
>>
>> If I have ever ACKed this I have been drunk. I take it back.
>>
> here is the ACK https://patchwork.kernel.org/patch/2721181/

And as suspected that version of the patch did not contain
this strange node property.

Don't keep my ACK on patches if you change basic stuff like
that, they need to be re-acked, this runs the risk of abusing
my trust amongst other subsystem maintainers who might
go and merge this because "aha the GPIO maintainer
thinks that this is OK".

>> This "base" is a Linux-specific thing and has no place in the
>> device tree, and shall not be there. You have to find some way to
>> avoid this, what do you think some other OS should do with
>> this value...
>>
>> All IRQs in Linux are assumed to be dynamically assigned numbers
>> nowadays, with a property like this you can never switch on
>> SPARSE_IRQ for the DaVinci.
>>
> Can you point to any alternative solution if you have any ?

First convert this GPIO driver to use an irqdomain to map
HW IRQs to Linux IRQs, and grab a few IRQ descriptors
dynamically off the irq descriptor heap.
Example: commit
a6c45b99a658521291cfb66ecf035cc58b38f206
"pinctrl/coh901: use irqdomain, allocate irqdescs"

Then on a longer term convert DaVinci to use dynamically
allocated IRQs for all interrupt controllers, and move it over
to SPARSE_IRQ so you know this works.

Yours,
Linus Walleij

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Prabhakar Lad]

Hi Linus,

On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
> <prabhakar.csengg@gmail.com> wrote:
>> On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>> <prabhakar.csengg@gmail.com> wrote:
>
>>>> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering
>>>> starts.
>>>
>>> What is this?
>>>
>>> If I have ever ACKed this I have been drunk. I take it back.
>>>
>> here is the ACK https://patchwork.kernel.org/patch/2721181/
>
> And as suspected that version of the patch did not contain
> this strange node property.
>
The property did exist in the patch 'intc_irq_num', I just renamed
it and gave a proper description to it.

> Don't keep my ACK on patches if you change basic stuff like
> that, they need to be re-acked, this runs the risk of abusing
> my trust amongst other subsystem maintainers who might
> go and merge this because "aha the GPIO maintainer
> thinks that this is OK".
>
Agreed, I carry forwarded the ACK since it had minor changes.

>>> This "base" is a Linux-specific thing and has no place in the
>>> device tree, and shall not be there. You have to find some way to
>>> avoid this, what do you think some other OS should do with
>>> this value...
>>>
>>> All IRQs in Linux are assumed to be dynamically assigned numbers
>>> nowadays, with a property like this you can never switch on
>>> SPARSE_IRQ for the DaVinci.
>>>
>> Can you point to any alternative solution if you have any ?
>
> First convert this GPIO driver to use an irqdomain to map
> HW IRQs to Linux IRQs, and grab a few IRQ descriptors
> dynamically off the irq descriptor heap.
> Example: commit
> a6c45b99a658521291cfb66ecf035cc58b38f206
> "pinctrl/coh901: use irqdomain, allocate irqdescs"
>
> Then on a longer term convert DaVinci to use dynamically
> allocated IRQs for all interrupt controllers, and move it over
> to SPARSE_IRQ so you know this works.
>
Thanks for the pointers.
-- 
Regards,
--Prabhakar Lad

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Linus Walleij]

On Fri, Oct 11, 2013 at 6:18 PM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
>> <prabhakar.csengg@gmail.com> wrote:
>>> On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>>> <prabhakar.csengg@gmail.com> wrote:
>>
>>>>> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering
>>>>> starts.
>>>>
>>>> What is this?
>>>>
>>>> If I have ever ACKed this I have been drunk. I take it back.
>>>>
>>> here is the ACK https://patchwork.kernel.org/patch/2721181/
>>
>> And as suspected that version of the patch did not contain
>> this strange node property.
>>
> The property did exist in the patch 'intc_irq_num', I just renamed
> it and gave a proper description to it.

Hm yeah you're right ... I didn't understand what it was actually
doing until I saw the revised documentation, I though it was
stating the number of (hardware) IRQs, but it was stating the
Linux-internal offset.

Yours,
Linus Walleij

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Grygorii Strashko]

Hi Prabhakar Lad,

On 10/11/2013 07:18 PM, Prabhakar Lad wrote:
> Hi Linus,
> 
> On 10/11/13, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
>> <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> On 10/11/13, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>>> <prabhakar.csengg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>>>> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering
>>>>> starts.
>>>>
>>>> What is this?
>>>>
>>>> If I have ever ACKed this I have been drunk. I take it back.
>>>>
>>> here is the ACK https://patchwork.kernel.org/patch/2721181/
>>
>> And as suspected that version of the patch did not contain
>> this strange node property.
>>
> The property did exist in the patch 'intc_irq_num', I just renamed
> it and gave a proper description to it.
> 
>> Don't keep my ACK on patches if you change basic stuff like
>> that, they need to be re-acked, this runs the risk of abusing
>> my trust amongst other subsystem maintainers who might
>> go and merge this because "aha the GPIO maintainer
>> thinks that this is OK".
>>
> Agreed, I carry forwarded the ACK since it had minor changes.
> 
>>>> This "base" is a Linux-specific thing and has no place in the
>>>> device tree, and shall not be there. You have to find some way to
>>>> avoid this, what do you think some other OS should do with
>>>> this value...
>>>>
>>>> All IRQs in Linux are assumed to be dynamically assigned numbers
>>>> nowadays, with a property like this you can never switch on
>>>> SPARSE_IRQ for the DaVinci.
>>>>
>>> Can you point to any alternative solution if you have any ?
>>
>> First convert this GPIO driver to use an irqdomain to map
>> HW IRQs to Linux IRQs, and grab a few IRQ descriptors
>> dynamically off the irq descriptor heap.
>> Example: commit
>> a6c45b99a658521291cfb66ecf035cc58b38f206
>> "pinctrl/coh901: use irqdomain, allocate irqdescs"
>>
>> Then on a longer term convert DaVinci to use dynamically
>> allocated IRQs for all interrupt controllers, and move it over
>> to SPARSE_IRQ so you know this works.
>>
> Thanks for the pointers.
> 

Could it be possible to use "interrupts" and "interrupt-names" to identify
assigned banked & unbanked IRQs?

For example DM646x (http://www.ti.com/lit/ug/sprueq8a/sprueq8a.pdf):

interrupts = <48 IRQ_TYPE_EDGE_BOTH>,
             <49 IRQ_TYPE_EDGE_BOTH>,
             <50 IRQ_TYPE_EDGE_BOTH>,
             <51 IRQ_TYPE_EDGE_BOTH>,
             <52 IRQ_TYPE_EDGE_BOTH>,
             <53 IRQ_TYPE_EDGE_BOTH>,
             <54 IRQ_TYPE_EDGE_BOTH>,
             <55 IRQ_TYPE_EDGE_BOTH>,
             <56 IRQ_TYPE_EDGE_BOTH>,
             <57 IRQ_TYPE_EDGE_BOTH>,
             <58 IRQ_TYPE_EDGE_BOTH>;
interrupt-names = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", "gpio7", "bank0", "bank1", "bank2";

For example OMAP-L138 (http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf):

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>;

interrupt-names = "bank0", "bank1", "bank2", "bank3", "bank4", "bank5", "bank6", "bank7", "bank8";

For example Keystone 66AK2E05/02
 (http://www.ti.com/lit/ds/sprs865a/sprs865a.pdf and http://www.ti.com/lit/ug/sprugv1/sprugv1.pdf):

interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
             <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
[..]
             <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>,
             <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>;


interrupt-names = "gpio0", "gpio1", [...], "gpio30", "gpio31";

So then, following properties would not be needed at all, because all inf. can be
taken from interrupt's properties: 
+- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering starts.
+- ti,davinci-gpio-unbanked: The number of GPIOs that have an individual interrupt line to processor.

It should work good if Davinci-gpio driver will be converted to use
linear IRQ domains for banked irqs.


Regards,
-grygorii

Re: [PATCH v3 1/3] gpio: davinci: add OF support

[Prabhakar Lad]

Hi Grygorii,

On Mon, Oct 14, 2013 at 5:55 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> Hi Prabhakar Lad,
>
> On 10/11/2013 07:18 PM, Prabhakar Lad wrote:
>> Hi Linus,
>>
>> On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
>>> <prabhakar.csengg@gmail.com> wrote:
>>>> On 10/11/13, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>>>> <prabhakar.csengg@gmail.com> wrote:
>>>
>>>>>> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering
>>>>>> starts.
>>>>>
>>>>> What is this?
>>>>>
>>>>> If I have ever ACKed this I have been drunk. I take it back.
>>>>>
>>>> here is the ACK https://patchwork.kernel.org/patch/2721181/
>>>
>>> And as suspected that version of the patch did not contain
>>> this strange node property.
>>>
>> The property did exist in the patch 'intc_irq_num', I just renamed
>> it and gave a proper description to it.
>>
>>> Don't keep my ACK on patches if you change basic stuff like
>>> that, they need to be re-acked, this runs the risk of abusing
>>> my trust amongst other subsystem maintainers who might
>>> go and merge this because "aha the GPIO maintainer
>>> thinks that this is OK".
>>>
>> Agreed, I carry forwarded the ACK since it had minor changes.
>>
>>>>> This "base" is a Linux-specific thing and has no place in the
>>>>> device tree, and shall not be there. You have to find some way to
>>>>> avoid this, what do you think some other OS should do with
>>>>> this value...
>>>>>
>>>>> All IRQs in Linux are assumed to be dynamically assigned numbers
>>>>> nowadays, with a property like this you can never switch on
>>>>> SPARSE_IRQ for the DaVinci.
>>>>>
>>>> Can you point to any alternative solution if you have any ?
>>>
>>> First convert this GPIO driver to use an irqdomain to map
>>> HW IRQs to Linux IRQs, and grab a few IRQ descriptors
>>> dynamically off the irq descriptor heap.
>>> Example: commit
>>> a6c45b99a658521291cfb66ecf035cc58b38f206
>>> "pinctrl/coh901: use irqdomain, allocate irqdescs"
>>>
>>> Then on a longer term convert DaVinci to use dynamically
>>> allocated IRQs for all interrupt controllers, and move it over
>>> to SPARSE_IRQ so you know this works.
>>>
>> Thanks for the pointers.
>>
>
> Could it be possible to use "interrupts" and "interrupt-names" to identify
> assigned banked & unbanked IRQs?
>
I did give thought for it but found out its taken as a single interrupt,
and comparing the names in the driver would be tedious. so as
of now I have kept as is for this.

> For example DM646x (http://www.ti.com/lit/ug/sprueq8a/sprueq8a.pdf):
>
> interrupts = <48 IRQ_TYPE_EDGE_BOTH>,
>              <49 IRQ_TYPE_EDGE_BOTH>,
>              <50 IRQ_TYPE_EDGE_BOTH>,
>              <51 IRQ_TYPE_EDGE_BOTH>,
>              <52 IRQ_TYPE_EDGE_BOTH>,
>              <53 IRQ_TYPE_EDGE_BOTH>,
>              <54 IRQ_TYPE_EDGE_BOTH>,
>              <55 IRQ_TYPE_EDGE_BOTH>,
>              <56 IRQ_TYPE_EDGE_BOTH>,
>              <57 IRQ_TYPE_EDGE_BOTH>,
>              <58 IRQ_TYPE_EDGE_BOTH>;
> interrupt-names = "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6", "gpio7", "bank0", "bank1", "bank2";
>
> For example OMAP-L138 (http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf):
>
> 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>;
>
> interrupt-names = "bank0", "bank1", "bank2", "bank3", "bank4", "bank5", "bank6", "bank7", "bank8";
>
> For example Keystone 66AK2E05/02
>  (http://www.ti.com/lit/ds/sprs865a/sprs865a.pdf and http://www.ti.com/lit/ug/sprugv1/sprugv1.pdf):
>
> interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
>              <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>,
> [..]
>              <GIC_SPI 150 IRQ_TYPE_LEVEL_HIGH>,
>              <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>;
>
>
> interrupt-names = "gpio0", "gpio1", [...], "gpio30", "gpio31";
>
> So then, following properties would not be needed at all, because all inf. can be
> taken from interrupt's properties:
> +- ti,davinci-gpio-irq-base: Base from where GPIO interrupt numbering starts.
Yeah dropped this.

> +- ti,davinci-gpio-unbanked: The number of GPIOs that have an individual interrupt line to processor.
>
Kept as is
> It should work good if Davinci-gpio driver will be converted to use
> linear IRQ domains for banked irqs.
>
Yeah migrated it to use IRQ domains.

I have posted the updated patch series at [1], please have a look at it.

[1] http://linux.omap.com/pipermail/davinci-linux-open-source/2013-November/028220.html

Regards,
--Prabhakar Lad