[PATCH] pinctrl: Add SPEAr pinctrl drivers

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Arnd Bergmann]

On Tuesday 03 April 2012, Viresh Kumar wrote:
> This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC
> drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Hi Viresh,

This is quite a lot of data, especially for the spear320. Have you
tried moving some or all of the data into device tree properties?

I can only guess that the spear13xx file would be even larger than
these.

> +
> +static struct of_device_id spear_pinctrl_of_match[] __devinitdata = {
> +#ifdef CONFIG_PINCTRL_SPEAR300
> +	{
> +		.compatible = "st,spear300-pinmux",
> +		.data = spear300_mach_init,
> +	},
> +#endif
> +#ifdef CONFIG_PINCTRL_SPEAR310
> +	{
> +		.compatible = "st,spear310-pinmux",
> +		.data = spear310_mach_init,
> +	},
> +#endif
> +#ifdef CONFIG_PINCTRL_SPEAR320
> +	{
> +		.compatible = "st,spear320-pinmux",
> +		.data = spear320_mach_init,
> +	},
> +#endif
> +	{},
> +};

I would recommend turning the logic around here: instead of having
a common driver that calls into the soc specific drivers, make the
common code a library that just exports a few symbols, and move
each of the socs into its own module with one init function
that calls the generic spear_pinctrl_probe, passing the appropriate
arguments on the code that that differs.

> +static struct platform_driver spear_pinctrl_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = spear_pinctrl_of_match,
> +	},
> +	.probe = spear_pinctrl_probe,
> +	.remove = __devexit_p(spear_pinctrl_remove),
> +};
> +
> +static int __init spear_pinctrl_init(void)
> +{
> +	return platform_driver_register(&spear_pinctrl_driver);
> +}
> +arch_initcall(spear_pinctrl_init);
> +
> +static void __exit spear_pinctrl_exit(void)
> +{
> +	platform_driver_unregister(&spear_pinctrl_driver);
> +}
> +module_exit(spear_pinctrl_exit);

Does this need to be an arch_initcall? If it becomes a regular
module_init(), you can condense all of this into a single
line using module_platform_driver(), one per soc if you do as
I suggest above.

The initialization order dependencies can now be handled using
the deferred probe mechanism, by returning -EPROBE_DEFER from
the probe() function of any driver that is loaded before its
pins are available.

> +static struct spear_pmx_mode pmx_mode_nand = {
> +	.name = "nand",
> +	.mode = NAND_MODE,
> +	.reg = MODE_CONFIG_REG,
> +	.mask = 0x0000000F,
> +	.val = 0x00,
> +};
> +
> +static struct spear_pmx_mode pmx_mode_nor = {
> +	.name = "nor",
> +	.mode = NOR_MODE,
> +	.reg = MODE_CONFIG_REG,
> +	.mask = 0x0000000F,
> +	.val = 0x01,
> +};
> +
> +static struct spear_pmx_mode pmx_mode_photo_frame = {
> +	.name = "photo frame mode",
> +	.mode = PHOTO_FRAME_MODE,
> +	.reg = MODE_CONFIG_REG,
> +	.mask = 0x0000000F,
> +	.val = 0x02,
> +};

These all look like they can easily get transformed into
device nodes in the device tree.

> +/* pingroups */
> +static struct spear_pingroup *spear310_pingroups[] = {
> +	SPEAR3XX_COMMON_PINGROUPS,
> +	&emi_cs_0_to_5_pingroup,
> +	&uart1_pingroup,
> +	&uart2_pingroup,
> +	&uart3_pingroup,
> +	&uart4_pingroup,
> +	&uart5_pingroup,
> +	&fsmc_pingroup,
> +	&rs485_0_pingroup,
> +	&rs485_1_pingroup,
> +	&tdm_pingroup,
> +};
> +
> +/* functions */
> +static struct spear_function *spear310_functions[] = {
> +	SPEAR3XX_COMMON_FUNCTIONS,
> +	&emi_cs_0_to_5_function,
> +	&uart1_function,
> +	&uart2_function,
> +	&uart3_function,
> +	&uart4_function,
> +	&uart5_function,
> +	&fsmc_function,
> +	&rs485_0_function,
> +	&rs485_1_function,
> +	&tdm_function,
> +};

I believe the macros like SPEAR3XX_COMMON_FUNCTIONS are not
needed any more, you can simply register multiple sets of pingroups
and functions.

	Arnd

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[viresh kumar]

