ARM: imx27 pinctrl

The imx pinctrl core driver does not support the imx21 register format yet. This patch adds support for the strange iomux control registers of imx21 and imx27. Next to a slightly different DT parse function.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c | 264 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 250 insertions(+), 14 deletions(-)

Hi Markus,

Quoted 4 lines · Markus Pargmann

The imx pinctrl core driver does not support the imx21 register format yet. This patch adds support for the strange iomux control registers of imx21 and imx27. Next to a slightly different DT parse function.

Quoted 4 lines

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c | 264 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 250 insertions(+), 14 deletions(-)

Is it really worth sharing the i.MX1/21/27 support with the imx driver? It seems most of the code is rewritten. I just tried and removed all lines from pinctrl-imx.c you don't use. There's not much left to share.

Quoted 3 lines

Is it really worth sharing the i.MX1/21/27 support with the imx driver? It seems most of the code is rewritten. I just tried and removed all lines from pinctrl-imx.c you don't use. There's not much left to share.

Yes it's really not that much to share. I will move it to a seperate file for imx1/21/27.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpio-mxc.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

drivers/gpio/gpio-mxc.c +3 -5

index 7176743..cee040f 100644
@@ -405,33 +405,31 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 
 	mxc_gpio_get_hw(pdev);
 
-	port = kzalloc(sizeof(struct mxc_gpio_port), GFP_KERNEL);
+	port = devm_kzalloc(&pdev->dev, sizeof(struct mxc_gpio_port),
+			GFP_KERNEL);
 	if (!port)
 		return -ENOMEM;
 
 	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!iores) {
-		err = -ENODEV;
-		goto out_kfree;
+		return -ENODEV;
 	}
 
-	if (!request_mem_region(iores->start, resource_size(iores),
-				pdev->name)) {

drivers/gpio/gpio-mxc.c +2 -9

 	port->irq_high = platform_get_irq(pdev, 1);
 	port->irq = platform_get_irq(pdev, 0);
 	if (port->irq < 0) {
-		err = -EINVAL;
-		goto out_iounmap;
+		return -EINVAL;
 	}
 
 	/* disable the interrupt and clear the status */

@@ -462,7 +460,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 			 port->base + GPIO_DR, NULL,
 			 port->base + GPIO_GDIR, NULL, 0);
 	if (err)
-		goto out_iounmap;
+		return err;
 
 	port->bgc.gc.to_irq = mxc_gpio_to_irq;
 	port->bgc.gc.base = (pdev->id < 0) ? of_alias_get_id(np, "gpio") * 32 :

@@ -498,12 +496,6 @@ out_gpiochip_remove:
 	WARN_ON(gpiochip_remove(&port->bgc.gc) < 0);
 out_bgpio_remove:
 	bgpio_remove(&port->bgc);
-out_iounmap:
-	iounmap(port->base);
-out_release_mem:
-	release_mem_region(iores->start, resource_size(iores));
-out_kfree:
-	kfree(port);
 	dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
 	return err;
 }

Signature · 1 line

1.8.3.2

It can be even cleaner by using devm_ioremap_resource().

Quoted 1 line · Shawn Guo

It can be even cleaner by using devm_ioremap_resource().

I have already sent a patch that converts to devm_ioremap_resource() and it is already in Linus Walleij's tree.

On imx27 and imx21, there is no clear seperation between iomux control registers and GPIO data registers. For easier pingroup definitions, the gpio drivers will be initialized as subnodes of the iomux controller. It is necessary to share the registers between iomux and gpio.

This patch adds support to pass a register memory region via platform data.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpio-mxc.c  | 35 +++++++++++++++++++----------------
 include/linux/gpio-mxc.h | 17 +++++++++++++++++
 2 files changed, 36 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/gpio-mxc.h

drivers/gpio/gpio-mxc.c +2 -7

index cee040f..0c4e32c 100644
@@ -26,6 +26,7 @@
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/gpio.h>
+#include <linux/gpio-mxc.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/basic_mmio_gpio.h>

@@ -400,6 +401,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct mxc_gpio_port *port;
 	struct resource *iores;
+	struct mxc_gpio_platform_data *pdata = pdev->dev.platform_data;
 	int irq_base;
 	int err;
 

@@ -410,27 +412,28 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
-	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iores) {
-		return -ENODEV;
-	}
-
-	if (!devm_request_mem_region(&pdev->dev, iores->start,
-				resource_size(iores), pdev->name)) {

drivers/gpio/gpio-mxc.c +11 -0

+	} else {
+		iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!iores)
+			return -ENODEV;
+
+		if (!devm_request_mem_region(&pdev->dev, iores->start,
+					resource_size(iores), pdev->name))
+			return -EBUSY;
+
+		port->base = devm_ioremap(&pdev->dev, iores->start,
+				resource_size(iores));

