[PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

Add simple pinctrl driver using device tree data.

Currently this driver only works on omap2+ series of
processors, where there is either an 8 or 16-bit padconf
register for each pin. Support for other similar pinmux
controllers could be added.

Note that this patch does not yet support pinconf_ops
or GPIO. Further.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
Here's this one finally updated to use the common pinctrl bindings.
That sure cleaned up a bunch of the nasty things in this driver :)
Nice job Stephen!
---
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
new file mode 100644
index 0000000..7b32263
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
@@ -0,0 +1,125 @@
+Generic simple device tree based pinctrl driver
+
+Required properties:
+- compatible :  one of:
+	- "pinctrl-simple"
+	- "ti,omap2420-padconf"
+	- "ti,omap2430-padconf"
+	- "ti,omap3-padconf"
+	- "ti,omap4-padconf"
+- reg : offset and length of the register set for the mux registers
+- #pinctrl-cells : width of the pinctrl array, currently only 2 is supported
+- pinctrl-simple,register-width : pinmux register access width
+- pinctrl-simple,function-mask : mask of allowed pinmux function bits
+- pinctrl-simple,function-off : function off mode for disabled state
+- pinctrl-simple,pinconf-mask : mask of allowed pinconf bits
+
+This driver uses the common pinctrl bindings as specified in
+pinctrl-bindings.txt document in this directory. The common bindings are used
+to specify the client device states using pinctrl-0 and pinctrl-names entries.
+
+This driver supports parsing one or more pinctrl functions as the subnodes of
+the pinctrl driver entry. One or more registers can be specified for each
+function, see uart2 and uart3 examples below. If you are concerned about boot
+time, parsing multiple registers in a single function is slightly faster.
+
+For setting all static board specific pins, see the pinmux_board_pins example
+below. If you are concerned about the boot time, set up the static pins in
+the bootloader, and only set up selected pins as device tree entries.
+
+This driver assumes currently that there is one register for each pin. If you
+have some pins with more complicated configuration, you can set up a separate
+hardware specific driver for those pins.
+
+Example:
+
+/* SoC common file, such as omap4.dtsi */
+omap4_pmx_core: pinmux@4a100040 {
+	compatible = "ti,omap4-padconf";
+	reg = <0x4a100040 0x0196>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	#pinctrl-cells = <2>;
+	pinctrl-simple,register-width = <16>;
+	pinctrl-simple,function-mask = <0x7>;
+	pinctrl-simple,function-off = <0xffffffff>;
+	pinctrl-simple,pinconf-mask = <0xfff8>;
+};
+
+omap4_pmx_wkup: pinmux@4a31e040 {
+	compatible = "ti,omap4-padconf";
+	reg = <0x4a31e040 0x0038>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	#pinctrl-cells = <2>;
+	pinctrl-simple,register-width = <16>;
+	pinctrl-simple,function-mask = <0x7>;
+	pinctrl-simple,function-off = <0xffffffff>;
+	pinctrl-simple,pinconf-mask = <0xfff8>;
+};
+
+
+/* board specific .dts file, such as omap4-sdp.dts */
+&omap4_pmx_core {
+
+	/*
+	 * map all board specific static pins enabled by the pinctrl driver
+	 * itself during the boot (or just set them up in the bootloader)
+	 */
+	pinctrl-names = "default";
+	pinctrl-0 = <&board_pins>;
+
+	board_pins: pinmux_board_pins {
+		board_static_pins {
+			pinctrl-simple,cells = <
+				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
+			>;
+		};
+	};
+
+
+	/* map all uart2 pins as a single function */
+	uart2_pins: pinmux_uart2_pins {
+		uart2_pins {
+			pinctrl-simple,cells = <
+				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+			>;
+		};
+	};
+
+	/* map all uart3 pins as separate functions */
+	uart3_pins: pinmux_uart3_pins {
+		uart3_cts_rctx.uart3_cts_rctx {
+			pinctrl-simple,cells = <0x100 0x118>;	/* OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
+                };
+
+		uart3_rts_sd.uart3_rts_sd {
+			pinctrl-simple,cells = <0x102 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+		};
+
+		uart3_rx_irrx.uart3_rx_irrx {
+			pinctrl-simple,cells = <0x104 0x100>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE0 */
+		};
+
+		uart3_tx_irtx.uart3_tx_irtx {
+			pinctrl-simple,cells = <0x106 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
+		};
+	};
+};
+
+&uart2 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&uart2_pins>;
+};
+
+&uart3 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&uart3_pins>;
+};
+
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index f73a5ea..df01cc5 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -83,6 +83,14 @@ config PINCTRL_COH901
 	  COH 901 335 and COH 901 571/3. They contain 3, 5 or 7
 	  ports of 8 GPIO pins each.
 
+config PINCTRL_SIMPLE
+	tristate "Simple device tree based pinctrl driver"
+	depends on OF
+	select PINMUX
+	select PINCONF
+	help
+	  This selects the device tree based generic pinctrl driver.
+
 endmenu
 
 endif
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 8e3c95a..1636faa 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_PINCTRL_TEGRA20)	+= pinctrl-tegra20.o
 obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
 obj-$(CONFIG_PINCTRL_COH901)	+= pinctrl-coh901.o
