From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754943AbcGEPSr (ORCPT ); Tue, 5 Jul 2016 11:18:47 -0400 Received: from smtpoutz26.laposte.net ([194.117.213.101]:41158 "EHLO smtp.laposte.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752088AbcGEPSq (ORCPT ); Tue, 5 Jul 2016 11:18:46 -0400 Subject: Re: [RFC PATCH v1] irqchip: add support for SMP irq router To: Jason Cooper References: <577542D1.4070307@laposte.net> <577A5260.3070001@free.fr> <577BA854.6090503@laposte.net> <20160705144151.GE3348@io.lakedaemon.net> Cc: Mason , LKML , Thomas Gleixner , Marc Zyngier From: Sebastian Frias Message-ID: <577BCFD2.8060203@laposte.net> Date: Tue, 5 Jul 2016 17:18:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160705144151.GE3348@io.lakedaemon.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-VR-SrcIP: 83.142.147.193 X-VR-FullState: 0 X-VR-Score: -100 X-VR-Cause-1: gggruggvucftvghtrhhoucdtuddrfeeltddrvdeggdekiecutefuodetggdotefrodftvfcurfhrohhf X-VR-Cause-2: ihhlvgemucfntefrqffuvffgnecuuegrihhlohhuthemucehtddtnecusecvtfgvtghiphhivghnthhs X-VR-Cause-3: ucdlqddutddtmdenucfjughrpefuvfhfhffkffgfgggjtgfgsehtjegrtddtfeehnecuhfhrohhmpefu X-VR-Cause-4: vggsrghsthhirghnucfhrhhirghsuceoshhfkeegsehlrghpohhsthgvrdhnvghtqeenucfkphepkeef X-VR-Cause-5: rddugedvrddugeejrdduleefnecurfgrrhgrmhepmhhouggvpehsmhhtphhouhhtpdhhvghloheplgdu X-VR-Cause-6: jedvrddvjedrtddrvddugegnpdhinhgvthepkeefrddugedvrddugeejrdduleefpdhmrghilhhfrhho X-VR-Cause-7: mhepshhfkeegsehlrghpohhsthgvrdhnvghtpdhrtghpthhtohepjhgrshhonheslhgrkhgvuggrvghm X-VR-Cause-8: ohhnrdhnvght X-VR-AvState: No X-VR-State: 0 X-VR-State: 0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jason, On 07/05/2016 04:41 PM, Jason Cooper wrote: > Hey Sebastian, Mason, > > * Please fix mailer to wrap text at a sane length. I've re-wrapped and > trimmed. > > On Tue, Jul 05, 2016 at 02:30:12PM +0200, Sebastian Frias wrote: >> On 07/04/2016 02:11 PM, Mason wrote: > ... >>>> .../sigma,smp87xx-irqrouter.txt | 69 +++ >>> >>> In the *actual* submission, we can't use a wildcard like smp87xx >>> we'll have to use an actual part number. >> >> Are you sure? >> That would hinder genericity. >> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt"). > > sigma,smp-irqrouter.txt should be fine. The devicetree maintainers > should yelp if they want something different. > >> To me there's no need to link the compatible string of a given HW >> module with that of the chip name the module it is embedded into. For >> example, the generic USB3 driver is "generic-xhci". While this module >> is not generic to be embedded in chips from different manufacturers, >> it is supposed to be generic within Sigma, and multiple Sigma chips >> (with potentially different denominations) can use it. >> >>> >>>> drivers/irqchip/Makefile | 1 + >>>> drivers/irqchip/irq-tango_v2.c | 594 +++++++++++++++++++++ >>> >>> Likewise, I don't like the "_v2" suffix, it's too generic. >>> Actual submission should use something more specific. >> >> Well, the other driver is irq-tango.c that is generic as well. >> I prefer versioning, as it is unrelated with the actual chip name. > > Is there a name, similar to 'tango', for this version of the IP? > Something that would spark recognition for someone looking for "the damn > driver for this XYZ irqchip I have". If not, irq-tango_v2.c is fine. > Thanks for your comments. So, aside from some naming issues, do you think the driver is ok? Indeed, I'm not sure about the following: 1) I had to unroll irq_of_parse_and_map() in order to get the HW IRQ declared in the device tree so that I can associate it with a given domain. Is there another way? 2) I'm not sure about the DT specification, in particular, the use of child nodes to declare different domains, but it works. Advise and/or hints regarding this are welcome. 3) I'm calling this an irq router to somehow highlight the fact that it is not a simple interrupt controller. Indeed it does not latch the IRQ lines by itself, and does not supports edge detection. Does anybody else has a better idea for the name? 4) Do I have to do something more to handle the affinity stuff? 5) checkpatch.pl reports warnings, etc. but I guess it's ok for now since it is a RFC 6) Do we need a lock in the cascade_irq callback set with irq_set_chained_handler_and_data() ? Thanks in advance. Best regards, Sebastian