Skip to contentSkip to conversation

pinctrl: Add SPEAr pinctrl drivers

PATCH New Arnd Bergmann 5 participants 13 messages Apr 3, 2012 – Apr 10, 2012 1 message not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): a1b7f612544a09989cec80a0318f35b6e48eb1d3.1333453148.git.viresh.kumar@st.com

#1 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 4 lines · Viresh Kumar

This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.

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.

Quoted 12 lines
(quoted diff) +12 -0
+
+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,

Review

Quoted 10 lines
(quoted diff) +10 -0
+	},
+#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.

Quoted 11 lines
(quoted diff) +11 -0
+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)

Review

Quoted 10 lines
(quoted diff) +10 -0
+{
+	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.

Quoted 13 lines
(quoted diff) +13 -0
+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,

Review

Quoted 10 lines
(quoted diff) +10 -0
+	.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.

Quoted 19 lines
(quoted diff) +19 -0
+/* 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,

Review

Quoted 10 lines
(quoted diff) +10 -0
+	&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

#2 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 9 lines · Arnd Bergmann
Quoted 5 lines · Viresh Kumar

This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC
drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.

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.

Quoted 2 lines

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.

Quoted 20 lines
Quoted 20 lines
(quoted diff) +20 -0
+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
+	{},
Quoted 8 lines
Quoted 1 line

+};

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.

Quoted 10 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 4 lines

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.

Quoted 11 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+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. :)

Quoted 25 lines
Quoted 25 lines
(quoted diff) +25 -0
+/* 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,
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	&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

#3 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 31 lines · viresh kumar
Quoted 21 lines · Arnd Bergmann
Quoted 20 lines · Viresh Kumar
(quoted diff) +20 -0
+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
+	{},
Quoted 8 lines
Quoted 1 line

+};

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.

Quoted 1 line

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.

Quoted 12 lines
Quoted 11 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+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.

Quoted 2 lines

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.

Quoted 34 lines
Quoted 25 lines
Quoted 25 lines
(quoted diff) +25 -0
+/* 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,
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	&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.

Quoted 2 lines

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

#4 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 5 lines · viresh kumar

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.

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines

Review

Quoted 8 lines
(quoted diff) +8 -0
+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.

Quoted 5 lines
Quoted 2 lines

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

#5 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 5 lines · Arnd Bergmann

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

Quoted 10 lines

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" },
       {},
};
Quoted 9 lines

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.

Signature · 1 line
viresh
#6 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

In reply to Arnd Bergmann
Quoted 6 lines · Arnd Bergmann
Quoted 4 lines · Viresh Kumar

This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.

Quoted 7 lines

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.

Quoted 25 lines
Quoted 25 lines
(quoted diff) +25 -0
+/* 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,
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+	&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.

#7 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 12 lines · Stephen Warren
Quoted 11 lines · Arnd Bergmann
Quoted 4 lines · Viresh Kumar

This adds pinctrl driver for SPEAr family. Currently it contains machine/SoC drivers for SPEAr3xx family only. SPEAr13xx family drivers will follow.

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?

Quoted 7 lines
Quoted 2 lines

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.

Quoted 34 lines
Quoted 25 lines
Quoted 25 lines
(quoted diff) +25 -0
+/* 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,
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+    &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.

Quoted 4 lines

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

#8 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

In reply to Arnd Bergmann
Quoted 4 lines · Arnd Bergmann

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

#9 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 21 lines · Linus Walleij
Quoted 4 lines · Arnd Bergmann

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;
        }
Quoted 6 lines

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.

#10 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 9 lines · Stephen Warren
Quoted 2 lines · Linus Walleij

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

#11 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

In reply to Linus Walleij
Quoted 3 lines · Linus Walleij

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

#12 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

In reply to a message not in the archive
Quoted 4 lines · Viresh Kumar
?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.

Quoted 9 lines
(quoted diff) +9 -0
+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
+ ? ? ? {

Review

Quoted 10 lines
(quoted diff) +10 -0
+ ? ? ? ? ? ? ? .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

#13 lore raw

PATCH 1/1 pinctrl: Add SPEAr pinctrl drivers

Quoted 2 lines · Linus Walleij

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 :)

Quoted 2 lines

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.

Signature · 1 line
viresh