+obj-$(CONFIG_PINCTRL_SIMPLE)	+= pinctrl-simple.o
diff --git a/drivers/pinctrl/pinctrl-simple.c b/drivers/pinctrl/pinctrl-simple.c
new file mode 100644
index 0000000..9d24109
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-simple.c
@@ -0,0 +1,1027 @@
+/*
+ * Generic simple device tree based pinctrl driver
+ *
+ * Copyright (C) 2012 Texas Instruments, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/list.h>
+
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+
+#define DRIVER_NAME			"pinctrl-simple"
+#define PCS_MUX_NAME			"pinctrl-simple,cells"
+#define PCS_MUX_CELLS			"#pinctrl-cells"
+#define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
+
+/**
+ * struct pcs_pingroup - pingroups for a function
+ * @np:		pingroup device node pointer
+ * @name:	pingroup name
+ * @gpins:	array of the pins in the group
+ * @ngpins:	number of pins in the group
+ */
+struct pcs_pingroup {
+	struct device_node *np;
+	const char *name;
+	int *gpins;
+	int ngpins;
+	struct list_head node;
+};
+
+/**
+ * struct pcs_func_vals - mux function register offset and value pair
+ * @reg:	register virtual address
+ * @defval:	default value
+ */
+struct pcs_func_vals {
+	void __iomem *reg;
+	unsigned defval;
+};
+
+/**
+ * struct pcs_function - pinctrl function
+ * @name:	pinctrl function name
+ * @vals:	register and vals array
+ * @nvals:	number of entries in vals array
+ * @pgnames:	array of pingroup names the function uses
+ * @npgnames:	number of pingroup names the function uses
+ * @node:	list node
+ */
+struct pcs_function {
+	const char *name;
+	struct pcs_func_vals *vals;
+	unsigned nvals;
+	const char **pgnames;
+	int npgnames;
+	struct list_head node;
+};
+
+/**
+ * struct pcs_data - wrapper for data needed by pinctrl framework
+ * @pa:		pindesc array
+ * @cur:	index to current element
+ */
+struct pcs_data {
+	struct pinctrl_pin_desc *pa;
+	int cur;
+};
+
+/**
+ * struct pcs_name - register name for a pin
+ * @name:	name of the pinctrl register
+ */
+struct pcs_name {
+	char name[PCS_REG_NAME_LEN];
+};
+
+/**
+ * struct pcs_device - mux device instance
+ * @res:	resources
+ * @base:	virtual address of the controller
+ * @size:	size of the ioremapped area
+ * @dev:	device entry
+ * @pctl:	pin controller device
+ * @mutex:	mutex protecting the lists
+ * @width:	bits per mux register
+ * @fmask:	function register mask
+ * @fshift:	function register shift
+ * @foff:	value to turn mux off
+ * @cmask:	pinconf mask
+ * @fmax:	max number of functions in fmask
+ * @cells:	width of the mux array
+ * @names:	array of register names for pins
+ * @pins:	physical pins on the SoC
+ * @pgtree:	pingroup index radix tree
+ * @ftree:	function index radix tree
+ * @pingroups:	list of pingroups
+ * @functions:	list of functions
+ * @ngroups:	number of pingroups
+ * @nfuncs:	number of functions
+ * @desc:	pin controller descriptor
+ * @read:	register read function to use
+ * @write:	register write function to use
+ */
+struct pcs_device {
+	struct resource *res;
+	void __iomem *base;
+	unsigned size;
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct mutex mutex;
+
+	unsigned width;
+	unsigned fmask;
+	unsigned fshift;
+	unsigned foff;
+	unsigned cmask;
+	unsigned fmax;
+	unsigned cells;
+
+	struct pcs_name *names;
+	struct pcs_data pins;
+	struct radix_tree_root pgtree;
+	struct radix_tree_root ftree;
+
+	struct list_head pingroups;
+	struct list_head functions;
+
+	unsigned ngroups;
+	unsigned nfuncs;
+
+	struct pinctrl_desc *desc;
+
+	unsigned (*read)(void __iomem *reg);
+	void (*write)(unsigned val, void __iomem *reg);
+};
+
+static unsigned __maybe_unused pcs_readb(void __iomem *reg)
+{
+	return readb(reg);
+}
+
+static unsigned __maybe_unused pcs_readw(void __iomem *reg)
+{
+	return readw(reg);
+}
+
+static unsigned __maybe_unused pcs_readl(void __iomem *reg)
+{
+	return readl(reg);
+}
+
+static void __maybe_unused pcs_writeb(unsigned val, void __iomem *reg)
+{
+	writeb(val, reg);
+}
+
+static void __maybe_unused pcs_writew(unsigned val, void __iomem *reg)
+{
+	writew(val, reg);
+}
+
+static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg)
+{
+	writel(val, reg);
+}
+
+static int pcs_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct pcs_device *pcs;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	return pcs->ngroups;
+}
+
+static const char *pcs_get_group_name(struct pinctrl_dev *pctldev,
+					unsigned gselector)
+{
+	struct pcs_device *pcs;
+	struct pcs_pingroup *group;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	group = radix_tree_lookup(&pcs->pgtree, gselector);
+	if (!group) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, gselector);
+		return NULL;
+	}
+
+	return group->name;
+}
+
+static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
+					unsigned gselector,
+					const unsigned **pins,
+					unsigned *npins)
+{
+	struct pcs_device *pcs;
+	struct pcs_pingroup *group;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	group = radix_tree_lookup(&pcs->pgtree, gselector);
+	if (!group) {
+		dev_err(pcs->dev, "%s could not find pingroup%i\n",
+			__func__, gselector);
+		return -EINVAL;
+	}
+
+	*pins = group->gpins;
+	*npins = group->ngpins;
+
+	return 0;
+}
+
+static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
+					struct seq_file *s,
+					unsigned offset)
+{
+	seq_printf(s, " " DRIVER_NAME);
+}
+
+static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
+				struct pinctrl_map *map, unsigned num_maps)
+{
+	struct pcs_device *pcs;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	devm_kfree(pcs->dev, map);
+}
+
+static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
+				struct device_node *np_config,
+				struct pinctrl_map **map, unsigned *num_maps);
+
+static struct pinctrl_ops pcs_pinctrl_ops = {
+	.get_groups_count = pcs_get_groups_count,
+	.get_group_name = pcs_get_group_name,
+	.get_group_pins = pcs_get_group_pins,
+	.pin_dbg_show = pcs_pin_dbg_show,
+	.dt_node_to_map = pcs_dt_node_to_map,
+	.dt_free_map = pcs_dt_free_map,
+};
+
+static int pcs_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	struct pcs_device *pcs;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	return pcs->nfuncs;
+}
+
+static const char *pcs_get_function_name(struct pinctrl_dev *pctldev,
+						unsigned fselector)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func) {
+		dev_err(pcs->dev, "%s could not find function%i\n",
+			__func__, fselector);
+		return NULL;
+	}
+
+	return func->name;
+}
+
+static int pcs_get_function_groups(struct pinctrl_dev *pctldev,
+					unsigned fselector,
+					const char * const **groups,
+					unsigned * const ngroups)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func) {
+		dev_err(pcs->dev, "%s could not find function%i\n",
+			__func__, fselector);
+		return -EINVAL;
+	}
+	*groups = func->pgnames;
+	*ngroups = func->npgnames;
+
+	return 0;
+}
+
+static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
+	unsigned group)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+	int i;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func)
+		return -EINVAL;
+
+	dev_dbg(pcs->dev, "enabling function%i %s\n",
+		fselector, func->name);
+
+	for (i = 0; i < func->nvals; i++) {
+		struct pcs_func_vals *vals;
+		unsigned val;
+
+		vals = &func->vals[i];
+		val = pcs->read(vals->reg);
+		val &= ~(pcs->cmask | pcs->fmask);
+		val |= vals->defval;
+		pcs->write(val, vals->reg);
+	}
+
+	return 0;
+}
+
+static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
+					unsigned group)
+{
+	struct pcs_device *pcs;
+	struct pcs_function *func;
+	int i;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	func = radix_tree_lookup(&pcs->ftree, fselector);
+	if (!func) {
+		dev_err(pcs->dev, "%s could not find function%i\n",
+			__func__, fselector);
+		return;
+	}
+
+	/*
+	 * Do not touch modes if off mode is larger than supported
+	 * modes. Some hardware does not have clearly defined off modes.
+	 */
+	if ((pcs->foff << pcs->fshift) > pcs->fshift) {
+		dev_dbg(pcs->dev, "not updating mode for disable\n");
+		return;
+	}
+
+	dev_dbg(pcs->dev, "disabling function%i %s\n",
+		fselector, func->name);
+
+	for (i = 0; i < func->nvals; i++) {
+		struct pcs_func_vals *vals;
+		unsigned val;
+
+		vals = &func->vals[i];
+		val = pcs->read(vals->reg);
+		val &= ~(pcs->cmask | pcs->fmask);
+		val |= pcs->foff << pcs->fshift;
+		pcs->write(val, vals->reg);
+	}
+}
+
+static int pcs_request_gpio(struct pinctrl_dev *pctldev,
+			struct pinctrl_gpio_range *range, unsigned offset)
+{
+	return -ENOTSUPP;
+}
+
+static struct pinmux_ops pcs_pinmux_ops = {
+	.get_functions_count = pcs_get_functions_count,
+	.get_function_name = pcs_get_function_name,
+	.get_function_groups = pcs_get_function_groups,
+	.enable = pcs_enable,
+	.disable = pcs_disable,
+	.gpio_request_enable = pcs_request_gpio,
+};
+
+static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
+				unsigned pin, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
+static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
+				unsigned pin, unsigned long config)
+{
+	return -ENOTSUPP;
+}
+
+static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
+				unsigned group, unsigned long *config)
+{
+	return -ENOTSUPP;
+}
+
+static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
+				unsigned group, unsigned long config)
+{
+	return -ENOTSUPP;
+}
+
+static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
+				struct seq_file *s, unsigned offset)
+{
+}
+
+static void pcs_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
+				struct seq_file *s, unsigned selector)
+{
+}
+
+static struct pinconf_ops pcs_pinconf_ops = {
+	.pin_config_get = pcs_pinconf_get,
+	.pin_config_set = pcs_pinconf_set,
+	.pin_config_group_get = pcs_pinconf_group_get,
+	.pin_config_group_set = pcs_pinconf_group_set,
+	.pin_config_dbg_show = pcs_pinconf_dbg_show,
+	.pin_config_group_dbg_show = pcs_pinconf_group_dbg_show,
+};
+
+/**
+ * pcs_add_pin() - add a pin to the static per controller pin array
+ * @pcs: pcs driver instance
+ * @offset: register offset from base
+ */
+static int __devinit pcs_add_pin(struct pcs_device *pcs, unsigned offset)
+{
+	struct pinctrl_pin_desc *pin;
+	struct pcs_name *pn;
+	char *name;
+	int i;
+
+	i = pcs->pins.cur;
+	if (i >= pcs->desc->npins) {
+		dev_err(pcs->dev, "too many pins, max %i\n",
+			pcs->desc->npins);
+		return -ENOMEM;
+	}
+
+	pin = &pcs->pins.pa[i];
+	pn = &pcs->names[i];
+	name = pn->name;
+	sprintf(name, "%lx",
+		(unsigned long)pcs->res->start + offset);
+	pin->name = name;
+	pin->number = i;
+	pcs->pins.cur++;
+
+	return i;
+}
+
+/**
+ * pcs_allocate_pin_table() - adds all the pins for the pinctrl driver
+ * @pcs: pcs driver instance
+ *
+ * In case of errors, resources are freed in pcs_free_resources.
+ *
+ * If your hardware needs holes in the address space, then just set
+ * up multiple driver instances.
+ *
+ * REVISIT: Handle the address space better for multiple registers
+ * per pin type hardware.
+ */
+static int __devinit pcs_allocate_pin_table(struct pcs_device *pcs)
+{
+	int mux_bytes, nr_pins, i;
+
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	nr_pins = pcs->size / mux_bytes;
+
+	dev_dbg(pcs->dev, "allocating %i pins\n", nr_pins);
+	pcs->pins.pa = devm_kzalloc(pcs->dev,
+				sizeof(*pcs->pins.pa) * nr_pins,
+				GFP_KERNEL);
+	if (!pcs->pins.pa)
+		return -ENOMEM;
+
+	pcs->names = devm_kzalloc(pcs->dev,
+				sizeof(struct pcs_name) * nr_pins,
+				GFP_KERNEL);
+	if (!pcs->names)
+		return -ENOMEM;
+
+	pcs->desc->pins = pcs->pins.pa;
+	pcs->desc->npins = nr_pins;
+
+	for (i = 0; i < pcs->desc->npins; i++) {
+		unsigned offset;
+		int res;
+
+		offset = i * mux_bytes;
+		res = pcs_add_pin(pcs, offset);
+		if (res < 0) {
+			dev_err(pcs->dev, "error adding pins: %i\n", res);
+			return res;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * pcs_add_function() - adds a new function to the function list
+ * @pcs: pcs driver instance
+ * @np: device node of the mux entry
+ * @name: name of the function
+ * @vals: array of mux register value pairs used by the function
+ * @nvals: number of mux register value pairs
+ * @pgnames: array of pingroup names for the function
+ * @npgnames: number of pingroup names
+ */
+static struct pcs_function *pcs_add_function(struct pcs_device *pcs,
+					struct device_node *np,
+					const char *name,
+					struct pcs_func_vals *vals,
+					unsigned nvals,
+					const char **pgnames,
+					unsigned npgnames)
+{
+	struct pcs_function *function;
+
+	function = devm_kzalloc(pcs->dev, sizeof(*function), GFP_KERNEL);
+	if (!function)
+		return NULL;
+
+	function->name = name;
+	function->vals = vals;
+	function->nvals = nvals;
+	function->pgnames = pgnames;
+	function->npgnames = npgnames;
+
+	mutex_lock(&pcs->mutex);
+	list_add_tail(&function->node, &pcs->functions);
+	radix_tree_insert(&pcs->ftree, pcs->nfuncs, function);
+	pcs->nfuncs++;
+	mutex_unlock(&pcs->mutex);
+
+	return function;
+}
+
+static void pcs_remove_function(struct pcs_device *pcs,
+				struct pcs_function *function)
+{
+	int i;
+
+	mutex_lock(&pcs->mutex);
+	for (i = 0; i < pcs->nfuncs; i++) {
+		struct pcs_function *found;
+
+		found = radix_tree_lookup(&pcs->ftree, i);
+		if (found == function)
+			radix_tree_delete(&pcs->ftree, i);
+	}
+	list_del(&function->node);
+	mutex_unlock(&pcs->mutex);
+}
+
+/**
+ * pcs_add_pingroup() - add a pingroup to the pingroup list
+ * @pcs: pcs driver instance
+ * @np: device node of the mux entry
+ * @name: name of the pingroup
+ * @gpins: array of the pins that belong to the group
+ * @ngpins: number of pins in the group
+ */
+static int pcs_add_pingroup(struct pcs_device *pcs,
+					struct device_node *np,
+					const char *name,
+					int *gpins,
+					int ngpins)
+{
+	struct pcs_pingroup *pingroup;
+
+	pingroup = devm_kzalloc(pcs->dev, sizeof(*pingroup), GFP_KERNEL);
+	if (!pingroup)
+		return -ENOMEM;
+
+	pingroup->name = name;
+	pingroup->np = np;
+	pingroup->gpins = gpins;
+	pingroup->ngpins = ngpins;
+
+	mutex_lock(&pcs->mutex);
+	list_add_tail(&pingroup->node, &pcs->pingroups);
+	radix_tree_insert(&pcs->pgtree, pcs->ngroups, pingroup);
+	pcs->ngroups++;
+	mutex_unlock(&pcs->mutex);
+
+	return 0;
+}
+
+/**
+ * pcs_get_pin_by_offset() - get a pin index based on the register offset
+ * @pcs: pcs driver instance
+ * @offset: register offset from the base
+ *
+ * Note that this is OK as long as the pins are in a static array.
+ */
+static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
+{
+	unsigned index;
+
+	if (offset >= pcs->size) {
+		dev_err(pcs->dev, "mux offset out of range: 0x%x (0x%x)\n",
+			offset, pcs->size);
+		return -EINVAL;
+	}
+
+	index = offset / (pcs->width / BITS_PER_BYTE);
+
+	return index;
+}
+
+/**
+ * smux_parse_one_pinctrl_entry() - parses a device tree mux entry
+ * @pcs: pinctrl driver instance
+ * @np: device node of the mux entry
+ * @map: map entry
+ * @pgnames: pingroup names
+ *
+ * Note that this currently supports only #pinctrl-cells = 2.
+ * This could be improved to parse controllers that have additional
+ * auxilary registers per mux.
+ */
+static int __init pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
+						struct device_node *np,
+						struct pinctrl_map **map,
+						const char **pgnames)
+{
+	struct pcs_func_vals *vals;
+	const __be32 *mux;
+	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
+	struct pcs_function *function;
+
+	if (pcs->cells != 2) {
+		dev_err(pcs->dev, "unhandled %s: %i\n", PCS_MUX_CELLS,
+			pcs->cells);
+		return -EINVAL;
+	}
+
+	mux = of_get_property(np, PCS_MUX_NAME, &size);
+	if ((!mux) || (size < sizeof(*mux) * pcs->cells)) {
+		dev_err(pcs->dev, "bad data for mux %s\n",
+			np->name);
+		return -EINVAL;
+	}
+
+	size /= sizeof(*mux);	/* Number of elements in array */
+	rows = size / pcs->cells;
+
+	vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows, GFP_KERNEL);
+	if (!vals)
+		return -ENOMEM;
+
+	pins = devm_kzalloc(pcs->dev, sizeof(*pins) * rows, GFP_KERNEL);
+	if (!pins)
+		goto free_vals;
+
+	while (index < size) {
+		unsigned offset, defval;
+		int pin;
+
+		offset = be32_to_cpup(mux + index++);
+		defval = be32_to_cpup(mux + index++);
+		vals[found].reg = pcs->base + offset;
+		vals[found].defval = defval;
+
+		pin = pcs_get_pin_by_offset(pcs, offset);
+		if (pin < 0) {
+			dev_err(pcs->dev,
+				"could not add functions for %s %ux\n",
+				np->name, offset);
+			break;
+		}
+		pins[found++] = pin;
+	}
+
+	pgnames[0] = np->name;
+	function = pcs_add_function(pcs, np, np->name, vals, found, pgnames, 1);
+	if (!function)
+		goto free_pins;
+
+	res = pcs_add_pingroup(pcs, np, np->name, pins, found);
+	if (res < 0)
+		goto free_function;
+
+	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)->data.mux.group = np->name;
+	(*map)->data.mux.function = np->name;
+
+	return 0;
+
+free_function:
+	pcs_remove_function(pcs, function);
+
+free_pins:
+	devm_kfree(pcs->dev, pins);
+
+free_vals:
+	devm_kfree(pcs->dev, vals);
+
+	return res;
+}
+/**
+ * pcs_dt_node_to_map() - allocates and parses pinctrl maps
+ * @pctldev: pinctrl instance
+ * @np_config: device tree pinmux entry
+ * @map: array of map entries
+ * @num_maps: number of maps
+ */
+static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
+				struct device_node *np_config,
+				struct pinctrl_map **map, unsigned *num_maps)
+{
+	struct pcs_device *pcs;
+	struct device_node *np;
+	struct pinctrl_map *cur_map;
+	const char **pgnames, **cur_name;
+	int ret, found_maps = 0, added_maps = 0;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+
+	for_each_child_of_node(np_config, np)
+		found_maps++;
+
+	*map = devm_kzalloc(pcs->dev, sizeof(**map) * found_maps,
+				GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	cur_map = *map;
+	*num_maps = 0;
+
+	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames) * found_maps,
+				GFP_KERNEL);
+	if (!pgnames) {
+		devm_kfree(pcs->dev, *map);
+		return -ENOMEM;
+	}
+
+	cur_name = pgnames;
+
+	for_each_child_of_node(np_config, np) {
+		ret = pcs_parse_one_pinctrl_entry(pcs, np, &cur_map, cur_name);
+		if (ret < 0) {
+			dev_err(pcs->dev, "added only %i/%i entries for %s\n",
+				added_maps, found_maps, np_config->name);
+			break;
+		}
+		cur_map++;
+		cur_name++;
+		added_maps++;
+	}
+
+	*num_maps = added_maps;
+
+	return 0;
+}
+
+/**
+ * pcs_free_funcs() - free memory used by functions
+ * @pcs: pcs driver instance
+ */
+static void pcs_free_funcs(struct pcs_device *pcs)
+{
+	struct list_head *pos, *tmp;
+	int i;
+
+	mutex_lock(&pcs->mutex);
+	for (i = 0; i < pcs->nfuncs; i++) {
+		struct pcs_function *func;
+
+		func = radix_tree_lookup(&pcs->ftree, i);
+		if (!func)
+			continue;
+		radix_tree_delete(&pcs->ftree, i);
+	}
+	list_for_each_safe(pos, tmp, &pcs->functions) {
+		struct pcs_function *function;
+
+		function = list_entry(pos, struct pcs_function, node);
+		list_del(&function->node);
+	}
+	mutex_unlock(&pcs->mutex);
+}
+
+/**
+ * pcs_free_pingroups() - free memory used by pingroups
+ * @pcs: pcs driver instance
+ */
+static void pcs_free_pingroups(struct pcs_device *pcs)
+{
+	struct list_head *pos, *tmp;
+	int i;
+
+	mutex_lock(&pcs->mutex);
+	for (i = 0; i < pcs->ngroups; i++) {
+		struct pcs_pingroup *pingroup;
+
+		pingroup = radix_tree_lookup(&pcs->pgtree, i);
+		if (!pingroup)
+			continue;
+		radix_tree_delete(&pcs->pgtree, i);
+	}
+	list_for_each_safe(pos, tmp, &pcs->pingroups) {
+		struct pcs_pingroup *pingroup;
+
+		pingroup = list_entry(pos, struct pcs_pingroup, node);
+		list_del(&pingroup->node);
+	}
+	mutex_unlock(&pcs->mutex);
+}
+
+/**
+ * pcs_free_resources() - free memory used by this driver
+ * @pcs: pcs driver instance
+ */
+static void pcs_free_resources(struct pcs_device *pcs)
+{
+	if (pcs->pctl)
+		pinctrl_unregister(pcs->pctl);
+
+	pcs_free_funcs(pcs);
+	pcs_free_pingroups(pcs);
+}
+
+/**
+ * pcs_register() - initializes and registers with pinctrl framework
+ * @pcs: pcs driver instance
+ */
+static int __devinit pcs_register(struct pcs_device *pcs)
+{
+	int ret;
+
+	if (!pcs->dev->of_node)
+		return -ENODEV;
+
+	pcs->desc = devm_kzalloc(pcs->dev, sizeof(*pcs->desc), GFP_KERNEL);
+	if (!pcs->desc)
+		return -ENOMEM;
+	pcs->desc->name = DRIVER_NAME;
+	pcs->desc->pctlops = &pcs_pinctrl_ops;
+	pcs->desc->pmxops = &pcs_pinmux_ops;
+	pcs->desc->confops = &pcs_pinconf_ops;
+	pcs->desc->owner = THIS_MODULE;
+
+	ret = pcs_allocate_pin_table(pcs);
+	if (ret < 0)
+		goto free;
+
+	pcs->pctl = pinctrl_register(pcs->desc, pcs->dev, pcs);
+	if (!pcs->pctl) {
+		dev_err(pcs->dev, "could not register simple pinctrl driver\n");
+		ret = -EINVAL;
+		goto free;
+	}
+
+	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
+		 pcs->desc->npins, pcs->base, pcs->size);
+
+	return 0;
+
+free:
+	pcs_free_resources(pcs);
+
+	return ret;
+}
+
+#define PCS_GET_PROP_U32(name, reg, err)				\
+	do {								\
+		ret = of_property_read_u32(np, name, reg);		\
+		if (ret) {						\
+			dev_err(pcs->dev, err);				\
+			goto out;					\
+		}							\
+	} while (0);
+
+static struct of_device_id pcs_of_match[];
+
+/*
+ * REVISIT: Handle hardware with multiple registers per pin
+ * better by adding other masks.
+ */
+static int __devinit pcs_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct resource res;
+	struct pcs_device *pcs;
+	int ret;
+
+	match = of_match_device(pcs_of_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+
+	pcs = devm_kzalloc(&pdev->dev, sizeof(*pcs), GFP_KERNEL);
+	if (!pcs) {
+		dev_err(&pdev->dev, "could not allocate\n");
+		return -ENOMEM;
+	}
+	pcs->dev = &pdev->dev;
+	mutex_init(&pcs->mutex);
+	INIT_LIST_HEAD(&pcs->pingroups);
+	INIT_LIST_HEAD(&pcs->functions);
+
+	PCS_GET_PROP_U32("pinctrl-simple,register-width", &pcs->width,
+			 "register width not specified\n");
+
+	PCS_GET_PROP_U32("pinctrl-simple,function-mask", &pcs->fmask,
+			 "function register mask not specified\n");
+	pcs->fshift = ffs(pcs->fmask) - 1;
+	pcs->fmax = pcs->fmask >> pcs->fshift;
+
+	PCS_GET_PROP_U32("pinctrl-simple,function-off", &pcs->foff,
+			 "function off mode not specified\n");
+
+	PCS_GET_PROP_U32("pinctrl-simple,pinconf-mask", &pcs->cmask,
+			 "pinconf mask not specified\n");
+
+	PCS_GET_PROP_U32("#pinctrl-cells", &pcs->cells,
+			 "#pinctrl-cells not specified\n");
+
+	ret = of_address_to_resource(pdev->dev.of_node, 0, &res);
+	if (ret) {
+		dev_err(pcs->dev, "could not get resource\n");
+		goto out;
+	}
+
+	pcs->res = devm_request_mem_region(pcs->dev, res.start,
+			resource_size(&res), DRIVER_NAME);
+	if (!pcs->res) {
+		dev_err(pcs->dev, "could not get mem_region\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	pcs->size = resource_size(pcs->res);
+	pcs->base = devm_ioremap(pcs->dev, pcs->res->start, pcs->size);
+	if (!pcs->base) {
+		dev_err(pcs->dev, "could not ioremap\n");
+		ret = -ENODEV;
+		goto out;
+	}
+
+	INIT_RADIX_TREE(&pcs->pgtree, GFP_KERNEL);
+	INIT_RADIX_TREE(&pcs->ftree, GFP_KERNEL);
+	platform_set_drvdata(pdev, pcs);
+
+	switch (pcs->width) {
+	case 8:
+		pcs->read = pcs_readb;
+		pcs->write = pcs_writeb;
+		break;
+	case 16:
+		pcs->read = pcs_readw;
+		pcs->write = pcs_writew;
+		break;
+	case 32:
+		pcs->read = pcs_readl;
+		pcs->write = pcs_writel;
+		break;
+	default:
+		break;
+	}
+
+	ret = pcs_register(pcs);
+	if (ret < 0) {
+		dev_err(pcs->dev, "could not add mux registers: %i\n", ret);
+		goto out;
+	}
+
+	return 0;
+
+out:
+	return ret;
+}
+
+static int __devexit pcs_remove(struct platform_device *pdev)
+{
+	struct pcs_device *pcs = platform_get_drvdata(pdev);
+
+	if (!pcs)
+		return 0;
+
+	pcs_free_resources(pcs);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct of_device_id pcs_of_match[] __devinitdata = {
+	{ .compatible = DRIVER_NAME, },
+	{ .compatible = "ti,omap2420-padconf", },
+	{ .compatible = "ti,omap2430-padconf", },
+	{ .compatible = "ti,omap3-padconf", },
+	{ .compatible = "ti,omap4-padconf", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pcs_of_match);
+
+static struct platform_driver pcs_driver = {
+	.probe		= pcs_probe,
+	.remove		= __devexit_p(pcs_remove),
+	.driver = {
+		.owner		= THIS_MODULE,
+		.name		= DRIVER_NAME,
+		.of_match_table	= pcs_of_match,
+	},
+};
+
+module_platform_driver(pcs_driver);
+
+MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
+MODULE_DESCRIPTION("Simple device tree pinctrl driver");
+MODULE_LICENSE("GPL");

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	I really like it

	I was working on something simillar

	but can we split the group management so we can use it on other
	bindings

Best Regards,
J.

On 10:24 Wed 02 May     , Tony Lindgren wrote:
> Add simple pinctrl driver using device tree data.
> 
> Currently this driver only works on omap2+ series of
> processors, where there is either an 8 or 16-bit padconf
> register for each pin. Support for other similar pinmux
> controllers could be added.
> 
> Note that this patch does not yet support pinconf_ops
> or GPIO. Further.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> Here's this one finally updated to use the common pinctrl bindings.
> That sure cleaned up a bunch of the nasty things in this driver :)
> Nice job Stephen!
> ---
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
> new file mode 100644
> index 0000000..7b32263
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
> @@ -0,0 +1,125 @@
> +Generic simple device tree based pinctrl driver
> +
> +Required properties:
> +- compatible :  one of:
> +	- "pinctrl-simple"
> +	- "ti,omap2420-padconf"
> +	- "ti,omap2430-padconf"
> +	- "ti,omap3-padconf"
> +	- "ti,omap4-padconf"
> +- reg : offset and length of the register set for the mux registers
> +- #pinctrl-cells : width of the pinctrl array, currently only 2 is supported
> +- pinctrl-simple,register-width : pinmux register access width
> +- pinctrl-simple,function-mask : mask of allowed pinmux function bits
> +- pinctrl-simple,function-off : function off mode for disabled state
> +- pinctrl-simple,pinconf-mask : mask of allowed pinconf bits
> +
> +This driver uses the common pinctrl bindings as specified in
> +pinctrl-bindings.txt document in this directory. The common bindings are used
> +to specify the client device states using pinctrl-0 and pinctrl-names entries.
> +
> +This driver supports parsing one or more pinctrl functions as the subnodes of
> +the pinctrl driver entry. One or more registers can be specified for each
> +function, see uart2 and uart3 examples below. If you are concerned about boot
> +time, parsing multiple registers in a single function is slightly faster.
> +
> +For setting all static board specific pins, see the pinmux_board_pins example
> +below. If you are concerned about the boot time, set up the static pins in
> +the bootloader, and only set up selected pins as device tree entries.
> +
> +This driver assumes currently that there is one register for each pin. If you
> +have some pins with more complicated configuration, you can set up a separate
> +hardware specific driver for those pins.
> +
> +Example:
> +
> +/* SoC common file, such as omap4.dtsi */
> +omap4_pmx_core: pinmux@4a100040 {
> +	compatible = "ti,omap4-padconf";
> +	reg = <0x4a100040 0x0196>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	#pinctrl-cells = <2>;
> +	pinctrl-simple,register-width = <16>;
> +	pinctrl-simple,function-mask = <0x7>;
> +	pinctrl-simple,function-off = <0xffffffff>;
> +	pinctrl-simple,pinconf-mask = <0xfff8>;
> +};
> +
> +omap4_pmx_wkup: pinmux@4a31e040 {
> +	compatible = "ti,omap4-padconf";
> +	reg = <0x4a31e040 0x0038>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	#pinctrl-cells = <2>;
> +	pinctrl-simple,register-width = <16>;
> +	pinctrl-simple,function-mask = <0x7>;
> +	pinctrl-simple,function-off = <0xffffffff>;
> +	pinctrl-simple,pinconf-mask = <0xfff8>;
> +};
> +
> +
> +/* board specific .dts file, such as omap4-sdp.dts */
> +&omap4_pmx_core {
> +
> +	/*
> +	 * map all board specific static pins enabled by the pinctrl driver
> +	 * itself during the boot (or just set them up in the bootloader)
> +	 */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&board_pins>;
> +
> +	board_pins: pinmux_board_pins {
> +		board_static_pins {
> +			pinctrl-simple,cells = <
> +				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +			>;
> +		};
> +	};
> +
> +
> +	/* map all uart2 pins as a single function */
> +	uart2_pins: pinmux_uart2_pins {
> +		uart2_pins {
> +			pinctrl-simple,cells = <
> +				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> +				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> +				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +			>;
> +		};
> +	};
> +
> +	/* map all uart3 pins as separate functions */
> +	uart3_pins: pinmux_uart3_pins {
> +		uart3_cts_rctx.uart3_cts_rctx {
> +			pinctrl-simple,cells = <0x100 0x118>;	/* OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> +                };
> +
> +		uart3_rts_sd.uart3_rts_sd {
> +			pinctrl-simple,cells = <0x102 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +		};
> +
> +		uart3_rx_irrx.uart3_rx_irrx {
> +			pinctrl-simple,cells = <0x104 0x100>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE0 */
> +		};
> +
> +		uart3_tx_irtx.uart3_tx_irtx {
> +			pinctrl-simple,cells = <0x106 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +		};
> +	};
> +};
> +
> +&uart2 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&uart2_pins>;
> +};
> +
> +&uart3 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&uart3_pins>;
> +};
> +
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index f73a5ea..df01cc5 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -83,6 +83,14 @@ config PINCTRL_COH901
>  	  COH 901 335 and COH 901 571/3. They contain 3, 5 or 7
>  	  ports of 8 GPIO pins each.
>  
> +config PINCTRL_SIMPLE
> +	tristate "Simple device tree based pinctrl driver"
> +	depends on OF
> +	select PINMUX
> +	select PINCONF
> +	help
> +	  This selects the device tree based generic pinctrl driver.
> +
>  endmenu
>  
>  endif
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 8e3c95a..1636faa 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_PINCTRL_TEGRA20)	+= pinctrl-tegra20.o
>  obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
>  obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o
>  obj-$(CONFIG_PINCTRL_COH901)	+= pinctrl-coh901.o
> +obj-$(CONFIG_PINCTRL_SIMPLE)	+= pinctrl-simple.o
> diff --git a/drivers/pinctrl/pinctrl-simple.c b/drivers/pinctrl/pinctrl-simple.c
> new file mode 100644
> index 0000000..9d24109
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-simple.c
> @@ -0,0 +1,1027 @@
> +/*
> + * Generic simple device tree based pinctrl driver
> + *
> + * Copyright (C) 2012 Texas Instruments, Inc.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/list.h>
> +
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +
> +#include "core.h"
> +
> +#define DRIVER_NAME			"pinctrl-simple"
> +#define PCS_MUX_NAME			"pinctrl-simple,cells"
> +#define PCS_MUX_CELLS			"#pinctrl-cells"
> +#define PCS_REG_NAME_LEN		((sizeof(unsigned long) * 2) + 1)
> +
> +/**
> + * struct pcs_pingroup - pingroups for a function
> + * @np:		pingroup device node pointer
> + * @name:	pingroup name
> + * @gpins:	array of the pins in the group
> + * @ngpins:	number of pins in the group
> + */
> +struct pcs_pingroup {
> +	struct device_node *np;
> +	const char *name;
> +	int *gpins;
> +	int ngpins;
> +	struct list_head node;
> +};
> +
> +/**
> + * struct pcs_func_vals - mux function register offset and value pair
> + * @reg:	register virtual address
> + * @defval:	default value
> + */
> +struct pcs_func_vals {
> +	void __iomem *reg;
> +	unsigned defval;
> +};
> +
> +/**
> + * struct pcs_function - pinctrl function
> + * @name:	pinctrl function name
> + * @vals:	register and vals array
> + * @nvals:	number of entries in vals array
> + * @pgnames:	array of pingroup names the function uses
> + * @npgnames:	number of pingroup names the function uses
> + * @node:	list node
> + */
> +struct pcs_function {
> +	const char *name;
> +	struct pcs_func_vals *vals;
> +	unsigned nvals;
> +	const char **pgnames;
> +	int npgnames;
> +	struct list_head node;
> +};
> +
> +/**
> + * struct pcs_data - wrapper for data needed by pinctrl framework
> + * @pa:		pindesc array
> + * @cur:	index to current element
> + */
> +struct pcs_data {
> +	struct pinctrl_pin_desc *pa;
> +	int cur;
> +};
> +
> +/**
> + * struct pcs_name - register name for a pin
> + * @name:	name of the pinctrl register
> + */
> +struct pcs_name {
> +	char name[PCS_REG_NAME_LEN];
> +};
> +
> +/**
> + * struct pcs_device - mux device instance
> + * @res:	resources
> + * @base:	virtual address of the controller
> + * @size:	size of the ioremapped area
> + * @dev:	device entry
> + * @pctl:	pin controller device
> + * @mutex:	mutex protecting the lists
> + * @width:	bits per mux register
> + * @fmask:	function register mask
> + * @fshift:	function register shift
> + * @foff:	value to turn mux off
> + * @cmask:	pinconf mask
> + * @fmax:	max number of functions in fmask
> + * @cells:	width of the mux array
> + * @names:	array of register names for pins
> + * @pins:	physical pins on the SoC
> + * @pgtree:	pingroup index radix tree
> + * @ftree:	function index radix tree
> + * @pingroups:	list of pingroups
> + * @functions:	list of functions
> + * @ngroups:	number of pingroups
> + * @nfuncs:	number of functions
> + * @desc:	pin controller descriptor
> + * @read:	register read function to use
> + * @write:	register write function to use
> + */
> +struct pcs_device {
> +	struct resource *res;
> +	void __iomem *base;
> +	unsigned size;
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +	struct mutex mutex;
> +
> +	unsigned width;
> +	unsigned fmask;
> +	unsigned fshift;
> +	unsigned foff;
> +	unsigned cmask;
> +	unsigned fmax;
> +	unsigned cells;
> +
> +	struct pcs_name *names;
> +	struct pcs_data pins;
> +	struct radix_tree_root pgtree;
> +	struct radix_tree_root ftree;
> +
> +	struct list_head pingroups;
> +	struct list_head functions;
> +
> +	unsigned ngroups;
> +	unsigned nfuncs;
> +
> +	struct pinctrl_desc *desc;
> +
> +	unsigned (*read)(void __iomem *reg);
> +	void (*write)(unsigned val, void __iomem *reg);
> +};
> +
> +static unsigned __maybe_unused pcs_readb(void __iomem *reg)
> +{
> +	return readb(reg);
> +}
> +
> +static unsigned __maybe_unused pcs_readw(void __iomem *reg)
> +{
> +	return readw(reg);
> +}
> +
> +static unsigned __maybe_unused pcs_readl(void __iomem *reg)
> +{
> +	return readl(reg);
> +}
> +
> +static void __maybe_unused pcs_writeb(unsigned val, void __iomem *reg)
> +{
> +	writeb(val, reg);
> +}
> +
> +static void __maybe_unused pcs_writew(unsigned val, void __iomem *reg)
> +{
> +	writew(val, reg);
> +}
> +
> +static void __maybe_unused pcs_writel(unsigned val, void __iomem *reg)
> +{
> +	writel(val, reg);
> +}
> +
> +static int pcs_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct pcs_device *pcs;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pcs->ngroups;
> +}
> +
> +static const char *pcs_get_group_name(struct pinctrl_dev *pctldev,
> +					unsigned gselector)
> +{
> +	struct pcs_device *pcs;
> +	struct pcs_pingroup *group;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	group = radix_tree_lookup(&pcs->pgtree, gselector);
> +	if (!group) {
> +		dev_err(pcs->dev, "%s could not find pingroup%i\n",
> +			__func__, gselector);
> +		return NULL;
> +	}
> +
> +	return group->name;
> +}
> +
> +static int pcs_get_group_pins(struct pinctrl_dev *pctldev,
> +					unsigned gselector,
> +					const unsigned **pins,
> +					unsigned *npins)
> +{
> +	struct pcs_device *pcs;
> +	struct pcs_pingroup *group;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	group = radix_tree_lookup(&pcs->pgtree, gselector);
> +	if (!group) {
> +		dev_err(pcs->dev, "%s could not find pingroup%i\n",
> +			__func__, gselector);
> +		return -EINVAL;
> +	}
> +
> +	*pins = group->gpins;
> +	*npins = group->ngpins;
> +
> +	return 0;
> +}
> +
> +static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
> +					struct seq_file *s,
> +					unsigned offset)
> +{
> +	seq_printf(s, " " DRIVER_NAME);
> +}
> +
> +static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
> +				struct pinctrl_map *map, unsigned num_maps)
> +{
> +	struct pcs_device *pcs;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	devm_kfree(pcs->dev, map);
> +}
> +
> +static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
> +				struct device_node *np_config,
> +				struct pinctrl_map **map, unsigned *num_maps);
> +
> +static struct pinctrl_ops pcs_pinctrl_ops = {
> +	.get_groups_count = pcs_get_groups_count,
> +	.get_group_name = pcs_get_group_name,
> +	.get_group_pins = pcs_get_group_pins,
> +	.pin_dbg_show = pcs_pin_dbg_show,
> +	.dt_node_to_map = pcs_dt_node_to_map,
> +	.dt_free_map = pcs_dt_free_map,
> +};
> +
> +static int pcs_get_functions_count(struct pinctrl_dev *pctldev)
> +{
> +	struct pcs_device *pcs;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pcs->nfuncs;
> +}
> +
> +static const char *pcs_get_function_name(struct pinctrl_dev *pctldev,
> +						unsigned fselector)
> +{
> +	struct pcs_device *pcs;
> +	struct pcs_function *func;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	func = radix_tree_lookup(&pcs->ftree, fselector);
> +	if (!func) {
> +		dev_err(pcs->dev, "%s could not find function%i\n",
> +			__func__, fselector);
> +		return NULL;
> +	}
> +
> +	return func->name;
> +}
> +
> +static int pcs_get_function_groups(struct pinctrl_dev *pctldev,
> +					unsigned fselector,
> +					const char * const **groups,
> +					unsigned * const ngroups)
> +{
> +	struct pcs_device *pcs;
> +	struct pcs_function *func;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	func = radix_tree_lookup(&pcs->ftree, fselector);
> +	if (!func) {
> +		dev_err(pcs->dev, "%s could not find function%i\n",
> +			__func__, fselector);
> +		return -EINVAL;
> +	}
> +	*groups = func->pgnames;
> +	*ngroups = func->npgnames;
> +
> +	return 0;
> +}
> +
> +static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector,
> +	unsigned group)
> +{
> +	struct pcs_device *pcs;
> +	struct pcs_function *func;
> +	int i;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	func = radix_tree_lookup(&pcs->ftree, fselector);
> +	if (!func)
> +		return -EINVAL;
> +
> +	dev_dbg(pcs->dev, "enabling function%i %s\n",
> +		fselector, func->name);
> +
> +	for (i = 0; i < func->nvals; i++) {
> +		struct pcs_func_vals *vals;
> +		unsigned val;
> +
> +		vals = &func->vals[i];
> +		val = pcs->read(vals->reg);
> +		val &= ~(pcs->cmask | pcs->fmask);
> +		val |= vals->defval;
> +		pcs->write(val, vals->reg);
> +	}
> +
> +	return 0;
> +}
> +
> +static void pcs_disable(struct pinctrl_dev *pctldev, unsigned fselector,
> +					unsigned group)
> +{
> +	struct pcs_device *pcs;
> +	struct pcs_function *func;
> +	int i;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +	func = radix_tree_lookup(&pcs->ftree, fselector);
> +	if (!func) {
> +		dev_err(pcs->dev, "%s could not find function%i\n",
> +			__func__, fselector);
> +		return;
> +	}
> +
> +	/*
> +	 * Do not touch modes if off mode is larger than supported
> +	 * modes. Some hardware does not have clearly defined off modes.
> +	 */
> +	if ((pcs->foff << pcs->fshift) > pcs->fshift) {
> +		dev_dbg(pcs->dev, "not updating mode for disable\n");
> +		return;
> +	}
> +
> +	dev_dbg(pcs->dev, "disabling function%i %s\n",
> +		fselector, func->name);
> +
> +	for (i = 0; i < func->nvals; i++) {
> +		struct pcs_func_vals *vals;
> +		unsigned val;
> +
> +		vals = &func->vals[i];
> +		val = pcs->read(vals->reg);
> +		val &= ~(pcs->cmask | pcs->fmask);
> +		val |= pcs->foff << pcs->fshift;
> +		pcs->write(val, vals->reg);
> +	}
> +}
> +
> +static int pcs_request_gpio(struct pinctrl_dev *pctldev,
> +			struct pinctrl_gpio_range *range, unsigned offset)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static struct pinmux_ops pcs_pinmux_ops = {
> +	.get_functions_count = pcs_get_functions_count,
> +	.get_function_name = pcs_get_function_name,
> +	.get_function_groups = pcs_get_function_groups,
> +	.enable = pcs_enable,
> +	.disable = pcs_disable,
> +	.gpio_request_enable = pcs_request_gpio,
> +};
> +
> +static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
> +				unsigned pin, unsigned long *config)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
> +				unsigned pin, unsigned long config)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int pcs_pinconf_group_get(struct pinctrl_dev *pctldev,
> +				unsigned group, unsigned long *config)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static int pcs_pinconf_group_set(struct pinctrl_dev *pctldev,
> +				unsigned group, unsigned long config)
> +{
> +	return -ENOTSUPP;
> +}
> +
> +static void pcs_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> +				struct seq_file *s, unsigned offset)
> +{
> +}
> +
> +static void pcs_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
> +				struct seq_file *s, unsigned selector)
> +{
> +}
> +
> +static struct pinconf_ops pcs_pinconf_ops = {
> +	.pin_config_get = pcs_pinconf_get,
> +	.pin_config_set = pcs_pinconf_set,
> +	.pin_config_group_get = pcs_pinconf_group_get,
> +	.pin_config_group_set = pcs_pinconf_group_set,
> +	.pin_config_dbg_show = pcs_pinconf_dbg_show,
> +	.pin_config_group_dbg_show = pcs_pinconf_group_dbg_show,
> +};
> +
> +/**
> + * pcs_add_pin() - add a pin to the static per controller pin array
> + * @pcs: pcs driver instance
> + * @offset: register offset from base
> + */
> +static int __devinit pcs_add_pin(struct pcs_device *pcs, unsigned offset)
> +{
> +	struct pinctrl_pin_desc *pin;
> +	struct pcs_name *pn;
> +	char *name;
> +	int i;
> +
> +	i = pcs->pins.cur;
> +	if (i >= pcs->desc->npins) {
> +		dev_err(pcs->dev, "too many pins, max %i\n",
> +			pcs->desc->npins);
> +		return -ENOMEM;
> +	}
> +
> +	pin = &pcs->pins.pa[i];
> +	pn = &pcs->names[i];
> +	name = pn->name;
> +	sprintf(name, "%lx",
> +		(unsigned long)pcs->res->start + offset);
> +	pin->name = name;
> +	pin->number = i;
> +	pcs->pins.cur++;
> +
> +	return i;
> +}
> +
> +/**
> + * pcs_allocate_pin_table() - adds all the pins for the pinctrl driver
> + * @pcs: pcs driver instance
> + *
> + * In case of errors, resources are freed in pcs_free_resources.
> + *
> + * If your hardware needs holes in the address space, then just set
> + * up multiple driver instances.
> + *
> + * REVISIT: Handle the address space better for multiple registers
> + * per pin type hardware.
> + */
> +static int __devinit pcs_allocate_pin_table(struct pcs_device *pcs)
> +{
> +	int mux_bytes, nr_pins, i;
> +
> +	mux_bytes = pcs->width / BITS_PER_BYTE;
> +	nr_pins = pcs->size / mux_bytes;
> +
> +	dev_dbg(pcs->dev, "allocating %i pins\n", nr_pins);
> +	pcs->pins.pa = devm_kzalloc(pcs->dev,
> +				sizeof(*pcs->pins.pa) * nr_pins,
> +				GFP_KERNEL);
> +	if (!pcs->pins.pa)
> +		return -ENOMEM;
> +
> +	pcs->names = devm_kzalloc(pcs->dev,
> +				sizeof(struct pcs_name) * nr_pins,
> +				GFP_KERNEL);
> +	if (!pcs->names)
> +		return -ENOMEM;
> +
> +	pcs->desc->pins = pcs->pins.pa;
> +	pcs->desc->npins = nr_pins;
> +
> +	for (i = 0; i < pcs->desc->npins; i++) {
> +		unsigned offset;
> +		int res;
> +
> +		offset = i * mux_bytes;
> +		res = pcs_add_pin(pcs, offset);
> +		if (res < 0) {
> +			dev_err(pcs->dev, "error adding pins: %i\n", res);
> +			return res;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * pcs_add_function() - adds a new function to the function list
> + * @pcs: pcs driver instance
> + * @np: device node of the mux entry
> + * @name: name of the function
> + * @vals: array of mux register value pairs used by the function
> + * @nvals: number of mux register value pairs
> + * @pgnames: array of pingroup names for the function
> + * @npgnames: number of pingroup names
> + */
> +static struct pcs_function *pcs_add_function(struct pcs_device *pcs,
> +					struct device_node *np,
> +					const char *name,
> +					struct pcs_func_vals *vals,
> +					unsigned nvals,
> +					const char **pgnames,
> +					unsigned npgnames)
> +{
> +	struct pcs_function *function;
> +
> +	function = devm_kzalloc(pcs->dev, sizeof(*function), GFP_KERNEL);
> +	if (!function)
> +		return NULL;
> +
> +	function->name = name;
> +	function->vals = vals;
> +	function->nvals = nvals;
> +	function->pgnames = pgnames;
> +	function->npgnames = npgnames;
> +
> +	mutex_lock(&pcs->mutex);
> +	list_add_tail(&function->node, &pcs->functions);
> +	radix_tree_insert(&pcs->ftree, pcs->nfuncs, function);
> +	pcs->nfuncs++;
> +	mutex_unlock(&pcs->mutex);
> +
> +	return function;
> +}
> +
> +static void pcs_remove_function(struct pcs_device *pcs,
> +				struct pcs_function *function)
> +{
> +	int i;
> +
> +	mutex_lock(&pcs->mutex);
> +	for (i = 0; i < pcs->nfuncs; i++) {
> +		struct pcs_function *found;
> +
> +		found = radix_tree_lookup(&pcs->ftree, i);
> +		if (found == function)
> +			radix_tree_delete(&pcs->ftree, i);
> +	}
> +	list_del(&function->node);
> +	mutex_unlock(&pcs->mutex);
> +}
> +
> +/**
> + * pcs_add_pingroup() - add a pingroup to the pingroup list
> + * @pcs: pcs driver instance
> + * @np: device node of the mux entry
> + * @name: name of the pingroup
> + * @gpins: array of the pins that belong to the group
> + * @ngpins: number of pins in the group
> + */
> +static int pcs_add_pingroup(struct pcs_device *pcs,
> +					struct device_node *np,
> +					const char *name,
> +					int *gpins,
> +					int ngpins)
> +{
> +	struct pcs_pingroup *pingroup;
> +
> +	pingroup = devm_kzalloc(pcs->dev, sizeof(*pingroup), GFP_KERNEL);
> +	if (!pingroup)
> +		return -ENOMEM;
> +
> +	pingroup->name = name;
> +	pingroup->np = np;
> +	pingroup->gpins = gpins;
> +	pingroup->ngpins = ngpins;
> +
> +	mutex_lock(&pcs->mutex);
> +	list_add_tail(&pingroup->node, &pcs->pingroups);
> +	radix_tree_insert(&pcs->pgtree, pcs->ngroups, pingroup);
> +	pcs->ngroups++;
> +	mutex_unlock(&pcs->mutex);
> +
> +	return 0;
> +}
> +
> +/**
> + * pcs_get_pin_by_offset() - get a pin index based on the register offset
> + * @pcs: pcs driver instance
> + * @offset: register offset from the base
> + *
> + * Note that this is OK as long as the pins are in a static array.
> + */
> +static int pcs_get_pin_by_offset(struct pcs_device *pcs, unsigned offset)
> +{
> +	unsigned index;
> +
> +	if (offset >= pcs->size) {
> +		dev_err(pcs->dev, "mux offset out of range: 0x%x (0x%x)\n",
> +			offset, pcs->size);
> +		return -EINVAL;
> +	}
> +
> +	index = offset / (pcs->width / BITS_PER_BYTE);
> +
> +	return index;
> +}
> +
> +/**
> + * smux_parse_one_pinctrl_entry() - parses a device tree mux entry
> + * @pcs: pinctrl driver instance
> + * @np: device node of the mux entry
> + * @map: map entry
> + * @pgnames: pingroup names
> + *
> + * Note that this currently supports only #pinctrl-cells = 2.
> + * This could be improved to parse controllers that have additional
> + * auxilary registers per mux.
> + */
> +static int __init pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> +						struct device_node *np,
> +						struct pinctrl_map **map,
> +						const char **pgnames)
> +{
> +	struct pcs_func_vals *vals;
> +	const __be32 *mux;
> +	int size, rows, *pins, index = 0, found = 0, res = -ENOMEM;
> +	struct pcs_function *function;
> +
> +	if (pcs->cells != 2) {
> +		dev_err(pcs->dev, "unhandled %s: %i\n", PCS_MUX_CELLS,
> +			pcs->cells);
> +		return -EINVAL;
> +	}
> +
> +	mux = of_get_property(np, PCS_MUX_NAME, &size);
> +	if ((!mux) || (size < sizeof(*mux) * pcs->cells)) {
> +		dev_err(pcs->dev, "bad data for mux %s\n",
> +			np->name);
> +		return -EINVAL;
> +	}
> +
> +	size /= sizeof(*mux);	/* Number of elements in array */
> +	rows = size / pcs->cells;
> +
> +	vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows, GFP_KERNEL);
> +	if (!vals)
> +		return -ENOMEM;
> +
> +	pins = devm_kzalloc(pcs->dev, sizeof(*pins) * rows, GFP_KERNEL);
> +	if (!pins)
> +		goto free_vals;
> +
> +	while (index < size) {
> +		unsigned offset, defval;
> +		int pin;
> +
> +		offset = be32_to_cpup(mux + index++);
> +		defval = be32_to_cpup(mux + index++);
> +		vals[found].reg = pcs->base + offset;
> +		vals[found].defval = defval;
> +
> +		pin = pcs_get_pin_by_offset(pcs, offset);
> +		if (pin < 0) {
> +			dev_err(pcs->dev,
> +				"could not add functions for %s %ux\n",
> +				np->name, offset);
> +			break;
> +		}
> +		pins[found++] = pin;
> +	}
> +
> +	pgnames[0] = np->name;
> +	function = pcs_add_function(pcs, np, np->name, vals, found, pgnames, 1);
> +	if (!function)
> +		goto free_pins;
> +
> +	res = pcs_add_pingroup(pcs, np, np->name, pins, found);
> +	if (res < 0)
> +		goto free_function;
> +
> +	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> +	(*map)->data.mux.group = np->name;
> +	(*map)->data.mux.function = np->name;
> +
> +	return 0;
> +
> +free_function:
> +	pcs_remove_function(pcs, function);
> +
> +free_pins:
> +	devm_kfree(pcs->dev, pins);
> +
> +free_vals:
> +	devm_kfree(pcs->dev, vals);
> +
> +	return res;
> +}
> +/**
> + * pcs_dt_node_to_map() - allocates and parses pinctrl maps
> + * @pctldev: pinctrl instance
> + * @np_config: device tree pinmux entry
> + * @map: array of map entries
> + * @num_maps: number of maps
> + */
> +static int pcs_dt_node_to_map(struct pinctrl_dev *pctldev,
> +				struct device_node *np_config,
> +				struct pinctrl_map **map, unsigned *num_maps)
> +{
> +	struct pcs_device *pcs;
> +	struct device_node *np;
> +	struct pinctrl_map *cur_map;
> +	const char **pgnames, **cur_name;
> +	int ret, found_maps = 0, added_maps = 0;
> +
> +	pcs = pinctrl_dev_get_drvdata(pctldev);
> +
> +	for_each_child_of_node(np_config, np)
> +		found_maps++;
> +
> +	*map = devm_kzalloc(pcs->dev, sizeof(**map) * found_maps,
> +				GFP_KERNEL);
> +	if (!map)
> +		return -ENOMEM;
> +
> +	cur_map = *map;
> +	*num_maps = 0;
> +
> +	pgnames = devm_kzalloc(pcs->dev, sizeof(*pgnames) * found_maps,
> +				GFP_KERNEL);
> +	if (!pgnames) {
> +		devm_kfree(pcs->dev, *map);
> +		return -ENOMEM;
> +	}
> +
> +	cur_name = pgnames;
> +
> +	for_each_child_of_node(np_config, np) {
> +		ret = pcs_parse_one_pinctrl_entry(pcs, np, &cur_map, cur_name);
> +		if (ret < 0) {
> +			dev_err(pcs->dev, "added only %i/%i entries for %s\n",
> +				added_maps, found_maps, np_config->name);
> +			break;
> +		}
> +		cur_map++;
> +		cur_name++;
> +		added_maps++;
> +	}
> +
> +	*num_maps = added_maps;
> +
> +	return 0;
> +}
> +
> +/**
> + * pcs_free_funcs() - free memory used by functions
> + * @pcs: pcs driver instance
> + */
> +static void pcs_free_funcs(struct pcs_device *pcs)
> +{
> +	struct list_head *pos, *tmp;
> +	int i;
> +
> +	mutex_lock(&pcs->mutex);
> +	for (i = 0; i < pcs->nfuncs; i++) {
> +		struct pcs_function *func;
> +
> +		func = radix_tree_lookup(&pcs->ftree, i);
> +		if (!func)
> +			continue;
> +		radix_tree_delete(&pcs->ftree, i);
> +	}
> +	list_for_each_safe(pos, tmp, &pcs->functions) {
> +		struct pcs_function *function;
> +
> +		function = list_entry(pos, struct pcs_function, node);
> +		list_del(&function->node);
> +	}
> +	mutex_unlock(&pcs->mutex);
> +}
> +
> +/**
> + * pcs_free_pingroups() - free memory used by pingroups
> + * @pcs: pcs driver instance
> + */
> +static void pcs_free_pingroups(struct pcs_device *pcs)
> +{
> +	struct list_head *pos, *tmp;
> +	int i;
> +
> +	mutex_lock(&pcs->mutex);
> +	for (i = 0; i < pcs->ngroups; i++) {
> +		struct pcs_pingroup *pingroup;
> +
> +		pingroup = radix_tree_lookup(&pcs->pgtree, i);
> +		if (!pingroup)
> +			continue;
> +		radix_tree_delete(&pcs->pgtree, i);
> +	}
> +	list_for_each_safe(pos, tmp, &pcs->pingroups) {
> +		struct pcs_pingroup *pingroup;
> +
> +		pingroup = list_entry(pos, struct pcs_pingroup, node);
> +		list_del(&pingroup->node);
> +	}
> +	mutex_unlock(&pcs->mutex);
> +}
> +
> +/**
> + * pcs_free_resources() - free memory used by this driver
> + * @pcs: pcs driver instance
> + */
> +static void pcs_free_resources(struct pcs_device *pcs)
> +{
> +	if (pcs->pctl)
> +		pinctrl_unregister(pcs->pctl);
> +
> +	pcs_free_funcs(pcs);
> +	pcs_free_pingroups(pcs);
> +}
> +
> +/**
> + * pcs_register() - initializes and registers with pinctrl framework
> + * @pcs: pcs driver instance
> + */
> +static int __devinit pcs_register(struct pcs_device *pcs)
> +{
> +	int ret;
> +
> +	if (!pcs->dev->of_node)
> +		return -ENODEV;
> +
> +	pcs->desc = devm_kzalloc(pcs->dev, sizeof(*pcs->desc), GFP_KERNEL);
> +	if (!pcs->desc)
> +		return -ENOMEM;
> +	pcs->desc->name = DRIVER_NAME;
> +	pcs->desc->pctlops = &pcs_pinctrl_ops;
> +	pcs->desc->pmxops = &pcs_pinmux_ops;
> +	pcs->desc->confops = &pcs_pinconf_ops;
> +	pcs->desc->owner = THIS_MODULE;
> +
> +	ret = pcs_allocate_pin_table(pcs);
> +	if (ret < 0)
> +		goto free;
> +
> +	pcs->pctl = pinctrl_register(pcs->desc, pcs->dev, pcs);
> +	if (!pcs->pctl) {
> +		dev_err(pcs->dev, "could not register simple pinctrl driver\n");
> +		ret = -EINVAL;
> +		goto free;
> +	}
> +
> +	dev_info(pcs->dev, "%i pins at pa %p size %u\n",
> +		 pcs->desc->npins, pcs->base, pcs->size);
> +
> +	return 0;
> +
> +free:
> +	pcs_free_resources(pcs);
> +
> +	return ret;
> +}
> +
> +#define PCS_GET_PROP_U32(name, reg, err)				\
> +	do {								\
> +		ret = of_property_read_u32(np, name, reg);		\
> +		if (ret) {						\
> +			dev_err(pcs->dev, err);				\
> +			goto out;					\
> +		}							\
> +	} while (0);
> +
> +static struct of_device_id pcs_of_match[];
> +
> +/*
> + * REVISIT: Handle hardware with multiple registers per pin
> + * better by adding other masks.
> + */
> +static int __devinit pcs_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct of_device_id *match;
> +	struct resource res;
> +	struct pcs_device *pcs;
> +	int ret;
> +
> +	match = of_match_device(pcs_of_match, &pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +
> +	pcs = devm_kzalloc(&pdev->dev, sizeof(*pcs), GFP_KERNEL);
> +	if (!pcs) {
> +		dev_err(&pdev->dev, "could not allocate\n");
> +		return -ENOMEM;
> +	}
> +	pcs->dev = &pdev->dev;
> +	mutex_init(&pcs->mutex);
> +	INIT_LIST_HEAD(&pcs->pingroups);
> +	INIT_LIST_HEAD(&pcs->functions);
> +
> +	PCS_GET_PROP_U32("pinctrl-simple,register-width", &pcs->width,
> +			 "register width not specified\n");
> +
> +	PCS_GET_PROP_U32("pinctrl-simple,function-mask", &pcs->fmask,
> +			 "function register mask not specified\n");
> +	pcs->fshift = ffs(pcs->fmask) - 1;
> +	pcs->fmax = pcs->fmask >> pcs->fshift;
> +
> +	PCS_GET_PROP_U32("pinctrl-simple,function-off", &pcs->foff,
> +			 "function off mode not specified\n");
> +
> +	PCS_GET_PROP_U32("pinctrl-simple,pinconf-mask", &pcs->cmask,
> +			 "pinconf mask not specified\n");
> +
> +	PCS_GET_PROP_U32("#pinctrl-cells", &pcs->cells,
> +			 "#pinctrl-cells not specified\n");
> +
> +	ret = of_address_to_resource(pdev->dev.of_node, 0, &res);
> +	if (ret) {
> +		dev_err(pcs->dev, "could not get resource\n");
> +		goto out;
> +	}
> +
> +	pcs->res = devm_request_mem_region(pcs->dev, res.start,
> +			resource_size(&res), DRIVER_NAME);
> +	if (!pcs->res) {
> +		dev_err(pcs->dev, "could not get mem_region\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	pcs->size = resource_size(pcs->res);
> +	pcs->base = devm_ioremap(pcs->dev, pcs->res->start, pcs->size);
> +	if (!pcs->base) {
> +		dev_err(pcs->dev, "could not ioremap\n");
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	INIT_RADIX_TREE(&pcs->pgtree, GFP_KERNEL);
> +	INIT_RADIX_TREE(&pcs->ftree, GFP_KERNEL);
> +	platform_set_drvdata(pdev, pcs);
> +
> +	switch (pcs->width) {
> +	case 8:
> +		pcs->read = pcs_readb;
> +		pcs->write = pcs_writeb;
> +		break;
> +	case 16:
> +		pcs->read = pcs_readw;
> +		pcs->write = pcs_writew;
> +		break;
> +	case 32:
> +		pcs->read = pcs_readl;
> +		pcs->write = pcs_writel;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	ret = pcs_register(pcs);
> +	if (ret < 0) {
> +		dev_err(pcs->dev, "could not add mux registers: %i\n", ret);
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	return ret;
> +}
> +
> +static int __devexit pcs_remove(struct platform_device *pdev)
> +{
> +	struct pcs_device *pcs = platform_get_drvdata(pdev);
> +
> +	if (!pcs)
> +		return 0;
> +
> +	pcs_free_resources(pcs);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id pcs_of_match[] __devinitdata = {
> +	{ .compatible = DRIVER_NAME, },
> +	{ .compatible = "ti,omap2420-padconf", },
> +	{ .compatible = "ti,omap2430-padconf", },
> +	{ .compatible = "ti,omap3-padconf", },
> +	{ .compatible = "ti,omap4-padconf", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, pcs_of_match);
> +
> +static struct platform_driver pcs_driver = {
> +	.probe		= pcs_probe,
> +	.remove		= __devexit_p(pcs_remove),
> +	.driver = {
> +		.owner		= THIS_MODULE,
> +		.name		= DRIVER_NAME,
> +		.of_match_table	= pcs_of_match,
> +	},
> +};
> +
> +module_platform_driver(pcs_driver);
> +
> +MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
> +MODULE_DESCRIPTION("Simple device tree pinctrl driver");
> +MODULE_LICENSE("GPL");
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

Hi,

* Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120503 00:16]:
> 
> 	I really like it
> 
> 	I was working on something simillar
> 
> 	but can we split the group management so we can use it on other
> 	bindings

Hmm I'm not sure I follow on the group management splitting, can you specify
what you have in mind here?

If you mean moving more things into pinctrl fwk, then yeah I'd assume that
will happen eventually and drivers like this will end up becoming more minimal.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/03/2012 09:27 AM, Tony Lindgren wrote:
> Hi,
> 
> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120503 00:16]:
>>
>> 	I really like it
>>
>> 	I was working on something simillar
>>
>> 	but can we split the group management so we can use it on other
>> 	bindings
> 
> Hmm I'm not sure I follow on the group management splitting, can you specify
> what you have in mind here?
> 
> If you mean moving more things into pinctrl fwk, then yeah I'd assume that
> will happen eventually and drivers like this will end up becoming more minimal.

Jean-Christophe, forgive me if I'm putting words in your mouth, but I
assume the following is what you mean:

There are two pieces of data required by the pinctrl subsystem:

a) The set of pins, functions, and groups that exist.

b) The specific function to select for each pin/group on a given board.

Item (a) can be represented in the pinctrl driver (e.g. as in the Tegra
driver), or can be represented in device tree in order to avoid large
tables in the driver.

Item (b) has to be represented in device tree, since the whole point is
that it's board-specific.

For all DT bindings I've seen that choose to represent (a) in the DT
rather than in the driver, the DT represents (b) directly, and (a) is
implicitly extracted/created based on (a).

When I was first thinking about DT bindings for pinctrl, I had hoped
that even if (a) was represented in DT, the DT nodes/properties for (a)
and (b) would be entirely separate, so that the binding for (b) could be
completely common across all SoCs, even though the binding for (a) would
perhaps be different across SoCs (if it existed at all).

So, perhaps Jean-Christophe is talking about splitting up (a) and (b) in
device tree?

Or perhaps Jean-Christophe only refers to the code that creates the
group and function definitions from (b), and not the actual DT binding
itself?

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:34 Thu 03 May     , Stephen Warren wrote:
> On 05/03/2012 09:27 AM, Tony Lindgren wrote:
> > Hi,
> > 
> > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120503 00:16]:
> >>
> >> 	I really like it
> >>
> >> 	I was working on something simillar
> >>
> >> 	but can we split the group management so we can use it on other
> >> 	bindings
> > 
> > Hmm I'm not sure I follow on the group management splitting, can you specify
> > what you have in mind here?
> > 
> > If you mean moving more things into pinctrl fwk, then yeah I'd assume that
> > will happen eventually and drivers like this will end up becoming more minimal.
> 
> Jean-Christophe, forgive me if I'm putting words in your mouth, but I
> assume the following is what you mean:
> 
> There are two pieces of data required by the pinctrl subsystem:
> 
> a) The set of pins, functions, and groups that exist.
> 
> b) The specific function to select for each pin/group on a given board.
> 
> Item (a) can be represented in the pinctrl driver (e.g. as in the Tegra
> driver), or can be represented in device tree in order to avoid large
> tables in the driver.
> 
> Item (b) has to be represented in device tree, since the whole point is
> that it's board-specific.
> 
> For all DT bindings I've seen that choose to represent (a) in the DT
> rather than in the driver, the DT represents (b) directly, and (a) is
> implicitly extracted/created based on (a).
> 
> When I was first thinking about DT bindings for pinctrl, I had hoped
> that even if (a) was represented in DT, the DT nodes/properties for (a)
> and (b) would be entirely separate, so that the binding for (b) could be
> completely common across all SoCs, even though the binding for (a) would
> perhaps be different across SoCs (if it existed at all).
> 
> So, perhaps Jean-Christophe is talking about splitting up (a) and (b) in
> device tree?
> 
> Or perhaps Jean-Christophe only refers to the code that creates the
> group and function definitions from (b), and not the actual DT binding
> itself?
yes you are right Stephen
I was thinking of both but the second could be a first step

today we tend all to represent the group of pin in DT

for TI, IM, MXC, at91, ST and others

the way to represend the groups are exactly the same

a group is a node with a set on pin

uart {
 pincfg......
}

and the group is uart

so we do need to have a common way to handle it in c

the code propose by Tony is really what I'm working on to acheive this

In my mind in the driver we do not have to care how to list
register/unregister the group. We just need to be able to do this

pinctrl_register_group(...)

or 

pinctrl_unregistewr_group(...)

On at91 we have this type of controller

one pin can have multiple function and each function can be on different pin
and we need to program and represent each of them one by one

And each pin have different parameter

so I was thinking to do like on gpio

uart {
	pin = < &pioA 12 {pararms} >

}

and use macro as basicaly we are just this

and this can be applied to tegra too as you will just refer the pin in this hw
pin block

Best Regards,
J.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120503 22:08]:
> 
> In my mind in the driver we do not have to care how to list
> register/unregister the group. We just need to be able to do this
> 
> pinctrl_register_group(...)
> 
> or 
> 
> pinctrl_unregistewr_group(...)
> 
> On at91 we have this type of controller