On 4/3/12, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 03 April 2012, Viresh Kumar wrote:
>> This adds pinctrl driver for SPEAr family. Currently it contains
>> machine/SoC
>> drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
>
> This is quite a lot of data, especially for the spear320. Have you
> tried moving some or all of the data into device tree properties?

I haven't tried DT for the properties till now.

> I can only guess that the spear13xx file would be even larger than
> these.

Not Actually. 13xx only has two SoCs - 1310 and 1340. And their
muxing is much lighter. 320 is a real big one.

>> +static struct of_device_id spear_pinctrl_of_match[] __devinitdata = {
>> +#ifdef CONFIG_PINCTRL_SPEAR300
>> +	{
>> +		.compatible = "st,spear300-pinmux",
>> +		.data = spear300_mach_init,
>> +	},
>> +#endif
>> +#ifdef CONFIG_PINCTRL_SPEAR310
>> +	{
>> +		.compatible = "st,spear310-pinmux",
>> +		.data = spear310_mach_init,
>> +	},
>> +#endif
>> +#ifdef CONFIG_PINCTRL_SPEAR320
>> +	{
>> +		.compatible = "st,spear320-pinmux",
>> +		.data = spear320_mach_init,
>> +	},
>> +#endif
>> +	{},
>> +};
>
> I would recommend turning the logic around here: instead of having
> a common driver that calls into the soc specific drivers, make the
> common code a library that just exports a few symbols, and move
> each of the socs into its own module with one init function
> that calls the generic spear_pinctrl_probe, passing the appropriate
> arguments on the code that that differs.

Sure.

>> +static int __init spear_pinctrl_init(void)
>> +{
>> +	return platform_driver_register(&spear_pinctrl_driver);
>> +}
>> +arch_initcall(spear_pinctrl_init);
>
> Does this need to be an arch_initcall? If it becomes a regular
> module_init(), you can condense all of this into a single
> line using module_platform_driver(), one per soc if you do as
> I suggest above.

I need to enable pinmux onetime from my soc dt_init() and
so need a arch_init here. For SPEAr, we would be doing
onetime pmx mostly.

> The initialization order dependencies can now be handled using
> the deferred probe mechanism, by returning -EPROBE_DEFER from
> the probe() function of any driver that is loaded before its
> pins are available.

Can be done, but i need arch_init() as explained earlier.

>> +static struct spear_pmx_mode pmx_mode_nand = {
>> +	.name = "nand",
>> +	.mode = NAND_MODE,
>> +	.reg = MODE_CONFIG_REG,
>> +	.mask = 0x0000000F,
>> +	.val = 0x00,
>> +};
>> +
>
> These all look like they can easily get transformed into
> device nodes in the device tree.

Any example driver doing this would be helpful. Can you
give me a link. :)

>> +/* pingroups */
>> +static struct spear_pingroup *spear310_pingroups[] = {
>> +	SPEAR3XX_COMMON_PINGROUPS,
>> +	&emi_cs_0_to_5_pingroup,
>> +	&uart1_pingroup,
>> +	&uart2_pingroup,
>> +	&uart3_pingroup,
>> +	&uart4_pingroup,
>> +	&uart5_pingroup,
>> +	&fsmc_pingroup,
>> +	&rs485_0_pingroup,
>> +	&rs485_1_pingroup,
>> +	&tdm_pingroup,
>> +};
>> +
>> +/* functions */
>> +static struct spear_function *spear310_functions[] = {
>> +	SPEAR3XX_COMMON_FUNCTIONS,
>> +	&emi_cs_0_to_5_function,
>> +	&uart1_function,
>> +	&uart2_function,
>> +	&uart3_function,
>> +	&uart4_function,
>> +	&uart5_function,
>> +	&fsmc_function,
>> +	&rs485_0_function,
>> +	&rs485_1_function,
>> +	&tdm_function,
>> +};
>
> I believe the macros like SPEAR3XX_COMMON_FUNCTIONS are not
> needed any more, you can simply register multiple sets of pingroups
> and functions.

You mean to say, we can do multiple pinctrl_register() and define multiple
struct pinctrl_desc?

--
Viresh

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Stephen Warren]

On 04/03/2012 07:47 AM, Arnd Bergmann wrote:
> On Tuesday 03 April 2012, Viresh Kumar wrote:
>> This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC
>> drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> 
> Hi Viresh,
> 
> This is quite a lot of data, especially for the spear320. Have you
> tried moving some or all of the data into device tree properties?
> 
> I can only guess that the spear13xx file would be even larger than
> these.

