[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Dong Aisheng]

---
This patch series shows the basic idea of driver design. There're still
some TODOes like adding more pinmux functions and gpio support.
The patch is here for request for comments on the driver design
and other might exist issues.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shanw.guo@freescale.com>
---
 drivers/pinctrl/Kconfig           |    6 +
 drivers/pinctrl/Makefile          |    2 +
 drivers/pinctrl/pinmux-imx-core.c |  284 +++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinmux-imx-core.h |   83 +++++++++++
 4 files changed, 375 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index ef56644..214d072 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -40,4 +40,10 @@ config PINMUX_U300
 	help
 	  Say Y here to enable the U300 pinmux driver
 
+config PINMUX_IMX
+	bool "IMX pinmux driver"
+	depends on ARCH_MXC
+	select PINMUX
+	help
+	  Say Y here to enable the IMX pinmux driver
 endif
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index bdc548a..764657b 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -4,5 +4,7 @@ ccflags-$(CONFIG_DEBUG_PINMUX)	+= -DDEBUG
 
 obj-$(CONFIG_PINCTRL)		+= core.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
+obj-$(CONFIG_PINMUX_IMX)	+= pinmux-imx-core.o \
+				   pinmux-imx53.o pinmux-imx6q.o
 obj-$(CONFIG_PINMUX_SIRF)	+= pinmux-sirf.o
 obj-$(CONFIG_PINMUX_U300)	+= pinmux-u300.o
diff --git a/drivers/pinctrl/pinmux-imx-core.c b/drivers/pinctrl/pinmux-imx-core.c
new file mode 100644
index 0000000..1e60932
--- /dev/null
+++ b/drivers/pinctrl/pinmux-imx-core.c
@@ -0,0 +1,284 @@
+/*
+ * Core driver for the imx pin controller
+ *
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ * Copyright (C) 2011 Linaro Ltd.
+ *
+ * Author: Dong Aisheng <dong.aisheng@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinmux-imx-core.h"
+
+#define DRIVER_NAME "pinmux-imx"
+
+/**
+ * @dev: a pointer back to containing device
+ * @virtbase: the offset to the controller in virtual memory
+ */
+struct imx_pmx {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	void __iomem *virtbase;
+	struct imx_pinctrl_info *info;
+};
+
+#define IMX_PINCTRL_REG_SIZE 4
+#define IMX_PINCTRL_MAX_FUNC 7
+
+extern struct imx_pinctrl_info mx53_pinctrl_info;
+extern struct imx_pinctrl_info mx6q_pinctrl_info;
+
+static struct platform_device_id imx_pinctrl_devtype[] = {
+	{
+		.name = "sdhci-pinctrl-imx53",
+		.driver_data = (kernel_ulong_t)&mx53_pinctrl_info,
+	}, {
+		.name = "sdhci-pinctrl-imx6q",
+		.driver_data = (kernel_ulong_t)&mx6q_pinctrl_info,
+	},
+};
+
+static int imx_list_groups(struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+
+	if (selector >= info->ngroups)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const char *imx_get_group_name(struct pinctrl_dev *pctldev,
+				       unsigned selector)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+
+	if (selector >= info->ngroups)
+		return NULL;
+
+	return info->groups[selector].name;
+}
+
+static int imx_get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
+			       const unsigned **pins,
+			       unsigned *num_pins)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+
+	if (selector >= info->ngroups)
+		return -EINVAL;
+
+	*pins = info->groups[selector].pins;
+	*num_pins = info->groups[selector].num_pins;
+
+	return 0;
+}
+
+static void imx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+		   unsigned offset)
+{
+	seq_printf(s, " " DRIVER_NAME);
+}
+
+static struct pinctrl_ops imx_pctrl_ops = {
+	.list_groups = imx_list_groups,
+	.get_group_name = imx_get_group_name,
+	.get_group_pins = imx_get_group_pins,
+	.pin_dbg_show = imx_pin_dbg_show,
+};
+
+static int imx_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
+			   unsigned group)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+	const unsigned *pins, *mux;
+	unsigned int num_pins, num_mux;
+	u32 regval, offset;
+	int i;
+
+	/*
+	 * Configure the mux mode for each pin in the group for a specific
+	 * function.
+	 */
+	pins = info->groups[group].pins;
+	num_pins = info->groups[group].num_pins;
+	mux = info->groups[group].mux_mode;
+	num_mux = info->groups[group].num_mux;
+
+	dev_dbg(ipmx->dev, "function %s group %s\n",
+		info->functions[selector].name, info->groups[group].name);
+
+	if (num_pins != num_mux) {
+		dev_err(ipmx->dev, "num_mux is not equal to num_pins\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < num_pins; i++) {
+		if (mux[i] > IMX_PINCTRL_MAX_FUNC)
+			dev_err(ipmx->dev, "exceeds the maximum mux mode(0x7)\n");
+		offset = info->mux_offset + pins[i] * IMX_PINCTRL_REG_SIZE;
+		regval = readl(ipmx->virtbase + offset);
+		regval &= ~IMX_PINCTRL_MAX_FUNC;
+		writel(mux[i] | regval, ipmx->virtbase + offset);
+		dev_dbg(ipmx->dev, "write: offset 0x%x val 0x%x\n",
+			offset, regval);
+	}
+
+	return 0;
+}
+
+static int imx_pmx_list_funcs(struct pinctrl_dev *pctldev, unsigned selector)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+
+	if (selector >= info->nfunctions)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const char *imx_pmx_get_func_name(struct pinctrl_dev *pctldev,
+					  unsigned selector)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+
+	return info->functions[selector].name;
+}
+
+static int imx_pmx_get_groups(struct pinctrl_dev *pctldev, unsigned selector,
+			       const char * const **groups,
+			       unsigned * const num_groups)
+{
+	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
+	struct imx_pinctrl_info *info = ipmx->info;
+
+	*groups = info->functions[selector].groups;
+	*num_groups = info->functions[selector].num_groups;
+
+	return 0;
+}
+
+static struct pinmux_ops imx_pmx_ops = {
+	.list_functions = imx_pmx_list_funcs,
+	.get_function_name = imx_pmx_get_func_name,
+	.get_function_groups = imx_pmx_get_groups,
+	.enable = imx_pmx_enable,
+};
+
+static struct pinctrl_desc imx_pmx_desc = {
+	.name = DRIVER_NAME,
+	.pctlops = &imx_pctrl_ops,
+	.pmxops = &imx_pmx_ops,
+	.owner = THIS_MODULE,
+};
+
+static inline void imx_pmx_desc_init(struct pinctrl_desc *pmx_desc,
+				const struct imx_pinctrl_info *info)
+{
+	pmx_desc->pins = info->pins;
+	pmx_desc->npins = info->npins;
+	pmx_desc->maxpin = info->maxpin;
+}
+
+static int __init imx_pmx_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_pmx *ipmx;
+	struct resource *res;
+	struct imx_pinctrl_info *info;
+	resource_size_t res_size;
+
+	info = (struct imx_pinctrl_info *)pdev->id_entry->driver_data;
+	if (!info || !info->pins || !info->groups || !info->functions
+		|| (info->maxpin > info->npins)) {
+		dev_err(&pdev->dev, "wrong pinctrl info\n");
+		return -EINVAL;
+	}
+
+	/* Create state holders etc for this driver */
+	ipmx = devm_kzalloc(&pdev->dev, sizeof(*ipmx), GFP_KERNEL);
+	if (!ipmx)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENOENT;
+
+	res_size = resource_size(res);
+	if (!devm_request_mem_region(dev, res->start, res_size, res->name))
+		return -EBUSY;
+
+	ipmx->virtbase = devm_ioremap_nocache(dev, res->start, res_size);
+	if (!ipmx->virtbase)
+		return -EBUSY;
+
+	imx_pmx_desc_init(&imx_pmx_desc, info);
+
+	ipmx->pctl = pinctrl_register(&imx_pmx_desc, &pdev->dev, ipmx);
+	if (!ipmx->pctl) {
+		dev_err(&pdev->dev, "could not register IMX pinmux driver\n");
+		return -EINVAL;
+	}
+
+	ipmx->info = info;
+	ipmx->dev = &pdev->dev;
+	platform_set_drvdata(pdev, ipmx);
+
+	dev_info(&pdev->dev, "initialized IMX pinmux driver\n");
+
+	return 0;
+}
+
+static int __exit imx_pmx_remove(struct platform_device *pdev)
+{
+	struct imx_pmx *ipmx = platform_get_drvdata(pdev);
+
+	pinctrl_unregister(ipmx->pctl);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver imx_pmx_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.owner = THIS_MODULE,
+	},
+	.id_table = imx_pinctrl_devtype,
+	.remove = __exit_p(imx_pmx_remove),
+};
+
+static int __init imx_pmx_init(void)
+{
+	return platform_driver_probe(&imx_pmx_driver, imx_pmx_probe);
+}
+arch_initcall(imx_pmx_init);
+
+static void __exit imx_pmx_exit(void)
+{
+	platform_driver_unregister(&imx_pmx_driver);
+}
+module_exit(imx_pmx_exit);
+
+MODULE_AUTHOR("Dong Aisheng <dong.aisheng@linaro.org>");
+MODULE_DESCRIPTION("IMX Pin Control Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pinctrl/pinmux-imx-core.h b/drivers/pinctrl/pinmux-imx-core.h
new file mode 100644
index 0000000..8ccc0c6
--- /dev/null
+++ b/drivers/pinctrl/pinmux-imx-core.h
@@ -0,0 +1,83 @@
+/*
+ * IMX pinmux core definitions
+ *
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ * Copyright (C) 2011 Linaro Ltd.
+ *
+ * Author: Dong Aisheng <dong.aisheng@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __DRIVERS_PINCTRL_PINMUX_IMX_H
+#define __DRIVERS_PINCTRL_PINMUX_IMX_H
+
+/* Supported Pinctrl type */
+enum imx_pinctrl_type {
+	MX53_PINCTRL,
+	MX6Q_PINCTRL,
+};
+
+/**
+ * struct imx_pin_group - describes an IMX pin group
+ * @name: the name of this specific pin group
+ * @pins: an array of discrete physical pins used in this group, taken
+ *	from the driver-local pin enumeration space
+ * @num_pins: the number of pins in this group array, i.e. the number of
+ *	elements in .pins so we can iterate over that array
+ * @mux_mode: the mux mode for each pins in this group. The size of this
+ *	array is the same as pins.
+ */
+struct imx_pin_group {
+	const char *name;
+	const unsigned int *pins;
+	const unsigned num_pins;
+	const unsigned int *mux_mode;
+	const unsigned num_mux;
+};
+
+/**
+ * struct imx_pmx_func - describes IMX pinmux functions
+ * @name: the name of this specific function
+ * @groups: corresponding pin groups
+ */
+struct imx_pmx_func {
+	const char *name;
+	const char * const *groups;
+	const unsigned num_groups;
+};
+
+struct imx_pinctrl_info {
+	u32 type;
+	const struct pinctrl_pin_desc *pins;
+	unsigned int npins;
+	unsigned int maxpin;
+	const struct imx_pin_group *groups;
+	unsigned int ngroups;
+	const struct imx_pmx_func *functions;
+	unsigned int nfunctions;
+	u32 mux_offset;
+};
+
+#define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)
+
+#define IMX_PIN_GROUP(n, p, m)  \
+	{			\
+		.name = n,	\
+		.pins = p,	\
+		.num_pins = ARRAY_SIZE(p),	\
+		.mux_mode = m,	\
+		.num_mux = ARRAY_SIZE(m),	\
+	}
+
+#define IMX_PMX_FUNC(n, g)  \
+	{			\
+		.name = n,	\
+		.groups = g,	\
+		.num_groups = ARRAY_SIZE(g),	\
+	}
+
+#endif /* __DRIVERS_PINCTRL_PINMUX_IMX_H */
-- 
1.7.0.4

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng]

Signed-off-by: Dong Aisheng <b29396@freescale.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shanw.guo@freescale.com>
---
 drivers/pinctrl/pinmux-imx53.c |  514 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 514 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinmux-imx53.c b/drivers/pinctrl/pinmux-imx53.c
