From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko =?iso-8859-1?q?St=FCbner?= Subject: Re: [PATCH v5 7/7] irqchip: s3c24xx: add devicetree support Date: Tue, 26 Mar 2013 08:38:42 +0100 Message-ID: <201303260838.42804.heiko@sntech.de> References: <201303252226.21402.heiko@sntech.de> <201303252234.21314.heiko@sntech.de> <201303252200.46258.arnd@arndb.de> Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from gloria.sntech.de ([95.129.55.99]:52099 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752825Ab3CZHin convert rfc822-to-8bit (ORCPT ); Tue, 26 Mar 2013 03:38:43 -0400 In-Reply-To: <201303252200.46258.arnd@arndb.de> Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: Arnd Bergmann Cc: Kukjin Kim , Grant Likely , Rob Herring , Thomas Abraham , devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org Hi Arnd, thanks again for taking the time to look at the changes. Am Montag, 25. M=E4rz 2013, 23:00:46 schrieb Arnd Bergmann: > On Monday 25 March 2013, Heiko St=FCbner wrote: > > Add the necessary code to initialize the interrupt controller > > thru devicetree data using the irqchip infrastructure. > >=20 > > Signed-off-by: Heiko Stuebner >=20 > The binding looks fine now. I have a few detail comments but am happy > with the series otherwise. >=20 > > +Required properties: > > +- compatible: Compatible property value should be "samsung,s3c24xx= -irq" > > + for non s3c2416 machines and "samsung,s3c2416-irq" for s3c2416 > > machines >=20 > We try to avoid wildcards in the "compatible" properties. Better use > the name of the first SoC that had this controller, and let the other > ones mark themselves as compatible with that one. >=20 > I guess "samsung,s3c2410-irq" would be the right identifier here. ok, sounds sensible > > +- #interrupt-cells : Specifies the number of cells needed to encod= e an > > + interrupt source. The value shall be 4 and interrupt descriptor = shall > > + have the following format: > > + > > + > > + ctrl_num contains the controller to use: > > + - 0 ... main controller > > + - 1 ... sub controller > > + - 2 ... second main controller on s3c2416 and s3c2450 > > + ctrl_irq contains the interrupt bit of the controller > > + parent_irq contains the parent bit in the main controller and wi= ll be > > + ignored in main controllers >=20 > I expected the second and third cell to be in the opposite order, so > the meaning of the second cell is always the same. ok, so we do , right? ... As it only= =20 involves exchanging the intspec values, that's easy :-) > > + /* we're using individual domains for the non-dt case > > + * and one big domain for the dt case where the subintc > > + * starts at hwirq number 32. > > + */ > > + offset =3D (intc->domain->of_node) ? 32 : 0; >=20 > Wouldn't it be easier to always use the same setup for the domains he= re? nope ... the non-dt domains are not uniform (different lengths and star= t-irqs)=20 to recreate the static irq mappings that are still needed. For the non-= dt case=20 it also implement the handling of the external interrupts that is not p= art of=20 the interrupt-controller itself but comes from the gpio-registers and w= ill=20 move to pinctrl for dt machines. My hope is still to move to dt in a "reasonable" time, so this stuff ca= n go=20 away then altogether. Heiko From mboxrd@z Thu Jan 1 00:00:00 1970 From: heiko@sntech.de (Heiko =?iso-8859-1?q?St=FCbner?=) Date: Tue, 26 Mar 2013 08:38:42 +0100 Subject: [PATCH v5 7/7] irqchip: s3c24xx: add devicetree support In-Reply-To: <201303252200.46258.arnd@arndb.de> References: <201303252226.21402.heiko@sntech.de> <201303252234.21314.heiko@sntech.de> <201303252200.46258.arnd@arndb.de> Message-ID: <201303260838.42804.heiko@sntech.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Arnd, thanks again for taking the time to look at the changes. Am Montag, 25. M?rz 2013, 23:00:46 schrieb Arnd Bergmann: > On Monday 25 March 2013, Heiko St?bner wrote: > > Add the necessary code to initialize the interrupt controller > > thru devicetree data using the irqchip infrastructure. > > > > Signed-off-by: Heiko Stuebner > > The binding looks fine now. I have a few detail comments but am happy > with the series otherwise. > > > +Required properties: > > +- compatible: Compatible property value should be "samsung,s3c24xx-irq" > > + for non s3c2416 machines and "samsung,s3c2416-irq" for s3c2416 > > machines > > We try to avoid wildcards in the "compatible" properties. Better use > the name of the first SoC that had this controller, and let the other > ones mark themselves as compatible with that one. > > I guess "samsung,s3c2410-irq" would be the right identifier here. ok, sounds sensible > > +- #interrupt-cells : Specifies the number of cells needed to encode an > > + interrupt source. The value shall be 4 and interrupt descriptor shall > > + have the following format: > > + > > + > > + ctrl_num contains the controller to use: > > + - 0 ... main controller > > + - 1 ... sub controller > > + - 2 ... second main controller on s3c2416 and s3c2450 > > + ctrl_irq contains the interrupt bit of the controller > > + parent_irq contains the parent bit in the main controller and will be > > + ignored in main controllers > > I expected the second and third cell to be in the opposite order, so > the meaning of the second cell is always the same. ok, so we do , right? ... As it only involves exchanging the intspec values, that's easy :-) > > + /* we're using individual domains for the non-dt case > > + * and one big domain for the dt case where the subintc > > + * starts at hwirq number 32. > > + */ > > + offset = (intc->domain->of_node) ? 32 : 0; > > Wouldn't it be easier to always use the same setup for the domains here? nope ... the non-dt domains are not uniform (different lengths and start-irqs) to recreate the static irq mappings that are still needed. For the non-dt case it also implement the handling of the external interrupts that is not part of the interrupt-controller itself but comes from the gpio-registers and will move to pinctrl for dt machines. My hope is still to move to dt in a "reasonable" time, so this stuff can go away then altogether. Heiko