Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1501195 > unrolled thread

[PATCH] mfd: tps65217: Drop call to irq_set_parent()

Back to article view | Back to linux.kernel

Contents

  [PATCH] mfd: tps65217: Drop call to irq_set_parent() Guenter Roeck <linux@roeck-us.net> - 2016-10-14 23:30 +0200
    Re: [PATCH] mfd: tps65217: Drop call to irq_set_parent() Thomas Gleixner <tglx@linutronix.de> - 2016-10-26 14:50 +0200
      Re: [PATCH] mfd: tps65217: Drop call to irq_set_parent() Lee Jones <lee.jones@linaro.org> - 2016-10-26 15:00 +0200
        Re: [PATCH] mfd: tps65217: Drop call to irq_set_parent() Guenter Roeck <linux@roeck-us.net> - 2016-10-26 15:30 +0200
    Re: [PATCH] mfd: tps65217: Drop call to irq_set_parent() Lee Jones <lee.jones@linaro.org> - 2016-10-26 15:10 +0200

#1501195 — [PATCH] mfd: tps65217: Drop call to irq_set_parent()

The call to irq_set_parent() causes the following build error if tps65217
is built as module.

ERROR: ".irq_set_parent" [drivers/mfd/tps65217.ko] undefined!

The problem was introduced with commit 6556bdacf646f ("mfd: tps65217: Add
support for IRQs").

The author states: "I have added irq_set_parent() similarly as in
drivers/base/regmap/regmap-irq.c. But to be honest I am not sure what it
really does in case of tps65217."

So let's drop it.

Fixes: 6556bdacf646f ("mfd: tps65217: Add support for IRQs")
Cc: Marcin Niestroj <m.niestroj@grinn-global.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/mfd/tps65217.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
index 9a4d8684dd32..2a57b444859c 100644
--- a/drivers/mfd/tps65217.c
+++ b/drivers/mfd/tps65217.c
@@ -170,7 +170,6 @@ static int tps65217_irq_map(struct irq_domain *h, unsigned int virq,
 	irq_set_chip_data(virq, tps);
 	irq_set_chip_and_handler(virq, &tps65217_irq_chip, handle_edge_irq);
 	irq_set_nested_thread(virq, 1);
-	irq_set_parent(virq, tps->irq);
 	irq_set_noprobe(virq);
 
 	return 0;
-- 
2.5.0

[toc] | [next] | [standalone]

#1509276

On Wed, 26 Oct 2016, Lee Jones wrote:
> On Fri, 14 Oct 2016, Guenter Roeck wrote:
> 
> > The call to irq_set_parent() causes the following build error if tps65217
> > is built as module.
> > 
> > ERROR: ".irq_set_parent" [drivers/mfd/tps65217.ko] undefined!
> > 
> > The problem was introduced with commit 6556bdacf646f ("mfd: tps65217: Add
> > support for IRQs").
> > 
> > The author states: "I have added irq_set_parent() similarly as in
> > drivers/base/regmap/regmap-irq.c. But to be honest I am not sure what it
> > really does in case of tps65217."
> > 
> > So let's drop it.
> > 
> > Fixes: 6556bdacf646f ("mfd: tps65217: Add support for IRQs")
> > Cc: Marcin Niestroj <m.niestroj@grinn-global.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/mfd/tps65217.c | 1 -
> >  1 file changed, 1 deletion(-)
> 
> This has been fixed now.

It was not fixed. The export was a work around as everyone was bitching
about the build robots failing forever.
 
So if the irq_set_parent() call is not required for functionality of the
driver then it should not be there in the first place.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1509300

On Wed, 26 Oct 2016, Thomas Gleixner wrote:

> On Wed, 26 Oct 2016, Lee Jones wrote:
> > On Fri, 14 Oct 2016, Guenter Roeck wrote:
> > 
> > > The call to irq_set_parent() causes the following build error if tps65217
> > > is built as module.
> > > 
> > > ERROR: ".irq_set_parent" [drivers/mfd/tps65217.ko] undefined!
> > > 
> > > The problem was introduced with commit 6556bdacf646f ("mfd: tps65217: Add
> > > support for IRQs").
> > > 
> > > The author states: "I have added irq_set_parent() similarly as in
> > > drivers/base/regmap/regmap-irq.c. But to be honest I am not sure what it
> > > really does in case of tps65217."
> > > 
> > > So let's drop it.
> > > 
> > > Fixes: 6556bdacf646f ("mfd: tps65217: Add support for IRQs")
> > > Cc: Marcin Niestroj <m.niestroj@grinn-global.com>
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  drivers/mfd/tps65217.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > 
> > This has been fixed now.
> 
> It was not fixed. The export was a work around as everyone was bitching
> about the build robots failing forever.
>  
> So if the irq_set_parent() call is not required for functionality of the
> driver then it should not be there in the first place.

Ah, I thought this was just another one of the many hacks I received
in response to the auto-builder's complains.  I've just been NACKing
them out of habit.

However, if is it true that we can simply just remove the call to
irq_set_parent(), then I'm happy to apply the patch.

Even happier with an Ack from you!

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1509396

On 10/26/2016 05:58 AM, Lee Jones wrote:
> On Wed, 26 Oct 2016, Thomas Gleixner wrote:
>
>> On Wed, 26 Oct 2016, Lee Jones wrote:
>>> On Fri, 14 Oct 2016, Guenter Roeck wrote:
>>>
>>>> The call to irq_set_parent() causes the following build error if tps65217
>>>> is built as module.
>>>>
>>>> ERROR: ".irq_set_parent" [drivers/mfd/tps65217.ko] undefined!
>>>>
>>>> The problem was introduced with commit 6556bdacf646f ("mfd: tps65217: Add
>>>> support for IRQs").
>>>>
>>>> The author states: "I have added irq_set_parent() similarly as in
>>>> drivers/base/regmap/regmap-irq.c. But to be honest I am not sure what it
>>>> really does in case of tps65217."
>>>>
>>>> So let's drop it.
>>>>
>>>> Fixes: 6556bdacf646f ("mfd: tps65217: Add support for IRQs")
>>>> Cc: Marcin Niestroj <m.niestroj@grinn-global.com>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>  drivers/mfd/tps65217.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>
>>> This has been fixed now.
>>
>> It was not fixed. The export was a work around as everyone was bitching
>> about the build robots failing forever.
>>
>> So if the irq_set_parent() call is not required for functionality of the
>> driver then it should not be there in the first place.
>
> Ah, I thought this was just another one of the many hacks I received
> in response to the auto-builder's complains.  I've just been NACKing
> them out of habit.
>

Well, it was, in a way. However, with the driver author being silent,
and with irq_set_parent() not that well documented, I considered it
a better solution than blindly exporting the function.

Having said that, I do suspect that its use might possibly be warranted
in this case, since the driver uses edge triggered interrupts and calls
handle_nested_irq(). But then many other drivers do the same and don't
call irq_set_parent(), so who knows. The use case for irq_set_parent()
isn't exactly well explained.

FWIW, since everyone seems to be bitching about auto-builders: You may not
care, but problems like this end up hiding other problems, can make
bisecting a pain, and can end up costing a lot of time in the future.
I have worked for companies where the common attitude was "who cares about
any builds but ours". Sounds great, until one needs to enable one more
configuration option and everything falls apart.

If you don't care about a driver being buildable as module, make it boolean.
Please.

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1509323

On Fri, 14 Oct 2016, Guenter Roeck wrote:

> The call to irq_set_parent() causes the following build error if tps65217
> is built as module.
> 
> ERROR: ".irq_set_parent" [drivers/mfd/tps65217.ko] undefined!
> 
> The problem was introduced with commit 6556bdacf646f ("mfd: tps65217: Add
> support for IRQs").
> 
> The author states: "I have added irq_set_parent() similarly as in
> drivers/base/regmap/regmap-irq.c. But to be honest I am not sure what it
> really does in case of tps65217."
> 
> So let's drop it.
> 
> Fixes: 6556bdacf646f ("mfd: tps65217: Add support for IRQs")
> Cc: Marcin Niestroj <m.niestroj@grinn-global.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/mfd/tps65217.c | 1 -
>  1 file changed, 1 deletion(-)

This has been fixed now.

3118dac501bc ("kernel/irq: Export irq_set_parent()")

> diff --git a/drivers/mfd/tps65217.c b/drivers/mfd/tps65217.c
> index 9a4d8684dd32..2a57b444859c 100644
> --- a/drivers/mfd/tps65217.c
> +++ b/drivers/mfd/tps65217.c
> @@ -170,7 +170,6 @@ static int tps65217_irq_map(struct irq_domain *h, unsigned int virq,
>  	irq_set_chip_data(virq, tps);
>  	irq_set_chip_and_handler(virq, &tps65217_irq_chip, handle_edge_irq);
>  	irq_set_nested_thread(virq, 1);
> -	irq_set_parent(virq, tps->irq);
>  	irq_set_noprobe(virq);
>  
>  	return 0;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web