From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Zhao Subject: Re: [PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers Date: Wed, 11 Jul 2012 17:33:32 +0800 Message-ID: <20120711093331.GA3025@b20223-02.ap.freescale.net> References: <1341565763-10074-1-git-send-email-b29396@freescale.com> <4FF709D1.40903@wwwdotorg.org> <20120709071019.GA28527@shlinux2.ap.freescale.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20120709071019.GA28527-Fb7DQEYuewWctlrPMvKcciBecyulp+rMXqFh9Ls21Oc@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Dong Aisheng Cc: Zhao Richard-B20223 , "linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org" , "devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org" , Guo Shawn-R65073 , Liu Hui-R64343 , "kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org" , "s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: devicetree@vger.kernel.org On Mon, Jul 09, 2012 at 03:10:21PM +0800, Dong Aisheng wrote: > On Fri, Jul 06, 2012 at 11:52:49PM +0800, Stephen Warren wrote: > > On 07/06/2012 03:09 AM, Dong Aisheng wrote: > > > From: Dong Aisheng > > > > > > The General Purpose Registers (GPR) is used to select operating modes for > > > general features in the SoC, usually not related to the IOMUX itself, > > > but it does belong to IOMUX controller. > > > We simply provide an convient API for driver to call to set the general purpose > > > register bits if needed. > > > > > +static struct imx_pinctrl *imx_pinctrl; > > > +/* > > > + * Set bits for general purpose registers > > > + */ > > > +void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value) > > > +{ > > > + u32 reg; > > > + > > > + /* general purpose register is 32 bits size */ > > > + WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32); > > > > Hmmm. It's going to be very hard to control the probe() order to ensure > > that this WARN doesn't fire all the time. > > > Correct, if this api is used by client driver before the pinctrl driver is > registered, the warning will show. > To avoid it, the current pinctrl driver register priority is arch_initcall. +postcore_initcall is not enough. Could you use postcore_initcall? So people can hack right after populate devices. > But maybe you're right, this may not be so sufficient since i'm afraid there > are still possible some devices may register earlier than pinctrl since it's not > controlled by pinctrl driver. > Then it's true that we need to resolve it. imx_pinctrl_set_gpr_register is SoC specific. People who use it must have sense of related registers and driver loading sequence. > > > I think it would be better to pass in a struct imx_pinctrl* or DT node > > to this function. Maybe we can do it, but why? It's kind of over-engineering. > The DT node for the device that's using this function > > should contain a phandle to the pinctrl device node, which it uses to > > get that handle. Or in a non-DT case, the client driver needs to be > > given the provider driver handle using some other mechanism. > > > > For example, look at how the Tegra30 SMMU uses services from the Tegra30 > > AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node > > "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves > > smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with > > this handle) and drivers/amba/tegra-ahb.c function > > tegra_ahb_enable_smmu() (implements the deferred probe checking to > > correctly order the client/provider driver probing) > > > Yes, i learned the code but i'm not sure it also fits for imx. > I have a few concerns: > 1) i'm not sure if we really need the client to provide pinctrl device > node as parameter to set gpr registers since there is only one pinctrl device > for each imx soc. Client devices may also not want to care that parameter. > 2) if passing device node to the api, that means every client driver > needs to define a phandle of pinctrl device in dts file and parsing it > in each driver respectively. > There're several client devices for imx6q, i'm not sure if it's worth to do that > considering this overhead. > > I think either passing device node or not passing, the goal is the same, > guarantee this api to be used properly without being affected by driver > loading sequence. > > If that, how about change to: > int imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value) > { > u32 reg; > > if (!imx_pinctrl) > return -EDEFER_PROBE; > > /* general purpose register is 32 bits size */ > WARN_ON(start_bit > 31 || num_bits > 32); Isn't it BUG ? Normally, people like write_register(addr, mask, value), and read_register(addr, mask); Thanks Richard > ... > } > EXPORT_SYMBOL_GPL(imx_pinctrl_set_gpr_register); > > Then it looks to me it may be able to solve the same issue. > > Regards > Dong Aisheng