Ah I see. Yeah makes sense. Also I think we should let the pinctrl
core eventually manage the pins more too. Right now the pins are
a static array in the driver, which makes things unnecessarily
complex for the DT case. It would be nice to also have something like
pinctrl_register/unregister_pin instead of requiring them all
be registered while registering with the framework initially.

But all that can be improved later on once we get the binding down..
 
> one pin can have multiple function and each function can be on different pin
> and we need to program and represent each of them one by one
> 
> And each pin have different parameter
> 
> so I was thinking to do like on gpio
> 
> uart {
> 	pin = < &pioA 12 {pararms} >
> 
> }

Hmm I assume the "12" above the gpio number?
 
> and use macro as basicaly we are just this
> 
> and this can be applied to tegra too as you will just refer the pin in this hw
> pin block

I was thinking of adding gpio eventually as a separate attribute with
something like the following. Here cam_d10 pin is used as gpio109:

cam_d10.gpio_109 {
	pinctrl-simple,cells = <0xfa 0x104>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE4 */
	gpio = <&gpio4 13 0>;			/* gpio109 */
};

The reasoning for this is that as we may not care about the gpio number
for all pins, it should be optional. Would that work for you?

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:03 Fri 04 May     , Tony Lindgren wrote:
> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120503 22:08]:
> > 
> > In my mind in the driver we do not have to care how to list
> > register/unregister the group. We just need to be able to do this
> > 
> > pinctrl_register_group(...)
> > 
> > or 
> > 
> > pinctrl_unregistewr_group(...)
> > 
> > On at91 we have this type of controller
> 
> Ah I see. Yeah makes sense. Also I think we should let the pinctrl
> core eventually manage the pins more too. Right now the pins are
> a static array in the driver, which makes things unnecessarily
> complex for the DT case. It would be nice to also have something like
> pinctrl_register/unregister_pin instead of requiring them all
> be registered while registering with the framework initially.
> 
> But all that can be improved later on once we get the binding down..
agreed at 100%
>  
> > one pin can have multiple function and each function can be on different pin
> > and we need to program and represent each of them one by one
> > 
> > And each pin have different parameter
> > 
> > so I was thinking to do like on gpio
> > 
> > uart {
> > 	pin = < &pioA 12 {pararms} >
> > 
> > }
> 
> Hmm I assume the "12" above the gpio number?
no pin number in the bank because it could not be gpio