Didn't we decide (at Linaro Connect) that is was perfectly acceptable to
put this data into the pinctrl drivers; it's just going to get parsed
out into exactly the same tables, and the data is presumably static for
each SoC, and hence there's little point putting it into DT.

>> +/* pingroups */
>> +static struct spear_pingroup *spear310_pingroups[] = {
>> +	SPEAR3XX_COMMON_PINGROUPS,
>> +	&emi_cs_0_to_5_pingroup,
>> +	&uart1_pingroup,
>> +	&uart2_pingroup,
>> +	&uart3_pingroup,
>> +	&uart4_pingroup,
>> +	&uart5_pingroup,
>> +	&fsmc_pingroup,
>> +	&rs485_0_pingroup,
>> +	&rs485_1_pingroup,
>> +	&tdm_pingroup,
>> +};
>> +
>> +/* functions */
>> +static struct spear_function *spear310_functions[] = {
>> +	SPEAR3XX_COMMON_FUNCTIONS,
>> +	&emi_cs_0_to_5_function,
>> +	&uart1_function,
>> +	&uart2_function,
>> +	&uart3_function,
>> +	&uart4_function,
>> +	&uart5_function,
>> +	&fsmc_function,
>> +	&rs485_0_function,
>> +	&rs485_1_function,
>> +	&tdm_function,
>> +};
> 
> I believe the macros like SPEAR3XX_COMMON_FUNCTIONS are not
> needed any more, you can simply register multiple sets of pingroups
> and functions.

For a given pin controller, there's a single set of pins, groups, and
functions; no incremental registration. You can register multiple pin
controllers, but that only makes sense if there really are multiple
physically separate pin controller modules in hardware.

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[viresh kumar]

On 4/3/12, viresh kumar <viresh.linux@gmail.com> wrote:
> On 4/3/12, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 03 April 2012, Viresh Kumar wrote:
>>> +static struct of_device_id spear_pinctrl_of_match[] __devinitdata = {
>>> +#ifdef CONFIG_PINCTRL_SPEAR300
>>> +	{
>>> +		.compatible = "st,spear300-pinmux",
>>> +		.data = spear300_mach_init,
>>> +	},
>>> +#endif
>>> +#ifdef CONFIG_PINCTRL_SPEAR310
>>> +	{
>>> +		.compatible = "st,spear310-pinmux",
>>> +		.data = spear310_mach_init,
>>> +	},
>>> +#endif
>>> +#ifdef CONFIG_PINCTRL_SPEAR320
>>> +	{
>>> +		.compatible = "st,spear320-pinmux",
>>> +		.data = spear320_mach_init,
>>> +	},
>>> +#endif
>>> +	{},
>>> +};
>>
>> I would recommend turning the logic around here: instead of having
>> a common driver that calls into the soc specific drivers, make the
>> common code a library that just exports a few symbols, and move
>> each of the socs into its own module with one init function
>> that calls the generic spear_pinctrl_probe, passing the appropriate
>> arguments on the code that that differs.
>
> Sure.

I tried doing this, but it didn't looked convincing to me. Lot of code
that is currently present in pinctrl-spear.c is getting duplicated per
SoC registered: 5 (3 - spear3xx, 2 - spear13xx). Almost all SoC's will
need exactly the same probe routine. So, i would like to retain the original
code posted.

>>> +static struct spear_pmx_mode pmx_mode_nand = {
>>> +	.name = "nand",
>>> +	.mode = NAND_MODE,
>>> +	.reg = MODE_CONFIG_REG,
>>> +	.mask = 0x0000000F,
>>> +	.val = 0x00,
>>> +};
>>> +
>>
>> These all look like they can easily get transformed into
>> device nodes in the device tree.
>
> Any example driver doing this would be helpful. Can you
> give me a link. :)

With Stephen's mail, i believe i don't really have to do it, for now
atleast.

