On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
<[email protected]> 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 <[email protected]>
> Signed-off-by: Philip Avinash <[email protected]>
> Acked-by: Linus Walleij <[email protected]>
^Don't trust this guy.
> [[email protected]: simplified the OF code and also
> the commit message]
> Signed-off-by: Lad, Prabhakar <[email protected]>
> ---
> .../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
Hi Linus ,
On 10/11/13, Linus Walleij <[email protected]> wrote:
> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
> <[email protected]> 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 <[email protected]>
>> Signed-off-by: Philip Avinash <[email protected]>
>> Acked-by: Linus Walleij <[email protected]>
>
> ^Don't trust this guy.
>
>> [[email protected]: simplified the OF code and also
>> the commit message]
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> ---
>> .../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
On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
<[email protected]> wrote:
> On 10/11/13, Linus Walleij <[email protected]> wrote:
>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>> <[email protected]> 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
Hi Linus,
On 10/11/13, Linus Walleij <[email protected]> wrote:
> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
> <[email protected]> wrote:
>> On 10/11/13, Linus Walleij <[email protected]> wrote:
>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>> <[email protected]> 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
On Fri, Oct 11, 2013 at 6:18 PM, Prabhakar Lad
<[email protected]> wrote:
> On 10/11/13, Linus Walleij <[email protected]> wrote:
>> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
>> <[email protected]> wrote:
>>> On 10/11/13, Linus Walleij <[email protected]> wrote:
>>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>>> <[email protected]> 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
Hi Prabhakar Lad,
On 10/11/2013 07:18 PM, Prabhakar Lad wrote:
> Hi Linus,
>
> On 10/11/13, Linus Walleij <[email protected]> wrote:
>> On Fri, Oct 11, 2013 at 4:59 PM, Prabhakar Lad
>> <[email protected]> wrote:
>>> On 10/11/13, Linus Walleij <[email protected]> wrote:
>>>> On Fri, Oct 4, 2013 at 6:03 PM, Prabhakar Lad
>>>> <[email protected]> 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