evenif on at91 and nearly on the controller I known it is the case
>  
> > and use macro as basicaly we are just this
> > 
> > and this can be applied to tegra too as you will just refer the pin in this hw
> > pin block
> 
> I was thinking of adding gpio eventually as a separate attribute with
> something like the following. Here cam_d10 pin is used as gpio109:
> 
> cam_d10.gpio_109 {
> 	pinctrl-simple,cells = <0xfa 0x104>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE4 */
> 	gpio = <&gpio4 13 0>;			/* gpio109 */
> };
> 
> The reasoning for this is that as we may not care about the gpio number
> for all pins, it should be optional. Would that work for you?
yes

but I was thinking to put it as a param but why not

my idea was this

pinctrl@fffff200 {
	#address-cells = <1>;
	#size-cells = <0>;
	compatible = "atmel,at91rm9200-pinctrl";

	atmel,mux-mask = <
		/*    A	  B     */
		 0xffffffff 0xffc003ff  /* pioA */
		 0xffffffff 0x800f8f00  /* pioB */
		 0xffffffff 0x00000e00  /* pioC */
		 0xffffffff 0xff0c1381  /* pioD */
		 0xffffffff 0x81ffff81  /* pioE */
		>;

	pioA: gpio@fffff200 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff200 0x100>;
		interrupts = <2 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioB: gpio@fffff400 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff400 0x100>;
		interrupts = <3 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioC: gpio@fffff600 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff600 0x100>;
		interrupts = <4 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioD: gpio@fffff800 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffff800 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	pioE: gpio@fffffa00 {
		compatible = "atmel,at91rm9200-gpio";
		reg = <0xfffffa00 0x100>;
		interrupts = <5 4>;
		#gpio-cells = <2>;
		gpio-controller;
		interrupt-controller;
	};

	dbgu {
		pins = < &pioB 12 0 0
			 &pioB 13 0 2 >;
	/* with macro */
		pins = < &pioB 12 MUX_A NO_PULL_UP
			 &pioB 13 MUX_A PULL_UP >;
	};

	/* and also the notion of linked group
	 * as on uart of network you have often the same subset of pin use.
	 *
	 * As example on uart rxd/txd is use for the group without rts/cts
	 * and the one with it
	 * on ethernet the RMII pin are use also on MII
	 */

	uart0_rxd_txd {
		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
	};

	uart0_rts_cts {
		groups = < &uart0_rxd_txd >;
		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
	};

	uart0_rts_cts_external_pull_up {
		groups = < &uart0_rts_cts >;
		gpios = <&pioC 1 0>;
	};
};

