diff mbox series

[1/3] ARM: davinci: define gpio interrupts as separate resources

Message ID 20181113135051.15308-2-brgl@bgdev.pl (mailing list archive)
State New, archived
Headers show
Series ARM: davinci: fix ethernet support on da850-evm | expand

Commit Message

Bartosz Golaszewski Nov. 13, 2018, 1:50 p.m. UTC 
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Since commit eb3744a2dd01 ("gpio: davinci: Do not assume continuous
IRQ numbering") the davinci GPIO driver fails to probe if we boot
in legacy mode from any of the board files. Since the driver now
expects every interrupt to be defined as a separate resource, split
the definition in devices-da8xx.c instead of having a single continuous
interrupt range.

Fixes: eb3744a2dd01 ("gpio: davinci: Do not assume continuous IRQ numbering")
Cc: stable@vger.kernel.org
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/devices-da8xx.c | 42 ++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

Sekhar Nori Nov. 19, 2018, 8:52 p.m. UTC | #1 
On 13/11/18 7:20 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Since commit eb3744a2dd01 ("gpio: davinci: Do not assume continuous
> IRQ numbering") the davinci GPIO driver fails to probe if we boot
> in legacy mode from any of the board files. Since the driver now
> expects every interrupt to be defined as a separate resource, split
> the definition in devices-da8xx.c instead of having a single continuous
> interrupt range.
> 
> Fixes: eb3744a2dd01 ("gpio: davinci: Do not assume continuous IRQ numbering")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

There are a number of other boards that need such fixing too. And the
commit in question does not do a good job of explaining why it was
needed in the first place. The description  just repeats what can be
inferred by reading the patch.


    gpio: davinci: Do not assume continuous IRQ numbering

    Currently the driver assumes that the interrupts are continuous
    and does platform_get_irq only once and assumes the rest are continuous,
    instead call platform_get_irq for all the interrupts and store them
    in an array for later use.

    Signed-off-by: Keerthy <j-keerthy@ti.com>
    Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Can we revert the offending commit instead?

Thanks,
Sekhar
J, KEERTHY Nov. 20, 2018, 6:38 a.m. UTC | #2 
On 11/20/2018 2:22 AM, Sekhar Nori wrote:
> On 13/11/18 7:20 PM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Since commit eb3744a2dd01 ("gpio: davinci: Do not assume continuous
>> IRQ numbering") the davinci GPIO driver fails to probe if we boot
>> in legacy mode from any of the board files. Since the driver now
>> expects every interrupt to be defined as a separate resource, split
>> the definition in devices-da8xx.c instead of having a single continuous
>> interrupt range.
>>
>> Fixes: eb3744a2dd01 ("gpio: davinci: Do not assume continuous IRQ numbering")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> There are a number of other boards that need such fixing too. And the
> commit in question does not do a good job of explaining why it was
> needed in the first place. The description  just repeats what can be
> inferred by reading the patch.

Cc Lokesh

Sekhar,

DT explicitly mentions every IRQ number. The patch in discussion
explicitly calls platform_get_irq for all the interrupts which to me is 
the right thing to do as: platform_get_irq--> 
of_irq_get-->irq_create_of_mapping--> sequence is to be done for every IRQ.

k3-am654 definitely will need explicit calls to platform_get_irq as it 
will be involving interrupt router and interrupt numbers need not be 
continuous.

So i do not think reverting the patch is the right idea.

Regards,
Keerthy

> 
> 
>      gpio: davinci: Do not assume continuous IRQ numbering
> 
>      Currently the driver assumes that the interrupts are continuous
>      and does platform_get_irq only once and assumes the rest are continuous,
>      instead call platform_get_irq for all the interrupts and store them
>      in an array for later use.
> 
>      Signed-off-by: Keerthy <j-keerthy@ti.com>
>      Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
>      Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> 
> Can we revert the offending commit instead?
> 
> Thanks,
> Sekhar
>
Sekhar Nori Nov. 20, 2018, 11:06 p.m. UTC | #3 
On 20/11/18 12:08 PM, J, KEERTHY wrote:
> 
> 
> On 11/20/2018 2:22 AM, Sekhar Nori wrote:
>> On 13/11/18 7:20 PM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> Since commit eb3744a2dd01 ("gpio: davinci: Do not assume continuous
>>> IRQ numbering") the davinci GPIO driver fails to probe if we boot
>>> in legacy mode from any of the board files. Since the driver now
>>> expects every interrupt to be defined as a separate resource, split
>>> the definition in devices-da8xx.c instead of having a single continuous
>>> interrupt range.
>>>
>>> Fixes: eb3744a2dd01 ("gpio: davinci: Do not assume continuous IRQ
>>> numbering")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> There are a number of other boards that need such fixing too. And the
>> commit in question does not do a good job of explaining why it was
>> needed in the first place. The description  just repeats what can be
>> inferred by reading the patch.
> 
> Cc Lokesh
> 
> Sekhar,
> 
> DT explicitly mentions every IRQ number. The patch in discussion
> explicitly calls platform_get_irq for all the interrupts which to me is
> the right thing to do as: platform_get_irq-->
> of_irq_get-->irq_create_of_mapping--> sequence is to be done for every IRQ.
> 
> k3-am654 definitely will need explicit calls to platform_get_irq as it
> will be involving interrupt router and interrupt numbers need not be
> continuous.
> 
> So i do not think reverting the patch is the right idea.

Well, all of this description of patch motivation should have been in
the patch description to begin with.

Bartosz, can you please extend this patch to fix this problem for other
DaVinci SoCs too? I am on the road this week, but will do my best to
queue these fixes at the earliest .

Thanks,
Sekhar
Bartosz Golaszewski Nov. 21, 2018, 8:51 a.m. UTC | #4 
śr., 21 lis 2018 o 00:07 Sekhar Nori <nsekhar@ti.com> napisał(a):
>
> On 20/11/18 12:08 PM, J, KEERTHY wrote:
> >
> >
> > On 11/20/2018 2:22 AM, Sekhar Nori wrote:
> >> On 13/11/18 7:20 PM, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>
> >>> Since commit eb3744a2dd01 ("gpio: davinci: Do not assume continuous
> >>> IRQ numbering") the davinci GPIO driver fails to probe if we boot
> >>> in legacy mode from any of the board files. Since the driver now
> >>> expects every interrupt to be defined as a separate resource, split
> >>> the definition in devices-da8xx.c instead of having a single continuous
> >>> interrupt range.
> >>>
> >>> Fixes: eb3744a2dd01 ("gpio: davinci: Do not assume continuous IRQ
> >>> numbering")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>
> >> There are a number of other boards that need such fixing too. And the
> >> commit in question does not do a good job of explaining why it was
> >> needed in the first place. The description  just repeats what can be
> >> inferred by reading the patch.
> >
> > Cc Lokesh
> >
> > Sekhar,
> >
> > DT explicitly mentions every IRQ number. The patch in discussion
> > explicitly calls platform_get_irq for all the interrupts which to me is
> > the right thing to do as: platform_get_irq-->
> > of_irq_get-->irq_create_of_mapping--> sequence is to be done for every IRQ.
> >
> > k3-am654 definitely will need explicit calls to platform_get_irq as it
> > will be involving interrupt router and interrupt numbers need not be
> > continuous.
> >
> > So i do not think reverting the patch is the right idea.
>
> Well, all of this description of patch motivation should have been in
> the patch description to begin with.
>
> Bartosz, can you please extend this patch to fix this problem for other
> DaVinci SoCs too? I am on the road this week, but will do my best to
> queue these fixes at the earliest .
>
> Thanks,
> Sekhar

Ok, to make it easier for you, I'll resend all the patches addressing
the GPIO issue.

Bart
diff mbox series

Patch

diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index 1fd3619f6a09..8c4ae9866e3c 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -701,9 +701,49 @@  static struct resource da8xx_gpio_resources[] = {
 	},
 	{ /* interrupt */
 		.start	= IRQ_DA8XX_GPIO0,
-		.end	= IRQ_DA8XX_GPIO8,
+		.end	= IRQ_DA8XX_GPIO0,
 		.flags	= IORESOURCE_IRQ,
 	},
+	{
+		.start	= IRQ_DA8XX_GPIO1,
+		.end	= IRQ_DA8XX_GPIO1,
+		.flags	= IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO2,
+		.end    = IRQ_DA8XX_GPIO2,
+		.flags  = IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO3,
+		.end    = IRQ_DA8XX_GPIO3,
+		.flags  = IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO4,
+		.end    = IRQ_DA8XX_GPIO4,
+		.flags  = IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO5,
+		.end    = IRQ_DA8XX_GPIO5,
+		.flags  = IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO6,
+		.end    = IRQ_DA8XX_GPIO6,
+		.flags  = IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO7,
+		.end    = IRQ_DA8XX_GPIO7,
+		.flags  = IORESOURCE_IRQ,
+	},
+	{
+		.start  = IRQ_DA8XX_GPIO8,
+		.end    = IRQ_DA8XX_GPIO8,
+		.flags  = IORESOURCE_IRQ,
+	}
 };
 
 static struct platform_device da8xx_gpio_device = {