drivers/gpio/gpio-mxc.c +0 -0

 	/* disable the interrupt and clear the status */
 	writel(0, port->base + GPIO_IMR);

include/linux/gpio-mxc.h new file +17 -0

new file mode 100644
index 0000000..6be51bc
@@ -0,0 +1,17 @@
+#ifndef _LINUX_GPIO_MXC_H
+#define _LINUX_GPIO_MXC_H
+
+#include <linux/types.h>
+
+/*
+ * MXC GPIO driver platform data. If this platform data is passed to the
+ * driver, it will use the memory base defined in the struct. This is used for
+ * iomuxc drivers on imx21 and imx27, where the GPIO data register is embedded
+ * between the iomuxc registers.
+ */
+
+struct mxc_gpio_platform_data {
+	void __iomem *base;
+};
+
+#endif /* _LINUX_GPIO_MXC_H */

Signature · 1 line

1.8.3.2

Quoted 8 lines · Markus Pargmann

On imx27 and imx21, there is no clear seperation between iomux control registers and GPIO data registers. For easier pingroup definitions, the gpio drivers will be initialized as subnodes of the iomux controller. It is necessary to share the registers between iomux and gpio.

This patch adds support to pass a register memory region via platform data.

Quoted 6 lines

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpio-mxc.c  | 35 +++++++++++++++++++----------------
 include/linux/gpio-mxc.h | 17 +++++++++++++++++
 2 files changed, 36 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/gpio-mxc.h

Since the header is created only for holding platform_data, we should probably put it into include/linux/platform_data/.

Quoted 36 lines

drivers/gpio/gpio-mxc.c +2 -11

index cee040f..0c4e32c 100644
@@ -26,6 +26,7 @@
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/gpio.h>
+#include <linux/gpio-mxc.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/basic_mmio_gpio.h>

@@ -400,6 +401,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct mxc_gpio_port *port;
 	struct resource *iores;
+	struct mxc_gpio_platform_data *pdata = pdev->dev.platform_data;
 	int irq_base;
 	int err;
 

@@ -410,27 +412,28 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (!port)
 		return -ENOMEM;
 
-	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!iores) {
-		return -ENODEV;
-	}
-
-	if (!devm_request_mem_region(&pdev->dev, iores->start,
-				resource_size(iores), pdev->name)) {
-		return -EBUSY;
-	}
-
-	port->base = devm_ioremap(&pdev->dev, iores->start,

drivers/gpio/gpio-mxc.c static int mxc_gpio_probe(struct platform_device *pdev) · line 417

Quoted 6 lines

drivers/gpio/gpio-mxc.c +3 -3

-			resource_size(iores));
-	if (!port->base) {
-		return -ENOMEM;
+	if (pdata) {
+		port->base = pdata->base;
+		iores = NULL;

Why is this needed?

Quoted 16 lines

drivers/gpio/gpio-mxc.c +13 -0

+	} else {
+		iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!iores)
+			return -ENODEV;
+
+		if (!devm_request_mem_region(&pdev->dev, iores->start,
+					resource_size(iores), pdev->name))
+			return -EBUSY;
+
+		port->base = devm_ioremap(&pdev->dev, iores->start,
+				resource_size(iores));
+		if (!port->base)
+			return -ENOMEM;
 	}
 

drivers/gpio/gpio-mxc.c static int mxc_gpio_probe(struct platform_device *pdev) · line 433

Quoted 6 lines

drivers/gpio/gpio-mxc.c +1 -2

 	port->irq_high = platform_get_irq(pdev, 1);
 	port->irq = platform_get_irq(pdev, 0);
-	if (port->irq < 0) {
+	if (port->irq < 0)
 		return -EINVAL;
-	}

The change belongs to patch #1.

Shawn

Quoted 29 lines

drivers/gpio/gpio-mxc.c +0 -0

 	/* disable the interrupt and clear the status */
 	writel(0, port->base + GPIO_IMR);

include/linux/gpio-mxc.h new file +17 -0

new file mode 100644
index 0000000..6be51bc
@@ -0,0 +1,17 @@
+#ifndef _LINUX_GPIO_MXC_H
+#define _LINUX_GPIO_MXC_H
+
+#include <linux/types.h>
+
+/*
+ * MXC GPIO driver platform data. If this platform data is passed to the
+ * driver, it will use the memory base defined in the struct. This is used for
+ * iomuxc drivers on imx21 and imx27, where the GPIO data register is embedded
+ * between the iomuxc registers.
+ */
+
+struct mxc_gpio_platform_data {
+	void __iomem *base;
+};
+
+#endif /* _LINUX_GPIO_MXC_H */