The idea is to avoid duplication the xlate for pins will be driver specific
with maybe a common implementation

the 3 or 4 first fix as done on gpio

Best Regards,
J.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
> On 08:03 Fri 04 May     , Tony Lindgren wrote:
> > > 
> > > so I was thinking to do like on gpio
> > > 
> > > uart {
> > > 	pin = < &pioA 12 {pararms} >
> > > 
> > > }
> > 
> > Hmm I assume the "12" above the gpio number?
> no pin number in the bank because it could not be gpio

Yes OK, but pin number 12 in the gpio bank, not in the mux register.
Got it.
 
> 	pioD: gpio@fffff800 {
> 		compatible = "atmel,at91rm9200-gpio";
> 		reg = <0xfffff800 0x100>;
> 		interrupts = <5 4>;
> 		#gpio-cells = <2>;
> 		gpio-controller;
> 		interrupt-controller;
> 	};
> 
> 	pioE: gpio@fffffa00 {
> 		compatible = "atmel,at91rm9200-gpio";
> 		reg = <0xfffffa00 0x100>;
> 		interrupts = <5 4>;
> 		#gpio-cells = <2>;
> 		gpio-controller;
> 		interrupt-controller;
> 	};
> 
> 	dbgu {
> 		pins = < &pioB 12 0 0
> 			 &pioB 13 0 2 >;
> 	/* with macro */
> 		pins = < &pioB 12 MUX_A NO_PULL_UP
> 			 &pioB 13 MUX_A PULL_UP >;
> 	};

I could change to use this too no problem. The only concern I have is
that is "&pioB 12" immutable for all gpio controllers?

Grepping the *.dts* files, at least exynos is using the following
for gpios:

gpios = <&gpx2 0 0 0 2>;

If we can conclude that phandle to the gpio controller instance and
the register offset is always enough here, then I'm OK changing to
that format. It would actually save some parsing in most cases.
 
> 	/* and also the notion of linked group
> 	 * as on uart of network you have often the same subset of pin use.
> 	 *
> 	 * As example on uart rxd/txd is use for the group without rts/cts
> 	 * and the one with it
> 	 * on ethernet the RMII pin are use also on MII
> 	 */
> 
> 	uart0_rxd_txd {
> 		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
> 			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
> 	};
> 
> 	uart0_rts_cts {
> 		groups = < &uart0_rxd_txd >;
> 		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
> 			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
> 	};
> 
> 	uart0_rts_cts_external_pull_up {
> 		groups = < &uart0_rts_cts >;
> 		gpios = <&pioC 1 0>;
> 	};
> };
> 
> The idea is to avoid duplication the xlate for pins will be driver specific
> with maybe a common implementation
> 
> the 3 or 4 first fix as done on gpio

Yeah sounds doable to me, but can probably be added later?

Regarding grouping, basically for most cases it makes sense to have
three states: default, active, idle instead of just active and idle.
The reason is that for most cases the default pins only need to be
set once for each devices. Only few pins need to toggle between
active and idle states.

For example, omap2 uart needs to toggle rx pin to gpio input everytime
it hits idle to provide wake-up events. Other pins do not need to change.
As this is done all the time, the active and idle toggling should be kept
to minimum.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:34 Fri 04 May     , Tony Lindgren wrote:
> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
> > On 08:03 Fri 04 May     , Tony Lindgren wrote:
> > > > 
> > > > so I was thinking to do like on gpio
> > > > 
> > > > uart {
> > > > 	pin = < &pioA 12 {pararms} >
> > > > 
> > > > }
> > > 
> > > Hmm I assume the "12" above the gpio number?
> > no pin number in the bank because it could not be gpio
> 
> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
> Got it.
>  
> > 	pioD: gpio@fffff800 {
> > 		compatible = "atmel,at91rm9200-gpio";
> > 		reg = <0xfffff800 0x100>;
> > 		interrupts = <5 4>;
> > 		#gpio-cells = <2>;
> > 		gpio-controller;
> > 		interrupt-controller;
> > 	};
> > 
> > 	pioE: gpio@fffffa00 {
> > 		compatible = "atmel,at91rm9200-gpio";
> > 		reg = <0xfffffa00 0x100>;
> > 		interrupts = <5 4>;
> > 		#gpio-cells = <2>;
> > 		gpio-controller;
> > 		interrupt-controller;
> > 	};
> > 
> > 	dbgu {
> > 		pins = < &pioB 12 0 0
> > 			 &pioB 13 0 2 >;
> > 	/* with macro */
> > 		pins = < &pioB 12 MUX_A NO_PULL_UP
> > 			 &pioB 13 MUX_A PULL_UP >;
> > 	};
> 
> I could change to use this too no problem. The only concern I have is
> that is "&pioB 12" immutable for all gpio controllers?
> 
> Grepping the *.dts* files, at least exynos is using the following
> for gpios:
> 
> gpios = <&gpx2 0 0 0 2>;
> 
> If we can conclude that phandle to the gpio controller instance and
> the register offset is always enough here, then I'm OK changing to
> that format. It would actually save some parsing in most cases.
I would said yes but we could use the same notion to create pin-bank

the idea is to refer to the bank and then the pin inside

after if a driver need more argumement as on exynos they will have to
implement their own xlate

as we did on at91 for the irq xlate
>  
> > 	/* and also the notion of linked group
> > 	 * as on uart of network you have often the same subset of pin use.
> > 	 *
> > 	 * As example on uart rxd/txd is use for the group without rts/cts
> > 	 * and the one with it
> > 	 * on ethernet the RMII pin are use also on MII
> > 	 */
> > 
> > 	uart0_rxd_txd {
> > 		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
> > 			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
> > 	};
> > 
> > 	uart0_rts_cts {
> > 		groups = < &uart0_rxd_txd >;
> > 		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
> > 			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
> > 	};
> > 
> > 	uart0_rts_cts_external_pull_up {
> > 		groups = < &uart0_rts_cts >;
> > 		gpios = <&pioC 1 0>;
> > 	};
> > };
> > 
> > The idea is to avoid duplication the xlate for pins will be driver specific
> > with maybe a common implementation
> > 
> > the 3 or 4 first fix as done on gpio
> 
> Yeah sounds doable to me, but can probably be added later?
for the sub-group stuff yes agreed
> 
> Regarding grouping, basically for most cases it makes sense to have
> three states: default, active, idle instead of just active and idle.
> The reason is that for most cases the default pins only need to be
> set once for each devices. Only few pins need to toggle between
> active and idle states.
yeah agreed

Best Regards,
J.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/04/2012 10:34 AM, Tony Lindgren wrote:
> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
>>>>
>>>> so I was thinking to do like on gpio
>>>>
>>>> uart {
>>>> 	pin = < &pioA 12 {pararms} >
>>>>
>>>> }
>>>
>>> Hmm I assume the "12" above the gpio number?
>> no pin number in the bank because it could not be gpio
> 
> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
> Got it.

I'd prefer to avoid any references to GPIOs here; not all muxable pins
are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
independent.

>> 	pioD: gpio@fffff800 {
>> 		compatible = "atmel,at91rm9200-gpio";
>> 		reg = <0xfffff800 0x100>;
>> 		interrupts = <5 4>;
>> 		#gpio-cells = <2>;
>> 		gpio-controller;
>> 		interrupt-controller;
>> 	};
>>
>> 	pioE: gpio@fffffa00 {
>> 		compatible = "atmel,at91rm9200-gpio";
>> 		reg = <0xfffffa00 0x100>;
>> 		interrupts = <5 4>;
>> 		#gpio-cells = <2>;
>> 		gpio-controller;
>> 		interrupt-controller;
>> 	};
>>
>> 	dbgu {
>> 		pins = < &pioB 12 0 0
>> 			 &pioB 13 0 2 >;
>> 	/* with macro */
>> 		pins = < &pioB 12 MUX_A NO_PULL_UP
>> 			 &pioB 13 MUX_A PULL_UP >;
>> 	};
> 
> I could change to use this too no problem. The only concern I have is
> that is "&pioB 12" immutable for all gpio controllers?

You mean is the number of cells used to specify a GPIO the same
everywhere? No. It's defined by #gpio-cells in the GPIO controller node.

But again, the GPIO binding shouldn't be related to the pinctrl binding
directly.

> Grepping the *.dts* files, at least exynos is using the following
> for gpios:
> 
> gpios = <&gpx2 0 0 0 2>;
> 
> If we can conclude that phandle to the gpio controller instance and
> the register offset is always enough here, then I'm OK changing to
> that format. It would actually save some parsing in most cases.
>  
>> 	/* and also the notion of linked group
>> 	 * as on uart of network you have often the same subset of pin use.
>> 	 *
>> 	 * As example on uart rxd/txd is use for the group without rts/cts
>> 	 * and the one with it
>> 	 * on ethernet the RMII pin are use also on MII
>> 	 */
>>
>> 	uart0_rxd_txd {
>> 		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
>> 			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
>> 	};

