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@xxxxxxxxxxxxxxxx>
> > > > > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > > > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > > > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > > > > ---
> > > > >  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