new file mode 100644
index 0000000..126e2a8
--- /dev/null
+++ b/drivers/pinctrl/pinmux-imx53.c
@@ -0,0 +1,514 @@
+/*
+ * imx53 pinmux driver based on imx pinmux core
+ *
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ * Copyright (C) 2011 Linaro, Inc.
+ *
+ * Author: Dong Aisheng <dong.aisheng@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinmux-imx-core.h"
+
+#define MX53_IOMUXC_MUX_OFFSET 0x20
+#define MX53_IOMUXC_MAXPIN	(23*23)
+
+enum imx_mx53_pads {
+	MX53_GPIO_19 = 0,
+	MX53_KEY_COL0 = 1,
+	MX53_KEY_ROW0 = 2,
+	MX53_KEY_COL1 = 3,
+	MX53_KEY_ROW1 = 4,
+	MX53_KEY_COL2 = 5,
+	MX53_KEY_ROW2 = 6,
+	MX53_KEY_COL3 = 7,
+	MX53_KEY_ROW3 = 8,
+	MX53_KEY_COL4 = 9,
+	MX53_KEY_ROW4 = 10,
+	MX53_DI0_DISP_CLK = 11,
+	MX53_DI0_PIN15 = 12,
+	MX53_DI0_PIN2 = 13,
+	MX53_DI0_PIN3 = 14,
+	MX53_DI0_PIN4 = 15,
+	MX53_DISP0_DAT0 = 16,
+	MX53_DISP0_DAT1 = 17,
+	MX53_DISP0_DAT2 = 18,
+	MX53_DISP0_DAT3 = 19,
+	MX53_DISP0_DAT4 = 20,
+	MX53_DISP0_DAT5 = 21,
+	MX53_DISP0_DAT6 = 22,
+	MX53_DISP0_DAT7 = 23,
+	MX53_DISP0_DAT8 = 24,
+	MX53_DISP0_DAT9 = 25,
+	MX53_DISP0_DAT10 = 26,
+	MX53_DISP0_DAT11 = 27,
+	MX53_DISP0_DAT12 = 28,
+	MX53_DISP0_DAT13 = 29,
+	MX53_DISP0_DAT14 = 30,
+	MX53_DISP0_DAT15 = 31,
+	MX53_DISP0_DAT16 = 32,
+	MX53_DISP0_DAT17 = 33,
+	MX53_DISP0_DAT18 = 34,
+	MX53_DISP0_DAT19 = 35,
+	MX53_DISP0_DAT20 = 36,
+	MX53_DISP0_DAT21 = 37,
+	MX53_DISP0_DAT22 = 38,
+	MX53_DISP0_DAT23 = 39,
+	MX53_CSI0_PIXCLK = 40,
+	MX53_CSI0_MCLK = 41,
+	MX53_CSI0_DATA_EN = 42,
+	MX53_CSI0_VSYNC = 43,
+	MX53_CSI0_DAT4 = 44,
+	MX53_CSI0_DAT5 = 45,
+	MX53_CSI0_DAT6 = 46,
+	MX53_CSI0_DAT7 = 47,
+	MX53_CSI0_DAT8 = 48,
+	MX53_CSI0_DAT9 = 49,
+	MX53_CSI0_DAT10 = 50,
+	MX53_CSI0_DAT11 = 51,
+	MX53_CSI0_DAT12 = 52,
+	MX53_CSI0_DAT13 = 53,
+	MX53_CSI0_DAT14 = 54,
+	MX53_CSI0_DAT15 = 55,
+	MX53_CSI0_DAT16 = 56,
+	MX53_CSI0_DAT17 = 57,
+	MX53_CSI0_DAT18 = 58,
+	MX53_CSI0_DAT19 = 59,
+	MX53_EIM_A25 = 60,
+	MX53_EIM_EB2 = 61,
+	MX53_EIM_D16 = 62,
+	MX53_EIM_D17 = 63,
+	MX53_EIM_D18 = 64,
+	MX53_EIM_D19 = 65,
+	MX53_EIM_D20 = 66,
+	MX53_EIM_D21 = 67,
+	MX53_EIM_D22 = 68,
+	MX53_EIM_D23 = 69,
+	MX53_EIM_EB3 = 70,
+	MX53_EIM_D24 = 71,
+	MX53_EIM_D25 = 72,
+	MX53_EIM_D26 = 73,
+	MX53_EIM_D27 = 74,
+	MX53_EIM_D28 = 75,
+	MX53_EIM_D29 = 76,
+	MX53_EIM_D30 = 77,
+	MX53_EIM_D31 = 78,
+	MX53_EIM_A24 = 79,
+	MX53_EIM_A23 = 80,
+	MX53_EIM_A22 = 81,
+	MX53_EIM_A21 = 82,
+	MX53_EIM_A20 = 83,
+	MX53_EIM_A19 = 84,
+	MX53_EIM_A18 = 85,
+	MX53_EIM_A17 = 86,
+	MX53_EIM_A16 = 87,
+	MX53_EIM_CS0 = 88,
+	MX53_EIM_CS1 = 89,
+	MX53_EIM_OE = 90,
+	MX53_EIM_RW = 91,
+	MX53_EIM_LBA = 92,
+	MX53_EIM_EB0 = 93,
+	MX53_EIM_EB1 = 94,
+	MX53_EIM_DA0 = 95,
+	MX53_EIM_DA1 = 96,
+	MX53_EIM_DA2 = 97,
+	MX53_EIM_DA3 = 98,
+	MX53_EIM_DA4 = 99,
+	MX53_EIM_DA5 = 100,
+	MX53_EIM_DA6 = 101,
+	MX53_EIM_DA7 = 102,
+	MX53_EIM_DA8 = 103,
+	MX53_EIM_DA9 = 104,
+	MX53_EIM_DA10 = 105,
+	MX53_EIM_DA11 = 106,
+	MX53_EIM_DA12 = 107,
+	MX53_EIM_DA13 = 108,
+	MX53_EIM_DA14 = 109,
+	MX53_EIM_DA15 = 110,
+	MX53_NANDF_WE_B = 111,
+	MX53_NANDF_RE_B = 112,
+	MX53_EIM_WAIT = 113,
+	MX53_EIM_BCLK = 114,
+	MX53_LVDS1_TX3_P = 115,
+	MX53_LVDS1_TX2_P = 116,
+	MX53_LVDS1_CLK_P = 117,
+	MX53_LVDS1_TX1_P = 118,
+	MX53_LVDS1_TX0_P = 119,
+	MX53_LVDS0_TX3_P = 120,
+	MX53_LVDS0_CLK_P = 121,
+	MX53_LVDS0_TX2_P = 122,
+	MX53_LVDS0_TX1_P = 123,
+	MX53_LVDS0_TX0_P = 124,
+	MX53_GPIO_10 = 125,
+	MX53_GPIO_11 = 126,
+	MX53_GPIO_12 = 127,
+	MX53_GPIO_13 = 128,
+	MX53_GPIO_14 = 129,
+	MX53_NANDF_CLE = 130,
+	MX53_NANDF_ALE = 131,
+	MX53_NANDF_WP_B = 132,
+	MX53_NANDF_RB0 = 133,
+	MX53_NANDF_CS0 = 134,
+	MX53_NANDF_CS1 = 135,
+	MX53_NANDF_CS2 = 136,
+	MX53_NANDF_CS3 = 137,
+	MX53_FEC_MDIO = 138,
+	MX53_FEC_REF_CLK = 139,
+	MX53_FEC_RX_ER = 140,
+	MX53_FEC_CRS_DV = 141,
+	MX53_FEC_RXD1 = 142,
+	MX53_FEC_RXD0 = 143,
+	MX53_FEC_TX_EN = 144,
+	MX53_FEC_TXD1 = 145,
+	MX53_FEC_TXD0 = 146,
+	MX53_FEC_MDC = 147,
+	MX53_PATA_DIOW = 148,
+	MX53_PATA_DMACK = 149,
+	MX53_PATA_DMARQ = 150,
+	MX53_PATA_BUFFER_EN = 151,
+	MX53_PATA_INTRQ = 152,
+	MX53_PATA_DIOR = 153,
+	MX53_PATA_RESET_B = 154,
+	MX53_PATA_IORDY = 155,
+	MX53_PATA_DA_0 = 156,
+	MX53_PATA_DA_1 = 157,
+	MX53_PATA_DA_2 = 158,
+	MX53_PATA_CS_0 = 159,
+	MX53_PATA_CS_1 = 160,
+	MX53_PATA_DATA0 = 161,
+	MX53_PATA_DATA1 = 162,
+	MX53_PATA_DATA2 = 163,
+	MX53_PATA_DATA3 = 164,
+	MX53_PATA_DATA4 = 165,
+	MX53_PATA_DATA5 = 166,
+	MX53_PATA_DATA6 = 167,
+	MX53_PATA_DATA7 = 168,
+	MX53_PATA_DATA8 = 169,
+	MX53_PATA_DATA9 = 170,
+	MX53_PATA_DATA10 = 171,
+	MX53_PATA_DATA11 = 172,
+	MX53_PATA_DATA12 = 173,
+	MX53_PATA_DATA13 = 174,
+	MX53_PATA_DATA14 = 175,
+	MX53_PATA_DATA15 = 176,
+	MX53_SD1_DATA0 = 177,
+	MX53_SD1_DATA1 = 178,
+	MX53_SD1_CMD = 179,
+	MX53_SD1_DATA2 = 180,
+	MX53_SD1_CLK = 181,
+	MX53_SD1_DATA3 = 182,
+	MX53_SD2_CLK = 183,
+	MX53_SD2_CMD = 184,
+	MX53_SD2_DATA3 = 185,
+	MX53_SD2_DATA2 = 186,
+	MX53_SD2_DATA1 = 187,
+	MX53_SD2_DATA0 = 188,
+	MX53_GPIO_0 = 189,
+	MX53_GPIO_1 = 190,
+	MX53_GPIO_9 = 191,
+	MX53_GPIO_3 = 192,
+	MX53_GPIO_6 = 193,
+	MX53_GPIO_2 = 194,
+	MX53_GPIO_4 = 195,
+	MX53_GPIO_5 = 196,
+	MX53_GPIO_7 = 197,
+	MX53_GPIO_8 = 198,
+	MX53_GPIO_16 = 199,
+	MX53_GPIO_17 = 200,
+	MX53_GPIO_18 = 201,
+};
+
+/* Pad names for the pinmux subsystem */
+static const struct pinctrl_pin_desc mx53_pads[] = {
+	IMX_PINCTRL_PIN(MX53_GPIO_19),
+	IMX_PINCTRL_PIN(MX53_KEY_COL0),
+	IMX_PINCTRL_PIN(MX53_KEY_ROW0),
+	IMX_PINCTRL_PIN(MX53_KEY_COL1),
+	IMX_PINCTRL_PIN(MX53_KEY_ROW1),
+	IMX_PINCTRL_PIN(MX53_KEY_COL2),
+	IMX_PINCTRL_PIN(MX53_KEY_ROW2),
+	IMX_PINCTRL_PIN(MX53_KEY_COL3),
+	IMX_PINCTRL_PIN(MX53_KEY_ROW3),
+	IMX_PINCTRL_PIN(MX53_KEY_COL4),
+	IMX_PINCTRL_PIN(MX53_KEY_ROW4),
+	IMX_PINCTRL_PIN(MX53_DI0_DISP_CLK),
+	IMX_PINCTRL_PIN(MX53_DI0_PIN15),
+	IMX_PINCTRL_PIN(MX53_DI0_PIN2),
+	IMX_PINCTRL_PIN(MX53_DI0_PIN3),
+	IMX_PINCTRL_PIN(MX53_DI0_PIN4),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT0),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT1),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT2),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT3),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT4),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT5),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT6),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT7),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT8),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT9),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT10),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT11),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT12),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT13),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT14),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT15),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT16),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT17),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT18),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT19),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT20),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT21),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT22),
+	IMX_PINCTRL_PIN(MX53_DISP0_DAT23),
+	IMX_PINCTRL_PIN(MX53_CSI0_PIXCLK),
+	IMX_PINCTRL_PIN(MX53_CSI0_MCLK),
+	IMX_PINCTRL_PIN(MX53_CSI0_DATA_EN),
+	IMX_PINCTRL_PIN(MX53_CSI0_VSYNC),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT4),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT5),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT6),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT7),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT8),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT9),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT10),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT11),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT12),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT13),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT14),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT15),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT16),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT17),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT18),
+	IMX_PINCTRL_PIN(MX53_CSI0_DAT19),
+	IMX_PINCTRL_PIN(MX53_EIM_A25),
+	IMX_PINCTRL_PIN(MX53_EIM_EB2),
+	IMX_PINCTRL_PIN(MX53_EIM_D16),
+	IMX_PINCTRL_PIN(MX53_EIM_D17),
+	IMX_PINCTRL_PIN(MX53_EIM_D18),
+	IMX_PINCTRL_PIN(MX53_EIM_D19),
+	IMX_PINCTRL_PIN(MX53_EIM_D20),
+	IMX_PINCTRL_PIN(MX53_EIM_D21),
+	IMX_PINCTRL_PIN(MX53_EIM_D22),
+	IMX_PINCTRL_PIN(MX53_EIM_D23),
+	IMX_PINCTRL_PIN(MX53_EIM_EB3),
+	IMX_PINCTRL_PIN(MX53_EIM_D24),
+	IMX_PINCTRL_PIN(MX53_EIM_D25),
+	IMX_PINCTRL_PIN(MX53_EIM_D26),
+	IMX_PINCTRL_PIN(MX53_EIM_D27),
+	IMX_PINCTRL_PIN(MX53_EIM_D28),
+	IMX_PINCTRL_PIN(MX53_EIM_D29),
+	IMX_PINCTRL_PIN(MX53_EIM_D30),
+	IMX_PINCTRL_PIN(MX53_EIM_D31),
+	IMX_PINCTRL_PIN(MX53_EIM_A24),
+	IMX_PINCTRL_PIN(MX53_EIM_A23),
+	IMX_PINCTRL_PIN(MX53_EIM_A22),
+	IMX_PINCTRL_PIN(MX53_EIM_A21),
+	IMX_PINCTRL_PIN(MX53_EIM_A20),
+	IMX_PINCTRL_PIN(MX53_EIM_A19),
+	IMX_PINCTRL_PIN(MX53_EIM_A18),
+	IMX_PINCTRL_PIN(MX53_EIM_A17),
+	IMX_PINCTRL_PIN(MX53_EIM_A16),
+	IMX_PINCTRL_PIN(MX53_EIM_CS0),
+	IMX_PINCTRL_PIN(MX53_EIM_CS1),
+	IMX_PINCTRL_PIN(MX53_EIM_OE),
+	IMX_PINCTRL_PIN(MX53_EIM_RW),
+	IMX_PINCTRL_PIN(MX53_EIM_LBA),
+	IMX_PINCTRL_PIN(MX53_EIM_EB0),
+	IMX_PINCTRL_PIN(MX53_EIM_EB1),
+	IMX_PINCTRL_PIN(MX53_EIM_DA0),
+	IMX_PINCTRL_PIN(MX53_EIM_DA1),
+	IMX_PINCTRL_PIN(MX53_EIM_DA2),
+	IMX_PINCTRL_PIN(MX53_EIM_DA3),
+	IMX_PINCTRL_PIN(MX53_EIM_DA4),
+	IMX_PINCTRL_PIN(MX53_EIM_DA5),
+	IMX_PINCTRL_PIN(MX53_EIM_DA6),
+	IMX_PINCTRL_PIN(MX53_EIM_DA7),
+	IMX_PINCTRL_PIN(MX53_EIM_DA8),
+	IMX_PINCTRL_PIN(MX53_EIM_DA9),
+	IMX_PINCTRL_PIN(MX53_EIM_DA10),
+	IMX_PINCTRL_PIN(MX53_EIM_DA11),
+	IMX_PINCTRL_PIN(MX53_EIM_DA12),
+	IMX_PINCTRL_PIN(MX53_EIM_DA13),
+	IMX_PINCTRL_PIN(MX53_EIM_DA14),
+	IMX_PINCTRL_PIN(MX53_EIM_DA15),
+	IMX_PINCTRL_PIN(MX53_NANDF_WE_B),
+	IMX_PINCTRL_PIN(MX53_NANDF_RE_B),
+	IMX_PINCTRL_PIN(MX53_EIM_WAIT),
+	IMX_PINCTRL_PIN(MX53_EIM_BCLK),
+	IMX_PINCTRL_PIN(MX53_LVDS1_TX3_P),
+	IMX_PINCTRL_PIN(MX53_LVDS1_TX2_P),
+	IMX_PINCTRL_PIN(MX53_LVDS1_CLK_P),
+	IMX_PINCTRL_PIN(MX53_LVDS1_TX1_P),
+	IMX_PINCTRL_PIN(MX53_LVDS1_TX0_P),
+	IMX_PINCTRL_PIN(MX53_LVDS0_TX3_P),
+	IMX_PINCTRL_PIN(MX53_LVDS0_CLK_P),
+	IMX_PINCTRL_PIN(MX53_LVDS0_TX2_P),
+	IMX_PINCTRL_PIN(MX53_LVDS0_TX1_P),
+	IMX_PINCTRL_PIN(MX53_LVDS0_TX0_P),
+	IMX_PINCTRL_PIN(MX53_GPIO_10),
+	IMX_PINCTRL_PIN(MX53_GPIO_11),
+	IMX_PINCTRL_PIN(MX53_GPIO_12),
+	IMX_PINCTRL_PIN(MX53_GPIO_13),
+	IMX_PINCTRL_PIN(MX53_GPIO_14),
+	IMX_PINCTRL_PIN(MX53_NANDF_CLE),
+	IMX_PINCTRL_PIN(MX53_NANDF_ALE),
+	IMX_PINCTRL_PIN(MX53_NANDF_WP_B),
+	IMX_PINCTRL_PIN(MX53_NANDF_RB0),
+	IMX_PINCTRL_PIN(MX53_NANDF_CS0),
+	IMX_PINCTRL_PIN(MX53_NANDF_CS1),
+	IMX_PINCTRL_PIN(MX53_NANDF_CS2),
+	IMX_PINCTRL_PIN(MX53_NANDF_CS3),
+	IMX_PINCTRL_PIN(MX53_FEC_MDIO),
+	IMX_PINCTRL_PIN(MX53_FEC_REF_CLK),
+	IMX_PINCTRL_PIN(MX53_FEC_RX_ER),
+	IMX_PINCTRL_PIN(MX53_FEC_CRS_DV),
+	IMX_PINCTRL_PIN(MX53_FEC_RXD1),
+	IMX_PINCTRL_PIN(MX53_FEC_RXD0),
+	IMX_PINCTRL_PIN(MX53_FEC_TX_EN),
+	IMX_PINCTRL_PIN(MX53_FEC_TXD1),
+	IMX_PINCTRL_PIN(MX53_FEC_TXD0),
+	IMX_PINCTRL_PIN(MX53_FEC_MDC),
+	IMX_PINCTRL_PIN(MX53_PATA_DIOW),
+	IMX_PINCTRL_PIN(MX53_PATA_DMACK),
+	IMX_PINCTRL_PIN(MX53_PATA_DMARQ),
+	IMX_PINCTRL_PIN(MX53_PATA_BUFFER_EN),
+	IMX_PINCTRL_PIN(MX53_PATA_INTRQ),
+	IMX_PINCTRL_PIN(MX53_PATA_DIOR),
+	IMX_PINCTRL_PIN(MX53_PATA_RESET_B),
+	IMX_PINCTRL_PIN(MX53_PATA_IORDY),
+	IMX_PINCTRL_PIN(MX53_PATA_DA_0),
+	IMX_PINCTRL_PIN(MX53_PATA_DA_1),
+	IMX_PINCTRL_PIN(MX53_PATA_DA_2),
+	IMX_PINCTRL_PIN(MX53_PATA_CS_0),
+	IMX_PINCTRL_PIN(MX53_PATA_CS_1),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA0),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA1),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA2),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA3),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA4),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA5),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA6),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA7),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA8),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA9),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA10),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA11),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA12),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA13),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA14),
+	IMX_PINCTRL_PIN(MX53_PATA_DATA15),
+	IMX_PINCTRL_PIN(MX53_SD1_DATA0),
+	IMX_PINCTRL_PIN(MX53_SD1_DATA1),
+	IMX_PINCTRL_PIN(MX53_SD1_CMD),
+	IMX_PINCTRL_PIN(MX53_SD1_DATA2),
+	IMX_PINCTRL_PIN(MX53_SD1_CLK),
+	IMX_PINCTRL_PIN(MX53_SD1_DATA3),
+	IMX_PINCTRL_PIN(MX53_SD2_CLK),
+	IMX_PINCTRL_PIN(MX53_SD2_CMD),
+	IMX_PINCTRL_PIN(MX53_SD2_DATA3),
+	IMX_PINCTRL_PIN(MX53_SD2_DATA2),
+	IMX_PINCTRL_PIN(MX53_SD2_DATA1),
+	IMX_PINCTRL_PIN(MX53_SD2_DATA0),
+	IMX_PINCTRL_PIN(MX53_GPIO_0),
+	IMX_PINCTRL_PIN(MX53_GPIO_1),
+	IMX_PINCTRL_PIN(MX53_GPIO_9),
+	IMX_PINCTRL_PIN(MX53_GPIO_3),
+	IMX_PINCTRL_PIN(MX53_GPIO_6),
+	IMX_PINCTRL_PIN(MX53_GPIO_2),
+	IMX_PINCTRL_PIN(MX53_GPIO_4),
+	IMX_PINCTRL_PIN(MX53_GPIO_5),
+	IMX_PINCTRL_PIN(MX53_GPIO_7),
+	IMX_PINCTRL_PIN(MX53_GPIO_8),
+	IMX_PINCTRL_PIN(MX53_GPIO_16),
+	IMX_PINCTRL_PIN(MX53_GPIO_17),
+	IMX_PINCTRL_PIN(MX53_GPIO_18),
+};
+
+/* mx53 pin groups and mux mode */
+static const unsigned mx53_fec_pins[] = {
+	MX53_FEC_MDC,
+	MX53_FEC_MDIO,
+	MX53_FEC_REF_CLK,
+	MX53_FEC_RX_ER,
+	MX53_FEC_CRS_DV,
+	MX53_FEC_RXD1,
+	MX53_FEC_RXD0,
+	MX53_FEC_TX_EN,
+	MX53_FEC_TXD1,
+	MX53_FEC_TXD0,
+};
+static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
+
+static const unsigned mx53_sd1_pins[] = {
+	MX53_SD1_CMD,
+	MX53_SD1_CLK,
+	MX53_SD1_DATA0,
+	MX53_SD1_DATA1,
+	MX53_SD1_DATA2,
+	MX53_SD1_DATA3,
+
+};
+static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
+
+static const unsigned mx53_sd3_pins[] = {
+	MX53_PATA_DATA8,
+	MX53_PATA_DATA9,
+	MX53_PATA_DATA10,
+	MX53_PATA_DATA11,
+	MX53_PATA_DATA0,
+	MX53_PATA_DATA1,
+	MX53_PATA_DATA2,
+	MX53_PATA_DATA3,
+	MX53_PATA_IORDY,
+	MX53_PATA_RESET_B,
+
+};
+static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
+
+static const unsigned mx53_uart1_pins[] = {
+	MX53_CSI0_DAT10,
+	MX53_CSI0_DAT11,
+};
+static const unsigned mx53_uart1_mux[] = { 2, 2 };
+
+static const struct imx_pin_group mx53_pin_groups[] = {
+	IMX_PIN_GROUP("fecgrp", mx53_fec_pins, mx53_fec_mux),
+	IMX_PIN_GROUP("sd1grp", mx53_sd1_pins, mx53_sd1_mux),
+	IMX_PIN_GROUP("sd3grp", mx53_sd3_pins, mx53_sd3_mux),
+	IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux),
+};
+
+/* mx53 funcs and groups */
+static const char * const fecgrps[] = { "fecgrp" };
+static const char * const sd1grps[] = { "sd1grp" };
+static const char * const sd3grps[] = { "sd3grp" };
+static const char * const uart1grps[] = { "uart1grp" };
+
+static const struct imx_pmx_func mx53_pmx_functions[] = {
+	IMX_PMX_FUNC("fec", fecgrps),
+	IMX_PMX_FUNC("sd1", sd1grps),
+	IMX_PMX_FUNC("sd3", sd3grps),
+	IMX_PMX_FUNC("uart1", uart1grps),
+};
+
+const struct imx_pinctrl_info mx53_pinctrl_info = {
+	.type = MX53_PINCTRL,
+	.pins = mx53_pads,
+	.npins = ARRAY_SIZE(mx53_pads),
+	.maxpin = MX53_IOMUXC_MAXPIN,
+	.groups = mx53_pin_groups,
+	.ngroups = ARRAY_SIZE(mx53_pin_groups),
+	.functions = mx53_pmx_functions,
+	.nfunctions = ARRAY_SIZE(mx53_pmx_functions),
+	.mux_offset = MX53_IOMUXC_MUX_OFFSET,
+};
-- 
1.7.0.4

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Sascha Hauer]

On Sun, Dec 04, 2011 at 07:49:43PM +0800, Dong Aisheng wrote:
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shanw.guo@freescale.com>
> ---
>  drivers/pinctrl/pinmux-imx53.c |  514 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 514 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinmux-imx53.c b/drivers/pinctrl/pinmux-imx53.c
> +
> +/* mx53 pin groups and mux mode */
> +static const unsigned mx53_fec_pins[] = {
> +	MX53_FEC_MDC,
> +	MX53_FEC_MDIO,
> +	MX53_FEC_REF_CLK,
> +	MX53_FEC_RX_ER,
> +	MX53_FEC_CRS_DV,
> +	MX53_FEC_RXD1,
> +	MX53_FEC_RXD0,
> +	MX53_FEC_TX_EN,
> +	MX53_FEC_TXD1,
> +	MX53_FEC_TXD0,
> +};
> +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };

The FEC_MDC could be routed to PAD_KEY_ROW2 or to PAD_FEC_MDC. Also
FEC_MDIO could be routed to either PAD_FEC_MDIO or to PAD_KEY_COL2.
For other fec pins also different options might exist. How does this
fit into this group scheme?

> +
> +static const unsigned mx53_sd1_pins[] = {
> +	MX53_SD1_CMD,
> +	MX53_SD1_CLK,
> +	MX53_SD1_DATA0,
> +	MX53_SD1_DATA1,
> +	MX53_SD1_DATA2,
> +	MX53_SD1_DATA3,
> +
> +};
> +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> +
> +static const unsigned mx53_sd3_pins[] = {
> +	MX53_PATA_DATA8,
> +	MX53_PATA_DATA9,
> +	MX53_PATA_DATA10,
> +	MX53_PATA_DATA11,
> +	MX53_PATA_DATA0,
> +	MX53_PATA_DATA1,
> +	MX53_PATA_DATA2,
> +	MX53_PATA_DATA3,
> +	MX53_PATA_IORDY,
> +	MX53_PATA_RESET_B,
> +
> +};
> +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
> +
> +static const unsigned mx53_uart1_pins[] = {
> +	MX53_CSI0_DAT10,
> +	MX53_CSI0_DAT11,
> +};
> +static const unsigned mx53_uart1_mux[] = { 2, 2 };

For uart1 indeed only one routing possibility exists, but look at uart2:

uart2 txd -> PAD_EIM_D26
	  -> PAD_PATA_DMARQ
	  -> PAD_GPIO_7

uart2 rxd -> PAD_EIM_D27
	  -> PAD_PATA_BUFFER_EN
	  -> PAD_GPIO_8

So this at least means that you should not name the array above
mx53_uart1_mux, but something like mx53_uart1_option1,
mx53_uart1_option2 and so on.

Then it's probably possible to use mixtures of different options
for the uart.

I don't think that this grouping of pads to their functions makes
sense. On i.MX every pad is muxed independently and not in groups.
Which pins belong to which function is board specific and not SoC
specific.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng]

Hi Sascha,

2011/12/5 Sascha Hauer <s.hauer@pengutronix.de>:
> On Sun, Dec 04, 2011 at 07:49:43PM +0800, Dong Aisheng wrote:
>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Shawn Guo <shanw.guo@freescale.com>
>> ---
>> ?drivers/pinctrl/pinmux-imx53.c | ?514 ++++++++++++++++++++++++++++++++++++++++
>> ?1 files changed, 514 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinmux-imx53.c b/drivers/pinctrl/pinmux-imx53.c
>> +
>> +/* mx53 pin groups and mux mode */
>> +static const unsigned mx53_fec_pins[] = {
>> + ? ? MX53_FEC_MDC,
>> + ? ? MX53_FEC_MDIO,
>> + ? ? MX53_FEC_REF_CLK,
>> + ? ? MX53_FEC_RX_ER,
>> + ? ? MX53_FEC_CRS_DV,
>> + ? ? MX53_FEC_RXD1,
>> + ? ? MX53_FEC_RXD0,
>> + ? ? MX53_FEC_TX_EN,
>> + ? ? MX53_FEC_TXD1,
>> + ? ? MX53_FEC_TXD0,
>> +};
>> +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>
> The FEC_MDC could be routed to PAD_KEY_ROW2 or to PAD_FEC_MDC. Also
> FEC_MDIO could be routed to either PAD_FEC_MDIO or to PAD_KEY_COL2.
> For other fec pins also different options might exist. How does this
> fit into this group scheme?
>
Thanks for the review.
Here it's only one group for fec function.
If i understood right, for current pinmux desgin, pins are arranged in
groups per specifc functions.
For the case FEC_MDC could be routed to KEY_ROW2, we need define a KEY group
for key function.

Currently, i only added one group for one function with the reference
of MX53 LOCO pinmux defines.
And it's a big work which is in the TODO list that for me to manually
add all possible functions and pin groups.
So I send out the patch first to see if any design issue for the FSL
iomux specialist.

>> +
>> +static const unsigned mx53_sd1_pins[] = {
>> + ? ? MX53_SD1_CMD,
>> + ? ? MX53_SD1_CLK,
>> + ? ? MX53_SD1_DATA0,
>> + ? ? MX53_SD1_DATA1,
>> + ? ? MX53_SD1_DATA2,
>> + ? ? MX53_SD1_DATA3,
>> +
>> +};
>> +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
>> +
>> +static const unsigned mx53_sd3_pins[] = {
>> + ? ? MX53_PATA_DATA8,
>> + ? ? MX53_PATA_DATA9,
>> + ? ? MX53_PATA_DATA10,
>> + ? ? MX53_PATA_DATA11,
>> + ? ? MX53_PATA_DATA0,
>> + ? ? MX53_PATA_DATA1,
>> + ? ? MX53_PATA_DATA2,
>> + ? ? MX53_PATA_DATA3,
>> + ? ? MX53_PATA_IORDY,
>> + ? ? MX53_PATA_RESET_B,
>> +
>> +};
>> +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
>> +
>> +static const unsigned mx53_uart1_pins[] = {
>> + ? ? MX53_CSI0_DAT10,
>> + ? ? MX53_CSI0_DAT11,
>> +};
>> +static const unsigned mx53_uart1_mux[] = { 2, 2 };
>
> For uart1 indeed only one routing possibility exists, but look at uart2:
>
> uart2 txd -> PAD_EIM_D26
> ? ? ? ? ?-> PAD_PATA_DMARQ
> ? ? ? ? ?-> PAD_GPIO_7
>
> uart2 rxd -> PAD_EIM_D27
> ? ? ? ? ?-> PAD_PATA_BUFFER_EN
> ? ? ? ? ?-> PAD_GPIO_8
>
> So this at least means that you should not name the array above
> mx53_uart1_mux, but something like mx53_uart1_option1,
> mx53_uart1_option2 and so on.
>
Yes, acked on this.
I may take this name option.

> Then it's probably possible to use mixtures of different options
> for the uart.
How mixture?
Can you describe more or give an example?

> I don't think that this grouping of pads to their functions makes
> sense. On i.MX every pad is muxed independently and not in groups.
> Which pins belong to which function is board specific and not SoC
> specific.
>
Yes, i noted this.
As i said above, pins seem to be arranged in groups per functions not
per pins itself.
So our to do is define all possible pin function and group for board to use.
But obviously, it's a big work. Especially for your UART2 work, it
might be a headache since many possible combination of pins.
I still not find the good idea.
My current plan is to define all (might be frequently) used functoin
and groups for the exist upstreamed board like 53 Loco and etc, is
that ok?

Regards
Dong Aisheng

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Sascha Hauer]