Signature · 1 line

1.8.3.2

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	if (pdata) {
+		port->base = pdata->base;
+		iores = NULL;

Why is this needed?

The memory regions used for iomux control overlap the regions used by the gpio driver. So we need to share the memory with the pinctrl driver.

Quoted 8 lines · Markus Pargmann

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	if (pdata) {
+		port->base = pdata->base;
+		iores = NULL;

Why is this needed?

The memory regions used for iomux control overlap the regions used by the gpio driver. So we need to share the memory with the pinctrl driver.

I meant for iores = NULL;

+ struct platform_device **gpio_dev;
+ struct mxc_gpio_platform_data *gpio_pdata;
+};

(quoted diff) +100 -1

+
+static int imx27_pinctrl_probe(struct platform_device *pdev)
+{
+ struct device_node *child;
+ struct imx_pinctrl *ipctl;
+ struct imx27_pinctrl_private *pctl;
+ int ret;
+ u32 base_addr;
+ int num_gpios = 0;
+ int i = 0;
+
+ ret = of_property_read_u32(pdev->dev.of_node, "reg", &base_addr);
+ if (ret)
+ return ret;
+
+ ret = imx_pinctrl_probe(pdev, &imx27_pinctrl_info);
+ if (ret)
+ return ret;
+
+ pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+ if (!pctl)
+ return -ENOMEM;
+
+ ipctl = platform_get_drvdata(pdev);
+
+ for_each_available_child_of_node(pdev->dev.of_node, child) {
+ u32 memoffset;
+ if (!of_match_node(imx21_gpio_of_match, child))
+ continue;
+ ret = of_property_read_u32(child, "reg", &memoffset);
+ if (ret)
+ continue;
+ ++num_gpios;
+ }
+
+ pctl->num_gpio_childs = num_gpios;
+ pctl->gpio_dev = devm_kzalloc(&pdev->dev,
+ sizeof(*pctl->gpio_dev) * num_gpios, GFP_KERNEL);
+ pctl->gpio_pdata = devm_kzalloc(&pdev->dev,
+ sizeof(*pctl->gpio_pdata) * num_gpios, GFP_KERNEL);
+
+ if (!pctl->gpio_pdata || !pctl->gpio_dev)
+ return -ENOMEM;
+
+
+ for_each_available_child_of_node(pdev->dev.of_node, child) {
+ u32 memoffset;
+
+ if (!of_match_node(imx21_gpio_of_match, child))
+ continue;
+ ret = of_property_read_u32(child, "reg", &memoffset);
+ if (ret)
+ continue;
+
+ memoffset -= base_addr;
+ pctl->gpio_pdata[i].base = ipctl->base + memoffset;
+
+ pctl->gpio_dev[i] = of_device_alloc(child, NULL, &pdev->dev);
+ pctl->gpio_dev[i]->dev.platform_data = &pctl->gpio_pdata[i];
+ pctl->gpio_dev[i]->dev.bus = &platform_bus_type;
+
+ ret = of_device_add(pctl->gpio_dev[i]);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to add child gpio device\n");
+ goto gpio_err;
+ }
+ ++i;
+ }
+ return 0;
+gpio_err:
+ while (i--)
+ put_device(&pctl->gpio_dev[i]->dev);
+ return ret;
+}
+
+static struct platform_driver imx27_pinctrl_driver = {
+ .driver = {
+ .name = "imx27-pinctrl",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(imx27_pinctrl_of_match),
+ },
+ .probe = imx27_pinctrl_probe,
+ .remove = imx_pinctrl_remove,
+};
+
+static int __init imx27_pinctrl_init(void)
+{
+ return platform_driver_register(&imx27_pinctrl_driver);
+}
+arch_initcall(imx27_pinctrl_init);
+
+static void __exit imx27_pinctrl_exit(void)
+{
+ platform_driver_unregister(&imx27_pinctrl_driver);
+}
+module_exit(imx27_pinctrl_exit);
+MODULE_AUTHOR("Markus Pargmann <[email protected]>");
+MODULE_DESCRIPTION("Freescale IMX27 pinctrl driver");
+MODULE_LICENSE("GPL v2");
--

1.8.3.2

Review

Quoted 12 lines · Markus Pargmann

(quoted diff) +12 -0