>>> +/* pingroups */
>>> +static struct spear_pingroup *spear310_pingroups[] = {
>>> +	SPEAR3XX_COMMON_PINGROUPS,
>>> +	&emi_cs_0_to_5_pingroup,
>>> +	&uart1_pingroup,
>>> +	&uart2_pingroup,
>>> +	&uart3_pingroup,
>>> +	&uart4_pingroup,
>>> +	&uart5_pingroup,
>>> +	&fsmc_pingroup,
>>> +	&rs485_0_pingroup,
>>> +	&rs485_1_pingroup,
>>> +	&tdm_pingroup,
>>> +};
>>> +
>>> +/* functions */
>>> +static struct spear_function *spear310_functions[] = {
>>> +	SPEAR3XX_COMMON_FUNCTIONS,
>>> +	&emi_cs_0_to_5_function,
>>> +	&uart1_function,
>>> +	&uart2_function,
>>> +	&uart3_function,
>>> +	&uart4_function,
>>> +	&uart5_function,
>>> +	&fsmc_function,
>>> +	&rs485_0_function,
>>> +	&rs485_1_function,
>>> +	&tdm_function,
>>> +};
>>
>> I believe the macros like SPEAR3XX_COMMON_FUNCTIONS are not
>> needed any more, you can simply register multiple sets of pingroups
>> and functions.
>
> You mean to say, we can do multiple pinctrl_register() and define multiple
> struct pinctrl_desc?

Again, with Stephen's comment, it is clear we can't register multiple
groups/functions
here, as they correspond to same set of pins and pinctrl. Would keep it as is.

--
Viresh

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Arnd Bergmann]

On Tuesday 03 April 2012, viresh kumar wrote:
> I tried doing this, but it didn't looked convincing to me. Lot of code
> that is currently present in pinctrl-spear.c is getting duplicated per
> SoC registered: 5 (3 - spear3xx, 2 - spear13xx). Almost all SoC's will
> need exactly the same probe routine. So, i would like to retain the original
> code posted.

Maybe I wasn't clear enough. I did not mean that code should be duplicated
here. Instead, you should keep the probe function and export it, with some
modifications, and do the same for the other common functions. This works
well for a lot of other drivers in a similar situation. It would result
in a function like

/* in the header file */
extern int __devinit spear3xx_pinctrl_probe(struct platform_device *pdev,
			const struct spear_pinctrl_mach_data *data);
extern void __devexit spear_pinctrl_remove(struct platform_device *pdev);

/* in pinctrl_spear320.c */
static const spear_pinctrl_mach_data spear320_machdata = {
	.groups = spear320_pingroups,
	.ngroups = ARRAY_SIZE(spear320_pingroups),
	.functions = spear320_functions,
	.nfunctions = _SIZE(spear320_functions),
	.modes_supported = true,
	.pmx_modes = spear320_pmx_modes,
	.npmx_modes = ARRAY_SIZE(spear320_pmx_modes),
};

static int __devinit spear320_pinctrl_probe(struct platform_device *pdev)
{
	return spear3xx_pinctrl_init(pdev, &spear320_machdata);
}

static struct of_device_id spear_pinctrl_of_match[] __devinitdata = {
       { .compatible = "st,spear320-pinmux" },
       {},
};

static struct platform_driver spear_pinctrl_driver = {
       .driver = {
               .name = DRIVER_NAME,
               .owner = THIS_MODULE,
               .of_match_table = spear_pinctrl_of_match,
       },
       .probe = spear320_pinctrl_probe,
       .remove = __devexit_p(spear_pinctrl_remove),
};

While you have to write a copy of this for each soc, there is not actually much
here that is not soc specific.

> >>> +static struct spear_pmx_mode pmx_mode_nand = {
> >>> +   .name = "nand",
> >>> +   .mode = NAND_MODE,
> >>> +   .reg = MODE_CONFIG_REG,
> >>> +   .mask = 0x0000000F,
> >>> +   .val = 0x00,
> >>> +};
> >>> +
> >>
> >> These all look like they can easily get transformed into
> >> device nodes in the device tree.
> >
> > Any example driver doing this would be helpful. Can you
> > give me a link. :)
> 
> With Stephen's mail, i believe i don't really have to do it, for now
> atleast.

No, you don't have to do it, but when you make the decision, you should
be sure that it's a good one, because changing this later would be a pain.

	Arnd

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Arnd Bergmann]

On Tuesday 03 April 2012, Stephen Warren wrote:
> On 04/03/2012 07:47 AM, Arnd Bergmann wrote:
> > On Tuesday 03 April 2012, Viresh Kumar wrote:
> >> This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC
> >> drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.
> >>
> >> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> > 
> > Hi Viresh,
> > 
> > This is quite a lot of data, especially for the spear320. Have you
> > tried moving some or all of the data into device tree properties?
> > 
> > I can only guess that the spear13xx file would be even larger than
> > these.
> 
> Didn't we decide (at Linaro Connect) that is was perfectly acceptable to
> put this data into the pinctrl drivers; it's just going to get parsed
> out into exactly the same tables, and the data is presumably static for
> each SoC, and hence there's little point putting it into DT.

