Re: [PATCH] mfd: ucb1x00: remove NO_IRQ check

[Lee Jones]

On Tue, 06 Sep 2016, Russell King - ARM Linux wrote:

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

There is no mention of you maintaining this driver in MAINTAINERS.
This is the first I've heard of it.  You haven't taken patches for
this driver since January 2012 (4 years, 8 months).  Over that period
I have accepted 9 patches and conducted more reviews and you haven't
taken part or shown any interest whatsoever.

The *only* logical reason you're making such a fuss now is because of
the discussion we had last week.  This behaviour is unacceptable.

> 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

This is why I asked the question.  I doubt any subsystem Maintainer
knows the intricacies of every driver they maintain.  We rely on
original driver writers and other experts (e.g. assigned vendor
engineers and the like) for guidance on the specifics.

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

Great review comment.  Thanks.

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

No problem.  I can do that for you (see below).

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

I'd seriously consider checking your mail filters if I were you (the
reply was even addressed To: you, just how you like it.  It took me
exactly an hour from you sending the patch to me reviewing and
accepting it:

  http://www.spinics.net/lists/arm-kernel/msg527414.html

It's even in -next, if you'd cared enough to look:

  http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=159aed02b0074bac1c21544befb773dce39b9fcb
 
> 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.

I fully support all of those rights.  I find keeping it in -next a bit
odd.  Especially since I already put in there for you, but I guess, so
long as you don't try to submit it to Linus which might likely cause a
merge-conflict, it's okay.

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

Not sure what you're trying to say here to be honest.  I offered my
help to Sam, the then MFD Maintainer [0], which was subsequently
accepted.  He then became busy with NFC, so I took the driver's seat.

  [0] http://www.spinics.net/lists/kernel/msg1525957.html

I try to Maintain MFD in a simple, effective manner, where my
priorities are letting good code easily pass through, keeping bad code
out and most relevant here, trying to prevent merge-conflicts so Linus
has an easy time during the merge-window.

It doesn't make sense for drivers which reside in the same subsystem
to be applied to multiple trees.  I like for all changes to MFD to be
reflected in the MFD pull-request.  If they *need* to go in via other
repos *as well*, that's fine too.  I am usually very happy to provide
pull-requests when those needs arise.

If you are insistent on applying this yourself, so long as you have a
*technical* (rather than territorial/spiteful) reason for doing so, I,
as always, will be happy to oblige you with a pull-request too.

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

I think this *rant* of yours might be based on very shakey foundations
then, because I *did* reply to your patch.  From what I saw, it was a
reasonable patch, so I took it.

Maybe if you would have known the driver as well as you profess to,
you would have fixed the issue properly and there would be no need for
this patch in the first place.  I tease of course!  ;)

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

Good explanation.  Just the sort of information I'd expect during a
patch review.  Much more helpful than "this patch doesn't make
sense.  Give it to me.  NACK".  Now I can work with you both to
resolve this issue properly and professionally.

So, since I already have half of the resolution, all I need to do is
squash this into it, right?  Or do we need to carry this GPIO patch
too?  If so, where is that currently?

The only issue I envisage is that is someone has to loose patch
authorship.

I propose to squash this patch into Russell's, keep both SoBs and
give Arnd written creds in the commit message.

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

So, long as I'm supplied with the facts I'm fine (continuing to)
making decisions, thanks.

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