I don't really see how that DT format represents pins, functions, and
nodes directly, and separately from which of those a board chooses to
use. I think this binding and the one Tony originally proposed are
eseentially semantically identical.

Going back to my idea of separating SoC and board configurations, if we
did that, I'd expect to see something more like:

soc.dtsi or board.dts:

This is the data that the pin controller driver needs to export to
pinctrl core. This can be completely enumerated in the soc.dtsi, or
perhaps for uncommonly used pins/groups/functions, only included in the
board.dts that actually uses it to cut down on soc.dtsi's size:

This data is not needed for SoCs whose pinctrl drivers include it in
their driver file instead of DT.

/ {
    pinctrl@... {
        pins {
            uart1_rx_pin: uart1_rx {
                /* register to program the pin if per-pin muxing*/
                reg = ...;
                name = "UART1_RX";
                ...;
            }
            uart1_tx_pin: uart1_tx {
                reg = ...;
                name = "UART1_TX";
                ...;
            }
            uart2_rx_pin: uart2_rx {
                reg = ...;
                name = "UART2_RX";
                ...;
            }
            uart2_tx_pin: uart2_tx {
                reg = ...;
                name = "UART2_TX";
                ...;
            }
        };
        pingroups {
            uart1_pg: uart1 {
                /* register to program the group, if per-grouop muxing */
                reg = ...;
                pins = <&pin_uart1_rx &pin_uart1_tx>;
            }
            uart2_pg: uart2 {
                pins = <&pin_uart2_rx &pin_uart2_tx>;
            }
        };
        functions {
            uart1_func: uart1 {
                selectable-on = <&uart1_pg 0>; /* where, mux value */
            };
            uart2_func: uart2 {
                selectable-on = <&uart2_pg 0>;
            };
            spi_func: spi {
                selectable-on = <&uart1_pg 1 &uart2_pg 1>;
            };
    };

soc.dtsi or board.dts:

This is part of the data used to construct the mapping table. Common
options for function X on pin/pingroup Y can be included in soc.dtsi to
reduce duplication in board files. Uncommon options can be included
directly in the board.dts that uses them, to avoid bloating soc.dtsi:

This data is needed irrespective of whether a SoC pinctrl driver stores
the pin/function/group information in DT or in the driver itself.

/ {
    pinctrl@... {
        uart1_uart1 {
            muxing = <&uart1_pg &uart1_func>;
            ... other pinconfig stuff perhaps;
        }
        spi1_uart2 {
            muxing = <&uart2_pg &uart2_func>;
            ... other pinconfig stuff perhaps;
        }
    }
};

board.dts:

Finally, each board defines which of the muxing options to actually use.

This data is needed irrespective of whether a SoC pinctrl driver stores
the pin/function/group information in DT or in the driver itself.

/ {
    uart@... {
        pinctrl-names = "default";
        pinctrl-0 = <&uart1_uart1>;
    };
    spi@... {
        pinctrl-names = "default";
        pinctrl-0 = <&spi1_uart2>;
    };
};

>> 	uart0_rts_cts {
>> 		groups = < &uart0_rxd_txd >;
>> 		pins = < &pioB 17 MUX_B NO_PULL_UP	/* rts */
>> 			 &pioB 15 MUX_B NO_PULL_UP >;	/* cts */
>> 	};
>>
>> 	uart0_rts_cts_external_pull_up {
>> 		groups = < &uart0_rts_cts >;
>> 		gpios = <&pioC 1 0>;
>> 	};
>> };
>>
>> The idea is to avoid duplication the xlate for pins will be driver specific
>> with maybe a common implementation
>>
>> the 3 or 4 first fix as done on gpio
> 
> Yeah sounds doable to me, but can probably be added later?
> 
> Regarding grouping, basically for most cases it makes sense to have
> three states: default, active, idle instead of just active and idle.

The set of state names should be determined by the client driver, not
the pinctrl core or driver binding, any pinctrl code.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/02/2012 11:24 AM, Tony Lindgren wrote:
> Add simple pinctrl driver using device tree data.
> 
> Currently this driver only works on omap2+ series of
> processors, where there is either an 8 or 16-bit padconf
> register for each pin. Support for other similar pinmux
> controllers could be added.
> 
> Note that this patch does not yet support pinconf_ops
> or GPIO. Further.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> Here's this one finally updated to use the common pinctrl bindings.
> That sure cleaned up a bunch of the nasty things in this driver :)
> Nice job Stephen!

Thanks.

> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt

> +Required properties:
> +- compatible :  one of:
> +	- "pinctrl-simple"
> +	- "ti,omap2420-padconf"
> +	- "ti,omap2430-padconf"
> +	- "ti,omap3-padconf"
> +	- "ti,omap4-padconf"

I'm in two minds about this.

If this is truly intended to be generic, I would not document any of the
ti compatible values here. Instead, I'd create a binding document for
the TI controllers that basically just says "this uses the bindings in
pinctrl-simple.txt, with the following additions", and list the
compatible values as an addition.

On the other hand, I worry about whether using "pinctrl-simple" here as
the compatible value is going to cause issues:

Certainly, this is a pretty simple driver, and most likely reasonably
generic an applicable to many SoCs. However, it doesn't cover a bunch of
cases that I'd still consider "simple". For example, what if each pin
has a separate mux and pinconf register? There are probably many such
small cases that would add up to something more complex. to cover those
cases, will we be able to extend pinctrl-simple to cover them, and
continue to be backwards compatible, or will we need to create a
binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3
each of which covers some different, yet still simple, configuration?

To resolve this, perhaps it would be best to rework this binding and
driver as a set of utility code that other bindings and drivers can
build upon, rather than making it a standalone directly usable driver
and binding.

Honestly, I'm not really sure which way to go here.

> +- reg : offset and length of the register set for the mux registers
> +- #pinctrl-cells : width of the pinctrl array, currently only 2 is supported
> +- pinctrl-simple,register-width : pinmux register access width
> +- pinctrl-simple,function-mask : mask of allowed pinmux function bits
> +- pinctrl-simple,function-off : function off mode for disabled state
> +- pinctrl-simple,pinconf-mask : mask of allowed pinconf bits
> +
> +This driver uses the common pinctrl bindings as specified in
> +pinctrl-bindings.txt document in this directory. The common bindings are used
> +to specify the client device states using pinctrl-0 and pinctrl-names entries.

I think just remove the second sentence there; it's implicit given the
first.

> +/* board specific .dts file, such as omap4-sdp.dts */
> +&omap4_pmx_core {
> +
> +	/*
> +	 * map all board specific static pins enabled by the pinctrl driver
> +	 * itself during the boot (or just set them up in the bootloader)
> +	 */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&board_pins>;
> +
> +	board_pins: pinmux_board_pins {
> +		board_static_pins {

I'd like to see a little more explanation of the node structure here.
I.e. what does each node represent, why are there two levels of nodes, etc.

Given that the "pinctrl-simple,cells" can specify both mux function and
pin configuration for each pin, i.e. everything you need to, I don't see
why you'd ever need two levels of nodes here. I'd expect each "pin
configuration node" (in pinctrl core bindings terminology) to be a
single level:

node {
    pinctrl-simple,cells = <...>;
};

where node is what's references in the pinctrl-0 property by pinctrl
client drivers.

> +			pinctrl-simple,cells = <

"cells" here doesn't seem like the right word. Perhaps "pins" or
"configuration"? "Cell"s to me seems semi-reserved for binding cell
count description, like #gpio-cells, #address-cells, etc.

Also, there's nothing in the free-form text part of this binding
document that says describes the set of properties that need to exist in
these "pin configuration nodes", nor what their content is.

> +				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> +			>;
> +		};
> +	};
> +
> +
> +	/* map all uart2 pins as a single function */
> +	uart2_pins: pinmux_uart2_pins {
> +		uart2_pins {
> +			pinctrl-simple,cells = <
> +				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> +				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> +				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +			>;
> +		};
> +	};
> +
> +	/* map all uart3 pins as separate functions */
> +	uart3_pins: pinmux_uart3_pins {

>From a binding perspective, I don't see why you'd want to allow two cases:

1) One node with multiple entries in pinctrl-simple,cells
2) Multiple nodes each with a single entry in pinctrl-simple,cells

Why not only allow (1)?

> +		uart3_cts_rctx.uart3_cts_rctx {
> +			pinctrl-simple,cells = <0x100 0x118>;	/* OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> +             };
> +
> +		uart3_rts_sd.uart3_rts_sd {
> +			pinctrl-simple,cells = <0x102 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +		};
> +
> +		uart3_rx_irrx.uart3_rx_irrx {
> +			pinctrl-simple,cells = <0x104 0x100>;	/* OMAP_PIN_INPUT | OMAP_MUX_MODE0 */
> +		};
> +
> +		uart3_tx_irtx.uart3_tx_irtx {
> +			pinctrl-simple,cells = <0x106 0>;	/* OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> +		};
> +	};
> +};

> diff --git a/drivers/pinctrl/pinctrl-simple.c b/drivers/pinctrl/pinctrl-simple.c

> +struct pcs_device {
...
> +	unsigned (*read)(void __iomem *reg);
> +	void (*write)(unsigned val, void __iomem *reg);
> +};

Why not use regmap instead of writing your own:

> +static unsigned __maybe_unused pcs_readb(void __iomem *reg)
> +static unsigned __maybe_unused pcs_readw(void __iomem *reg)
...

regmap recently gained support for memory-mapped IO, so should work for
this. Also, using regmap might allow this binding/driver to be easily
extended to support e.g. I2C-/SPI-based pin controller hardware, since
regmap would handle all the IO details for you.

Supporting these different IO modes might require a little more thought
though. For example, an I2C device's reg refers to its location on the
parent bus, rather than the address of the registers within the device.
We'd probably need to augment parent busses with some method for child
devices to acquire a regmap object for their own IO in order to hide
this from drivers.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120504 12:27]:
> On 05/02/2012 11:24 AM, Tony Lindgren wrote:
> 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
> 
> > +Required properties:
> > +- compatible :  one of:
> > +	- "pinctrl-simple"
> > +	- "ti,omap2420-padconf"
> > +	- "ti,omap2430-padconf"
> > +	- "ti,omap3-padconf"
> > +	- "ti,omap4-padconf"
> 
> I'm in two minds about this.
> 
> If this is truly intended to be generic, I would not document any of the
> ti compatible values here. Instead, I'd create a binding document for
> the TI controllers that basically just says "this uses the bindings in
> pinctrl-simple.txt, with the following additions", and list the
> compatible values as an addition.

Hmm sure makes sense. I'll move the TI specific stuff into a separate
doc.
 
> On the other hand, I worry about whether using "pinctrl-simple" here as
> the compatible value is going to cause issues:
> 
> Certainly, this is a pretty simple driver, and most likely reasonably
> generic an applicable to many SoCs. However, it doesn't cover a bunch of
> cases that I'd still consider "simple". For example, what if each pin
> has a separate mux and pinconf register? There are probably many such
> small cases that would add up to something more complex. to cover those
> cases, will we be able to extend pinctrl-simple to cover them, and
> continue to be backwards compatible, or will we need to create a
> binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3
> each of which covers some different, yet still simple, configuration?

Yes getting the binding right is the critical part here, everything else
can be added as needed. I was thinking about using separate properties
for auxilary registers, but now thinking about it a bit more, it may not
be sufficient.

How about we make some of these properties into arrays? For example:

#pinctrl-cells = 6;
pinctrl-simple,function-mask = <0x0000ffff 0x0000ffff 0xffff0000>;
pinctrl-simple,function-off = <0x7 0x7 0x70000>;
pinctrl-simple,pinconf-mask = <0xffff0000 0xffff0000 0x0000ffff>;

Then for handling the set/clear bits case of registers, we could do that
automatically if both masks are 0 for some registers. We actually have
that for some omap1 where there are three registers per mux with bits
to set or clear. And these bits are not the same across all registers..

> To resolve this, perhaps it would be best to rework this binding and
> driver as a set of utility code that other bindings and drivers can
> build upon, rather than making it a standalone directly usable driver
> and binding.
> 
> Honestly, I'm not really sure which way to go here.

Sure that's doable, but sounds like that can be also done later after
the binding is agreed on.

> > +- reg : offset and length of the register set for the mux registers
> > +- #pinctrl-cells : width of the pinctrl array, currently only 2 is supported
> > +- pinctrl-simple,register-width : pinmux register access width
> > +- pinctrl-simple,function-mask : mask of allowed pinmux function bits
> > +- pinctrl-simple,function-off : function off mode for disabled state
> > +- pinctrl-simple,pinconf-mask : mask of allowed pinconf bits
> > +
> > +This driver uses the common pinctrl bindings as specified in
> > +pinctrl-bindings.txt document in this directory. The common bindings are used
> > +to specify the client device states using pinctrl-0 and pinctrl-names entries.
> 
> I think just remove the second sentence there; it's implicit given the
> first.

Sure.
 
> > +/* board specific .dts file, such as omap4-sdp.dts */
> > +&omap4_pmx_core {
> > +
> > +	/*
> > +	 * map all board specific static pins enabled by the pinctrl driver
> > +	 * itself during the boot (or just set them up in the bootloader)
> > +	 */
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&board_pins>;
> > +
> > +	board_pins: pinmux_board_pins {
> > +		board_static_pins {
> 
> I'd like to see a little more explanation of the node structure here.
> I.e. what does each node represent, why are there two levels of nodes, etc.
> 
> Given that the "pinctrl-simple,cells" can specify both mux function and
> pin configuration for each pin, i.e. everything you need to, I don't see
> why you'd ever need two levels of nodes here. I'd expect each "pin
> configuration node" (in pinctrl core bindings terminology) to be a
> single level:
> 
> node {
>     pinctrl-simple,cells = <...>;
> };
> 
> where node is what's references in the pinctrl-0 property by pinctrl
> client drivers.

Good point, it seems we can remove the subnodes for the cases where we
parse multiple registers in one entry. Two levels of nodes are needed for
the cases where we need to specify GPIO for a pin.
 
> > +			pinctrl-simple,cells = <
> 
> "cells" here doesn't seem like the right word. Perhaps "pins" or
> "configuration"? "Cell"s to me seems semi-reserved for binding cell
> count description, like #gpio-cells, #address-cells, etc.

Sure, "pins" works just as well here, it's shorter than "configuration".
 
> Also, there's nothing in the free-form text part of this binding
> document that says describes the set of properties that need to exist in
> these "pin configuration nodes", nor what their content is.

Will add.
 
> > +				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> > +				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> > +				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> > +				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
> > +			>;
> > +		};
> > +	};
> > +
> > +
> > +	/* map all uart2 pins as a single function */
> > +	uart2_pins: pinmux_uart2_pins {
> > +		uart2_pins {
> > +			pinctrl-simple,cells = <
> > +				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> > +				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> > +				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
> > +				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
> > +			>;
> > +		};
> > +	};
> > +
> > +	/* map all uart3 pins as separate functions */
> > +	uart3_pins: pinmux_uart3_pins {
> 
> From a binding perspective, I don't see why you'd want to allow two cases:
> 
> 1) One node with multiple entries in pinctrl-simple,cells
> 2) Multiple nodes each with a single entry in pinctrl-simple,cells
> 
> Why not only allow (1)?

Because we need to specify GPIO for some pins. There may be additional flags
too, we do have external DMA request lines for few pins available.. I'm not
saying pinctrl fwk should know about that, but it's a similar mapping of pins
to GPIO lines.

We don't want to do (2) only because it makes things slower. Typically
a board would mux most pins (could be a few hundred) as a single type (1)
entry, and then have some driver specific type (2) entries.
 
> > diff --git a/drivers/pinctrl/pinctrl-simple.c b/drivers/pinctrl/pinctrl-simple.c
> 
> > +struct pcs_device {
> ...
> > +	unsigned (*read)(void __iomem *reg);
> > +	void (*write)(unsigned val, void __iomem *reg);
> > +};
> 
> Why not use regmap instead of writing your own:
> 
> > +static unsigned __maybe_unused pcs_readb(void __iomem *reg)
> > +static unsigned __maybe_unused pcs_readw(void __iomem *reg)
> ...
> 
> regmap recently gained support for memory-mapped IO, so should work for
> this. Also, using regmap might allow this binding/driver to be easily
> extended to support e.g. I2C-/SPI-based pin controller hardware, since
> regmap would handle all the IO details for you.

Cool, will take a look.
 
> Supporting these different IO modes might require a little more thought
> though. For example, an I2C device's reg refers to its location on the
> parent bus, rather than the address of the registers within the device.
> We'd probably need to augment parent busses with some method for child
> devices to acquire a regmap object for their own IO in order to hide
> this from drivers.

Yeah that could be added later.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120504 11:59]:
> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
> > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
> >> On 08:03 Fri 04 May     , Tony Lindgren wrote:
> >>>>
> >>>> so I was thinking to do like on gpio
> >>>>
> >>>> uart {
> >>>> 	pin = < &pioA 12 {pararms} >
> >>>>
> >>>> }
> >>>
> >>> Hmm I assume the "12" above the gpio number?
> >> no pin number in the bank because it could not be gpio
> > 
> > Yes OK, but pin number 12 in the gpio bank, not in the mux register.
> > Got it.
> 
> I'd prefer to avoid any references to GPIOs here; not all muxable pins
> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
> independent.

