[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Richard Zhao]

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 <dong.aisheng at linaro.org>
> > > 
> > > 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