Message-ID: <20160906162807.GA1041@n2100.armlinux.org.uk>
Date:   Tue, 6 Sep 2016 17:28:08 +0100
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Hans-Christian Egtvedt <egtvedt@...fundet.no>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mfd: ucb1x00: remove NO_IRQ check

On Tue, Sep 06, 2016 at 04:45:30PM +0100, Lee Jones wrote:
> On Tue, 06 Sep 2016, Russell King - ARM Linux wrote:
> 
> > You need to send this _to_ me as I need to merge it with my other
> > changes.  This patch on its own does not make sense - it only makes
> > sense with the rest of my SA11x0 patch stack.
> > 
> > NAK for Lee to merge this.
> 
> So if I were to accept this patch, would anything break?  In other
> words, is there an ordering issue where this this change relies on
> something you have in your tree?

1) This is my driver which I've maintained in the past myself, including
   handling all patches for.  This pre-dates your decision to take over
   the mfd stuff.  I'm still maintaining this driver and I have not
   passed the responsibility to you.

2) If you review the driver and consider the effect of the change (which,
   as you don't know the driver, you should have done before replying if
   you're wanting to claim to be responsible for it) you would have
   noticed that the patch on its own subsitutes one form of brokenness
   with a different form of brokenness - ucb1x00_detect_irq() can return
   NO_IRQ.  So checking for !ucb->irq breaks when ucb1x00_detect_irq()
   does so.

3) I already have a patch which removes that NO_IRQ usage, so the only
   way that _this_ patch makes sense is when combined with my patch.

4) I find it annoying that you've picked up on this patch in the way
   that you have (as a result of my NAK), yet you have failed to make
   any comment what so ever on _my_ patch, which you were copied with
   on the 30th August.

Moreover, I reserve the right to keep it in my tree as the driver author,
possibly publishing it in linux-next.  I reserve the right to review
patches to my driver.  I reserve the right to NAK patches to my driver
for whatever reason I deem appropriate.

This is part of the problem I have here with your attitude with mfd:
you decided yourself to become maintainer of everything in drivers/mfd,
riding rough-shod over your fellow maintainers.  And when your fellow
maintainers try to reassert control, you get upset about it.  Sorry,
you can't have it both ways.

Now, if you had discussed my patch from the 30th _first_ then I would
not be NAKing this patch, and I probably would not be making such a
big deal about this.  But let's put that aside and stick to the
technicalities.

There are two dependencies here.  Right now, the probe_irq_on()
returning zero on SA11x0 platforms, causing ucb1x00_detect_irq() to
return NO_IRQ, which then causes the probe function to fail.  Whether
that happens on PXA or not, I don't know and I have no way to test
anymore as I donated all my PXA platforms to Robert.

Applying Arnd's patch on its own, or applying my patch on its own will
cause ucb1x00 to initialise with either NO_IRQ or 0 in ucb->irq, which
causes the device to have no interrupts - or worse, it screws up any
configuration of IRQ0 that the platform may have (eg, PXA).  Applying
both together results in the probe function continuing to fail.

My patch is not supposed to be applied on its own; it is supposed to
be applied with the third patch already in place - a GPIO patch.  So
yes, there _were_ dependencies here.

That dependency can be solved by taking _both_ my patch (first) and
Arnd's patch.  However, given that each patch introduces its own
different form of breakage, it makes sense to combine the two patches
to avoid that breakage.  So, when I'm less busy sorting out other
SA11x0 stuff, I want to discuss with Arnd about combining them into
a single patch.

_Then_ we need to have a discussion about how to handle the patch.

So, it's not as simple as you can see, because you don't have the
knowledge to make the decisions for this driver.

I hope this is clear enough.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Powered by blists - more mailing lists