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

[Dong Aisheng]

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.
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.

> I think it would be better to pass in a struct imx_pinctrl* or DT node
> to this function. 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);
...
}
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