From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756990AbcIGKZt (ORCPT ); Wed, 7 Sep 2016 06:25:49 -0400 Received: from mail-wm0-f43.google.com ([74.125.82.43]:38195 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756391AbcIGKZr (ORCPT ); Wed, 7 Sep 2016 06:25:47 -0400 Date: Wed, 7 Sep 2016 11:27:37 +0100 From: Lee Jones To: Russell King - ARM Linux Cc: Arnd Bergmann , Linus Walleij , Hans-Christian Egtvedt , Dmitry Torokhov , Thomas Gleixner , linux-kernel@vger.kernel.org Subject: Re: [PATCH] mfd: ucb1x00: remove NO_IRQ check Message-ID: <20160907102737.GX4921@dell> References: <20160906130409.1691750-1-arnd@arndb.de> <20160906131730.GV1041@n2100.armlinux.org.uk> <20160906154530.GW4921@dell> <20160906162807.GA1041@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160906162807.GA1041@n2100.armlinux.org.uk> User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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