On Mon, Dec 05, 2011 at 10:43:02AM +0800, Dong Aisheng wrote:
> Hi Sascha,
> 
> 2011/12/5 Sascha Hauer <s.hauer@pengutronix.de>:
> > On Sun, Dec 04, 2011 at 07:49:43PM +0800, Dong Aisheng wrote:
> >> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> >> Cc: Linus Walleij <linus.walleij@linaro.org>
> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >> Cc: Shawn Guo <shanw.guo@freescale.com>
> >> ---
> >> ?drivers/pinctrl/pinmux-imx53.c | ?514 ++++++++++++++++++++++++++++++++++++++++
> >> ?1 files changed, 514 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/pinmux-imx53.c b/drivers/pinctrl/pinmux-imx53.c
> >> +
> >> +/* mx53 pin groups and mux mode */
> >> +static const unsigned mx53_fec_pins[] = {
> >> + ? ? MX53_FEC_MDC,
> >> + ? ? MX53_FEC_MDIO,
> >> + ? ? MX53_FEC_REF_CLK,
> >> + ? ? MX53_FEC_RX_ER,
> >> + ? ? MX53_FEC_CRS_DV,
> >> + ? ? MX53_FEC_RXD1,
> >> + ? ? MX53_FEC_RXD0,
> >> + ? ? MX53_FEC_TX_EN,
> >> + ? ? MX53_FEC_TXD1,
> >> + ? ? MX53_FEC_TXD0,
> >> +};
> >> +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> >
> > The FEC_MDC could be routed to PAD_KEY_ROW2 or to PAD_FEC_MDC. Also
> > FEC_MDIO could be routed to either PAD_FEC_MDIO or to PAD_KEY_COL2.
> > For other fec pins also different options might exist. How does this
> > fit into this group scheme?
> >
> Thanks for the review.
> Here it's only one group for fec function.
> If i understood right, for current pinmux desgin, pins are arranged in
> groups per specifc functions.
> For the case FEC_MDC could be routed to KEY_ROW2, we need define a KEY group
> for key function.
> 
> Currently, i only added one group for one function with the reference
> of MX53 LOCO pinmux defines.
> And it's a big work which is in the TODO list that for me to manually
> add all possible functions and pin groups.
> So I send out the patch first to see if any design issue for the FSL
> iomux specialist.
> 
> >> +
> >> +static const unsigned mx53_sd1_pins[] = {
> >> + ? ? MX53_SD1_CMD,
> >> + ? ? MX53_SD1_CLK,
> >> + ? ? MX53_SD1_DATA0,
> >> + ? ? MX53_SD1_DATA1,
> >> + ? ? MX53_SD1_DATA2,
> >> + ? ? MX53_SD1_DATA3,
> >> +
> >> +};
> >> +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> >> +
> >> +static const unsigned mx53_sd3_pins[] = {
> >> + ? ? MX53_PATA_DATA8,
> >> + ? ? MX53_PATA_DATA9,
> >> + ? ? MX53_PATA_DATA10,
> >> + ? ? MX53_PATA_DATA11,
> >> + ? ? MX53_PATA_DATA0,
> >> + ? ? MX53_PATA_DATA1,
> >> + ? ? MX53_PATA_DATA2,
> >> + ? ? MX53_PATA_DATA3,
> >> + ? ? MX53_PATA_IORDY,
> >> + ? ? MX53_PATA_RESET_B,
> >> +
> >> +};
> >> +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
> >> +
> >> +static const unsigned mx53_uart1_pins[] = {
> >> + ? ? MX53_CSI0_DAT10,
> >> + ? ? MX53_CSI0_DAT11,
> >> +};
> >> +static const unsigned mx53_uart1_mux[] = { 2, 2 };
> >
> > For uart1 indeed only one routing possibility exists, but look at uart2:
> >
> > uart2 txd -> PAD_EIM_D26
> > ? ? ? ? ?-> PAD_PATA_DMARQ
> > ? ? ? ? ?-> PAD_GPIO_7
> >
> > uart2 rxd -> PAD_EIM_D27
> > ? ? ? ? ?-> PAD_PATA_BUFFER_EN
> > ? ? ? ? ?-> PAD_GPIO_8
> >
> > So this at least means that you should not name the array above
> > mx53_uart1_mux, but something like mx53_uart1_option1,
> > mx53_uart1_option2 and so on.
> >
> Yes, acked on this.
> I may take this name option.

BTW the name _option1 was only meant as an example. mx53_uart2_eim,
mx53_uart2_pata might make more sense.

> 
> > Then it's probably possible to use mixtures of different options
> > for the uart.
> How mixture?
> Can you describe more or give an example?

For example a board might have uart2 txd -> PAD_EIM_D26 and uart2 rxd ->
PAD_PATA_BUFFER_EN. I don't know if this is done somewhere, but it's
technically possible.

> 
> > I don't think that this grouping of pads to their functions makes
> > sense. On i.MX every pad is muxed independently and not in groups.
> > Which pins belong to which function is board specific and not SoC
> > specific.
> >
> Yes, i noted this.
> As i said above, pins seem to be arranged in groups per functions not
> per pins itself.
> So our to do is define all possible pin function and group for board to use.
> But obviously, it's a big work. Especially for your UART2 work, it
> might be a headache since many possible combination of pins.
> I still not find the good idea.
> My current plan is to define all (might be frequently) used functoin
> and groups for the exist upstreamed board like 53 Loco and etc, is
> that ok?

As said, I don't think that it's a good idea to group pins together on a
per SoC base. I rather think that if we want to go for groups of pins
belonging to a device, the board has to define those groups, not the
SoC.
If you really want to go for SoC specific Pins groups I don't
necessarily want to see 'everything for LOCO', I rather want to see
'everything for uart2', including RTS/CTS, modem control pins (if
applicable) and an example of using those pins as gpio. Another
interesting thing is the IPU. While a UART might have three groups
(rxd+txd, rts+cts, dtr+dcd+dsr) the IPU groups are not so easy to
identify. We have pins for two displays, each of them might be 8bit,
15bit, 18bit or 24bit. There are different sync signals for each
display, some of them maybe unused on certain setups. The unused pins
shouldn't be in the groups as you could use them as gpios instead.
By putting the iomux pins in SoC specific groups you try to press the
hardware into a software design for which it is not designed for. It
will always be easy to find a case which is not covered by any of
the predefined groups.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng-B29396]

Hi Sascha,

Sorry for the late reply.
I took one day leave for my passport processing yesterday.

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Monday, December 05, 2011 3:51 PM
> To: Dong Aisheng
> Cc: Dong Aisheng-B29396; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
> R65073; kernel at pengutronix.de
> Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> 
> On Mon, Dec 05, 2011 at 10:43:02AM +0800, Dong Aisheng wrote:
> > Hi Sascha,
> >
> > 2011/12/5 Sascha Hauer <s.hauer@pengutronix.de>:
> > > On Sun, Dec 04, 2011 at 07:49:43PM +0800, Dong Aisheng wrote:
> > >> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > >> Cc: Linus Walleij <linus.walleij@linaro.org>
> > >> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > >> Cc: Shawn Guo <shanw.guo@freescale.com>
> > >> ---
> > >> ?drivers/pinctrl/pinmux-imx53.c | ?514
> > >> ++++++++++++++++++++++++++++++++++++++++
> > >> ?1 files changed, 514 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/drivers/pinctrl/pinmux-imx53.c
> > >> b/drivers/pinctrl/pinmux-imx53.c
> > >> +
> > >> +/* mx53 pin groups and mux mode */ static const unsigned
> > >> +mx53_fec_pins[] = {
> > >> + ? ? MX53_FEC_MDC,
> > >> + ? ? MX53_FEC_MDIO,
> > >> + ? ? MX53_FEC_REF_CLK,
> > >> + ? ? MX53_FEC_RX_ER,
> > >> + ? ? MX53_FEC_CRS_DV,
> > >> + ? ? MX53_FEC_RXD1,
> > >> + ? ? MX53_FEC_RXD0,
> > >> + ? ? MX53_FEC_TX_EN,
> > >> + ? ? MX53_FEC_TXD1,
> > >> + ? ? MX53_FEC_TXD0,
> > >> +};
> > >> +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0,
> > >> +0, 0 };
> > >
> > > The FEC_MDC could be routed to PAD_KEY_ROW2 or to PAD_FEC_MDC. Also
> > > FEC_MDIO could be routed to either PAD_FEC_MDIO or to PAD_KEY_COL2.
> > > For other fec pins also different options might exist. How does this
> > > fit into this group scheme?
> > >
> > Thanks for the review.
> > Here it's only one group for fec function.
> > If i understood right, for current pinmux desgin, pins are arranged in
> > groups per specifc functions.
> > For the case FEC_MDC could be routed to KEY_ROW2, we need define a KEY
> > group for key function.
> >
> > Currently, i only added one group for one function with the reference
> > of MX53 LOCO pinmux defines.
> > And it's a big work which is in the TODO list that for me to manually
> > add all possible functions and pin groups.
> > So I send out the patch first to see if any design issue for the FSL
> > iomux specialist.
> >
> > >> +
> > >> +static const unsigned mx53_sd1_pins[] = {
> > >> + ? ? MX53_SD1_CMD,
> > >> + ? ? MX53_SD1_CLK,
> > >> + ? ? MX53_SD1_DATA0,
> > >> + ? ? MX53_SD1_DATA1,
> > >> + ? ? MX53_SD1_DATA2,
> > >> + ? ? MX53_SD1_DATA3,
> > >> +
> > >> +};
> > >> +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> > >> +
> > >> +static const unsigned mx53_sd3_pins[] = {
> > >> + ? ? MX53_PATA_DATA8,
> > >> + ? ? MX53_PATA_DATA9,
> > >> + ? ? MX53_PATA_DATA10,
> > >> + ? ? MX53_PATA_DATA11,
> > >> + ? ? MX53_PATA_DATA0,
> > >> + ? ? MX53_PATA_DATA1,
> > >> + ? ? MX53_PATA_DATA2,
> > >> + ? ? MX53_PATA_DATA3,
> > >> + ? ? MX53_PATA_IORDY,
> > >> + ? ? MX53_PATA_RESET_B,
> > >> +
> > >> +};
> > >> +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4,
> > >> +2, 2 };
> > >> +
> > >> +static const unsigned mx53_uart1_pins[] = {
> > >> + ? ? MX53_CSI0_DAT10,
> > >> + ? ? MX53_CSI0_DAT11,
> > >> +};
> > >> +static const unsigned mx53_uart1_mux[] = { 2, 2 };
> > >
> > > For uart1 indeed only one routing possibility exists, but look at
> uart2:
> > >
> > > uart2 txd -> PAD_EIM_D26
> > > ? ? ? ? ?-> PAD_PATA_DMARQ
> > > ? ? ? ? ?-> PAD_GPIO_7
> > >
> > > uart2 rxd -> PAD_EIM_D27
> > > ? ? ? ? ?-> PAD_PATA_BUFFER_EN
> > > ? ? ? ? ?-> PAD_GPIO_8
> > >
> > > So this at least means that you should not name the array above
> > > mx53_uart1_mux, but something like mx53_uart1_option1,
> > > mx53_uart1_option2 and so on.
> > >
> > Yes, acked on this.
> > I may take this name option.
> 
> BTW the name _option1 was only meant as an example. mx53_uart2_eim,
> mx53_uart2_pata might make more sense.
> 
I will take account of it.

> >
> > > Then it's probably possible to use mixtures of different options for
> > > the uart.
> > How mixture?
> > Can you describe more or give an example?
> 
> For example a board might have uart2 txd -> PAD_EIM_D26 and uart2 rxd ->
> PAD_PATA_BUFFER_EN. I don't know if this is done somewhere, but it's
> technically possible.
>
Yes, that's possible and I understand it might be a big trouble if we define
the pin groups per soc base per your idea.

> >
> > > I don't think that this grouping of pads to their functions makes
> > > sense. On i.MX every pad is muxed independently and not in groups.
> > > Which pins belong to which function is board specific and not SoC
> > > specific.
> > >
> > Yes, i noted this.
> > As i said above, pins seem to be arranged in groups per functions not
> > per pins itself.
> > So our to do is define all possible pin function and group for board to
> use.
> > But obviously, it's a big work. Especially for your UART2 work, it
> > might be a headache since many possible combination of pins.
> > I still not find the good idea.
> > My current plan is to define all (might be frequently) used functoin
> > and groups for the exist upstreamed board like 53 Loco and etc, is
> > that ok?
> 
> As said, I don't think that it's a good idea to group pins together on a
> per SoC base. I rather think that if we want to go for groups of pins
> belonging to a device, the board has to define those groups, not the SoC.
Acked.

> If you really want to go for SoC specific Pins groups I don't necessarily
> want to see 'everything for LOCO', I rather want to see 'everything for
> uart2', including RTS/CTS, modem control pins (if
> applicable) and an example of using those pins as gpio. Another
> interesting thing is the IPU. While a UART might have three groups
> (rxd+txd, rts+cts, dtr+dcd+dsr) the IPU groups are not so easy to
> identify. We have pins for two displays, each of them might be 8bit,
> 15bit, 18bit or 24bit. There are different sync signals for each display,
> some of them maybe unused on certain setups. The unused pins shouldn't be
> in the groups as you could use them as gpios instead.
Agree.

> By putting the iomux pins in SoC specific groups you try to press the
> hardware into a software design for which it is not designed for. It will
> always be easy to find a case which is not covered by any of the
> predefined groups.
> 
Agree.

I really understand your point for possible issues you mentioned here.
As you said, it may be suitable to define groups of pins in boards file, that
could save us much trouble and could make the things simple.

I want to say that your response is indeed what I need and it is the original
purpose I sent out the patches first for discussion without fully complete it.
Then, we can get a correct way to go.
Thanks for your suggestions.

Regards
Dong Aisheng

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Linus Walleij]

On Mon, Dec 5, 2011 at 3:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:

> My current plan is to define all (might be frequently) used functoin
> and groups for the exist upstreamed board like 53 Loco and etc, is
> that ok?

Yes, but do it in respective board file, so if we say, one day
stops to support a certain board we can just delete that board
file and be done with it.

Plus this gives us a nice separation as we move toward
device trees. (I think.)

Yours,
Linus Walleij

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Linus Walleij]

On Sun, Dec 4, 2011 at 5:11 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> I don't think that this grouping of pads to their functions makes
> sense. On i.MX every pad is muxed independently and not in groups.
> Which pins belong to which function is board specific and not SoC
> specific.

True, Dong can this be moved to the imx pinctrl platform data
and passed in from arch/arm/[plat|mach]-* board files instead?
That is probably where it belongs today.

I know that this data is in drivers/pinctrl for the U300 and SIRF,
but that is because they only have one single board variant.

Yours,
Linus Walleij

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij at linaro.org]
> Sent: Tuesday, December 06, 2011 1:01 AM
> To: Sascha Hauer
> Cc: Dong Aisheng-B29396; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
> R65073; kernel at pengutronix.de
> Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> 
> On Sun, Dec 4, 2011 at 5:11 PM, Sascha Hauer <s.hauer@pengutronix.de>
> wrote:
> 
> > I don't think that this grouping of pads to their functions makes
> > sense. On i.MX every pad is muxed independently and not in groups.
> > Which pins belong to which function is board specific and not SoC
> > specific.
> 
> True, Dong can this be moved to the imx pinctrl platform data and passed
> in from arch/arm/[plat|mach]-* board files instead?
> That is probably where it belongs today.
> 
Yes, I will do that.

> I know that this data is in drivers/pinctrl for the U300 and SIRF, but
> that is because they only have one single board variant.
> 
> Yours,
> Linus Walleij

Regards
Dong Aisheng

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Linus Walleij]

On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote:

> +enum imx_mx53_pads {
> + ? ? ? MX53_GPIO_19 = 0,
> + ? ? ? MX53_KEY_COL0 = 1,
(...)

First I thought it looked a bit strange since you needed enums for all pads
but then I realized that your macros use the same enumerator name to
name the pad and then it looks sort of clever.

But maybe put in a comment about that here:

> +/* Pad names for the pinmux subsystem */

Like this:

/*
 * Pad names for the pinmux subsystem.
 * These pad names are constructed from the pin enumerator names
 * in the IMX_PINCTRL_PIN() macro.
 */

> +static const struct pinctrl_pin_desc mx53_pads[] = {
> + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
(...)

> +/* mx53 pin groups and mux mode */
> +static const unsigned mx53_fec_pins[] = {
> + ? ? ? MX53_FEC_MDC,
> + ? ? ? MX53_FEC_MDIO,
> + ? ? ? MX53_FEC_REF_CLK,
> + ? ? ? MX53_FEC_RX_ER,
> + ? ? ? MX53_FEC_CRS_DV,
> + ? ? ? MX53_FEC_RXD1,
> + ? ? ? MX53_FEC_RXD0,
> + ? ? ? MX53_FEC_TX_EN,
> + ? ? ? MX53_FEC_TXD1,
> + ? ? ? MX53_FEC_TXD0,
> +};

I understand this.

> +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };

But what is this? Just zeroes? Why?
Especially with a const so they really cannot be anything
else. The same pin (0) can only be enumerated once.

> +static const unsigned mx53_sd1_pins[] = {
> + ? ? ? MX53_SD1_CMD,
> + ? ? ? MX53_SD1_CLK,
> + ? ? ? MX53_SD1_DATA0,
> + ? ? ? MX53_SD1_DATA1,
> + ? ? ? MX53_SD1_DATA2,
> + ? ? ? MX53_SD1_DATA3,
> +
> +};
> +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };

And here again.

> +static const unsigned mx53_sd3_pins[] = {
> + ? ? ? MX53_PATA_DATA8,
> + ? ? ? MX53_PATA_DATA9,
> + ? ? ? MX53_PATA_DATA10,
> + ? ? ? MX53_PATA_DATA11,
> + ? ? ? MX53_PATA_DATA0,
> + ? ? ? MX53_PATA_DATA1,
> + ? ? ? MX53_PATA_DATA2,
> + ? ? ? MX53_PATA_DATA3,
> + ? ? ? MX53_PATA_IORDY,
> + ? ? ? MX53_PATA_RESET_B,
> +
> +};
> +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };

This also looks strange. Can you explain what these muxes are?

> +static const unsigned mx53_uart1_pins[] = {
> + ? ? ? MX53_CSI0_DAT10,
> + ? ? ? MX53_CSI0_DAT11,
> +};
> +static const unsigned mx53_uart1_mux[] = { 2, 2 };

And here again?

> +static const struct imx_pin_group mx53_pin_groups[] = {
> + ? ? ? IMX_PIN_GROUP("fecgrp", mx53_fec_pins, mx53_fec_mux),
> + ? ? ? IMX_PIN_GROUP("sd1grp", mx53_sd1_pins, mx53_sd1_mux),
> + ? ? ? IMX_PIN_GROUP("sd3grp", mx53_sd3_pins, mx53_sd3_mux),
> + ? ? ? IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux),
> +};

So I understand the first and second argument to IMX_PIN_GROUP()
but not the third.

> +/* mx53 funcs and groups */
> +static const char * const fecgrps[] = { "fecgrp" };
> +static const char * const sd1grps[] = { "sd1grp" };
> +static const char * const sd3grps[] = { "sd3grp" };
> +static const char * const uart1grps[] = { "uart1grp" };
> +
> +static const struct imx_pmx_func mx53_pmx_functions[] = {
> + ? ? ? IMX_PMX_FUNC("fec", fecgrps),
> + ? ? ? IMX_PMX_FUNC("sd1", sd1grps),
> + ? ? ? IMX_PMX_FUNC("sd3", sd3grps),
> + ? ? ? IMX_PMX_FUNC("uart1", uart1grps),
> +};

This looks good.

Yours,
Linus Walleij

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Sascha Hauer]