And it seems that &pioA 12 is not always enough information for the pinctrl
driver to request a GPIO. So it's best to specify it separately.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:55 Fri 04 May     , Stephen Warren wrote:
> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
> > * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
> >> On 08:03 Fri 04 May     , Tony Lindgren wrote:
> >>>>
> >>>> so I was thinking to do like on gpio
> >>>>
> >>>> uart {
> >>>> 	pin = < &pioA 12 {pararms} >
> >>>>
> >>>> }
> >>>
> >>> Hmm I assume the "12" above the gpio number?
> >> no pin number in the bank because it could not be gpio
> > 
> > Yes OK, but pin number 12 in the gpio bank, not in the mux register.
> > Got it.
> 
> I'd prefer to avoid any references to GPIOs here; not all muxable pins
> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
> independent.
my idea was to have a phandle pinctrl specific to represent the bank
and use it in the same way as done on gpio
> 
> >> 	pioD: gpio@fffff800 {
> >> 		compatible = "atmel,at91rm9200-gpio";
> >> 		reg = <0xfffff800 0x100>;
> >> 		interrupts = <5 4>;
> >> 		#gpio-cells = <2>;
> >> 		gpio-controller;
> >> 		interrupt-controller;
> >> 	};
> >>
> >> 	pioE: gpio@fffffa00 {
> >> 		compatible = "atmel,at91rm9200-gpio";
> >> 		reg = <0xfffffa00 0x100>;
> >> 		interrupts = <5 4>;
> >> 		#gpio-cells = <2>;
> >> 		gpio-controller;
> >> 		interrupt-controller;
> >> 	};
> >>
> >> 	dbgu {
> >> 		pins = < &pioB 12 0 0
> >> 			 &pioB 13 0 2 >;
> >> 	/* with macro */
> >> 		pins = < &pioB 12 MUX_A NO_PULL_UP
> >> 			 &pioB 13 MUX_A PULL_UP >;
> >> 	};
> > 
> > I could change to use this too no problem. The only concern I have is
> > that is "&pioB 12" immutable for all gpio controllers?
> 
> You mean is the number of cells used to specify a GPIO the same
> everywhere? No. It's defined by #gpio-cells in the GPIO controller node.
> 
> But again, the GPIO binding shouldn't be related to the pinctrl binding
> directly.
> 
> > Grepping the *.dts* files, at least exynos is using the following
> > for gpios:
> > 
> > gpios = <&gpx2 0 0 0 2>;
> > 
> > If we can conclude that phandle to the gpio controller instance and
> > the register offset is always enough here, then I'm OK changing to
> > that format. It would actually save some parsing in most cases.
> >  
> >> 	/* and also the notion of linked group
> >> 	 * as on uart of network you have often the same subset of pin use.
> >> 	 *
> >> 	 * As example on uart rxd/txd is use for the group without rts/cts
> >> 	 * and the one with it
> >> 	 * on ethernet the RMII pin are use also on MII
> >> 	 */
> >>
> >> 	uart0_rxd_txd {
> >> 		pins = < &pioB 19 MUX_A PULL_UP		/* rxd */
> >> 			 &pioB 18 MUX_A NO_PULL_UP >;	/* txd */
> >> 	};
> 
> I don't really see how that DT format represents pins, functions, and
> nodes directly, and separately from which of those a board chooses to
> use. I think this binding and the one Tony originally proposed are
> eseentially semantically identical.
> 
> Going back to my idea of separating SoC and board configurations, if we
> did that, I'd expect to see something more like:
> 
> soc.dtsi or board.dts:
> 
> This is the data that the pin controller driver needs to export to
> pinctrl core. This can be completely enumerated in the soc.dtsi, or
> perhaps for uncommonly used pins/groups/functions, only included in the
> board.dts that actually uses it to cut down on soc.dtsi's size:
> 
> This data is not needed for SoCs whose pinctrl drivers include it in
> their driver file instead of DT.
I agree on at91 I propose exactly this but get the following comment tat we
are going to have too much node.

so the idea I propoose with the pins array is to avoid this issue

my first bindings on at91

functions {
};

1) we describe one functin per pin

functions {
	rxd_pb12 {
		atmel,pin-id = <&pioB 12>;
		atmel,mux = <0>;
	};

	txd_pb13 {
		atmel,pin-id = <&piaB 13>;
		atmel,pull = <2>;
		atmel,mux = <0>;
	};

	txd0_pb19 {
		atmel,pin-id = <&pioB 19>;
		atmel,pull = <2>;
		atmel,mux = <0>;
	};

	rxd0_pb18 {
		atmel,pin-id = <&pioB 18>;
		atmel,mux = <0>;
	};

	rts0_pb17 {
		atmel,pin-id = <&pioB 17>;
		atmel,mux = <1>;
	};

	cts0_pb15 {
		atmel,pin-id = <&pioB 15>;
		atmel,mux = <1>;
	};
};

groups {
	dbgu {
		pinctrl,functions = <
			&rxd_pb12 &txd_pb13 >;
	};

	uart0_rxd_txd {
		pinctrl,functions = <
			&rxd0_pb18 &txd0_pb19 >;
	};

	uart0_rts_cts {
		pinctrl,functions = <
			&rxd0_pb18 &txd0_pb19
			&rts0_pb17 &cts0_pb15 >;
	};
};

Best Regards,
J.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Linus Walleij]

On Fri, May 4, 2012 at 5:03 PM, Tony Lindgren <tony@atomide.com> wrote:

> Also I think we should let the pinctrl
> core eventually manage the pins more too. Right now the pins are
> a static array in the driver, which makes things unnecessarily
> complex for the DT case. It would be nice to also have something like
> pinctrl_register/unregister_pin instead of requiring them all
> be registered while registering with the framework initially.

Not instead of but in addition to :-)

One does not exclude the other. But a good general idea if you need
this, just introduce it.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/04/2012 03:57 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [120504 12:27]:
>> On 05/02/2012 11:24 AM, Tony Lindgren wrote:
>>
>>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
...
>> On the other hand, I worry about whether using "pinctrl-simple" here as
>> the compatible value is going to cause issues:
>>
>> Certainly, this is a pretty simple driver, and most likely reasonably
>> generic an applicable to many SoCs. However, it doesn't cover a bunch of
>> cases that I'd still consider "simple". For example, what if each pin
>> has a separate mux and pinconf register? There are probably many such
>> small cases that would add up to something more complex. to cover those
>> cases, will we be able to extend pinctrl-simple to cover them, and
>> continue to be backwards compatible, or will we need to create a
>> binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3
>> each of which covers some different, yet still simple, configuration?
> 
> Yes getting the binding right is the critical part here, everything else
> can be added as needed. I was thinking about using separate properties
> for auxilary registers, but now thinking about it a bit more, it may not
> be sufficient.
> 
> How about we make some of these properties into arrays? For example:
> 
> #pinctrl-cells = 6;
> pinctrl-simple,function-mask = <0x0000ffff 0x0000ffff 0xffff0000>;
> pinctrl-simple,function-off = <0x7 0x7 0x70000>;
> pinctrl-simple,pinconf-mask = <0xffff0000 0xffff0000 0x0000ffff>;

I'm not sure what the 3 entries in the array are meant to describe?

> Then for handling the set/clear bits case of registers, we could do that
> automatically if both masks are 0 for some registers. We actually have
> that for some omap1 where there are three registers per mux with bits
> to set or clear. And these bits are not the same across all registers..

>>> +				0x6c 0xf	/* CSI21_DX3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +				0x6e 0xf	/* CSI21_DY3 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +				0x70 0xf	/* CSI21_DX4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +				0x72 0xf	/* CSI21_DY4 OMAP_PIN_OUTPUT | OMAP_MUX_MODE7 */
>>> +			>;
>>> +		};
>>> +	};
>>> +
>>> +
>>> +	/* map all uart2 pins as a single function */
>>> +	uart2_pins: pinmux_uart2_pins {
>>> +		uart2_pins {
>>> +			pinctrl-simple,cells = <
>>> +				0xd8 0x118	/* UART2_CTS OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
>>> +				0xda 0		/* UART2_RTS OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
>>> +				0xdc 0x118	/* UART2_RX OMAP_PIN_INPUT_PULLUP | OMAP_MUX_MODE0 */
>>> +				0xde 0		/* UART2_TX OMAP_PIN_OUTPUT | OMAP_MUX_MODE0 */
>>> +			>;
>>> +		};
>>> +	};
>>> +
>>> +	/* map all uart3 pins as separate functions */
>>> +	uart3_pins: pinmux_uart3_pins {
>>
>> From a binding perspective, I don't see why you'd want to allow two cases:
>>
>> 1) One node with multiple entries in pinctrl-simple,cells
>> 2) Multiple nodes each with a single entry in pinctrl-simple,cells
>>
>> Why not only allow (1)?
> 
> Because we need to specify GPIO for some pins. There may be additional flags

What do you mean by "specify GPIO"?

Nothing in this pinctrl-simple binding seems to imply that it's also a
GPIO controller.

If "GPIO" is one of the functions that can be mux'd onto a pin, then I'd
expect that to be represented in exactly the same way as any other
function that could be mux'd onto the pin.

So, I'm not sure what GPIO-related information you want to represent.

> too, we do have external DMA request lines for few pins available.. I'm not
> saying pinctrl fwk should know about that, but it's a similar mapping of pins
> to GPIO lines.

Aren't DMA request lines also just another function that can be mux'd
onto a pin?

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/04/2012 04:08 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [120504 11:59]:
>> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
>>> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
>>>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
>>>>>>
>>>>>> so I was thinking to do like on gpio
>>>>>>
>>>>>> uart {
>>>>>> 	pin = < &pioA 12 {pararms} >
>>>>>>
>>>>>> }
>>>>>
>>>>> Hmm I assume the "12" above the gpio number?
>>>> no pin number in the bank because it could not be gpio
>>>
>>> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
>>> Got it.
>>
>> I'd prefer to avoid any references to GPIOs here; not all muxable pins
>> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
>> independent.
> 
> And it seems that &pioA 12 is not always enough information for the pinctrl
> driver to request a GPIO. So it's best to specify it separately.

Why would a pinctrl driver "request a GPIO"?

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/04/2012 08:04 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:55 Fri 04 May     , Stephen Warren wrote:
>> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
>>> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
>>>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
>>>>>>
>>>>>> so I was thinking to do like on gpio
>>>>>>
>>>>>> uart {
>>>>>> 	pin = < &pioA 12 {pararms} >
>>>>>>
>>>>>> }
>>>>>
>>>>> Hmm I assume the "12" above the gpio number?
>>>> no pin number in the bank because it could not be gpio
>>>
>>> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
>>> Got it.
>>
>> I'd prefer to avoid any references to GPIOs here; not all muxable pins
>> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
>> independent.
>
> my idea was to have a phandle pinctrl specific to represent the bank
> and use it in the same way as done on gpio

OK, so when you're talking about GPIOs in this thread, you really mean
pins; nothing to do with real GPIOs? You're just using the existing GPIO
binding as an example/base for your pinctrl pin proposal.

...
>> I don't really see how that DT format represents pins, functions, and
>> nodes directly, and separately from which of those a board chooses to
>> use. I think this binding and the one Tony originally proposed are
>> eseentially semantically identical.
>>
>> Going back to my idea of separating SoC and board configurations, if we
>> did that, I'd expect to see something more like:
>>
>> soc.dtsi or board.dts:
>>
>> This is the data that the pin controller driver needs to export to
>> pinctrl core. This can be completely enumerated in the soc.dtsi, or
>> perhaps for uncommonly used pins/groups/functions, only included in the
>> board.dts that actually uses it to cut down on soc.dtsi's size:
>>
>> This data is not needed for SoCs whose pinctrl drivers include it in
>> their driver file instead of DT.
>
> I agree on at91 I propose exactly this but get the following comment tat we
> are going to have too much node.

That's why I put it in the Tegra driver not DT:-)

> so the idea I propoose with the pins array is to avoid this issue

OK, to some extent that makes sense, but it doesn't allow you to do
stuff like have the correct names for each pin, since each pin would be
something like "Bank 1 Pin 5" or "Bank 6 Pin 2"; not very descriptive.

> my first bindings on at91
...
> 1) we describe one functin per pin
> 
> functions {
> 	rxd_pb12 {
> 		atmel,pin-id = <&pioB 12>;
> 		atmel,mux = <0>;
> 	};
> 
> 	txd_pb13 {
> 		atmel,pin-id = <&piaB 13>;
> 		atmel,pull = <2>;
> 		atmel,mux = <0>;
> 	};

A function is a specific mux value you select on a pin or group. As
such, specifying "pull" within the definition of a function doesn't
really make sense.

Now, I know that many people want to auto-generate the functions and
groups from device tree, but actually calling them functions in the DT
binding isn't right, because those nodes represent the entire pin
configuration of a pin, not a function, which is some signal you can mux
onto a pin.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120509 13:22]:
> On 05/04/2012 04:08 PM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [120504 11:59]:
> >> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
> >>> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
> >>>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
> >>>>>>
> >>>>>> so I was thinking to do like on gpio
> >>>>>>
> >>>>>> uart {
> >>>>>> 	pin = < &pioA 12 {pararms} >
> >>>>>>
> >>>>>> }
> >>>>>
> >>>>> Hmm I assume the "12" above the gpio number?
> >>>> no pin number in the bank because it could not be gpio
> >>>
> >>> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
> >>> Got it.
> >>
> >> I'd prefer to avoid any references to GPIOs here; not all muxable pins
> >> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
> >> independent.
> > 
> > And it seems that &pioA 12 is not always enough information for the pinctrl
> > driver to request a GPIO. So it's best to specify it separately.
> 
> Why would a pinctrl driver "request a GPIO"?

Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [120509 02:13]:
> On Fri, May 4, 2012 at 5:03 PM, Tony Lindgren <tony@atomide.com> wrote:
> 
> > Also I think we should let the pinctrl
> > core eventually manage the pins more too. Right now the pins are
> > a static array in the driver, which makes things unnecessarily
> > complex for the DT case. It would be nice to also have something like
> > pinctrl_register/unregister_pin instead of requiring them all
> > be registered while registering with the framework initially.
> 
> Not instead of but in addition to :-)
> 
> One does not exclude the other. But a good general idea if you need
> this, just introduce it.

Sure agreed it does not exclude the other. Will take a look when
I have a chance.

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120509 13:19]:
> On 05/04/2012 03:57 PM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [120504 12:27]:
> >> On 05/02/2012 11:24 AM, Tony Lindgren wrote:
> >>
> >>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-simple.txt
> ...
> >> On the other hand, I worry about whether using "pinctrl-simple" here as
> >> the compatible value is going to cause issues:
> >>
> >> Certainly, this is a pretty simple driver, and most likely reasonably
> >> generic an applicable to many SoCs. However, it doesn't cover a bunch of
> >> cases that I'd still consider "simple". For example, what if each pin
> >> has a separate mux and pinconf register? There are probably many such
> >> small cases that would add up to something more complex. to cover those
> >> cases, will we be able to extend pinctrl-simple to cover them, and
> >> continue to be backwards compatible, or will we need to create a
> >> binding/driver for pinctrl-simple-1, pinctrl-simple-2, pinctrl-simple-3
> >> each of which covers some different, yet still simple, configuration?
> > 
> > Yes getting the binding right is the critical part here, everything else
> > can be added as needed. I was thinking about using separate properties
> > for auxilary registers, but now thinking about it a bit more, it may not
> > be sufficient.
> > 
> > How about we make some of these properties into arrays? For example:
> > 
> > #pinctrl-cells = 6;
> > pinctrl-simple,function-mask = <0x0000ffff 0x0000ffff 0xffff0000>;
> > pinctrl-simple,function-off = <0x7 0x7 0x70000>;
> > pinctrl-simple,pinconf-mask = <0xffff0000 0xffff0000 0x0000ffff>;
> 
> I'm not sure what the 3 entries in the array are meant to describe?

If you have let's say three registers per pin, those would be the
related function and pinconf masks for those registers.
 
> > Because we need to specify GPIO for some pins. There may be additional flags
> 
> What do you mean by "specify GPIO"?
> 
> Nothing in this pinctrl-simple binding seems to imply that it's also a
> GPIO controller.

It is not a GPIO controller, but eventually needs to deal with existing
GPIO controllers.
 
> If "GPIO" is one of the functions that can be mux'd onto a pin, then I'd
> expect that to be represented in exactly the same way as any other
> function that could be mux'd onto the pin.

Right. But additionally we also need to know the mux register to GPIO
mapping for things like irq_set_irq_wake()/enable_irq_wake()/disable_irq_wake()
that may be set dynamically depending on what the user wants.
 
> So, I'm not sure what GPIO-related information you want to represent.

It seems that we should be able to do pinctrl_request_gpio that uses
an external GPIO controller and also sets up the desired wake-up flags
as needed. Anyways, not needed yet.

> > too, we do have external DMA request lines for few pins available.. I'm not
> > saying pinctrl fwk should know about that, but it's a similar mapping of pins
> > to GPIO lines.
> 
> Aren't DMA request lines also just another function that can be mux'd
> onto a pin?

Yes it's a function for routing the signal. But that also needs to be
configured in the DMA controller. Right now there's no need to have
that mapping in the binding.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/09/2012 02:49 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [120509 13:22]:
>> On 05/04/2012 04:08 PM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren@wwwdotorg.org> [120504 11:59]:
>>>> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
>>>>> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
>>>>>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
>>>>>>>>
>>>>>>>> so I was thinking to do like on gpio
>>>>>>>>
>>>>>>>> uart {
>>>>>>>> 	pin = < &pioA 12 {pararms} >
>>>>>>>>
>>>>>>>> }
>>>>>>>
>>>>>>> Hmm I assume the "12" above the gpio number?
>>>>>> no pin number in the bank because it could not be gpio
>>>>>
>>>>> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
>>>>> Got it.
>>>>
>>>> I'd prefer to avoid any references to GPIOs here; not all muxable pins
>>>> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
>>>> independent.
>>>
>>> And it seems that &pioA 12 is not always enough information for the pinctrl
>>> driver to request a GPIO. So it's best to specify it separately.
>>
>> Why would a pinctrl driver "request a GPIO"?
> 
> Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?

Well, that's a GPIO driver requesting a GPIO from the pinctrl system,
rather than the pinctrl driver requesting a GPIO (sorry to be picky).