Yes, but it's also acceptable to put it into the device tree. It's mostly
a question of how you prioritise it as a maintainer. For platforms that
have a lot of different socs, it may make much more sense than it does
for those that only have a couple of different pincontrol variants.

> >> +/* pingroups */
> >> +static struct spear_pingroup *spear310_pingroups[] = {
> >> +    SPEAR3XX_COMMON_PINGROUPS,
> >> +    &emi_cs_0_to_5_pingroup,
> >> +    &uart1_pingroup,
> >> +    &uart2_pingroup,
> >> +    &uart3_pingroup,
> >> +    &uart4_pingroup,
> >> +    &uart5_pingroup,
> >> +    &fsmc_pingroup,
> >> +    &rs485_0_pingroup,
> >> +    &rs485_1_pingroup,
> >> +    &tdm_pingroup,
> >> +};
> >> +
> >> +/* functions */
> >> +static struct spear_function *spear310_functions[] = {
> >> +    SPEAR3XX_COMMON_FUNCTIONS,
> >> +    &emi_cs_0_to_5_function,
> >> +    &uart1_function,
> >> +    &uart2_function,
> >> +    &uart3_function,
> >> +    &uart4_function,
> >> +    &uart5_function,
> >> +    &fsmc_function,
> >> +    &rs485_0_function,
> >> +    &rs485_1_function,
> >> +    &tdm_function,
> >> +};
> > 
> > I believe the macros like SPEAR3XX_COMMON_FUNCTIONS are not
> > needed any more, you can simply register multiple sets of pingroups
> > and functions.
> 
> For a given pin controller, there's a single set of pins, groups, and
> functions; no incremental registration. You can register multiple pin
> controllers, but that only makes sense if there really are multiple
> physically separate pin controller modules in hardware.

Right, my fault. I confused this with pinmux_register_mappings, which
can be called multiple times.

	Arnd

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Linus Walleij]

On Tue, Apr 3, 2012 at 1:40 PM, Viresh Kumar <viresh.kumar@st.com> wrote:

> ?drivers/pinctrl/spear/pinctrl-spear300.c ? ? ? ? ? | ?661 ++++
> ?drivers/pinctrl/spear/pinctrl-spear310.c ? ? ? ? ? | ?384 +++
> ?drivers/pinctrl/spear/pinctrl-spear320.c ? ? ? ? ? | 3421 ++++++++++++++++++++
> ?drivers/pinctrl/spear/pinctrl-spear3xx.c ? ? ? ? ? | ?626 ++++

A lot of data but as we've said, it's OK if you want to do it this way.

> +static struct of_device_id spear_pinctrl_of_match[] __devinitdata = {
> +#ifdef CONFIG_PINCTRL_SPEAR300
> + ? ? ? {
> + ? ? ? ? ? ? ? .compatible = "st,spear300-pinmux",
> + ? ? ? ? ? ? ? .data = spear300_mach_init,
> + ? ? ? },
> +#endif
> +#ifdef CONFIG_PINCTRL_SPEAR310
> + ? ? ? {
> + ? ? ? ? ? ? ? .compatible = "st,spear310-pinmux",
> + ? ? ? ? ? ? ? .data = spear310_mach_init,
> + ? ? ? },
> +#endif
> +#ifdef CONFIG_PINCTRL_SPEAR320
> + ? ? ? {
> + ? ? ? ? ? ? ? .compatible = "st,spear320-pinmux",
> + ? ? ? ? ? ? ? .data = spear320_mach_init,
> + ? ? ? },
> +#endif

Ugh this is not looking good, please check Arnds advice in how to restructure
this a bit... #ifdef is ugly (and error-prone).

Apart from this it looks pretty straight-forward. Make sure to rebase
onto your own patches in linux-next for next version.

Linus Walleij

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Linus Walleij]

On Tue, Apr 3, 2012 at 3:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> The initialization order dependencies can now be handled using
> the deferred probe mechanism, by returning -EPROBE_DEFER from
> the probe() function of any driver that is loaded before its
> pins are available.

So how do I as a driver writer do that?

Example: drivers/tty/serial/sirfsoc_uart.c:

#include <linux/pinctrl/consumer.h>

