Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753698Ab3HVPlZ (ORCPT ); Thu, 22 Aug 2013 11:41:25 -0400 Received: from mail-ee0-f46.google.com ([74.125.83.46]:55655 "EHLO mail-ee0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753635Ab3HVPlW (ORCPT ); Thu, 22 Aug 2013 11:41:22 -0400 Date: Thu, 22 Aug 2013 16:41:16 +0100 From: Lee Jones To: Mark Rutland Cc: Linus Walleij , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Arnd Bergmann , Linus WALLEIJ , Srinidhi KASAGAR , "devicetree@vger.kernel.org" Subject: Re: [PATCH 09/33] ARM: ux500: Supply the I2C clocks lookup to the DBX500 DT Message-ID: <20130822154116.GC17154@lee--X1> References: <1370521041-32318-1-git-send-email-lee.jones@linaro.org> <1370521041-32318-10-git-send-email-lee.jones@linaro.org> <20130820093034.GL31036@pengutronix.de> <20130822133730.GB23152@e106331-lin.cambridge.arm.com> <20130822141900.GB17154@lee--X1> <20130822151723.GE23152@e106331-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20130822151723.GE23152@e106331-lin.cambridge.arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3812 Lines: 90 On Thu, 22 Aug 2013, Mark Rutland wrote: > On Thu, Aug 22, 2013 at 03:19:00PM +0100, Lee Jones wrote: > > On Thu, 22 Aug 2013, Mark Rutland wrote: > > > > > On Tue, Aug 20, 2013 at 10:30:34AM +0100, Sascha Hauer wrote: > > > > On Tue, Aug 20, 2013 at 11:11:19AM +0200, Linus Walleij wrote: > > > > > On Thu, Jun 6, 2013 at 2:16 PM, Lee Jones wrote: > > > > > > > > > > > +++ b/arch/arm/boot/dts/dbx5x0.dtsi > > > > > > @@ -572,6 +572,8 @@ > > > > > > v-i2c-supply = <&db8500_vape_reg>; > > > > > > > > > > > > clock-frequency = <400000>; > > > > > > + clocks = <&prcc_kclk 3 3>, <&prcc_pclk 3 3>; > > > > > > + clock-names = "nmk-i2c.0", "apb_pclk"; > > > > > > > > Why do most clocks in this series have the instance number in the clock > > > > names? This looks very wrong to me. > > > > > > +1. The clock names should be the input names to the unit, they > > > shouldn't vary per instance. > > > > So I just had a quick look, and it looks like they each have their own > > clock: > > > > clk = clk_reg_prcc_kclk("p1_i2c1_kclk", "i2cclk", > > clkrst1_base, BIT(2), CLK_SET_RATE_GATE); > > clk = clk_reg_prcc_kclk("p1_i2c2_kclk", "i2cclk", > > clkrst1_base, BIT(6), CLK_SET_RATE_GATE); > > clk = clk_reg_prcc_kclk("p2_i2c3_kclk", "i2cclk", > > clkrst2_base, BIT(0), CLK_SET_RATE_GATE); > > clk_register_clkdev(clk, NULL, "nmk-i2c.3"); > > > > /* etc */ > > > > When using the names in Device Tree it doesn't actually matter what > > you call the first clock. You can call it "fred" if you wanted and it > > would still work, but in light of the naming conventions above and the > > fact that each clock can all be controlled independently, do we still > > want to use the name of the parent clock i.e. i2cclk? > > Sorry, I don't follow. > > > The name should be the name of the clock _input_ on the block described, > as should be listed in documentation for the i2c block. The name should > not vary with instance, and the name should not (necessarily) match the > _output_ of the provider. Surely there's documentation for the i2c block > that gives a name for the clock input(s)? It's okay, I've fixed the patches. Linus, branch fixed-up and pushed. > On a related note, I see that this doesn't follow the primecell clock > bindings, which seem to rely on "apb_pclk" being first in the list. I > see that other primecell device bindings don't follow that in dts or > drivers, so I'm not sure how to fix that brokenness. To which bindings do you refer? After taking a *quick* look, I see it being the other way around. Whenever the "apb_pclk" is requested, it is done so by name: drivers/amba/bus.c: struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk"); So when clk_get() calls of_clk_get_by_name(), the clock-name index is returned correctly by of_property_match_string(), which then passes that index through of_clk_get() and does the right thing. When most of the other clocks that we deal with are being requested, they rely on being index zero: drivers/i2c/busses/i2c-nomadik.c: dev->clk = clk_get(&adev->dev, NULL); At the moment this works for us, so I don't think we need to go around populating the name params, but we might have to if this falls apart in some way (probably likely if you 'fixed' whatever brokenness you're wanting to fix ;-) ) -- Lee Jones Linaro ST-Ericsson Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/