On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:
> On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote:
> 
> > +enum imx_mx53_pads {
> > + ? ? ? MX53_GPIO_19 = 0,
> > + ? ? ? MX53_KEY_COL0 = 1,
> (...)
> 
> First I thought it looked a bit strange since you needed enums for all pads
> but then I realized that your macros use the same enumerator name to
> name the pad and then it looks sort of clever.
> 
> But maybe put in a comment about that here:
> 
> > +/* Pad names for the pinmux subsystem */
> 
> Like this:
> 
> /*
>  * Pad names for the pinmux subsystem.
>  * These pad names are constructed from the pin enumerator names
>  * in the IMX_PINCTRL_PIN() macro.
>  */
> 
> > +static const struct pinctrl_pin_desc mx53_pads[] = {
> > + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> > + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> (...)
> 
> > +/* mx53 pin groups and mux mode */
> > +static const unsigned mx53_fec_pins[] = {
> > + ? ? ? MX53_FEC_MDC,
> > + ? ? ? MX53_FEC_MDIO,
> > + ? ? ? MX53_FEC_REF_CLK,
> > + ? ? ? MX53_FEC_RX_ER,
> > + ? ? ? MX53_FEC_CRS_DV,
> > + ? ? ? MX53_FEC_RXD1,
> > + ? ? ? MX53_FEC_RXD0,
> > + ? ? ? MX53_FEC_TX_EN,
> > + ? ? ? MX53_FEC_TXD1,
> > + ? ? ? MX53_FEC_TXD0,
> > +};
> 
> I understand this.
> 
> > +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> 
> But what is this? Just zeroes? Why?
> Especially with a const so they really cannot be anything
> else. The same pin (0) can only be enumerated once.
> 
> > +static const unsigned mx53_sd1_pins[] = {
> > + ? ? ? MX53_SD1_CMD,
> > + ? ? ? MX53_SD1_CLK,
> > + ? ? ? MX53_SD1_DATA0,
> > + ? ? ? MX53_SD1_DATA1,
> > + ? ? ? MX53_SD1_DATA2,
> > + ? ? ? MX53_SD1_DATA3,
> > +
> > +};
> > +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> 
> And here again.
> 
> > +static const unsigned mx53_sd3_pins[] = {
> > + ? ? ? MX53_PATA_DATA8,
> > + ? ? ? MX53_PATA_DATA9,
> > + ? ? ? MX53_PATA_DATA10,
> > + ? ? ? MX53_PATA_DATA11,
> > + ? ? ? MX53_PATA_DATA0,
> > + ? ? ? MX53_PATA_DATA1,
> > + ? ? ? MX53_PATA_DATA2,
> > + ? ? ? MX53_PATA_DATA3,
> > + ? ? ? MX53_PATA_IORDY,
> > + ? ? ? MX53_PATA_RESET_B,
> > +
> > +};
> > +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
> 
> This also looks strange. Can you explain what these muxes are?

Freescale has named the pins after their primary function which is quite
confusing.

The above means:

MX53_PATA_DATA8 -> mux mode 4
MX53_PATA_DATA9 -> mux mode 4
...

This brings me to the point that currently we have the pins described as

#define MX53_PAD_<name>__<function>

which means that you don't have to look into the datasheet to get the
different options for a pin (and don't have a chance to get it wrong).
I don't really want to lose this.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, December 06, 2011 5:19 AM
> To: Linus Walleij
> Cc: Dong Aisheng-B29396; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
> R65073; kernel at pengutronix.de
> Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> 
> On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:
> > On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com>
> wrote:
> >
> > > +enum imx_mx53_pads {
> > > + ? ? ? MX53_GPIO_19 = 0,
> > > + ? ? ? MX53_KEY_COL0 = 1,
> > (...)
> >
> > First I thought it looked a bit strange since you needed enums for all
> > pads but then I realized that your macros use the same enumerator name
> > to name the pad and then it looks sort of clever.
> >
> > But maybe put in a comment about that here:
> >
> > > +/* Pad names for the pinmux subsystem */
> >
> > Like this:
> >
> > /*
> >  * Pad names for the pinmux subsystem.
> >  * These pad names are constructed from the pin enumerator names
> >  * in the IMX_PINCTRL_PIN() macro.
> >  */
> >
> > > +static const struct pinctrl_pin_desc mx53_pads[] = {
> > > + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> > > + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> > (...)
> >
> > > +/* mx53 pin groups and mux mode */
> > > +static const unsigned mx53_fec_pins[] = {
> > > + ? ? ? MX53_FEC_MDC,
> > > + ? ? ? MX53_FEC_MDIO,
> > > + ? ? ? MX53_FEC_REF_CLK,
> > > + ? ? ? MX53_FEC_RX_ER,
> > > + ? ? ? MX53_FEC_CRS_DV,
> > > + ? ? ? MX53_FEC_RXD1,
> > > + ? ? ? MX53_FEC_RXD0,
> > > + ? ? ? MX53_FEC_TX_EN,
> > > + ? ? ? MX53_FEC_TXD1,
> > > + ? ? ? MX53_FEC_TXD0,
> > > +};
> >
> > I understand this.
> >
> > > +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > > +0 };
> >
> > But what is this? Just zeroes? Why?
> > Especially with a const so they really cannot be anything else. The
> > same pin (0) can only be enumerated once.
> >
> > > +static const unsigned mx53_sd1_pins[] = {
> > > + ? ? ? MX53_SD1_CMD,
> > > + ? ? ? MX53_SD1_CLK,
> > > + ? ? ? MX53_SD1_DATA0,
> > > + ? ? ? MX53_SD1_DATA1,
> > > + ? ? ? MX53_SD1_DATA2,
> > > + ? ? ? MX53_SD1_DATA3,
> > > +
> > > +};
> > > +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> >
> > And here again.
> >
> > > +static const unsigned mx53_sd3_pins[] = {
> > > + ? ? ? MX53_PATA_DATA8,
> > > + ? ? ? MX53_PATA_DATA9,
> > > + ? ? ? MX53_PATA_DATA10,
> > > + ? ? ? MX53_PATA_DATA11,
> > > + ? ? ? MX53_PATA_DATA0,
> > > + ? ? ? MX53_PATA_DATA1,
> > > + ? ? ? MX53_PATA_DATA2,
> > > + ? ? ? MX53_PATA_DATA3,
> > > + ? ? ? MX53_PATA_IORDY,
> > > + ? ? ? MX53_PATA_RESET_B,
> > > +
> > > +};
> > > +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2,
> > > +2 };
> >
> > This also looks strange. Can you explain what these muxes are?
> 
> Freescale has named the pins after their primary function which is quite
> confusing.
> 
> The above means:
> 
> MX53_PATA_DATA8 -> mux mode 4
> MX53_PATA_DATA9 -> mux mode 4
> ...
> 
> This brings me to the point that currently we have the pins described as
> 
> #define MX53_PAD_<name>__<function>
> 
> which means that you don't have to look into the datasheet to get the
> different options for a pin (and don't have a chance to get it wrong).
> I don't really want to lose this.
> 
Obviously current used pin defines in that way is pretty good.
And I also don't want to lose this.

Actually I also have tried to see if we can reuse the current iomux-v3 code.

For current pinmux driver, one approach I can see is that define mux
in enum for each pin like:

enum MX53_PAD_GPIO_19_MUX {
        MX53_PAD_GPIO_19__KPP_COL_5,
        MX53_PAD_GPIO_19__GPIO4_5,
        MX53_PAD_GPIO_19__CCM_CLKO,
        MX53_PAD_GPIO_19__SPDIF_OUT1,
        MX53_PAD_GPIO_19__RTC_CE_RTC_EXT_TRIG2,
        MX53_PAD_GPIO_19__ECSPI1_RDY,
        MX53_PAD_GPIO_19__FEC_TDATA_3,
        MX53_PAD_GPIO_19__SRC_INT_BOOT,
};
Then put them in a common file for each mx53 based board to use.

Take uart1 as an example, it could be:
static const unsigned mx53_uart1_pins[] = {
        MX53_CSI0_DAT10,
        MX53_CSI0_DAT11,
};

static const unsigned mx53_uart1_mux[] = {
        MX53_CSI0_DAT10__UART1_TXD_MUX,
        MX53_CSI0_DAT11__UART1_RXD_MUX
};

static const struct imx_pin_group mx53_pin_groups[] = {
        IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux),
};

The benefit is that it's very clear and good maintainable.
The defect is it will increase the code size a lot by defining all
pin's mux enum and each pin's mux array in board file.

Do you think if it's ok or you have any better idea?

Regards
Dong Aisheng

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Shawn Guo]

On Tue, Dec 06, 2011 at 01:54:35PM +0800, Dong Aisheng-B29396 wrote:
> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > Sent: Tuesday, December 06, 2011 5:19 AM
> > To: Linus Walleij
> > Cc: Dong Aisheng-B29396; linux-kernel at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
> > R65073; kernel at pengutronix.de
> > Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> > 
> > On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:
> > > On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com>
> > wrote:
> > >
> > > > +enum imx_mx53_pads {
> > > > + ? ? ? MX53_GPIO_19 = 0,
> > > > + ? ? ? MX53_KEY_COL0 = 1,
> > > (...)
> > >
> > > First I thought it looked a bit strange since you needed enums for all
> > > pads but then I realized that your macros use the same enumerator name
> > > to name the pad and then it looks sort of clever.
> > >
> > > But maybe put in a comment about that here:
> > >
> > > > +/* Pad names for the pinmux subsystem */
> > >
> > > Like this:
> > >
> > > /*
> > >  * Pad names for the pinmux subsystem.
> > >  * These pad names are constructed from the pin enumerator names
> > >  * in the IMX_PINCTRL_PIN() macro.
> > >  */
> > >
> > > > +static const struct pinctrl_pin_desc mx53_pads[] = {
> > > > + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> > > > + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> > > (...)
> > >
> > > > +/* mx53 pin groups and mux mode */
> > > > +static const unsigned mx53_fec_pins[] = {
> > > > + ? ? ? MX53_FEC_MDC,
> > > > + ? ? ? MX53_FEC_MDIO,
> > > > + ? ? ? MX53_FEC_REF_CLK,
> > > > + ? ? ? MX53_FEC_RX_ER,
> > > > + ? ? ? MX53_FEC_CRS_DV,
> > > > + ? ? ? MX53_FEC_RXD1,
> > > > + ? ? ? MX53_FEC_RXD0,
> > > > + ? ? ? MX53_FEC_TX_EN,
> > > > + ? ? ? MX53_FEC_TXD1,
> > > > + ? ? ? MX53_FEC_TXD0,
> > > > +};
> > >
> > > I understand this.
> > >
> > > > +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > > > +0 };
> > >
> > > But what is this? Just zeroes? Why?
> > > Especially with a const so they really cannot be anything else. The
> > > same pin (0) can only be enumerated once.
> > >
> > > > +static const unsigned mx53_sd1_pins[] = {
> > > > + ? ? ? MX53_SD1_CMD,
> > > > + ? ? ? MX53_SD1_CLK,
> > > > + ? ? ? MX53_SD1_DATA0,
> > > > + ? ? ? MX53_SD1_DATA1,
> > > > + ? ? ? MX53_SD1_DATA2,
> > > > + ? ? ? MX53_SD1_DATA3,
> > > > +
> > > > +};
> > > > +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> > >
> > > And here again.
> > >
> > > > +static const unsigned mx53_sd3_pins[] = {
> > > > + ? ? ? MX53_PATA_DATA8,
> > > > + ? ? ? MX53_PATA_DATA9,
> > > > + ? ? ? MX53_PATA_DATA10,
> > > > + ? ? ? MX53_PATA_DATA11,
> > > > + ? ? ? MX53_PATA_DATA0,
> > > > + ? ? ? MX53_PATA_DATA1,
> > > > + ? ? ? MX53_PATA_DATA2,
> > > > + ? ? ? MX53_PATA_DATA3,
> > > > + ? ? ? MX53_PATA_IORDY,
> > > > + ? ? ? MX53_PATA_RESET_B,
> > > > +
> > > > +};
> > > > +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2,
> > > > +2 };
> > >
> > > This also looks strange. Can you explain what these muxes are?
> > 
> > Freescale has named the pins after their primary function which is quite
> > confusing.
> > 
> > The above means:
> > 
> > MX53_PATA_DATA8 -> mux mode 4
> > MX53_PATA_DATA9 -> mux mode 4
> > ...
> > 
> > This brings me to the point that currently we have the pins described as
> > 
> > #define MX53_PAD_<name>__<function>
> > 
> > which means that you don't have to look into the datasheet to get the
> > different options for a pin (and don't have a chance to get it wrong).
> > I don't really want to lose this.
> > 
> Obviously current used pin defines in that way is pretty good.
> And I also don't want to lose this.
> 
> Actually I also have tried to see if we can reuse the current iomux-v3 code.
> 
> For current pinmux driver, one approach I can see is that define mux
> in enum for each pin like:
> 
> enum MX53_PAD_GPIO_19_MUX {
>         MX53_PAD_GPIO_19__KPP_COL_5,
>         MX53_PAD_GPIO_19__GPIO4_5,
>         MX53_PAD_GPIO_19__CCM_CLKO,
>         MX53_PAD_GPIO_19__SPDIF_OUT1,
>         MX53_PAD_GPIO_19__RTC_CE_RTC_EXT_TRIG2,
>         MX53_PAD_GPIO_19__ECSPI1_RDY,
>         MX53_PAD_GPIO_19__FEC_TDATA_3,
>         MX53_PAD_GPIO_19__SRC_INT_BOOT,
> };

I would say, no, do not do that, because it simply does not worth.
Most of the definitions will probably never be used.

IMO, we can just focus on the support for device tree case (imx6) for
now.  With proper DT binding for pinctrl settled, all these data can
go into DT.  For those non-DT cases, we may want to leave them as they
are for now.

Regards,
Shawn

> Then put them in a common file for each mx53 based board to use.
> 
> Take uart1 as an example, it could be:
> static const unsigned mx53_uart1_pins[] = {
>         MX53_CSI0_DAT10,
>         MX53_CSI0_DAT11,
> };
> 
> static const unsigned mx53_uart1_mux[] = {
>         MX53_CSI0_DAT10__UART1_TXD_MUX,
>         MX53_CSI0_DAT11__UART1_RXD_MUX
> };
> 
> static const struct imx_pin_group mx53_pin_groups[] = {
>         IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux),
> };
> 
> The benefit is that it's very clear and good maintainable.
> The defect is it will increase the code size a lot by defining all
> pin's mux enum and each pin's mux array in board file.
> 
> Do you think if it's ok or you have any better idea?
> 
> Regards
> Dong Aisheng

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Guo Shawn-R65073
> Sent: Tuesday, December 06, 2011 2:58 PM
> To: Dong Aisheng-B29396
> Cc: Sascha Hauer; Linus Walleij; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
> R65073; kernel at pengutronix.de
> Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> 
> On Tue, Dec 06, 2011 at 01:54:35PM +0800, Dong Aisheng-B29396 wrote:
> > > -----Original Message-----
> > > From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> > > Sent: Tuesday, December 06, 2011 5:19 AM
> > > To: Linus Walleij
> > > Cc: Dong Aisheng-B29396; linux-kernel at vger.kernel.org; linux-arm-
> > > kernel at lists.infradead.org; linus.walleij at stericsson.com; Guo Shawn-
> > > R65073; kernel at pengutronix.de
> > > Subject: Re: [RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support
> > >
> > > On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:
> > > > On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng
> > > > <b29396@freescale.com>
> > > wrote:
> > > >
> > > > > +enum imx_mx53_pads {
> > > > > + ? ? ? MX53_GPIO_19 = 0,
> > > > > + ? ? ? MX53_KEY_COL0 = 1,
> > > > (...)
> > > >
> > > > First I thought it looked a bit strange since you needed enums for
> > > > all pads but then I realized that your macros use the same
> > > > enumerator name to name the pad and then it looks sort of clever.
> > > >
> > > > But maybe put in a comment about that here:
> > > >
> > > > > +/* Pad names for the pinmux subsystem */
> > > >
> > > > Like this:
> > > >
> > > > /*
> > > >  * Pad names for the pinmux subsystem.
> > > >  * These pad names are constructed from the pin enumerator names
> > > >  * in the IMX_PINCTRL_PIN() macro.
> > > >  */
> > > >
> > > > > +static const struct pinctrl_pin_desc mx53_pads[] = {
> > > > > + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> > > > > + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> > > > (...)
> > > >
> > > > > +/* mx53 pin groups and mux mode */ static const unsigned
> > > > > +mx53_fec_pins[] = {
> > > > > + ? ? ? MX53_FEC_MDC,
> > > > > + ? ? ? MX53_FEC_MDIO,
> > > > > + ? ? ? MX53_FEC_REF_CLK,
> > > > > + ? ? ? MX53_FEC_RX_ER,
> > > > > + ? ? ? MX53_FEC_CRS_DV,
> > > > > + ? ? ? MX53_FEC_RXD1,
> > > > > + ? ? ? MX53_FEC_RXD0,
> > > > > + ? ? ? MX53_FEC_TX_EN,
> > > > > + ? ? ? MX53_FEC_TXD1,
> > > > > + ? ? ? MX53_FEC_TXD0,
> > > > > +};
> > > >
> > > > I understand this.
> > > >
> > > > > +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0,
> > > > > +0, 0, 0 };
> > > >
> > > > But what is this? Just zeroes? Why?
> > > > Especially with a const so they really cannot be anything else.
> > > > The same pin (0) can only be enumerated once.
> > > >
> > > > > +static const unsigned mx53_sd1_pins[] = {
> > > > > + ? ? ? MX53_SD1_CMD,
> > > > > + ? ? ? MX53_SD1_CLK,
> > > > > + ? ? ? MX53_SD1_DATA0,
> > > > > + ? ? ? MX53_SD1_DATA1,
> > > > > + ? ? ? MX53_SD1_DATA2,
> > > > > + ? ? ? MX53_SD1_DATA3,
> > > > > +
> > > > > +};
> > > > > +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> > > >
> > > > And here again.
> > > >
> > > > > +static const unsigned mx53_sd3_pins[] = {
> > > > > + ? ? ? MX53_PATA_DATA8,
> > > > > + ? ? ? MX53_PATA_DATA9,
> > > > > + ? ? ? MX53_PATA_DATA10,
> > > > > + ? ? ? MX53_PATA_DATA11,
> > > > > + ? ? ? MX53_PATA_DATA0,
> > > > > + ? ? ? MX53_PATA_DATA1,
> > > > > + ? ? ? MX53_PATA_DATA2,
> > > > > + ? ? ? MX53_PATA_DATA3,
> > > > > + ? ? ? MX53_PATA_IORDY,
> > > > > + ? ? ? MX53_PATA_RESET_B,
> > > > > +
> > > > > +};
> > > > > +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4,
> > > > > +4, 2,
> > > > > +2 };
> > > >
> > > > This also looks strange. Can you explain what these muxes are?
> > >
> > > Freescale has named the pins after their primary function which is
> > > quite confusing.
> > >
> > > The above means:
> > >
> > > MX53_PATA_DATA8 -> mux mode 4
> > > MX53_PATA_DATA9 -> mux mode 4
> > > ...
> > >
> > > This brings me to the point that currently we have the pins
> > > described as
> > >
> > > #define MX53_PAD_<name>__<function>
> > >
> > > which means that you don't have to look into the datasheet to get
> > > the different options for a pin (and don't have a chance to get it
> wrong).
> > > I don't really want to lose this.
> > >
> > Obviously current used pin defines in that way is pretty good.
> > And I also don't want to lose this.
> >
> > Actually I also have tried to see if we can reuse the current iomux-v3
> code.
> >
> > For current pinmux driver, one approach I can see is that define mux
> > in enum for each pin like:
> >
> > enum MX53_PAD_GPIO_19_MUX {
> >         MX53_PAD_GPIO_19__KPP_COL_5,
> >         MX53_PAD_GPIO_19__GPIO4_5,
> >         MX53_PAD_GPIO_19__CCM_CLKO,
> >         MX53_PAD_GPIO_19__SPDIF_OUT1,
> >         MX53_PAD_GPIO_19__RTC_CE_RTC_EXT_TRIG2,
> >         MX53_PAD_GPIO_19__ECSPI1_RDY,
> >         MX53_PAD_GPIO_19__FEC_TDATA_3,
> >         MX53_PAD_GPIO_19__SRC_INT_BOOT, };
> 
> I would say, no, do not do that, because it simply does not worth.
> Most of the definitions will probably never be used.
> 
> IMO, we can just focus on the support for device tree case (imx6) for now.
> With proper DT binding for pinctrl settled, all these data can go into DT.
> For those non-DT cases, we may want to leave them as they are for now.
> 

Can current pinctrl framework support DT well?
Linus,
Can you help answer it or if you have a plan on DT support if it's still not ready?

I was ever thought it might not support DT that why I changed to run the
driver for imx53 first.

> 
> > Then put them in a common file for each mx53 based board to use.
> >
> > Take uart1 as an example, it could be:
> > static const unsigned mx53_uart1_pins[] = {
> >         MX53_CSI0_DAT10,
> >         MX53_CSI0_DAT11,
> > };
> >
> > static const unsigned mx53_uart1_mux[] = {
> >         MX53_CSI0_DAT10__UART1_TXD_MUX,
> >         MX53_CSI0_DAT11__UART1_RXD_MUX };
> >
> > static const struct imx_pin_group mx53_pin_groups[] = {
> >         IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux), };
> >
> > The benefit is that it's very clear and good maintainable.
> > The defect is it will increase the code size a lot by defining all
> > pin's mux enum and each pin's mux array in board file.
> >
> > Do you think if it's ok or you have any better idea?
> >
> > Regards
> > Dong Aisheng

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Shawn Guo]

On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote:
> On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:
> > On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote:
> > 
> > > +enum imx_mx53_pads {
> > > + ? ? ? MX53_GPIO_19 = 0,
> > > + ? ? ? MX53_KEY_COL0 = 1,
> > (...)
> > 
> > First I thought it looked a bit strange since you needed enums for all pads
> > but then I realized that your macros use the same enumerator name to
> > name the pad and then it looks sort of clever.
> > 
> > But maybe put in a comment about that here:
> > 
> > > +/* Pad names for the pinmux subsystem */
> > 
> > Like this:
> > 
> > /*
> >  * Pad names for the pinmux subsystem.
> >  * These pad names are constructed from the pin enumerator names
> >  * in the IMX_PINCTRL_PIN() macro.
> >  */
> > 
> > > +static const struct pinctrl_pin_desc mx53_pads[] = {
> > > + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> > > + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> > (...)
> > 
> > > +/* mx53 pin groups and mux mode */
> > > +static const unsigned mx53_fec_pins[] = {
> > > + ? ? ? MX53_FEC_MDC,
> > > + ? ? ? MX53_FEC_MDIO,
> > > + ? ? ? MX53_FEC_REF_CLK,
> > > + ? ? ? MX53_FEC_RX_ER,
> > > + ? ? ? MX53_FEC_CRS_DV,
> > > + ? ? ? MX53_FEC_RXD1,
> > > + ? ? ? MX53_FEC_RXD0,
> > > + ? ? ? MX53_FEC_TX_EN,
> > > + ? ? ? MX53_FEC_TXD1,
> > > + ? ? ? MX53_FEC_TXD0,
> > > +};
> > 
> > I understand this.
> > 
> > > +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> > 
> > But what is this? Just zeroes? Why?
> > Especially with a const so they really cannot be anything
> > else. The same pin (0) can only be enumerated once.
> > 
> > > +static const unsigned mx53_sd1_pins[] = {
> > > + ? ? ? MX53_SD1_CMD,
> > > + ? ? ? MX53_SD1_CLK,
> > > + ? ? ? MX53_SD1_DATA0,
> > > + ? ? ? MX53_SD1_DATA1,
> > > + ? ? ? MX53_SD1_DATA2,
> > > + ? ? ? MX53_SD1_DATA3,
> > > +
> > > +};
> > > +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> > 
> > And here again.
> > 
> > > +static const unsigned mx53_sd3_pins[] = {
> > > + ? ? ? MX53_PATA_DATA8,
> > > + ? ? ? MX53_PATA_DATA9,
> > > + ? ? ? MX53_PATA_DATA10,
> > > + ? ? ? MX53_PATA_DATA11,
> > > + ? ? ? MX53_PATA_DATA0,
> > > + ? ? ? MX53_PATA_DATA1,
> > > + ? ? ? MX53_PATA_DATA2,
> > > + ? ? ? MX53_PATA_DATA3,
> > > + ? ? ? MX53_PATA_IORDY,
> > > + ? ? ? MX53_PATA_RESET_B,
> > > +
> > > +};
> > > +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
> > 
> > This also looks strange. Can you explain what these muxes are?
> 
> Freescale has named the pins after their primary function which is quite
> confusing.
> 
> The above means:
> 
> MX53_PATA_DATA8 -> mux mode 4
> MX53_PATA_DATA9 -> mux mode 4
> ...
> 
> This brings me to the point that currently we have the pins described as
> 
> #define MX53_PAD_<name>__<function>
> 
But that's also the reason why we have so many lengthy iomux-mx*.h on
imx.  Taking iomux-mx53.h for example, it's a 109K header with 1219
LOC, but probably only 10% of the definitions will actually be used.

> which means that you don't have to look into the datasheet to get the
> different options for a pin

Looking at the datasheet when we write code is a pretty natural thing
to me.

-- 
Regards,
Shawn

> (and don't have a chance to get it wrong).
> I don't really want to lose this.
> 

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote:
> > Freescale has named the pins after their primary function which is quite
> > confusing.
> > 
> > The above means:
> > 
> > MX53_PATA_DATA8 -> mux mode 4
> > MX53_PATA_DATA9 -> mux mode 4
> > ...
> > 
> > This brings me to the point that currently we have the pins described as
> > 
> > #define MX53_PAD_<name>__<function>
> > 
> But that's also the reason why we have so many lengthy iomux-mx*.h on
> imx.  Taking iomux-mx53.h for example, it's a 109K header with 1219
> LOC, but probably only 10% of the definitions will actually be used.
> 
Which has the benefit of having correct pin definitions for everyone
to use. If developers who need to use currently unused pindefs have to
create them on their own, there will always be a good chance in
getting them wrong.

> > which means that you don't have to look into the datasheet to get the
> > different options for a pin
> 
> Looking at the datasheet when we write code is a pretty natural thing
> to me.
> 
The pindefs are like interrupt numbers or IO addresses for which there
also is a complete list of definitions in the kernel no matter whether
they are actually all in use.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Shawn Guo]

On Tue, Dec 06, 2011 at 08:33:28AM +0100, Lothar Wa?mann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote:
> > > Freescale has named the pins after their primary function which is quite
> > > confusing.
> > > 
> > > The above means:
> > > 
> > > MX53_PATA_DATA8 -> mux mode 4
> > > MX53_PATA_DATA9 -> mux mode 4
> > > ...
> > > 
> > > This brings me to the point that currently we have the pins described as
> > > 
> > > #define MX53_PAD_<name>__<function>
> > > 
> > But that's also the reason why we have so many lengthy iomux-mx*.h on
> > imx.  Taking iomux-mx53.h for example, it's a 109K header with 1219
> > LOC, but probably only 10% of the definitions will actually be used.
> > 
> Which has the benefit of having correct pin definitions for everyone
> to use. If developers who need to use currently unused pindefs have to
> create them on their own, there will always be a good chance in
> getting them wrong.

If the configuration is not working, they will notice it's wrong anyway.

> 
> > > which means that you don't have to look into the datasheet to get the
> > > different options for a pin
> > 
> > Looking at the datasheet when we write code is a pretty natural thing
> > to me.
> > 
> The pindefs are like interrupt numbers or IO addresses for which there
> also is a complete list of definitions in the kernel no matter whether
> they are actually all in use.
> 
The interrupt list is much shorter than iomux list here, which enumerate
every single function for every single pad.  Also, most interrupts stand
a pretty good chance to be used, while most of the pad functions do not.

-- 
Regards,
Shawn

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Uwe Kleine-König]

Hi,

On Tue, Dec 06, 2011 at 08:33:28AM +0100, Lothar Wa?mann wrote:
> Shawn Guo writes:
> > On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote:
> > > Freescale has named the pins after their primary function which is quite
> > > confusing.
> > > 
> > > The above means:
> > > 
> > > MX53_PATA_DATA8 -> mux mode 4
> > > MX53_PATA_DATA9 -> mux mode 4
> > > ...
> > > 
> > > This brings me to the point that currently we have the pins described as
> > > 
> > > #define MX53_PAD_<name>__<function>
> > > 
> > But that's also the reason why we have so many lengthy iomux-mx*.h on
> > imx.  Taking iomux-mx53.h for example, it's a 109K header with 1219
> > LOC, but probably only 10% of the definitions will actually be used.
> > 
> Which has the benefit of having correct pin definitions for everyone
> to use. If developers who need to use currently unused pindefs have to
> create them on their own, there will always be a good chance in
> getting them wrong.
Another upside is, that adding the complete list once doesn't result in
conflicts if two people add new definitions in the same timeframe. As
the header are only cpp symbols that don't hurt at runtime I think
having them complete is better than the incremental approach.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Linus Walleij]

On Tue, Dec 6, 2011 at 8:33 AM, Lothar Wa?mann <LW@karo-electronics.de> wrote:
> Shawn Guo writes:
>> On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote:

>> > This brings me to the point that currently we have the pins described as
>> >
>> > #define MX53_PAD_<name>__<function>
>> >
>> But that's also the reason why we have so many lengthy iomux-mx*.h on
>> imx. ?Taking iomux-mx53.h for example, it's a 109K header with 1219
>> LOC, but probably only 10% of the definitions will actually be used.
>>
> Which has the benefit of having correct pin definitions for everyone
> to use. If developers who need to use currently unused pindefs have to
> create them on their own, there will always be a good chance in
> getting them wrong.
>
>> > which means that you don't have to look into the datasheet to get the
>> > different options for a pin
>>
>> Looking at the datasheet when we write code is a pretty natural thing
>> to me.
>>
> The pindefs are like interrupt numbers or IO addresses for which there
> also is a complete list of definitions in the kernel no matter whether
> they are actually all in use.

In both cases I'd say it's not the business of the pin control
implementation to worry about size of .h files in arch/arm/*

Getting rid of such defines and board data is the business of the
device tree and nothing else, if I understand the way people are
thinking about this.

So I would prefer to keep these two concepts separate:
1) get something in place that integrates nicely with pinctrl
2) get device tree in place
3) get rid of old board files and huge .h define lists including
   I/O, irqs, pinmux lists...
4) ...
5) profit

So don't try to solve all things at once or you'll end up trying
to drink the ocean.

Yours,
Linus Walleij

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Sascha Hauer]

On Tue, Dec 06, 2011 at 02:25:01PM +0800, Shawn Guo wrote:
> On Mon, Dec 05, 2011 at 10:18:38PM +0100, Sascha Hauer wrote:
> > On Mon, Dec 05, 2011 at 05:57:42PM +0100, Linus Walleij wrote:
> > > On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote:
> > > 
> > > > +enum imx_mx53_pads {
> > > > + ? ? ? MX53_GPIO_19 = 0,
> > > > + ? ? ? MX53_KEY_COL0 = 1,
> > > (...)
> > > 
> > > First I thought it looked a bit strange since you needed enums for all pads
> > > but then I realized that your macros use the same enumerator name to
> > > name the pad and then it looks sort of clever.
> > > 
> > > But maybe put in a comment about that here:
> > > 
> > > > +/* Pad names for the pinmux subsystem */
> > > 
> > > Like this:
> > > 
> > > /*
> > >  * Pad names for the pinmux subsystem.
> > >  * These pad names are constructed from the pin enumerator names
> > >  * in the IMX_PINCTRL_PIN() macro.
> > >  */
> > > 
> > > > +static const struct pinctrl_pin_desc mx53_pads[] = {
> > > > + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
> > > > + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> > > (...)
> > > 
> > > > +/* mx53 pin groups and mux mode */
> > > > +static const unsigned mx53_fec_pins[] = {
> > > > + ? ? ? MX53_FEC_MDC,
> > > > + ? ? ? MX53_FEC_MDIO,
> > > > + ? ? ? MX53_FEC_REF_CLK,
> > > > + ? ? ? MX53_FEC_RX_ER,
> > > > + ? ? ? MX53_FEC_CRS_DV,
> > > > + ? ? ? MX53_FEC_RXD1,
> > > > + ? ? ? MX53_FEC_RXD0,
> > > > + ? ? ? MX53_FEC_TX_EN,
> > > > + ? ? ? MX53_FEC_TXD1,
> > > > + ? ? ? MX53_FEC_TXD0,
> > > > +};
> > > 
> > > I understand this.
> > > 
> > > > +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> > > 
> > > But what is this? Just zeroes? Why?
> > > Especially with a const so they really cannot be anything
> > > else. The same pin (0) can only be enumerated once.
> > > 
> > > > +static const unsigned mx53_sd1_pins[] = {
> > > > + ? ? ? MX53_SD1_CMD,
> > > > + ? ? ? MX53_SD1_CLK,
> > > > + ? ? ? MX53_SD1_DATA0,
> > > > + ? ? ? MX53_SD1_DATA1,
> > > > + ? ? ? MX53_SD1_DATA2,
> > > > + ? ? ? MX53_SD1_DATA3,
> > > > +
> > > > +};
> > > > +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
> > > 
> > > And here again.
> > > 
> > > > +static const unsigned mx53_sd3_pins[] = {
> > > > + ? ? ? MX53_PATA_DATA8,
> > > > + ? ? ? MX53_PATA_DATA9,
> > > > + ? ? ? MX53_PATA_DATA10,
> > > > + ? ? ? MX53_PATA_DATA11,
> > > > + ? ? ? MX53_PATA_DATA0,
> > > > + ? ? ? MX53_PATA_DATA1,
> > > > + ? ? ? MX53_PATA_DATA2,
> > > > + ? ? ? MX53_PATA_DATA3,
> > > > + ? ? ? MX53_PATA_IORDY,
> > > > + ? ? ? MX53_PATA_RESET_B,
> > > > +
> > > > +};
> > > > +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
> > > 
> > > This also looks strange. Can you explain what these muxes are?
> > 
> > Freescale has named the pins after their primary function which is quite
> > confusing.
> > 
> > The above means:
> > 
> > MX53_PATA_DATA8 -> mux mode 4
> > MX53_PATA_DATA9 -> mux mode 4
> > ...
> > 
> > This brings me to the point that currently we have the pins described as
> > 
> > #define MX53_PAD_<name>__<function>
> > 
> But that's also the reason why we have so many lengthy iomux-mx*.h on
> imx.  Taking iomux-mx53.h for example, it's a 109K header with 1219
> LOC, but probably only 10% of the definitions will actually be used.
> 
> > which means that you don't have to look into the datasheet to get the
> > different options for a pin
> 
> Looking at the datasheet when we write code is a pretty natural thing
> to me.

Yes, but digging again and again for the same pinmux setting (and the
corresponding sel_input registers) is a PITA.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[RFC PATCH 2/3] pinctrl: imx: add pinmux-imx53 support

[Dong Aisheng]

Hi Linus,

First  thanks for your review.

2011/12/6 Linus Walleij <linus.walleij@linaro.org>:
> On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote:
>
>> +enum imx_mx53_pads {
>> + ? ? ? MX53_GPIO_19 = 0,
>> + ? ? ? MX53_KEY_COL0 = 1,
> (...)
>
> First I thought it looked a bit strange since you needed enums for all pads
> but then I realized that your macros use the same enumerator name to
> name the pad and then it looks sort of clever.
>
> But maybe put in a comment about that here:
>
>> +/* Pad names for the pinmux subsystem */
>
> Like this:
>
> /*
> ?* Pad names for the pinmux subsystem.
> ?* These pad names are constructed from the pin enumerator names
> ?* in the IMX_PINCTRL_PIN() macro.
> ?*/
>
Indeed.
Thanks for your info.

>> +static const struct pinctrl_pin_desc mx53_pads[] = {
>> + ? ? ? IMX_PINCTRL_PIN(MX53_GPIO_19),
>> + ? ? ? IMX_PINCTRL_PIN(MX53_KEY_COL0),
> (...)
>
>> +/* mx53 pin groups and mux mode */
>> +static const unsigned mx53_fec_pins[] = {
>> + ? ? ? MX53_FEC_MDC,
>> + ? ? ? MX53_FEC_MDIO,
>> + ? ? ? MX53_FEC_REF_CLK,
>> + ? ? ? MX53_FEC_RX_ER,
>> + ? ? ? MX53_FEC_CRS_DV,
>> + ? ? ? MX53_FEC_RXD1,
>> + ? ? ? MX53_FEC_RXD0,
>> + ? ? ? MX53_FEC_TX_EN,
>> + ? ? ? MX53_FEC_TXD1,
>> + ? ? ? MX53_FEC_TXD0,
>> +};
>
> I understand this.
>
>> +static const unsigned mx53_fec_mux[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
>
> But what is this? Just zeroes? Why?
> Especially with a const so they really cannot be anything
> else. The same pin (0) can only be enumerated once.
>
It's the mux mode for each pins in that group for a specific
function(here it's fec).
I add the comments in pinmux-imx-core.h when define this group structure.
Maybe i still need to add some comments here to avoid confusing.

>> +static const unsigned mx53_sd1_pins[] = {
>> + ? ? ? MX53_SD1_CMD,
>> + ? ? ? MX53_SD1_CLK,
>> + ? ? ? MX53_SD1_DATA0,
>> + ? ? ? MX53_SD1_DATA1,
>> + ? ? ? MX53_SD1_DATA2,
>> + ? ? ? MX53_SD1_DATA3,
>> +
>> +};
>> +static const unsigned mx53_sd1_mux[] = { 0, 0, 0, 0, 0, 0 };
>
> And here again.
>
>> +static const unsigned mx53_sd3_pins[] = {
>> + ? ? ? MX53_PATA_DATA8,
>> + ? ? ? MX53_PATA_DATA9,
>> + ? ? ? MX53_PATA_DATA10,
>> + ? ? ? MX53_PATA_DATA11,
>> + ? ? ? MX53_PATA_DATA0,
>> + ? ? ? MX53_PATA_DATA1,
>> + ? ? ? MX53_PATA_DATA2,
>> + ? ? ? MX53_PATA_DATA3,
>> + ? ? ? MX53_PATA_IORDY,
>> + ? ? ? MX53_PATA_RESET_B,
>> +
>> +};
>> +static const unsigned mx53_sd3_mux[] = { 4, 4, 4, 4, 4, 4, 4, 4, 2, 2 };
>
> This also looks strange. Can you explain what these muxes are?
>
>> +static const unsigned mx53_uart1_pins[] = {
>> + ? ? ? MX53_CSI0_DAT10,
>> + ? ? ? MX53_CSI0_DAT11,
>> +};
>> +static const unsigned mx53_uart1_mux[] = { 2, 2 };
>
> And here again?
>
>> +static const struct imx_pin_group mx53_pin_groups[] = {
>> + ? ? ? IMX_PIN_GROUP("fecgrp", mx53_fec_pins, mx53_fec_mux),
>> + ? ? ? IMX_PIN_GROUP("sd1grp", mx53_sd1_pins, mx53_sd1_mux),
>> + ? ? ? IMX_PIN_GROUP("sd3grp", mx53_sd3_pins, mx53_sd3_mux),
>> + ? ? ? IMX_PIN_GROUP("uart1grp", mx53_uart1_pins, mx53_uart1_mux),
>> +};
>
> So I understand the first and second argument to IMX_PIN_GROUP()
> but not the third.
>
>> +/* mx53 funcs and groups */
>> +static const char * const fecgrps[] = { "fecgrp" };
>> +static const char * const sd1grps[] = { "sd1grp" };
>> +static const char * const sd3grps[] = { "sd3grp" };
>> +static const char * const uart1grps[] = { "uart1grp" };
>> +
>> +static const struct imx_pmx_func mx53_pmx_functions[] = {
>> + ? ? ? IMX_PMX_FUNC("fec", fecgrps),
>> + ? ? ? IMX_PMX_FUNC("sd1", sd1grps),
>> + ? ? ? IMX_PMX_FUNC("sd3", sd3grps),
>> + ? ? ? IMX_PMX_FUNC("uart1", uart1grps),
>> +};
>
> This looks good.
>

Regards
Dong Aisheng

[RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support

[Dong Aisheng]

Signed-off-by: Dong Aisheng <b29396@freescale.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shanw.guo@freescale.com>
---
 drivers/pinctrl/pinmux-imx6q.c |  464 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 464 insertions(+), 0 deletions(-)

diff --git a/drivers/pinctrl/pinmux-imx6q.c b/drivers/pinctrl/pinmux-imx6q.c
new file mode 100644
index 0000000..5641199
--- /dev/null
+++ b/drivers/pinctrl/pinmux-imx6q.c
@@ -0,0 +1,464 @@
+/*
+ * imx6q pinmux driver based on imx pinmux core
+ *
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ * Copyright (C) 2011 Linaro, Inc.
+ *
+ * Author: Dong Aisheng <dong.aisheng@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "pinmux-imx-core.h"
+
+#define MX6Q_IOMUXC_MUX_OFSSET	0x4c
+#define MX6Q_IOMUXC_MAXPIN	(25*25)
+
+enum imx_mx6q_pads {
+	MX6Q_SD2_DAT1 = 0,
+	MX6Q_SD2_DAT2 = 1,
+	MX6Q_SD2_DAT0 = 2,
+	MX6Q_RGMII_TXC = 3,
+	MX6Q_RGMII_TD0 = 4,
+	MX6Q_RGMII_TD1 = 5,
+	MX6Q_RGMII_TD2 = 6,
+	MX6Q_RGMII_TD3 = 7,
+	MX6Q_RGMII_RX_CTL = 8,
+	MX6Q_RGMII_RD0 = 9,
+	MX6Q_RGMII_TX_CTL = 10,
+	MX6Q_RGMII_RD1 = 11,
+	MX6Q_RGMII_RD2 = 12,
+	MX6Q_RGMII_RD3 = 13,
+	MX6Q_RGMII_RXC = 14,
+	MX6Q_EIM_A25 = 15,
+	MX6Q_EIM_EB2 = 16,
+	MX6Q_EIM_D16 = 17,
+	MX6Q_EIM_D17 = 18,
+	MX6Q_EIM_D18 = 19,
+	MX6Q_EIM_D19 = 20,
+	MX6Q_EIM_D20 = 21,
+	MX6Q_EIM_D21 = 22,
+	MX6Q_EIM_D22 = 23,
+	MX6Q_EIM_D23 = 24,
+	MX6Q_EIM_EB3 = 25,
+	MX6Q_EIM_D24 = 26,
+	MX6Q_EIM_D25 = 27,
+	MX6Q_EIM_D26 = 28,
+	MX6Q_EIM_D27 = 29,
+	MX6Q_EIM_D28 = 30,
+	MX6Q_EIM_D29 = 31,
+	MX6Q_EIM_D30 = 32,
+	MX6Q_EIM_D31 = 33,
+	MX6Q_EIM_A24 = 34,
+	MX6Q_EIM_A23 = 35,
+	MX6Q_EIM_A22 = 36,
+	MX6Q_EIM_A21 = 37,
+	MX6Q_EIM_A20 = 38,
+	MX6Q_EIM_A19 = 39,
+	MX6Q_EIM_A18 = 40,
+	MX6Q_EIM_A17 = 41,
+	MX6Q_EIM_A16 = 42,
+	MX6Q_EIM_CS0 = 43,
+	MX6Q_EIM_CS1 = 44,
+	MX6Q_EIM_OE = 45,
+	MX6Q_EIM_RW = 46,
+	MX6Q_EIM_LBA = 47,
+	MX6Q_EIM_EB0 = 48,
+	MX6Q_EIM_EB1 = 49,
+	MX6Q_EIM_DA0 = 50,
+	MX6Q_EIM_DA1 = 51,
+	MX6Q_EIM_DA2 = 52,
+	MX6Q_EIM_DA3 = 53,
+	MX6Q_EIM_DA4 = 54,
+	MX6Q_EIM_DA5 = 55,
+	MX6Q_EIM_DA6 = 56,
+	MX6Q_EIM_DA7 = 57,
+	MX6Q_EIM_DA8 = 58,
+	MX6Q_EIM_DA9 = 59,
+	MX6Q_EIM_DA10 = 60,
+	MX6Q_EIM_DA11 = 61,
+	MX6Q_EIM_DA12 = 62,
+	MX6Q_EIM_DA13 = 63,
+	MX6Q_EIM_DA14 = 64,
+	MX6Q_EIM_DA15 = 65,
+	MX6Q_EIM_WAIT = 66,
+	MX6Q_EIM_BCLK = 67,
+	MX6Q_DI0_DISP_CLK = 68,
+	MX6Q_DI0_PIN15 = 69,
+	MX6Q_DI0_PIN2 = 70,
+	MX6Q_DI0_PIN3 = 71,
+	MX6Q_DI0_PIN4 = 72,
+	MX6Q_DISP0_DAT0 = 73,
+	MX6Q_DISP0_DAT1 = 74,
+	MX6Q_DISP0_DAT2 = 75,
+	MX6Q_DISP0_DAT3 = 76,
+	MX6Q_DISP0_DAT4 = 77,
+	MX6Q_DISP0_DAT5 = 78,
+	MX6Q_DISP0_DAT6 = 79,
+	MX6Q_DISP0_DAT7 = 80,
+	MX6Q_DISP0_DAT8 = 81,
+	MX6Q_DISP0_DAT9 = 82,
+	MX6Q_DISP0_DAT10 = 83,
+	MX6Q_DISP0_DAT11 = 84,
+	MX6Q_DISP0_DAT12 = 85,
+	MX6Q_DISP0_DAT13 = 86,
+	MX6Q_DISP0_DAT14 = 87,
+	MX6Q_DISP0_DAT15 = 88,
+	MX6Q_DISP0_DAT16 = 89,
+	MX6Q_DISP0_DAT17 = 90,
+	MX6Q_DISP0_DAT18 = 91,
+	MX6Q_DISP0_DAT19 = 92,
+	MX6Q_DISP0_DAT20 = 93,
+	MX6Q_DISP0_DAT21 = 94,
+	MX6Q_DISP0_DAT22 = 95,
+	MX6Q_DISP0_DAT23 = 96,
+	MX6Q_ENET_MDIO = 97,
+	MX6Q_ENET_REF_CLK = 98,
+	MX6Q_ENET_RX_ER = 99,
+	MX6Q_ENET_CRS_DV = 100,
+	MX6Q_ENET_RXD1 = 101,
+	MX6Q_ENET_RXD0 = 102,
+	MX6Q_ENET_TX_EN = 103,
+	MX6Q_ENET_TXD1 = 104,
+	MX6Q_ENET_TXD0 = 105,
+	MX6Q_ENET_MDC = 106,
+	MX6Q_KEY_COL0 = 107,
+	MX6Q_KEY_ROW0 = 108,
+	MX6Q_KEY_COL1 = 109,
+	MX6Q_KEY_ROW1 = 110,
+	MX6Q_KEY_COL2 = 111,
+	MX6Q_KEY_ROW2 = 112,
+	MX6Q_KEY_COL3 = 113,
+	MX6Q_KEY_ROW3 = 114,
+	MX6Q_KEY_COL4 = 115,
+	MX6Q_KEY_ROW4 = 116,
+	MX6Q_GPIO_0 = 117,
+	MX6Q_GPIO_1 = 118,
+	MX6Q_GPIO_9 = 119,
+	MX6Q_GPIO_3 = 120,
+	MX6Q_GPIO_6 = 121,
+	MX6Q_GPIO_2 = 122,
+	MX6Q_GPIO_4 = 123,
+	MX6Q_GPIO_5 = 124,
+	MX6Q_GPIO_7 = 125,
+	MX6Q_GPIO_8 = 126,
+	MX6Q_GPIO_16 = 127,
+	MX6Q_GPIO_17 = 128,
+	MX6Q_GPIO_18 = 129,
+	MX6Q_GPIO_19 = 130,
+	MX6Q_CSI0_PIXCLK = 131,
+	MX6Q_CSI0_MCLK = 132,
+	MX6Q_CSI0_DATA_EN = 133,
+	MX6Q_CSI0_VSYNC = 134,
+	MX6Q_CSI0_DAT4 = 135,
+	MX6Q_CSI0_DAT5 = 136,
+	MX6Q_CSI0_DAT6 = 137,
+	MX6Q_CSI0_DAT7 = 138,
+	MX6Q_CSI0_DAT8 = 139,
+	MX6Q_CSI0_DAT9 = 140,
+	MX6Q_CSI0_DAT10 = 141,
+	MX6Q_CSI0_DAT11 = 142,
+	MX6Q_CSI0_DAT12 = 143,
+	MX6Q_CSI0_DAT13 = 144,
+	MX6Q_CSI0_DAT14 = 145,
+	MX6Q_CSI0_DAT15 = 146,
+	MX6Q_CSI0_DAT16 = 147,
+	MX6Q_CSI0_DAT17 = 148,
+	MX6Q_CSI0_DAT18 = 149,
+	MX6Q_CSI0_DAT19 = 150,
+	MX6Q_SD3_DAT7 = 151,
+	MX6Q_SD3_DAT6 = 152,
+	MX6Q_SD3_DAT5 = 153,
+	MX6Q_SD3_DAT4 = 154,
+	MX6Q_SD3_CMD = 155,
+	MX6Q_SD3_CLK = 156,
+	MX6Q_SD3_DAT0 = 157,
+	MX6Q_SD3_DAT1 = 158,
+	MX6Q_SD3_DAT2 = 159,
+	MX6Q_SD3_DAT3 = 160,
+	MX6Q_SD3_RST = 161,
+	MX6Q_NANDF_CLE = 162,
+	MX6Q_NANDF_ALE = 163,
+	MX6Q_NANDF_WP_B = 164,
+	MX6Q_NANDF_RB0 = 165,
+	MX6Q_NANDF_CS0 = 166,
+	MX6Q_NANDF_CS1 = 167,
+	MX6Q_NANDF_CS2 = 168,
+	MX6Q_NANDF_CS3 = 169,
+	MX6Q_SD4_CMD = 170,
+	MX6Q_SD4_CLK = 171,
+	MX6Q_NANDF_D0 = 172,
+	MX6Q_NANDF_D1 = 173,
+	MX6Q_NANDF_D2 = 174,
+	MX6Q_NANDF_D3 = 175,
+	MX6Q_NANDF_D4 = 176,
+	MX6Q_NANDF_D5 = 177,
+	MX6Q_NANDF_D6 = 178,
+	MX6Q_NANDF_D7 = 179,
+	MX6Q_SD4_DAT0 = 180,
+	MX6Q_SD4_DAT1 = 181,
+	MX6Q_SD4_DAT2 = 182,
+	MX6Q_SD4_DAT3 = 183,
+	MX6Q_SD4_DAT4 = 184,
+	MX6Q_SD4_DAT5 = 185,
+	MX6Q_SD4_DAT6 = 186,
+	MX6Q_SD4_DAT7 = 187,
+	MX6Q_SD1_DAT1 = 188,
+	MX6Q_SD1_DAT0 = 189,
+	MX6Q_SD1_DAT3 = 190,
+	MX6Q_SD1_CMD = 191,
+	MX6Q_SD1_DAT2 = 192,
+	MX6Q_SD1_CLK = 193,
+	MX6Q_SD2_CLK = 194,
+	MX6Q_SD2_CMD = 195,
+	MX6Q_SD2_DAT3 = 196
+};
+
+/* Pad names for the pinmux subsystem */
+static const struct pinctrl_pin_desc mx6q_pads[] = {
+	IMX_PINCTRL_PIN(MX6Q_SD2_DAT1),
+	IMX_PINCTRL_PIN(MX6Q_SD2_DAT2),
+	IMX_PINCTRL_PIN(MX6Q_SD2_DAT0),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_TXC),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_TD0),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_TD1),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_TD2),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_TD3),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_RX_CTL),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_RD0),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_TX_CTL),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_RD1),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_RD2),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_RD3),
+	IMX_PINCTRL_PIN(MX6Q_RGMII_RXC),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A25),
+	IMX_PINCTRL_PIN(MX6Q_EIM_EB2),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D16),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D17),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D18),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D19),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D20),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D21),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D22),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D23),
+	IMX_PINCTRL_PIN(MX6Q_EIM_EB3),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D24),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D25),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D26),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D27),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D28),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D29),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D30),
+	IMX_PINCTRL_PIN(MX6Q_EIM_D31),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A24),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A23),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A22),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A21),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A20),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A19),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A18),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A17),
+	IMX_PINCTRL_PIN(MX6Q_EIM_A16),
+	IMX_PINCTRL_PIN(MX6Q_EIM_CS0),
+	IMX_PINCTRL_PIN(MX6Q_EIM_CS1),
+	IMX_PINCTRL_PIN(MX6Q_EIM_OE),
+	IMX_PINCTRL_PIN(MX6Q_EIM_RW),
+	IMX_PINCTRL_PIN(MX6Q_EIM_LBA),
+	IMX_PINCTRL_PIN(MX6Q_EIM_EB0),
+	IMX_PINCTRL_PIN(MX6Q_EIM_EB1),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA0),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA1),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA2),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA3),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA4),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA5),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA6),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA7),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA8),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA9),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA10),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA11),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA12),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA13),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA14),
+	IMX_PINCTRL_PIN(MX6Q_EIM_DA15),
+	IMX_PINCTRL_PIN(MX6Q_EIM_WAIT),
+	IMX_PINCTRL_PIN(MX6Q_EIM_BCLK),
+	IMX_PINCTRL_PIN(MX6Q_DI0_DISP_CLK),
+	IMX_PINCTRL_PIN(MX6Q_DI0_PIN15),
+	IMX_PINCTRL_PIN(MX6Q_DI0_PIN2),
+	IMX_PINCTRL_PIN(MX6Q_DI0_PIN3),
+	IMX_PINCTRL_PIN(MX6Q_DI0_PIN4),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT0),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT1),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT2),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT3),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT4),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT5),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT6),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT7),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT8),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT9),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT10),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT11),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT12),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT13),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT14),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT15),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT16),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT17),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT18),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT19),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT20),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT21),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT22),
+	IMX_PINCTRL_PIN(MX6Q_DISP0_DAT23),
+	IMX_PINCTRL_PIN(MX6Q_ENET_MDIO),
+	IMX_PINCTRL_PIN(MX6Q_ENET_REF_CLK),
+	IMX_PINCTRL_PIN(MX6Q_ENET_RX_ER),
+	IMX_PINCTRL_PIN(MX6Q_ENET_CRS_DV),
+	IMX_PINCTRL_PIN(MX6Q_ENET_RXD1),
+	IMX_PINCTRL_PIN(MX6Q_ENET_RXD0),
+	IMX_PINCTRL_PIN(MX6Q_ENET_TX_EN),
+	IMX_PINCTRL_PIN(MX6Q_ENET_TXD1),
+	IMX_PINCTRL_PIN(MX6Q_ENET_TXD0),
+	IMX_PINCTRL_PIN(MX6Q_ENET_MDC),
+	IMX_PINCTRL_PIN(MX6Q_KEY_COL0),
+	IMX_PINCTRL_PIN(MX6Q_KEY_ROW0),
+	IMX_PINCTRL_PIN(MX6Q_KEY_COL1),
+	IMX_PINCTRL_PIN(MX6Q_KEY_ROW1),
+	IMX_PINCTRL_PIN(MX6Q_KEY_COL2),
+	IMX_PINCTRL_PIN(MX6Q_KEY_ROW2),
+	IMX_PINCTRL_PIN(MX6Q_KEY_COL3),
+	IMX_PINCTRL_PIN(MX6Q_KEY_ROW3),
+	IMX_PINCTRL_PIN(MX6Q_KEY_COL4),
+	IMX_PINCTRL_PIN(MX6Q_KEY_ROW4),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_0),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_1),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_9),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_3),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_6),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_2),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_4),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_5),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_7),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_8),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_16),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_17),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_18),
+	IMX_PINCTRL_PIN(MX6Q_GPIO_19),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_PIXCLK),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_MCLK),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DATA_EN),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_VSYNC),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT4),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT5),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT6),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT7),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT8),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT9),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT10),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT11),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT12),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT13),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT14),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT15),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT16),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT17),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT18),
+	IMX_PINCTRL_PIN(MX6Q_CSI0_DAT19),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT7),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT6),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT5),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT4),
+	IMX_PINCTRL_PIN(MX6Q_SD3_CMD),
+	IMX_PINCTRL_PIN(MX6Q_SD3_CLK),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT0),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT1),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT2),
+	IMX_PINCTRL_PIN(MX6Q_SD3_DAT3),
+	IMX_PINCTRL_PIN(MX6Q_SD3_RST),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_CLE),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_ALE),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_WP_B),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_RB0),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_CS0),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_CS1),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_CS2),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_CS3),
+	IMX_PINCTRL_PIN(MX6Q_SD4_CMD),
+	IMX_PINCTRL_PIN(MX6Q_SD4_CLK),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D0),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D1),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D2),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D3),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D4),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D5),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D6),
+	IMX_PINCTRL_PIN(MX6Q_NANDF_D7),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT0),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT1),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT2),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT3),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT4),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT5),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT6),
+	IMX_PINCTRL_PIN(MX6Q_SD4_DAT7),
+	IMX_PINCTRL_PIN(MX6Q_SD1_DAT1),
+	IMX_PINCTRL_PIN(MX6Q_SD1_DAT0),
+	IMX_PINCTRL_PIN(MX6Q_SD1_DAT3),
+	IMX_PINCTRL_PIN(MX6Q_SD1_CMD),
+	IMX_PINCTRL_PIN(MX6Q_SD1_DAT2),
+	IMX_PINCTRL_PIN(MX6Q_SD1_CLK),
+	IMX_PINCTRL_PIN(MX6Q_SD2_CLK),
+	IMX_PINCTRL_PIN(MX6Q_SD2_CMD),
+	IMX_PINCTRL_PIN(MX6Q_SD2_DAT3),
+};
+
+/*
+ * The x_mux array contains pin mux mode value for each pins defined
+ * in x_pins array for a specific function
+ */
+
+/* mx6q pin groups and mux mode */
+static const unsigned mx6q_uart4_pins[] = { MX6Q_KEY_COL0, MX6Q_KEY_ROW0};
+static const unsigned mx6q_uart4_mux[] = { 4, 4 };
+static const unsigned mx6q_sd4_pins[] = { MX6Q_SD4_CLK, MX6Q_SD4_CMD,
+	MX6Q_SD4_DAT0, MX6Q_SD4_DAT1, MX6Q_SD4_DAT2, MX6Q_SD4_DAT3,
+	MX6Q_SD4_DAT4, MX6Q_SD4_DAT5, MX6Q_SD4_DAT6, MX6Q_SD4_DAT7};
+static const unsigned mx6q_sd4_mux[] = { 0, 0, 1, 1, 1, 1, 1, 1, 1, 1 };
+
+static const struct imx_pin_group mx6q_pin_groups[] = {
+	IMX_PIN_GROUP("uart4grp", mx6q_uart4_pins, mx6q_uart4_mux),
+	IMX_PIN_GROUP("sd4grp", mx6q_sd4_pins, mx6q_sd4_mux),
+};
+
+/* mx6q funcs and groups */
+static const char * const uart4grps[] = { "uart4grp" };
+static const char * const sd4grps[] = { "sd4grp" };
+
+static const struct imx_pmx_func mx6q_pmx_functions[] = {
+	IMX_PMX_FUNC("uart4", uart4grps),
+	IMX_PMX_FUNC("sd4", sd4grps),
+};
+
+const struct imx_pinctrl_info mx6q_pinctrl_info = {
+	.type = MX6Q_PINCTRL,
+	.pins = mx6q_pads,
+	.npins = ARRAY_SIZE(mx6q_pads),
+	.maxpin = MX6Q_IOMUXC_MAXPIN,
+	.groups = mx6q_pin_groups,
+	.ngroups = ARRAY_SIZE(mx6q_pin_groups),
+	.functions = mx6q_pmx_functions,
+	.nfunctions = ARRAY_SIZE(mx6q_pmx_functions),
+	.mux_offset = MX6Q_IOMUXC_MUX_OFSSET,
+};
-- 
1.7.0.4

[RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support

[Shawn Guo]

On Sun, Dec 04, 2011 at 07:49:44PM +0800, Dong Aisheng wrote:
> Signed-off-by: Dong Aisheng <b29396@freescale.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shanw.guo@freescale.com>
> ---
>  drivers/pinctrl/pinmux-imx6q.c |  464 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 464 insertions(+), 0 deletions(-)
> 
As I do not see the device tree probing added in pinmux-imx-core.c,
I expect the patch has not got the chance to run on imx6, right?

> diff --git a/drivers/pinctrl/pinmux-imx6q.c b/drivers/pinctrl/pinmux-imx6q.c
> new file mode 100644
> index 0000000..5641199
> --- /dev/null
> +++ b/drivers/pinctrl/pinmux-imx6q.c
> @@ -0,0 +1,464 @@
> +/*
> + * imx6q pinmux driver based on imx pinmux core
> + *
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + * Copyright (C) 2011 Linaro, Inc.
> + *
> + * Author: Dong Aisheng <dong.aisheng@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "pinmux-imx-core.h"
> +
> +#define MX6Q_IOMUXC_MUX_OFSSET	0x4c
> +#define MX6Q_IOMUXC_MAXPIN	(25*25)
> +
I would suggest replace all the occurrences of 'MX6Q'/'mx6q' with
'IMX6Q'/'imx6q' to force the consistency.

> +enum imx_mx6q_pads {

IMO, 'imx6q_pads' is good enough.

> +	MX6Q_SD2_DAT1 = 0,
> +	MX6Q_SD2_DAT2 = 1,
> +	MX6Q_SD2_DAT0 = 2,
> +	MX6Q_RGMII_TXC = 3,
> +	MX6Q_RGMII_TD0 = 4,
> +	MX6Q_RGMII_TD1 = 5,
> +	MX6Q_RGMII_TD2 = 6,
> +	MX6Q_RGMII_TD3 = 7,
> +	MX6Q_RGMII_RX_CTL = 8,
> +	MX6Q_RGMII_RD0 = 9,
> +	MX6Q_RGMII_TX_CTL = 10,
> +	MX6Q_RGMII_RD1 = 11,
> +	MX6Q_RGMII_RD2 = 12,
> +	MX6Q_RGMII_RD3 = 13,
> +	MX6Q_RGMII_RXC = 14,
> +	MX6Q_EIM_A25 = 15,
> +	MX6Q_EIM_EB2 = 16,
> +	MX6Q_EIM_D16 = 17,
> +	MX6Q_EIM_D17 = 18,
> +	MX6Q_EIM_D18 = 19,
> +	MX6Q_EIM_D19 = 20,
> +	MX6Q_EIM_D20 = 21,
> +	MX6Q_EIM_D21 = 22,
> +	MX6Q_EIM_D22 = 23,
> +	MX6Q_EIM_D23 = 24,
> +	MX6Q_EIM_EB3 = 25,
> +	MX6Q_EIM_D24 = 26,
> +	MX6Q_EIM_D25 = 27,
> +	MX6Q_EIM_D26 = 28,
> +	MX6Q_EIM_D27 = 29,
> +	MX6Q_EIM_D28 = 30,
> +	MX6Q_EIM_D29 = 31,
> +	MX6Q_EIM_D30 = 32,
> +	MX6Q_EIM_D31 = 33,
> +	MX6Q_EIM_A24 = 34,
> +	MX6Q_EIM_A23 = 35,
> +	MX6Q_EIM_A22 = 36,
> +	MX6Q_EIM_A21 = 37,
> +	MX6Q_EIM_A20 = 38,
> +	MX6Q_EIM_A19 = 39,
> +	MX6Q_EIM_A18 = 40,
> +	MX6Q_EIM_A17 = 41,
> +	MX6Q_EIM_A16 = 42,
> +	MX6Q_EIM_CS0 = 43,
> +	MX6Q_EIM_CS1 = 44,
> +	MX6Q_EIM_OE = 45,
> +	MX6Q_EIM_RW = 46,
> +	MX6Q_EIM_LBA = 47,
> +	MX6Q_EIM_EB0 = 48,
> +	MX6Q_EIM_EB1 = 49,
> +	MX6Q_EIM_DA0 = 50,
> +	MX6Q_EIM_DA1 = 51,
> +	MX6Q_EIM_DA2 = 52,
> +	MX6Q_EIM_DA3 = 53,
> +	MX6Q_EIM_DA4 = 54,
> +	MX6Q_EIM_DA5 = 55,
> +	MX6Q_EIM_DA6 = 56,
> +	MX6Q_EIM_DA7 = 57,
> +	MX6Q_EIM_DA8 = 58,
> +	MX6Q_EIM_DA9 = 59,
> +	MX6Q_EIM_DA10 = 60,
> +	MX6Q_EIM_DA11 = 61,
> +	MX6Q_EIM_DA12 = 62,
> +	MX6Q_EIM_DA13 = 63,
> +	MX6Q_EIM_DA14 = 64,
> +	MX6Q_EIM_DA15 = 65,
> +	MX6Q_EIM_WAIT = 66,
> +	MX6Q_EIM_BCLK = 67,
> +	MX6Q_DI0_DISP_CLK = 68,
> +	MX6Q_DI0_PIN15 = 69,
> +	MX6Q_DI0_PIN2 = 70,
> +	MX6Q_DI0_PIN3 = 71,
> +	MX6Q_DI0_PIN4 = 72,
> +	MX6Q_DISP0_DAT0 = 73,
> +	MX6Q_DISP0_DAT1 = 74,
> +	MX6Q_DISP0_DAT2 = 75,
> +	MX6Q_DISP0_DAT3 = 76,
> +	MX6Q_DISP0_DAT4 = 77,
> +	MX6Q_DISP0_DAT5 = 78,
> +	MX6Q_DISP0_DAT6 = 79,
> +	MX6Q_DISP0_DAT7 = 80,
> +	MX6Q_DISP0_DAT8 = 81,
> +	MX6Q_DISP0_DAT9 = 82,
> +	MX6Q_DISP0_DAT10 = 83,
> +	MX6Q_DISP0_DAT11 = 84,
> +	MX6Q_DISP0_DAT12 = 85,
> +	MX6Q_DISP0_DAT13 = 86,
> +	MX6Q_DISP0_DAT14 = 87,
> +	MX6Q_DISP0_DAT15 = 88,
> +	MX6Q_DISP0_DAT16 = 89,
> +	MX6Q_DISP0_DAT17 = 90,
> +	MX6Q_DISP0_DAT18 = 91,
> +	MX6Q_DISP0_DAT19 = 92,
> +	MX6Q_DISP0_DAT20 = 93,
> +	MX6Q_DISP0_DAT21 = 94,
> +	MX6Q_DISP0_DAT22 = 95,
> +	MX6Q_DISP0_DAT23 = 96,
> +	MX6Q_ENET_MDIO = 97,
> +	MX6Q_ENET_REF_CLK = 98,
> +	MX6Q_ENET_RX_ER = 99,
> +	MX6Q_ENET_CRS_DV = 100,
> +	MX6Q_ENET_RXD1 = 101,
> +	MX6Q_ENET_RXD0 = 102,
> +	MX6Q_ENET_TX_EN = 103,
> +	MX6Q_ENET_TXD1 = 104,
> +	MX6Q_ENET_TXD0 = 105,
> +	MX6Q_ENET_MDC = 106,
> +	MX6Q_KEY_COL0 = 107,
> +	MX6Q_KEY_ROW0 = 108,
> +	MX6Q_KEY_COL1 = 109,
> +	MX6Q_KEY_ROW1 = 110,
> +	MX6Q_KEY_COL2 = 111,
> +	MX6Q_KEY_ROW2 = 112,
> +	MX6Q_KEY_COL3 = 113,
> +	MX6Q_KEY_ROW3 = 114,
> +	MX6Q_KEY_COL4 = 115,
> +	MX6Q_KEY_ROW4 = 116,
> +	MX6Q_GPIO_0 = 117,
> +	MX6Q_GPIO_1 = 118,
> +	MX6Q_GPIO_9 = 119,
> +	MX6Q_GPIO_3 = 120,
> +	MX6Q_GPIO_6 = 121,
> +	MX6Q_GPIO_2 = 122,
> +	MX6Q_GPIO_4 = 123,
> +	MX6Q_GPIO_5 = 124,
> +	MX6Q_GPIO_7 = 125,
> +	MX6Q_GPIO_8 = 126,
> +	MX6Q_GPIO_16 = 127,
> +	MX6Q_GPIO_17 = 128,
> +	MX6Q_GPIO_18 = 129,
> +	MX6Q_GPIO_19 = 130,
> +	MX6Q_CSI0_PIXCLK = 131,
> +	MX6Q_CSI0_MCLK = 132,
> +	MX6Q_CSI0_DATA_EN = 133,
> +	MX6Q_CSI0_VSYNC = 134,
> +	MX6Q_CSI0_DAT4 = 135,
> +	MX6Q_CSI0_DAT5 = 136,
> +	MX6Q_CSI0_DAT6 = 137,
> +	MX6Q_CSI0_DAT7 = 138,
> +	MX6Q_CSI0_DAT8 = 139,
> +	MX6Q_CSI0_DAT9 = 140,
> +	MX6Q_CSI0_DAT10 = 141,
> +	MX6Q_CSI0_DAT11 = 142,
> +	MX6Q_CSI0_DAT12 = 143,
> +	MX6Q_CSI0_DAT13 = 144,
> +	MX6Q_CSI0_DAT14 = 145,
> +	MX6Q_CSI0_DAT15 = 146,
> +	MX6Q_CSI0_DAT16 = 147,
> +	MX6Q_CSI0_DAT17 = 148,
> +	MX6Q_CSI0_DAT18 = 149,
> +	MX6Q_CSI0_DAT19 = 150,
> +	MX6Q_SD3_DAT7 = 151,
> +	MX6Q_SD3_DAT6 = 152,
> +	MX6Q_SD3_DAT5 = 153,
> +	MX6Q_SD3_DAT4 = 154,
> +	MX6Q_SD3_CMD = 155,
> +	MX6Q_SD3_CLK = 156,
> +	MX6Q_SD3_DAT0 = 157,
> +	MX6Q_SD3_DAT1 = 158,
> +	MX6Q_SD3_DAT2 = 159,
> +	MX6Q_SD3_DAT3 = 160,
> +	MX6Q_SD3_RST = 161,
> +	MX6Q_NANDF_CLE = 162,
> +	MX6Q_NANDF_ALE = 163,
> +	MX6Q_NANDF_WP_B = 164,
> +	MX6Q_NANDF_RB0 = 165,
> +	MX6Q_NANDF_CS0 = 166,
> +	MX6Q_NANDF_CS1 = 167,
> +	MX6Q_NANDF_CS2 = 168,
> +	MX6Q_NANDF_CS3 = 169,
> +	MX6Q_SD4_CMD = 170,
> +	MX6Q_SD4_CLK = 171,
> +	MX6Q_NANDF_D0 = 172,
> +	MX6Q_NANDF_D1 = 173,
> +	MX6Q_NANDF_D2 = 174,
> +	MX6Q_NANDF_D3 = 175,
> +	MX6Q_NANDF_D4 = 176,
> +	MX6Q_NANDF_D5 = 177,
> +	MX6Q_NANDF_D6 = 178,
> +	MX6Q_NANDF_D7 = 179,
> +	MX6Q_SD4_DAT0 = 180,
> +	MX6Q_SD4_DAT1 = 181,
> +	MX6Q_SD4_DAT2 = 182,
> +	MX6Q_SD4_DAT3 = 183,
> +	MX6Q_SD4_DAT4 = 184,
> +	MX6Q_SD4_DAT5 = 185,
> +	MX6Q_SD4_DAT6 = 186,
> +	MX6Q_SD4_DAT7 = 187,
> +	MX6Q_SD1_DAT1 = 188,
> +	MX6Q_SD1_DAT0 = 189,
> +	MX6Q_SD1_DAT3 = 190,
> +	MX6Q_SD1_CMD = 191,
> +	MX6Q_SD1_DAT2 = 192,
> +	MX6Q_SD1_CLK = 193,
> +	MX6Q_SD2_CLK = 194,
> +	MX6Q_SD2_CMD = 195,
> +	MX6Q_SD2_DAT3 = 196

Please add a comma for even the last item.  It's a common patter for
easing the future expanding on the list.

> +};
> +
> +/* Pad names for the pinmux subsystem */
> +static const struct pinctrl_pin_desc mx6q_pads[] = {

Probably use imx6q_pinctrl_pins or imx6q_pinctrl_pads here to
distinguish from the name imx6q_pads suggested above.

Regards,
Shawn

[RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Guo Shawn-R65073
> Sent: Tuesday, December 06, 2011 3:23 PM
> To: Dong Aisheng-B29396
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> kernel at pengutronix.de
> Subject: Re: [RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support
> 
> On Sun, Dec 04, 2011 at 07:49:44PM +0800, Dong Aisheng wrote:
> > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Shawn Guo <shanw.guo@freescale.com>
> > ---
> >  drivers/pinctrl/pinmux-imx6q.c |  464
> > ++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 464 insertions(+), 0 deletions(-)
> >
> As I do not see the device tree probing added in pinmux-imx-core.c, I
> expect the patch has not got the chance to run on imx6, right?
> 
Yes, I would add it if the pinctrl framework supports DT.

> > diff --git a/drivers/pinctrl/pinmux-imx6q.c
> > b/drivers/pinctrl/pinmux-imx6q.c new file mode 100644 index
> > 0000000..5641199
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinmux-imx6q.c
> > @@ -0,0 +1,464 @@
> > +/*
> > + * imx6q pinmux driver based on imx pinmux core
> > + *
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + * Copyright (C) 2011 Linaro, Inc.
> > + *
> > + * Author: Dong Aisheng <dong.aisheng@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/err.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> > +
> > +#include "pinmux-imx-core.h"
> > +
> > +#define MX6Q_IOMUXC_MUX_OFSSET	0x4c
> > +#define MX6Q_IOMUXC_MAXPIN	(25*25)
> > +
> I would suggest replace all the occurrences of 'MX6Q'/'mx6q' with
> 'IMX6Q'/'imx6q' to force the consistency.
> 
> > +enum imx_mx6q_pads {
> 
> IMO, 'imx6q_pads' is good enough.
> 
> > +	MX6Q_SD2_DAT1 = 0,
> > +	MX6Q_SD2_DAT2 = 1,
> > +	MX6Q_SD2_DAT0 = 2,
> > +	MX6Q_RGMII_TXC = 3,
> > +	MX6Q_RGMII_TD0 = 4,
> > +	MX6Q_RGMII_TD1 = 5,
> > +	MX6Q_RGMII_TD2 = 6,
> > +	MX6Q_RGMII_TD3 = 7,
> > +	MX6Q_RGMII_RX_CTL = 8,
> > +	MX6Q_RGMII_RD0 = 9,
> > +	MX6Q_RGMII_TX_CTL = 10,
> > +	MX6Q_RGMII_RD1 = 11,
> > +	MX6Q_RGMII_RD2 = 12,
> > +	MX6Q_RGMII_RD3 = 13,
> > +	MX6Q_RGMII_RXC = 14,
> > +	MX6Q_EIM_A25 = 15,
> > +	MX6Q_EIM_EB2 = 16,
> > +	MX6Q_EIM_D16 = 17,
> > +	MX6Q_EIM_D17 = 18,
> > +	MX6Q_EIM_D18 = 19,
> > +	MX6Q_EIM_D19 = 20,
> > +	MX6Q_EIM_D20 = 21,
> > +	MX6Q_EIM_D21 = 22,
> > +	MX6Q_EIM_D22 = 23,
> > +	MX6Q_EIM_D23 = 24,
> > +	MX6Q_EIM_EB3 = 25,
> > +	MX6Q_EIM_D24 = 26,
> > +	MX6Q_EIM_D25 = 27,
> > +	MX6Q_EIM_D26 = 28,
> > +	MX6Q_EIM_D27 = 29,
> > +	MX6Q_EIM_D28 = 30,
> > +	MX6Q_EIM_D29 = 31,
> > +	MX6Q_EIM_D30 = 32,
> > +	MX6Q_EIM_D31 = 33,
> > +	MX6Q_EIM_A24 = 34,
> > +	MX6Q_EIM_A23 = 35,
> > +	MX6Q_EIM_A22 = 36,
> > +	MX6Q_EIM_A21 = 37,
> > +	MX6Q_EIM_A20 = 38,
> > +	MX6Q_EIM_A19 = 39,
> > +	MX6Q_EIM_A18 = 40,
> > +	MX6Q_EIM_A17 = 41,
> > +	MX6Q_EIM_A16 = 42,
> > +	MX6Q_EIM_CS0 = 43,
> > +	MX6Q_EIM_CS1 = 44,
> > +	MX6Q_EIM_OE = 45,
> > +	MX6Q_EIM_RW = 46,
> > +	MX6Q_EIM_LBA = 47,
> > +	MX6Q_EIM_EB0 = 48,
> > +	MX6Q_EIM_EB1 = 49,
> > +	MX6Q_EIM_DA0 = 50,
> > +	MX6Q_EIM_DA1 = 51,
> > +	MX6Q_EIM_DA2 = 52,
> > +	MX6Q_EIM_DA3 = 53,
> > +	MX6Q_EIM_DA4 = 54,
> > +	MX6Q_EIM_DA5 = 55,
> > +	MX6Q_EIM_DA6 = 56,
> > +	MX6Q_EIM_DA7 = 57,
> > +	MX6Q_EIM_DA8 = 58,
> > +	MX6Q_EIM_DA9 = 59,
> > +	MX6Q_EIM_DA10 = 60,
> > +	MX6Q_EIM_DA11 = 61,
> > +	MX6Q_EIM_DA12 = 62,
> > +	MX6Q_EIM_DA13 = 63,
> > +	MX6Q_EIM_DA14 = 64,
> > +	MX6Q_EIM_DA15 = 65,
> > +	MX6Q_EIM_WAIT = 66,
> > +	MX6Q_EIM_BCLK = 67,
> > +	MX6Q_DI0_DISP_CLK = 68,
> > +	MX6Q_DI0_PIN15 = 69,
> > +	MX6Q_DI0_PIN2 = 70,
> > +	MX6Q_DI0_PIN3 = 71,
> > +	MX6Q_DI0_PIN4 = 72,
> > +	MX6Q_DISP0_DAT0 = 73,
> > +	MX6Q_DISP0_DAT1 = 74,
> > +	MX6Q_DISP0_DAT2 = 75,
> > +	MX6Q_DISP0_DAT3 = 76,
> > +	MX6Q_DISP0_DAT4 = 77,
> > +	MX6Q_DISP0_DAT5 = 78,
> > +	MX6Q_DISP0_DAT6 = 79,
> > +	MX6Q_DISP0_DAT7 = 80,
> > +	MX6Q_DISP0_DAT8 = 81,
> > +	MX6Q_DISP0_DAT9 = 82,
> > +	MX6Q_DISP0_DAT10 = 83,
> > +	MX6Q_DISP0_DAT11 = 84,
> > +	MX6Q_DISP0_DAT12 = 85,
> > +	MX6Q_DISP0_DAT13 = 86,
> > +	MX6Q_DISP0_DAT14 = 87,
> > +	MX6Q_DISP0_DAT15 = 88,
> > +	MX6Q_DISP0_DAT16 = 89,
> > +	MX6Q_DISP0_DAT17 = 90,
> > +	MX6Q_DISP0_DAT18 = 91,
> > +	MX6Q_DISP0_DAT19 = 92,
> > +	MX6Q_DISP0_DAT20 = 93,
> > +	MX6Q_DISP0_DAT21 = 94,
> > +	MX6Q_DISP0_DAT22 = 95,
> > +	MX6Q_DISP0_DAT23 = 96,
> > +	MX6Q_ENET_MDIO = 97,
> > +	MX6Q_ENET_REF_CLK = 98,
> > +	MX6Q_ENET_RX_ER = 99,
> > +	MX6Q_ENET_CRS_DV = 100,
> > +	MX6Q_ENET_RXD1 = 101,
> > +	MX6Q_ENET_RXD0 = 102,
> > +	MX6Q_ENET_TX_EN = 103,
> > +	MX6Q_ENET_TXD1 = 104,
> > +	MX6Q_ENET_TXD0 = 105,
> > +	MX6Q_ENET_MDC = 106,
> > +	MX6Q_KEY_COL0 = 107,
> > +	MX6Q_KEY_ROW0 = 108,
> > +	MX6Q_KEY_COL1 = 109,
> > +	MX6Q_KEY_ROW1 = 110,
> > +	MX6Q_KEY_COL2 = 111,
> > +	MX6Q_KEY_ROW2 = 112,
> > +	MX6Q_KEY_COL3 = 113,
> > +	MX6Q_KEY_ROW3 = 114,
> > +	MX6Q_KEY_COL4 = 115,
> > +	MX6Q_KEY_ROW4 = 116,
> > +	MX6Q_GPIO_0 = 117,
> > +	MX6Q_GPIO_1 = 118,
> > +	MX6Q_GPIO_9 = 119,
> > +	MX6Q_GPIO_3 = 120,
> > +	MX6Q_GPIO_6 = 121,
> > +	MX6Q_GPIO_2 = 122,
> > +	MX6Q_GPIO_4 = 123,
> > +	MX6Q_GPIO_5 = 124,
> > +	MX6Q_GPIO_7 = 125,
> > +	MX6Q_GPIO_8 = 126,
> > +	MX6Q_GPIO_16 = 127,
> > +	MX6Q_GPIO_17 = 128,
> > +	MX6Q_GPIO_18 = 129,
> > +	MX6Q_GPIO_19 = 130,
> > +	MX6Q_CSI0_PIXCLK = 131,
> > +	MX6Q_CSI0_MCLK = 132,
> > +	MX6Q_CSI0_DATA_EN = 133,
> > +	MX6Q_CSI0_VSYNC = 134,
> > +	MX6Q_CSI0_DAT4 = 135,
> > +	MX6Q_CSI0_DAT5 = 136,
> > +	MX6Q_CSI0_DAT6 = 137,
> > +	MX6Q_CSI0_DAT7 = 138,
> > +	MX6Q_CSI0_DAT8 = 139,
> > +	MX6Q_CSI0_DAT9 = 140,
> > +	MX6Q_CSI0_DAT10 = 141,
> > +	MX6Q_CSI0_DAT11 = 142,
> > +	MX6Q_CSI0_DAT12 = 143,
> > +	MX6Q_CSI0_DAT13 = 144,
> > +	MX6Q_CSI0_DAT14 = 145,
> > +	MX6Q_CSI0_DAT15 = 146,
> > +	MX6Q_CSI0_DAT16 = 147,
> > +	MX6Q_CSI0_DAT17 = 148,
> > +	MX6Q_CSI0_DAT18 = 149,
> > +	MX6Q_CSI0_DAT19 = 150,
> > +	MX6Q_SD3_DAT7 = 151,
> > +	MX6Q_SD3_DAT6 = 152,
> > +	MX6Q_SD3_DAT5 = 153,
> > +	MX6Q_SD3_DAT4 = 154,
> > +	MX6Q_SD3_CMD = 155,
> > +	MX6Q_SD3_CLK = 156,
> > +	MX6Q_SD3_DAT0 = 157,
> > +	MX6Q_SD3_DAT1 = 158,
> > +	MX6Q_SD3_DAT2 = 159,
> > +	MX6Q_SD3_DAT3 = 160,
> > +	MX6Q_SD3_RST = 161,
> > +	MX6Q_NANDF_CLE = 162,
> > +	MX6Q_NANDF_ALE = 163,
> > +	MX6Q_NANDF_WP_B = 164,
> > +	MX6Q_NANDF_RB0 = 165,
> > +	MX6Q_NANDF_CS0 = 166,
> > +	MX6Q_NANDF_CS1 = 167,
> > +	MX6Q_NANDF_CS2 = 168,
> > +	MX6Q_NANDF_CS3 = 169,
> > +	MX6Q_SD4_CMD = 170,
> > +	MX6Q_SD4_CLK = 171,
> > +	MX6Q_NANDF_D0 = 172,
> > +	MX6Q_NANDF_D1 = 173,
> > +	MX6Q_NANDF_D2 = 174,
> > +	MX6Q_NANDF_D3 = 175,
> > +	MX6Q_NANDF_D4 = 176,
> > +	MX6Q_NANDF_D5 = 177,
> > +	MX6Q_NANDF_D6 = 178,
> > +	MX6Q_NANDF_D7 = 179,
> > +	MX6Q_SD4_DAT0 = 180,
> > +	MX6Q_SD4_DAT1 = 181,
> > +	MX6Q_SD4_DAT2 = 182,
> > +	MX6Q_SD4_DAT3 = 183,
> > +	MX6Q_SD4_DAT4 = 184,
> > +	MX6Q_SD4_DAT5 = 185,
> > +	MX6Q_SD4_DAT6 = 186,
> > +	MX6Q_SD4_DAT7 = 187,
> > +	MX6Q_SD1_DAT1 = 188,
> > +	MX6Q_SD1_DAT0 = 189,
> > +	MX6Q_SD1_DAT3 = 190,
> > +	MX6Q_SD1_CMD = 191,
> > +	MX6Q_SD1_DAT2 = 192,
> > +	MX6Q_SD1_CLK = 193,
> > +	MX6Q_SD2_CLK = 194,
> > +	MX6Q_SD2_CMD = 195,
> > +	MX6Q_SD2_DAT3 = 196
> 
> Please add a comma for even the last item.  It's a common patter for
> easing the future expanding on the list.
> 
Acked.

> > +};
> > +
> > +/* Pad names for the pinmux subsystem */ static const struct
> > +pinctrl_pin_desc mx6q_pads[] = {
> 
> Probably use imx6q_pinctrl_pins or imx6q_pinctrl_pads here to distinguish
> from the name imx6q_pads suggested above.
> 
I will take this suggestion.

Regards
Dong Aisheng

[RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support

[Shawn Guo]

On Tue, Dec 06, 2011 at 03:23:34PM +0800, Dong Aisheng-B29396 wrote:
> > -----Original Message-----
> > From: Guo Shawn-R65073
> > Sent: Tuesday, December 06, 2011 3:23 PM
> > To: Dong Aisheng-B29396
> > Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> > kernel at pengutronix.de
> > Subject: Re: [RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support
> > 
> > On Sun, Dec 04, 2011 at 07:49:44PM +0800, Dong Aisheng wrote:
> > > Signed-off-by: Dong Aisheng <b29396@freescale.com>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Shawn Guo <shanw.guo@freescale.com>
> > > ---
> > >  drivers/pinctrl/pinmux-imx6q.c |  464
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 464 insertions(+), 0 deletions(-)
> > >
> > As I do not see the device tree probing added in pinmux-imx-core.c, I
> > expect the patch has not got the chance to run on imx6, right?
> > 
> Yes, I would add it if the pinctrl framework supports DT.
> 
It's not really the business of pinctrl framework.  We can run your
patch on imx6 (DT) right now.  I attached the quick hacking based on
your series below, so that we can push pinctrl DT bindings forward
(move those data into device tree).

Regards,
Shawn

---
 arch/arm/boot/dts/imx6q.dtsi       |    1 +
 arch/arm/mach-imx/Kconfig          |    1 +
 arch/arm/mach-imx/mach-imx6q.c     |    8 +++++++-
 drivers/mmc/host/sdhci-esdhc-imx.c |   20 ++++++++++++++++++++
 drivers/pinctrl/pinmux-imx-core.c  |   36 +++++++++++++++++++-----------------
 drivers/pinctrl/pinmux-imx-core.h  |    2 ++
 6 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 7dda599..42499bb 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -386,6 +386,7 @@
 			};
 
 			iomuxc at 020e0000 {
+				compatible = "fsl,imx6q-iomuxc";
 				reg = <0x020e0000 0x4000>;
 			};
 
diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index c44aa97..1e0befa 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -602,6 +602,7 @@ config SOC_IMX6Q
 	select HAVE_IMX_GPC
 	select HAVE_IMX_MMDC
 	select HAVE_IMX_SRC
+	select PINCTRL
 	select USE_OF
 
 	help
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 9cd860a..e12ae63 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/pinctrl/machine.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/hardware/gic.h>
 #include <asm/mach/arch.h>
@@ -23,10 +24,15 @@
 #include <mach/common.h>
 #include <mach/hardware.h>
 
+static struct pinmux_map imx6q_pinmux_map[] = {
+	PINMUX_MAP_PRIMARY("usdhc4", "sd4", "219c000.usdhc"),
+};
+
 static void __init imx6q_init_machine(void)
 {
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
-
+	pinmux_register_mappings(imx6q_pinmux_map,
+				 ARRAY_SIZE(imx6q_pinmux_map));
 	imx6q_pm_init();
 }
 
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 4b976f0..4504136 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
+#include <linux/pinctrl/pinmux.h>
 #include <mach/esdhc.h>
 #include "sdhci-pltfm.h"
 #include "sdhci-esdhc.h"
@@ -68,6 +69,7 @@ struct pltfm_imx_data {
 	int flags;
 	u32 scratchpad;
 	enum imx_esdhc_type devtype;
+	struct pinmux *pmx;
 	struct esdhc_platform_data boarddata;
 };
 
@@ -439,6 +441,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	struct clk *clk;
 	int err;
 	struct pltfm_imx_data *imx_data;
+	struct pinmux *pmx;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_esdhc_imx_pdata);
 	if (IS_ERR(host))
@@ -466,6 +469,16 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	clk_enable(clk);
 	pltfm_host->clk = clk;
 
+	pmx = pinmux_get(&pdev->dev, NULL);
+	if (IS_ERR(pmx)) {
+		err = PTR_ERR(pmx);
+		goto err_pmx_get;
+	}
+	err = pinmux_enable(pmx);
+	if (err)
+		goto err_pmx_enable;
+	imx_data->pmx = pmx;
+
 	if (!is_imx25_esdhc(imx_data))
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
@@ -558,6 +571,10 @@ no_card_detect_irq:
 		gpio_free(boarddata->wp_gpio);
 no_card_detect_pin:
 no_board_data:
+	pinmux_disable(imx_data->pmx);
+err_pmx_enable:
+	pinmux_put(imx_data->pmx);
+err_pmx_get:
 	clk_disable(pltfm_host->clk);
 	clk_put(pltfm_host->clk);
 err_clk_get:
@@ -585,6 +602,9 @@ static int __devexit sdhci_esdhc_imx_remove(struct platform_device *pdev)
 		gpio_free(boarddata->cd_gpio);
 	}
 
+	pinmux_disable(imx_data->pmx);
+	pinmux_put(imx_data->pmx);
+
 	clk_disable(pltfm_host->clk);
 	clk_put(pltfm_host->clk);
 	kfree(imx_data);
diff --git a/drivers/pinctrl/pinmux-imx-core.c b/drivers/pinctrl/pinmux-imx-core.c
index 1e60932..4d5c384 100644
--- a/drivers/pinctrl/pinmux-imx-core.c
+++ b/drivers/pinctrl/pinmux-imx-core.c
@@ -14,7 +14,7 @@
 
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
+#include <linux/of_device.h>
 #include <linux/io.h>
 #include <linux/err.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -38,19 +38,6 @@ struct imx_pmx {
 #define IMX_PINCTRL_REG_SIZE 4
 #define IMX_PINCTRL_MAX_FUNC 7
 
-extern struct imx_pinctrl_info mx53_pinctrl_info;
-extern struct imx_pinctrl_info mx6q_pinctrl_info;
-
-static struct platform_device_id imx_pinctrl_devtype[] = {
-	{
-		.name = "sdhci-pinctrl-imx53",
-		.driver_data = (kernel_ulong_t)&mx53_pinctrl_info,
-	}, {
-		.name = "sdhci-pinctrl-imx6q",
-		.driver_data = (kernel_ulong_t)&mx6q_pinctrl_info,
-	},
-};
-
 static int imx_list_groups(struct pinctrl_dev *pctldev, unsigned selector)
 {
 	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
@@ -144,6 +131,12 @@ static int imx_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 	return 0;
 }
 
+static void imx_pmx_disable(struct pinctrl_dev *pctldev, unsigned func_selector,
+			    unsigned group_selector)
+{
+	/* nothing to do here */
+}
+
 static int imx_pmx_list_funcs(struct pinctrl_dev *pctldev, unsigned selector)
 {
 	struct imx_pmx *ipmx = pinctrl_dev_get_drvdata(pctldev);
@@ -182,6 +175,7 @@ static struct pinmux_ops imx_pmx_ops = {
 	.get_function_name = imx_pmx_get_func_name,
 	.get_function_groups = imx_pmx_get_groups,
 	.enable = imx_pmx_enable,
+	.disable = imx_pmx_disable,
 };
 
 static struct pinctrl_desc imx_pmx_desc = {
@@ -199,17 +193,25 @@ static inline void imx_pmx_desc_init(struct pinctrl_desc *pmx_desc,
 	pmx_desc->maxpin = info->maxpin;
 }
 
+static const struct of_device_id imx_pmx_dt_ids[] = {
+	{ .compatible = "fsl,imx6q-iomuxc", .data = (void *) &mx6q_pinctrl_info, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_pmx_dt_ids);
+
 static int __init imx_pmx_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id =
+			of_match_device(imx_pmx_dt_ids, &pdev->dev);
 	struct device *dev = &pdev->dev;
 	struct imx_pmx *ipmx;
 	struct resource *res;
 	struct imx_pinctrl_info *info;
 	resource_size_t res_size;
 
-	info = (struct imx_pinctrl_info *)pdev->id_entry->driver_data;
+	info = of_id->data;
 	if (!info || !info->pins || !info->groups || !info->functions
-		|| (info->maxpin > info->npins)) {
+		|| !(info->maxpin > info->npins)) {
 		dev_err(&pdev->dev, "wrong pinctrl info\n");
 		return -EINVAL;
 	}
@@ -262,8 +264,8 @@ static struct platform_driver imx_pmx_driver = {
 	.driver = {
 		.name = DRIVER_NAME,
 		.owner = THIS_MODULE,
+		.of_match_table = imx_pmx_dt_ids,
 	},
-	.id_table = imx_pinctrl_devtype,
 	.remove = __exit_p(imx_pmx_remove),
 };
 
diff --git a/drivers/pinctrl/pinmux-imx-core.h b/drivers/pinctrl/pinmux-imx-core.h
index 8ccc0c6..09d988d 100644
--- a/drivers/pinctrl/pinmux-imx-core.h
+++ b/drivers/pinctrl/pinmux-imx-core.h
@@ -62,6 +62,8 @@ struct imx_pinctrl_info {
 	u32 mux_offset;
 };
 
+extern const struct imx_pinctrl_info mx6q_pinctrl_info;
+
 #define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)
 
 #define IMX_PIN_GROUP(n, p, m)  \
-- 
1.7.4.1

[RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support

[Linus Walleij]

On Tue, Dec 6, 2011 at 8:23 AM, Dong Aisheng-B29396
<B29396@freescale.com> wrote:
>> From: Guo Shawn-R65073
>> As I do not see the device tree probing added in pinmux-imx-core.c, I
>> expect the patch has not got the chance to run on imx6, right?
>>
> Yes, I would add it if the pinctrl framework supports DT.

There are many levels of DT.

You can of course add DT-support for anything driver-specific, such
as init data for some registers, or anything else you would normally
pass in through platform data. So please just do that if imx6 already
has support for such things.

What we're discussing about right now on devicetree-discuss
is where to put the mapping tables etc that is currently registered
by the board, that has no obvious solution as there is some
disagreement on how to do that.

Linus Walleij

[RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij at linaro.org]
> Sent: Wednesday, December 07, 2011 5:10 PM
> To: Dong Aisheng-B29396; Guo Shawn-R65073
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> kernel at pengutronix.de
> Subject: Re: [RFC PATCH 3/3] pinctrl: imx: add pinmux-imx6q support
> 
> On Tue, Dec 6, 2011 at 8:23 AM, Dong Aisheng-B29396 <B29396@freescale.com>
> wrote:
> >> From: Guo Shawn-R65073
> >> As I do not see the device tree probing added in pinmux-imx-core.c, I
> >> expect the patch has not got the chance to run on imx6, right?
> >>
> > Yes, I would add it if the pinctrl framework supports DT.
> 
> There are many levels of DT.
> 
> You can of course add DT-support for anything driver-specific, such as
> init data for some registers, or anything else you would normally pass in
> through platform data. So please just do that if imx6 already has support
> for such things.
> 
> What we're discussing about right now on devicetree-discuss is where to
> put the mapping tables etc that is currently registered by the board,
> that has no obvious solution as there is some disagreement on how to do
> that.
> 
Yes, thanks for your info.
I'm new to device tree and may take some time to get more familiar with it.

I discussed with Shawn today and now we're going to move data to dt and run
this driver on mx6q first.

Regards
Dong Aisheng

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Linus Walleij]

On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com> wrote:

> ---
> This patch series shows the basic idea of driver design. There're still
> some TODOes like adding more pinmux functions and gpio support.
> The patch is here for request for comments on the driver design
> and other might exist issues.
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shanw.guo@freescale.com>

Dong, this is looking real good from pinctrl subsystem point of view.

Thanks!
Linus Walleij

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij at linaro.org]
> Sent: Monday, December 05, 2011 9:10 PM
> To: Dong Aisheng-B29396
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at stericsson.com; s.hauer at pengutronix.de; Guo Shawn-R65073;
> kernel at pengutronix.de
> Subject: Re: [RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver
> 
> On Sun, Dec 4, 2011 at 12:49 PM, Dong Aisheng <b29396@freescale.com>
> wrote:
> 
> > ---
> > This patch series shows the basic idea of driver design. There're
> > still some TODOes like adding more pinmux functions and gpio support.
> > The patch is here for request for comments on the driver design and
> > other might exist issues.
> >
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Shawn Guo <shanw.guo@freescale.com>
> 
> Dong, this is looking real good from pinctrl subsystem point of view.
> 

It's good if it's ok.
Thanks for your review.

Regards
Dong Aisheng

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Shawn Guo]

On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> ---
> This patch series shows the basic idea of driver design. There're still
> some TODOes like adding more pinmux functions and gpio support.
> The patch is here for request for comments on the driver design
> and other might exist issues.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shanw.guo@freescale.com>
> ---
>  drivers/pinctrl/Kconfig           |    6 +
>  drivers/pinctrl/Makefile          |    2 +
>  drivers/pinctrl/pinmux-imx-core.c |  284 +++++++++++++++++++++++++++++++++++++
>  drivers/pinctrl/pinmux-imx-core.h |   83 +++++++++++
>  4 files changed, 375 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index ef56644..214d072 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -40,4 +40,10 @@ config PINMUX_U300
>  	help
>  	  Say Y here to enable the U300 pinmux driver
>  
> +config PINMUX_IMX
> +	bool "IMX pinmux driver"
> +	depends on ARCH_MXC
> +	select PINMUX
> +	help
> +	  Say Y here to enable the IMX pinmux driver
>  endif
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index bdc548a..764657b 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -4,5 +4,7 @@ ccflags-$(CONFIG_DEBUG_PINMUX)	+= -DDEBUG
>  
>  obj-$(CONFIG_PINCTRL)		+= core.o
>  obj-$(CONFIG_PINMUX)		+= pinmux.o
> +obj-$(CONFIG_PINMUX_IMX)	+= pinmux-imx-core.o \
> +				   pinmux-imx53.o pinmux-imx6q.o

You do not have pinmux-imx53.c and pinmux-imx6q.c in this patch yet.

>  obj-$(CONFIG_PINMUX_SIRF)	+= pinmux-sirf.o
>  obj-$(CONFIG_PINMUX_U300)	+= pinmux-u300.o
> diff --git a/drivers/pinctrl/pinmux-imx-core.c b/drivers/pinctrl/pinmux-imx-core.c
> new file mode 100644
> index 0000000..1e60932
> --- /dev/null
> +++ b/drivers/pinctrl/pinmux-imx-core.c
> @@ -0,0 +1,284 @@
> +/*
> + * Core driver for the imx pin controller
> + *
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + * Copyright (C) 2011 Linaro Ltd.
> + *
> + * Author: Dong Aisheng <dong.aisheng@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "pinmux-imx-core.h"
> +
> +#define DRIVER_NAME "pinmux-imx"
> +
> +/**
> + * @dev: a pointer back to containing device
> + * @virtbase: the offset to the controller in virtual memory
> + */
> +struct imx_pmx {
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +	void __iomem *virtbase;
> +	struct imx_pinctrl_info *info;
> +};
> +
> +#define IMX_PINCTRL_REG_SIZE 4
> +#define IMX_PINCTRL_MAX_FUNC 7
> +
> +extern struct imx_pinctrl_info mx53_pinctrl_info;
> +extern struct imx_pinctrl_info mx6q_pinctrl_info;
> +

You do not have mx53_pinctrl_info and mx6q_pinctrl_info in this patch
yet.

Regards,
Shawn

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Guo Shawn-R65073
> Sent: Tuesday, December 06, 2011 3:06 PM
> To: Dong Aisheng-B29396
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> kernel at pengutronix.de
> Subject: Re: [RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver
> 
> On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> > ---
> > This patch series shows the basic idea of driver design. There're
> > still some TODOes like adding more pinmux functions and gpio support.
> > The patch is here for request for comments on the driver design and
> > other might exist issues.
> >
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Shawn Guo <shanw.guo@freescale.com>
> > ---
> >  drivers/pinctrl/Kconfig           |    6 +
> >  drivers/pinctrl/Makefile          |    2 +
> >  drivers/pinctrl/pinmux-imx-core.c |  284
> +++++++++++++++++++++++++++++++++++++
> >  drivers/pinctrl/pinmux-imx-core.h |   83 +++++++++++
> >  4 files changed, 375 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> > ef56644..214d072 100644
> > --- a/drivers/pinctrl/Kconfig
> > +++ b/drivers/pinctrl/Kconfig
> > @@ -40,4 +40,10 @@ config PINMUX_U300
> >  	help
> >  	  Say Y here to enable the U300 pinmux driver
> >
> > +config PINMUX_IMX
> > +	bool "IMX pinmux driver"
> > +	depends on ARCH_MXC
> > +	select PINMUX
> > +	help
> > +	  Say Y here to enable the IMX pinmux driver
> >  endif
> > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> > bdc548a..764657b 100644
> > --- a/drivers/pinctrl/Makefile
> > +++ b/drivers/pinctrl/Makefile
> > @@ -4,5 +4,7 @@ ccflags-$(CONFIG_DEBUG_PINMUX)	+= -DDEBUG
> >
> >  obj-$(CONFIG_PINCTRL)		+= core.o
> >  obj-$(CONFIG_PINMUX)		+= pinmux.o
> > +obj-$(CONFIG_PINMUX_IMX)	+= pinmux-imx-core.o \
> > +				   pinmux-imx53.o pinmux-imx6q.o
> 
> You do not have pinmux-imx53.c and pinmux-imx6q.c in this patch yet.
> 
> >  obj-$(CONFIG_PINMUX_SIRF)	+= pinmux-sirf.o
> >  obj-$(CONFIG_PINMUX_U300)	+= pinmux-u300.o
> > diff --git a/drivers/pinctrl/pinmux-imx-core.c
> > b/drivers/pinctrl/pinmux-imx-core.c
> > new file mode 100644
> > index 0000000..1e60932
> > --- /dev/null
> > +++ b/drivers/pinctrl/pinmux-imx-core.c
> > @@ -0,0 +1,284 @@
> > +/*
> > + * Core driver for the imx pin controller
> > + *
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + * Copyright (C) 2011 Linaro Ltd.
> > + *
> > + * Author: Dong Aisheng <dong.aisheng@linaro.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/err.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> > +
> > +#include "pinmux-imx-core.h"
> > +
> > +#define DRIVER_NAME "pinmux-imx"
> > +
> > +/**
> > + * @dev: a pointer back to containing device
> > + * @virtbase: the offset to the controller in virtual memory  */
> > +struct imx_pmx {
> > +	struct device *dev;
> > +	struct pinctrl_dev *pctl;
> > +	void __iomem *virtbase;
> > +	struct imx_pinctrl_info *info;
> > +};
> > +
> > +#define IMX_PINCTRL_REG_SIZE 4
> > +#define IMX_PINCTRL_MAX_FUNC 7
> > +
> > +extern struct imx_pinctrl_info mx53_pinctrl_info; extern struct
> > +imx_pinctrl_info mx6q_pinctrl_info;
> > +
> 
> You do not have mx53_pinctrl_info and mx6q_pinctrl_info in this patch yet.
> 
Yes, it's defined in another file in this patch series.
I just separate them for clear.
Maybe I should put them together in one patch or change sequence right?
BTW, if taking it from platform_data, I may not have this issue.

Regards
Dong Aisheng

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Shawn Guo]

On Tue, Dec 06, 2011 at 03:13:16PM +0800, Dong Aisheng-B29396 wrote:
> > -----Original Message-----
> > From: Guo Shawn-R65073
> > Sent: Tuesday, December 06, 2011 3:06 PM
> > To: Dong Aisheng-B29396
> > Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> > kernel at pengutronix.de
> > Subject: Re: [RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver
> > 
> > On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> > > ---
> > > This patch series shows the basic idea of driver design. There're
> > > still some TODOes like adding more pinmux functions and gpio support.
> > > The patch is here for request for comments on the driver design and
> > > other might exist issues.
> > >
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Shawn Guo <shanw.guo@freescale.com>
> > > ---
> > >  drivers/pinctrl/Kconfig           |    6 +
> > >  drivers/pinctrl/Makefile          |    2 +
> > >  drivers/pinctrl/pinmux-imx-core.c |  284
> > +++++++++++++++++++++++++++++++++++++
> > >  drivers/pinctrl/pinmux-imx-core.h |   83 +++++++++++
> > >  4 files changed, 375 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> > > ef56644..214d072 100644
> > > --- a/drivers/pinctrl/Kconfig
> > > +++ b/drivers/pinctrl/Kconfig
> > > @@ -40,4 +40,10 @@ config PINMUX_U300
> > >  	help
> > >  	  Say Y here to enable the U300 pinmux driver
> > >
> > > +config PINMUX_IMX
> > > +	bool "IMX pinmux driver"
> > > +	depends on ARCH_MXC
> > > +	select PINMUX
> > > +	help
> > > +	  Say Y here to enable the IMX pinmux driver
> > >  endif
> > > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> > > bdc548a..764657b 100644
> > > --- a/drivers/pinctrl/Makefile
> > > +++ b/drivers/pinctrl/Makefile
> > > @@ -4,5 +4,7 @@ ccflags-$(CONFIG_DEBUG_PINMUX)	+= -DDEBUG
> > >
> > >  obj-$(CONFIG_PINCTRL)		+= core.o
> > >  obj-$(CONFIG_PINMUX)		+= pinmux.o
> > > +obj-$(CONFIG_PINMUX_IMX)	+= pinmux-imx-core.o \
> > > +				   pinmux-imx53.o pinmux-imx6q.o
> > 
> > You do not have pinmux-imx53.c and pinmux-imx6q.c in this patch yet.
> > 
> > >  obj-$(CONFIG_PINMUX_SIRF)	+= pinmux-sirf.o
> > >  obj-$(CONFIG_PINMUX_U300)	+= pinmux-u300.o
> > > diff --git a/drivers/pinctrl/pinmux-imx-core.c
> > > b/drivers/pinctrl/pinmux-imx-core.c
> > > new file mode 100644
> > > index 0000000..1e60932
> > > --- /dev/null
> > > +++ b/drivers/pinctrl/pinmux-imx-core.c
> > > @@ -0,0 +1,284 @@
> > > +/*
> > > + * Core driver for the imx pin controller
> > > + *
> > > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > > + * Copyright (C) 2011 Linaro Ltd.
> > > + *
> > > + * Author: Dong Aisheng <dong.aisheng@linaro.org>
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > +modify
> > > + * it under the terms of the GNU General Public License as published
> > > +by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/err.h>
> > > +#include <linux/pinctrl/pinctrl.h>
> > > +#include <linux/pinctrl/pinmux.h>
> > > +
> > > +#include "pinmux-imx-core.h"
> > > +
> > > +#define DRIVER_NAME "pinmux-imx"
> > > +
> > > +/**
> > > + * @dev: a pointer back to containing device
> > > + * @virtbase: the offset to the controller in virtual memory  */
> > > +struct imx_pmx {
> > > +	struct device *dev;
> > > +	struct pinctrl_dev *pctl;
> > > +	void __iomem *virtbase;
> > > +	struct imx_pinctrl_info *info;
> > > +};
> > > +
> > > +#define IMX_PINCTRL_REG_SIZE 4
> > > +#define IMX_PINCTRL_MAX_FUNC 7
> > > +
> > > +extern struct imx_pinctrl_info mx53_pinctrl_info; extern struct
> > > +imx_pinctrl_info mx6q_pinctrl_info;
> > > +
> > 
> > You do not have mx53_pinctrl_info and mx6q_pinctrl_info in this patch yet.
> > 
> Yes, it's defined in another file in this patch series.
> I just separate them for clear.

To make the patch bisectable, you cannot reference anything after it.
That said if I build this patch, it will fail.

> Maybe I should put them together in one patch or change sequence right?

You need to add imx53 stuff here in patch #2, and add imx6q stuff in
patch #3.

-- 
Regards,
Shawn

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Shawn Guo]

On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> +static struct platform_device_id imx_pinctrl_devtype[] = {
> +	{
> +		.name = "sdhci-pinctrl-imx53",
> +		.driver_data = (kernel_ulong_t)&mx53_pinctrl_info,
> +	}, {
> +		.name = "sdhci-pinctrl-imx6q",
> +		.driver_data = (kernel_ulong_t)&mx6q_pinctrl_info,
> +	},
> +};

The platform_device_id table needs an empty entry to be the sentinel.

-- 
Regards,
Shawn

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Guo Shawn-R65073
> Sent: Tuesday, December 06, 2011 3:39 PM
> To: Dong Aisheng-B29396
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> kernel at pengutronix.de
> Subject: Re: [RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver
> 
> On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> > +static struct platform_device_id imx_pinctrl_devtype[] = {
> > +	{
> > +		.name = "sdhci-pinctrl-imx53",
> > +		.driver_data = (kernel_ulong_t)&mx53_pinctrl_info,
> > +	}, {
> > +		.name = "sdhci-pinctrl-imx6q",
> > +		.driver_data = (kernel_ulong_t)&mx6q_pinctrl_info,
> > +	},
> > +};
> 
> The platform_device_id table needs an empty entry to be the sentinel.
> 
Acked.

Regards
Dong Aisheng

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Shawn Guo]

On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> +static struct pinmux_ops imx_pmx_ops = {
> +	.list_functions = imx_pmx_list_funcs,
> +	.get_function_name = imx_pmx_get_func_name,
> +	.get_function_groups = imx_pmx_get_groups,
> +	.enable = imx_pmx_enable,
> +};

The pinmux core code will check the pinmux_ops as below.  !ops->disable
is one of the checking, while you do not have .disable hook.

int pinmux_check_ops(const struct pinmux_ops *ops)
{
        /* Check that we implement required operations */
        if (!ops->list_functions ||
            !ops->get_function_name ||
            !ops->get_function_groups ||
            !ops->enable ||
            !ops->disable)
                return -EINVAL;

        return 0;
}

[...]

> +static int __init imx_pmx_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx_pmx *ipmx;
> +	struct resource *res;
> +	struct imx_pinctrl_info *info;
> +	resource_size_t res_size;
> +
> +	info = (struct imx_pinctrl_info *)pdev->id_entry->driver_data;
> +	if (!info || !info->pins || !info->groups || !info->functions
> +		|| (info->maxpin > info->npins)) {

You must mean !(info->maxpin > info->npins) here?

Regards,
Shawn

> +		dev_err(&pdev->dev, "wrong pinctrl info\n");
> +		return -EINVAL;
> +	}

[RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Guo Shawn-R65073
> Sent: Tuesday, December 06, 2011 5:42 PM
> To: Dong Aisheng-B29396
> Cc: linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linus.walleij at stericsson.com; s.hauer at pengutronix.de;
> kernel at pengutronix.de
> Subject: Re: [RFC PATCH 1/3] pinctrl: imx: add pinmux imx core driver
> 
> On Sun, Dec 04, 2011 at 07:49:42PM +0800, Dong Aisheng wrote:
> > +static struct pinmux_ops imx_pmx_ops = {
> > +	.list_functions = imx_pmx_list_funcs,
> > +	.get_function_name = imx_pmx_get_func_name,
> > +	.get_function_groups = imx_pmx_get_groups,
> > +	.enable = imx_pmx_enable,
> > +};
> 
> The pinmux core code will check the pinmux_ops as below.  !ops->disable
> is one of the checking, while you do not have .disable hook.
> 
Then I have to add a disable function.

> int pinmux_check_ops(const struct pinmux_ops *ops) {
>         /* Check that we implement required operations */
>         if (!ops->list_functions ||
>             !ops->get_function_name ||
>             !ops->get_function_groups ||
>             !ops->enable ||
>             !ops->disable)
Not sure if we must check !ops->disable here since some soc may not have
disable function.

>                 return -EINVAL;
> 
>         return 0;
> }
> 
> [...]
> 
> > +static int __init imx_pmx_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct imx_pmx *ipmx;
> > +	struct resource *res;
> > +	struct imx_pinctrl_info *info;
> > +	resource_size_t res_size;
> > +
> > +	info = (struct imx_pinctrl_info *)pdev->id_entry->driver_data;
> > +	if (!info || !info->pins || !info->groups || !info->functions
> > +		|| (info->maxpin > info->npins)) {
> 
> You must mean !(info->maxpin > info->npins) here?
> 
Yes, thanks for this finding.

> Regards,
> Shawn
> 
> > +		dev_err(&pdev->dev, "wrong pinctrl info\n");
> > +		return -EINVAL;
> > +	}

Regards
Dong Aisheng