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

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

[Guenter Roeck]

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

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

[Lee Jones]

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

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

[Thomas Gleixner]

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

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

[Lee Jones]

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

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

[Guenter Roeck]

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

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

[Lee Jones]

On Wed, 26 Oct 2016, Guenter Roeck wrote:

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

Final call; am I taking this patch or not?

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

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