It wasn't at all obvious to me from your binding proposal that you
intended the pinctrl-simple driver to support the GPIO operations at
all. If you do want this, I think you'd need some properties (perhaps
some kind of explicit table) in order to set up the GPIO ID -> pinctrl
pin ID mapping. I don't recall seeing those; did I just miss them? I
think we'd want this to be explicit because:

a) It may well be the case that not all users of pinctrl-simple actually
mux/control GPIOs at all. It's certainly possible to only mux "special
functions", and have dedicated pins for a GPIO controller.

b) Even when GPIOs do come into the picture, it may be that only some of
the pins are available as GPIOs.

Also, were you intending pinctrl-simple to actually be the GPIO
controller itself? That'd be another case that one might consider fairly
simple, but then extends to being gpio-simple as well as pinctrl-simple...

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120510 10:09]:
> On 05/09/2012 02:49 PM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [120509 13:22]:
> >> On 05/04/2012 04:08 PM, Tony Lindgren wrote:
> >>> * Stephen Warren <swarren@wwwdotorg.org> [120504 11:59]:
> >>>> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
> >>>>> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
> >>>>>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
> >>>>>>>>
> >>>>>>>> so I was thinking to do like on gpio
> >>>>>>>>
> >>>>>>>> uart {
> >>>>>>>> 	pin = < &pioA 12 {pararms} >
> >>>>>>>>
> >>>>>>>> }
> >>>>>>>
> >>>>>>> Hmm I assume the "12" above the gpio number?
> >>>>>> no pin number in the bank because it could not be gpio
> >>>>>
> >>>>> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
> >>>>> Got it.
> >>>>
> >>>> I'd prefer to avoid any references to GPIOs here; not all muxable pins
> >>>> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
> >>>> independent.
> >>>
> >>> And it seems that &pioA 12 is not always enough information for the pinctrl
> >>> driver to request a GPIO. So it's best to specify it separately.
> >>
> >> Why would a pinctrl driver "request a GPIO"?
> > 
> > Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?
> 
> Well, that's a GPIO driver requesting a GPIO from the pinctrl system,
> rather than the pinctrl driver requesting a GPIO (sorry to be picky).

Oh sorry maybe I misunderstood what pinctrl_request_gpio is doing.
Seems like that should work the same way from binding point of view.
 
> It wasn't at all obvious to me from your binding proposal that you
> intended the pinctrl-simple driver to support the GPIO operations at
> all. If you do want this, I think you'd need some properties (perhaps
> some kind of explicit table) in order to set up the GPIO ID -> pinctrl
> pin ID mapping. I don't recall seeing those; did I just miss them? I
> think we'd want this to be explicit because:
> 
> a) It may well be the case that not all users of pinctrl-simple actually
> mux/control GPIOs at all. It's certainly possible to only mux "special
> functions", and have dedicated pins for a GPIO controller.
> 
> b) Even when GPIOs do come into the picture, it may be that only some of
> the pins are available as GPIOs.

Right, we need some pinctrl to gpio mapping eventually. That comes
automatically from DT for the pins and gpios we care about.

And that's why the binding needs to be flexible enough so we can add
optional pin specific options as needed in addition to parsing a larger
set of pins with no options.

> Also, were you intending pinctrl-simple to actually be the GPIO
> controller itself? That'd be another case that one might consider fairly
> simple, but then extends to being gpio-simple as well as pinctrl-simple...

No, I was thinking that we can have other drivers using pinctrl simple
and add the extra features for the pins that have those.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/10/2012 11:27 AM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [120510 10:09]:
>> On 05/09/2012 02:49 PM, Tony Lindgren wrote:
>>> * Stephen Warren <swarren@wwwdotorg.org> [120509 13:22]:
>>>> On 05/04/2012 04:08 PM, Tony Lindgren wrote:
>>>>> * Stephen Warren <swarren@wwwdotorg.org> [120504 11:59]:
>>>>>> On 05/04/2012 10:34 AM, Tony Lindgren wrote:
>>>>>>> * Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> [120504 08:58]:
>>>>>>>> On 08:03 Fri 04 May     , Tony Lindgren wrote:
>>>>>>>>>>
>>>>>>>>>> so I was thinking to do like on gpio
>>>>>>>>>>
>>>>>>>>>> uart {
>>>>>>>>>> 	pin = < &pioA 12 {pararms} >
>>>>>>>>>>
>>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Hmm I assume the "12" above the gpio number?
>>>>>>>> no pin number in the bank because it could not be gpio
>>>>>>>
>>>>>>> Yes OK, but pin number 12 in the gpio bank, not in the mux register.
>>>>>>> Got it.
>>>>>>
>>>>>> I'd prefer to avoid any references to GPIOs here; not all muxable pins
>>>>>> are GPIOs and not all GPIOs are muxable pins. Lets keep the two concepts
>>>>>> independent.
>>>>>
>>>>> And it seems that &pioA 12 is not always enough information for the pinctrl
>>>>> driver to request a GPIO. So it's best to specify it separately.
>>>>
>>>> Why would a pinctrl driver "request a GPIO"?
>>>
>>> Hmm what would pinctrl_request_gpio do if the GPIO driver is separate driver?
>>
>> Well, that's a GPIO driver requesting a GPIO from the pinctrl system,
>> rather than the pinctrl driver requesting a GPIO (sorry to be picky).
> 
> Oh sorry maybe I misunderstood what pinctrl_request_gpio is doing.
> Seems like that should work the same way from binding point of view.
>  
>> It wasn't at all obvious to me from your binding proposal that you
>> intended the pinctrl-simple driver to support the GPIO operations at
>> all. If you do want this, I think you'd need some properties (perhaps
>> some kind of explicit table) in order to set up the GPIO ID -> pinctrl
>> pin ID mapping. I don't recall seeing those; did I just miss them? I
>> think we'd want this to be explicit because:
>>
>> a) It may well be the case that not all users of pinctrl-simple actually
>> mux/control GPIOs at all. It's certainly possible to only mux "special
>> functions", and have dedicated pins for a GPIO controller.
>>
>> b) Even when GPIOs do come into the picture, it may be that only some of
>> the pins are available as GPIOs.
> 
> Right, we need some pinctrl to gpio mapping eventually. That comes
> automatically from DT for the pins and gpios we care about.
> 
> And that's why the binding needs to be flexible enough so we can add
> optional pin specific options as needed in addition to parsing a larger
> set of pins with no options.

The mapping of GPIO to pinctrl pins would presumably be driven solely by
the HW design of the pin controller and GPIO, and not by the mux
selection in the pin controller (otherwise, I'd argue this isn't a
simple case that should be handled by pinctrl-simple).

As such, I'd expect some properties/table at the top-level of the pin
controller object to describe the GPIO mapping. In turn, that implies
that the individual per-pin mux-selection/configuration nodes don't need
to describe any GPIO-related information.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120511 12:21]:
> 
> The mapping of GPIO to pinctrl pins would presumably be driven solely by
> the HW design of the pin controller and GPIO, and not by the mux
> selection in the pin controller (otherwise, I'd argue this isn't a
> simple case that should be handled by pinctrl-simple).
> 
> As such, I'd expect some properties/table at the top-level of the pin
> controller object to describe the GPIO mapping. In turn, that implies
> that the individual per-pin mux-selection/configuration nodes don't need
> to describe any GPIO-related information.

Yes good point. I agree it's a HW design issue, and could be in the properties
for the pin controller object.

Just to summarize, the things to consider with the GPIO to mux mapping are:

1. Having this table as static data in the driver is is not a nice
   solution as it seems that we'd currently need six mapping tables for
   omap2+ alone.

2. This table is not needed for most of the (hundreds of) pins, it's
   only needed for a few selected pins, let's say ten or so on an average
   device. So there's no need to stuff the kernel with information about
   the unused GPIO pins.

It seems that the conclusion here is that we don't need to worry about
GPIOs in the pinctrl-simple binding for now, and it can be added later
without having to change the basic binding.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/11/2012 01:51 PM, Tony Lindgren wrote:
> * Stephen Warren <swarren@wwwdotorg.org> [120511 12:21]:
>>
>> The mapping of GPIO to pinctrl pins would presumably be driven solely by
>> the HW design of the pin controller and GPIO, and not by the mux
>> selection in the pin controller (otherwise, I'd argue this isn't a
>> simple case that should be handled by pinctrl-simple).
>>
>> As such, I'd expect some properties/table at the top-level of the pin
>> controller object to describe the GPIO mapping. In turn, that implies
>> that the individual per-pin mux-selection/configuration nodes don't need
>> to describe any GPIO-related information.
> 
> Yes good point. I agree it's a HW design issue, and could be in the properties
> for the pin controller object.
> 
> Just to summarize, the things to consider with the GPIO to mux mapping are:
> 
> 1. Having this table as static data in the driver is is not a nice
>    solution as it seems that we'd currently need six mapping tables for
>    omap2+ alone.
> 
> 2. This table is not needed for most of the (hundreds of) pins, it's
>    only needed for a few selected pins, let's say ten or so on an average
>    device. So there's no need to stuff the kernel with information about
>    the unused GPIO pins.
> 
> It seems that the conclusion here is that we don't need to worry about
> GPIOs in the pinctrl-simple binding for now, and it can be added later
> without having to change the basic binding.

The one thing I wanted to resolve here wasn't so much the binding for
GPIO interaction here, but the following comment:

You wrote:
> I wrote:
>> From a binding perspective, I don't see why you'd want to allow two cases:
>>
>> 1) One node with multiple entries in pinctrl-simple,cells
>> 2) Multiple nodes each with a single entry in pinctrl-simple,cells
>>
>> Why not only allow (1)?
>
> Because we need to specify GPIO for some pins. There may be additional flags
> too, we do have external DMA request lines for few pins available.. I'm not
> saying pinctrl fwk should know about that, but it's a similar mapping of pins
> to GPIO lines.

I'm asserting that since any GPIO mapping information would be at the
top-level of the pinctrl-simple binding, we can in fact only allow
option (1) above for the individual pin configuration nodes.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120511 14:08]:
> On 05/11/2012 01:51 PM, Tony Lindgren wrote:
> > * Stephen Warren <swarren@wwwdotorg.org> [120511 12:21]:
> >>
> >> The mapping of GPIO to pinctrl pins would presumably be driven solely by
> >> the HW design of the pin controller and GPIO, and not by the mux
> >> selection in the pin controller (otherwise, I'd argue this isn't a
> >> simple case that should be handled by pinctrl-simple).
> >>
> >> As such, I'd expect some properties/table at the top-level of the pin
> >> controller object to describe the GPIO mapping. In turn, that implies
> >> that the individual per-pin mux-selection/configuration nodes don't need
> >> to describe any GPIO-related information.
> > 
> > Yes good point. I agree it's a HW design issue, and could be in the properties
> > for the pin controller object.
> > 
> > Just to summarize, the things to consider with the GPIO to mux mapping are:
> > 
> > 1. Having this table as static data in the driver is is not a nice
> >    solution as it seems that we'd currently need six mapping tables for
> >    omap2+ alone.
> > 
> > 2. This table is not needed for most of the (hundreds of) pins, it's
> >    only needed for a few selected pins, let's say ten or so on an average
> >    device. So there's no need to stuff the kernel with information about
> >    the unused GPIO pins.
> > 
> > It seems that the conclusion here is that we don't need to worry about
> > GPIOs in the pinctrl-simple binding for now, and it can be added later
> > without having to change the basic binding.
> 
> The one thing I wanted to resolve here wasn't so much the binding for
> GPIO interaction here, but the following comment:
> 
> You wrote:
> > I wrote:
> >> From a binding perspective, I don't see why you'd want to allow two cases:
> >>
> >> 1) One node with multiple entries in pinctrl-simple,cells
> >> 2) Multiple nodes each with a single entry in pinctrl-simple,cells
> >>
> >> Why not only allow (1)?
> >
> > Because we need to specify GPIO for some pins. There may be additional flags
> > too, we do have external DMA request lines for few pins available.. I'm not
> > saying pinctrl fwk should know about that, but it's a similar mapping of pins
> > to GPIO lines.
> 
> I'm asserting that since any GPIO mapping information would be at the
> top-level of the pinctrl-simple binding, we can in fact only allow
> option (1) above for the individual pin configuration nodes.

Yes I agree. Based on what we have discussed #2 binding can now be dropped.
At least I don't have any other reasons for pin specific flags that would
need to be passed in the binding #2 above.

The only additional data #2 binding provides are the real function names
for the mux signals. But those can be eventually be shown by userspace debug
tools, so no need for the kernel to care about them. From kernel and device
driver point of view the named states are much more generic.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Linus Walleij]

On Thu, May 10, 2012 at 7:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Also, were you intending pinctrl-simple to actually be the GPIO
> controller itself? That'd be another case that one might consider fairly
> simple, but then extends to being gpio-simple as well as pinctrl-simple...

We have some pinctrl drivers implementing gpiolib too already,
and it's unavoidable I think, as some recent discussion about
matcing struct gpio_chip and pinctrl GPIO ranges shows.

Maybe "-simple" isn't such a good name for this thing. Noone thinks
any kind of pin control is simple in any sense of the word anyway :-D

Tony, would pinctrl-dt-only.c be a better name perhaps?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/12/2012 05:49 PM, Linus Walleij wrote:
> On Thu, May 10, 2012 at 7:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Also, were you intending pinctrl-simple to actually be the GPIO
>> controller itself? That'd be another case that one might consider fairly
>> simple, but then extends to being gpio-simple as well as pinctrl-simple...
> 
> We have some pinctrl drivers implementing gpiolib too already,
> and it's unavoidable I think, as some recent discussion about
> matcing struct gpio_chip and pinctrl GPIO ranges shows.

I strongly believe we should only do this when the exact same HW module
is both pinctrl and GPIO.

When there are separate HW modules, we should have separate drivers. The
fact that the two drivers need to co-ordinate with each-other isn't a
good argument to make them one driver.

And irrespective of how the drivers are structured, if there are two HW
modules, we really need two separate nodes in DT to describe them, since
the SW architecture (1 vs. 2 drivers) shouldn't influence the DT
representation unduly.

> Maybe "-simple" isn't such a good name for this thing. Noone thinks
> any kind of pin control is simple in any sense of the word anyway :-D
> 
> Tony, would pinctrl-dt-only.c be a better name perhaps?

That might be OK for the filename, but it doesn't seem like a useful
change for the DT compatible value.

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Tony Lindgren]

* Stephen Warren <swarren@wwwdotorg.org> [120514 11:42]:
> On 05/12/2012 05:49 PM, Linus Walleij wrote:
> > On Thu, May 10, 2012 at 7:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > 
> >> Also, were you intending pinctrl-simple to actually be the GPIO
> >> controller itself? That'd be another case that one might consider fairly
> >> simple, but then extends to being gpio-simple as well as pinctrl-simple...
> > 
> > We have some pinctrl drivers implementing gpiolib too already,
> > and it's unavoidable I think, as some recent discussion about
> > matcing struct gpio_chip and pinctrl GPIO ranges shows.
> 
> I strongly believe we should only do this when the exact same HW module
> is both pinctrl and GPIO.
> 
> When there are separate HW modules, we should have separate drivers. The
> fact that the two drivers need to co-ordinate with each-other isn't a
> good argument to make them one driver.
> 
> And irrespective of how the drivers are structured, if there are two HW
> modules, we really need two separate nodes in DT to describe them, since
> the SW architecture (1 vs. 2 drivers) shouldn't influence the DT
> representation unduly.

Yes.
 
> > Maybe "-simple" isn't such a good name for this thing. Noone thinks
> > any kind of pin control is simple in any sense of the word anyway :-D
> > 
> > Tony, would pinctrl-dt-only.c be a better name perhaps?
> 
> That might be OK for the filename, but it doesn't seem like a useful
> change for the DT compatible value.

Yeah let's see if we can come up with some better name.

Regards,

Tony

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Linus Walleij]

On Mon, May 14, 2012 at 8:38 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/12/2012 05:49 PM, Linus Walleij wrote:
>>
>> We have some pinctrl drivers implementing gpiolib too already,
>> and it's unavoidable I think, as some recent discussion about
>> matcing struct gpio_chip and pinctrl GPIO ranges shows.
>
> I strongly believe we should only do this when the exact same HW module
> is both pinctrl and GPIO.

Yep so this is what I have done for pinctrl-nomadik.c

> When there are separate HW modules, we should have separate drivers. The
> fact that the two drivers need to co-ordinate with each-other isn't a
> good argument to make them one driver.

So this matches the pinctrl-u300.c/pinctrl-coh901.c design pattern.

>> Maybe "-simple" isn't such a good name for this thing. Noone thinks
>> any kind of pin control is simple in any sense of the word anyway :-D
>>
>> Tony, would pinctrl-dt-only.c be a better name perhaps?
>
> That might be OK for the filename, but it doesn't seem like a useful
> change for the DT compatible value.

Oh I didn't think of these. Hm from a generic world running Windows
Mobile etc I take it that this is seen as simple mappings then?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: Add generic pinctrl-simple driver that supports omap2+ padconf

[Stephen Warren]

On 05/16/2012 01:14 AM, Linus Walleij wrote:
> On Mon, May 14, 2012 at 8:38 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/12/2012 05:49 PM, Linus Walleij wrote:
...
>>> Maybe "-simple" isn't such a good name for this thing. Noone thinks
>>> any kind of pin control is simple in any sense of the word anyway :-D
>>>
>>> Tony, would pinctrl-dt-only.c be a better name perhaps?
>>
>> That might be OK for the filename, but it doesn't seem like a useful
>> change for the DT compatible value.
> 
> Oh I didn't think of these. Hm from a generic world running Windows
> Mobile etc I take it that this is seen as simple mappings then?

I can't think of anything that's much better than pinctrl-simple.
Perhaps pinctrl-generic; at least that doesn't imply it's simple if we
keep adding features:-)