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

[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 at 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

[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 at 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

[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

[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

[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

[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@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?

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

[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