probe() {
       if (sirfport->hw_flow_ctrl) {
                sirfport->p = pinctrl_get_select_default(&pdev->dev);
                ret = IS_ERR(sirfport->p);
                if (ret)
                        goto pin_err;
        }

pin_err:
    (...)
   return ret;

So basically the pinctrl subsystem needs to return -EPROBE_DEFER
to the driver in this case, so it can escalate that further.

Shall we go in and patch not just pinctrl but also regulator,
clk etc to return this if the subsystem is not yet up and no maps
and drivers at all have been yet been registered?

Yours,
Linus Walleij

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Stephen Warren]

On 04/03/2012 03:24 PM, Linus Walleij wrote:
> On Tue, Apr 3, 2012 at 3:47 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 
>> The initialization order dependencies can now be handled using
>> the deferred probe mechanism, by returning -EPROBE_DEFER from
>> the probe() function of any driver that is loaded before its
>> pins are available.
> 
> So how do I as a driver writer do that?
> 
> Example: drivers/tty/serial/sirfsoc_uart.c:
> 
> #include <linux/pinctrl/consumer.h>
> 
> probe() {
>        if (sirfport->hw_flow_ctrl) {
>                 sirfport->p = pinctrl_get_select_default(&pdev->dev);
>                 ret = IS_ERR(sirfport->p);
>                 if (ret)
>                         goto pin_err;
>         }
> 
> pin_err:
>     (...)
>    return ret;
> 
> So basically the pinctrl subsystem needs to return -EPROBE_DEFER
> to the driver in this case, so it can escalate that further.

Yes, I believe so.

There should be a couple of comments in pinctrl now (well, perhaps some
of them aren't there yet until my DT patches are applied!) that indicate
where -EPROBE_DEFER should be returned.

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Viresh Kumar]

On 4/4/2012 1:03 AM, Arnd Bergmann wrote:
> Maybe I wasn't clear enough. I did not mean that code should be duplicated
> here. Instead, you should keep the probe function and export it, with some
> modifications, and do the same for the other common functions. This works
> well for a lot of other drivers in a similar situation. It would result
> in a function like

No. You were very clear earlier too. :)

> static int __devinit spear320_pinctrl_probe(struct platform_device *pdev)
> {
> 	return spear3xx_pinctrl_init(pdev, &spear320_machdata);
> }
> 
> static struct of_device_id spear_pinctrl_of_match[] __devinitdata = {
>        { .compatible = "st,spear320-pinmux" },
>        {},
> };
> 
> static struct platform_driver spear_pinctrl_driver = {
>        .driver = {
>                .name = DRIVER_NAME,
>                .owner = THIS_MODULE,
>                .of_match_table = spear_pinctrl_of_match,
>        },
>        .probe = spear320_pinctrl_probe,
>        .remove = __devexit_p(spear_pinctrl_remove),
> };

I was worrying about duplicating above code. :)
Anyway, i have already fixed up my code as you suggested.

-- 
viresh

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Viresh Kumar]

On 4/4/2012 2:48 AM, Linus Walleij wrote:
> Ugh this is not looking good, please check Arnds advice in how to restructure
> this a bit... #ifdef is ugly (and error-prone).

Ugliness inherited from Tegra :)

> Apart from this it looks pretty straight-forward. Make sure to rebase
> onto your own patches in linux-next for next version.

I already rebased it latest linux-next/master + my earlier pinctrl patch.
Will fetch linux-next again when i send V2. If my earlier patches for pinmux
are still not present in linux-next/master i will apply my earlier patches
before this one.

-- 
viresh

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Arnd Bergmann]

On Tuesday 03 April 2012, Linus Walleij wrote:
> Shall we go in and patch not just pinctrl but also regulator,
> clk etc to return this if the subsystem is not yet up and no maps
> and drivers at all have been yet been registered?

Regulators already do it, IIRC, but a patch for clk and other things
you notice would be nice.

	Arnd

[PATCH] pinctrl: Add SPEAr pinctrl drivers

[Linus Walleij]

On Wed, Apr 4, 2012 at 12:43 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/03/2012 03:24 PM, Linus Walleij wrote:
>> So basically the pinctrl subsystem needs to return -EPROBE_DEFER
>> to the driver in this case, so it can escalate that further.
>
> Yes, I believe so.
>
> There should be a couple of comments in pinctrl now (well, perhaps some
> of them aren't there yet until my DT patches are applied!) that indicate
> where -EPROBE_DEFER should be returned.

OK I'll fix something...

Yours,
Linus Walleij