+static struct imx_pinctrl_soc_info imx27_pinctrl_info = {
+ .pins = imx27_pinctrl_pads,
+ .npins = ARRAY_SIZE(imx27_pinctrl_pads),
+};
+
+static struct of_device_id imx27_pinctrl_of_match[] = {
+ { .compatible = "fsl,imx27-iomuxc", },
+ { /* sentinel */ }
+};
+
+static struct of_device_id imx21_gpio_of_match[] = {
+ { .compatible = "fsl,imx21-gpio", },

Since we also have "fsl,imx27-gpio" in the compatible string, we can use it here.

Review

Quoted 6 lines

+ { /* sentinel */ }

(quoted diff) +4 -0

+};
+
+struct imx27_pinctrl_private {
+ int num_gpio_childs;

Maybe I missed something. But I see this member is used nowhere.

Quoted 3 lines

+ struct platform_device **gpio_dev;
+ struct mxc_gpio_platform_data *gpio_pdata;
+};

I do not see too much point to have this structure at all.

Shawn

Quoted 103 lines

(quoted diff) +100 -1

+
+static int imx27_pinctrl_probe(struct platform_device *pdev)
+{
+ struct device_node *child;
+ struct imx_pinctrl *ipctl;
+ struct imx27_pinctrl_private *pctl;
+ int ret;
+ u32 base_addr;
+ int num_gpios = 0;
+ int i = 0;
+
+ ret = of_property_read_u32(pdev->dev.of_node, "reg", &base_addr);
+ if (ret)
+ return ret;
+
+ ret = imx_pinctrl_probe(pdev, &imx27_pinctrl_info);
+ if (ret)
+ return ret;
+
+ pctl = devm_kzalloc(&pdev->dev, sizeof(*pctl), GFP_KERNEL);
+ if (!pctl)
+ return -ENOMEM;
+
+ ipctl = platform_get_drvdata(pdev);
+
+ for_each_available_child_of_node(pdev->dev.of_node, child) {
+ u32 memoffset;
+ if (!of_match_node(imx21_gpio_of_match, child))
+ continue;
+ ret = of_property_read_u32(child, "reg", &memoffset);
+ if (ret)
+ continue;
+ ++num_gpios;
+ }
+
+ pctl->num_gpio_childs = num_gpios;
+ pctl->gpio_dev = devm_kzalloc(&pdev->dev,
+ sizeof(*pctl->gpio_dev) * num_gpios, GFP_KERNEL);
+ pctl->gpio_pdata = devm_kzalloc(&pdev->dev,
+ sizeof(*pctl->gpio_pdata) * num_gpios, GFP_KERNEL);
+
+ if (!pctl->gpio_pdata || !pctl->gpio_dev)
+ return -ENOMEM;
+
+
+ for_each_available_child_of_node(pdev->dev.of_node, child) {
+ u32 memoffset;
+
+ if (!of_match_node(imx21_gpio_of_match, child))
+ continue;
+ ret = of_property_read_u32(child, "reg", &memoffset);
+ if (ret)
+ continue;
+
+ memoffset -= base_addr;
+ pctl->gpio_pdata[i].base = ipctl->base + memoffset;
+
+ pctl->gpio_dev[i] = of_device_alloc(child, NULL, &pdev->dev);
+ pctl->gpio_dev[i]->dev.platform_data = &pctl->gpio_pdata[i];
+ pctl->gpio_dev[i]->dev.bus = &platform_bus_type;
+
+ ret = of_device_add(pctl->gpio_dev[i]);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to add child gpio device\n");
+ goto gpio_err;
+ }
+ ++i;
+ }
+ return 0;
+gpio_err:
+ while (i--)
+ put_device(&pctl->gpio_dev[i]->dev);
+ return ret;
+}
+
+static struct platform_driver imx27_pinctrl_driver = {
+ .driver = {
+ .name = "imx27-pinctrl",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(imx27_pinctrl_of_match),
+ },
+ .probe = imx27_pinctrl_probe,
+ .remove = imx_pinctrl_remove,
+};
+
+static int __init imx27_pinctrl_init(void)
+{
+ return platform_driver_register(&imx27_pinctrl_driver);
+}
+arch_initcall(imx27_pinctrl_init);
+
+static void __exit imx27_pinctrl_exit(void)
+{
+ platform_driver_unregister(&imx27_pinctrl_driver);
+}
+module_exit(imx27_pinctrl_exit);
+MODULE_AUTHOR("Markus Pargmann <[email protected]>");
+MODULE_DESCRIPTION("Freescale IMX27 pinctrl driver");
+MODULE_LICENSE("GPL v2");
--

1.8.3.2